Refactor dyn-compatibility error and suggestions #133372
Conversation
rustbot
added
S-waiting-on-review
|
HIR ty lowering was modified cc @fmease |
|
|
||
| trait Child: Super {} | ||
|
|
||
| fn take_dyn(_: &dyn Child) {} |
There was a problem hiding this comment.
This duplication and mis-attribution is the original source of the AsyncFn issue. I haven't yet resolved it as part of this PR, but this one started to get quite large again, so I figured I'd save it. I'd be happy to poke around more or try things out if you have ideas!
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
9dcb36e to
4ad5567
Compare
|
Some changes occurred in src/tools/cargo cc @ehuss |
|
You've touched several git submodules. See https://rustc-dev-guide.rust-lang.org/git.html#i-changed-a-submodule-by-accident |
This comment has been minimized.
This comment has been minimized.
fmease
added
S-waiting-on-author
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -276,7 +271,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ { | |||
| self.dcx(), | |||
| i.bottom().1, | |||
| E0038, | |||
| "the {} `{}` cannot be made into an object", | |||
| "the {} `{}` is not dyn-compatible", | |||
There was a problem hiding this comment.
Obviously a nit, but do we want to hyphenate this use or not? @fmease, how does this fit with the system you've been using elsewhere?
We didn't hyphenate over in:
My grammatical intuition is that in most uses it should be unhyphenated, as with "object safety", "trait object", or "forward compatible".
There was a problem hiding this comment.
We discussed in #133267 (comment)
My read of the APA style is that hyphens are required when the adjective comes before the object (e.g. "dyn-compatible trait") but optional when the adjective comes after (e.g. "this trait is dyn compatible" / "this trait is dyn-compatible") except where required for resolving ambiguity.
IMO we can make any choice here. It seems like folks besides myself prefer the un-hyphenated version for "postfix" adjectives. Should we resolve to always use that form?
I'll update this PR.
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
4ad5567 to
0c4161f
Compare
@fmease Thanks for the link! I should make sure my search-and-replace tooling doesn't recurse into submodules by accident :) |
This comment was marked as resolved.
This comment was marked as resolved.
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
0c4161f to
02d748e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
cramertj
added
S-waiting-on-review
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
02d748e to
2f9e7ec
Compare
This comment was marked as resolved.
This comment was marked as resolved.
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
2f9e7ec to
cf3816f
Compare
| "for a trait to be dyn compatible it needs to allow building a vtable\n\ | ||
| for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>", |
There was a problem hiding this comment.
Preexisting: Except for the URL I find this diagnostic note questionable at best.
Clearly this note is targeted at beginners — I guess ones who blindly copied or used dyn on some trait without being familiar with trait objects? This feels a bit farfetched already imo (so, this is likely more relevant / helpful in Rust <2021 with bare Trait). Anyways, assuming that's the case then what good does it do mentioning the word vtable? The word isn't even mentioned in the Reference (it being an impl detail barring the unstable DynMetadata).
There was a problem hiding this comment.
However, I see you removed the part "to allow the call to be resolvable dynamically" which might've given the word vtable the necessary context, maybe? Idk, probably not.
(For one, if we were to keep it should at least say "to allow method calls to resolvable dynamically" talking about method calls generally and not about a specific one which might not even exist in the erroneous snippet (e.g., in let _: dyn Sized;).)
There was a problem hiding this comment.
Perhaps we could change it to something like
note: traits are only dyn compatible if they follow the rules documented in
https://doc.rust-lang.org/reference/items/traits.html#object-safety
WDYT?
what good does it do [to] mention... the word vtable?
IMO mentioning "vtable" is useful to new Rust developers who are already familiar with C/C++. For me personally, "what kinds of things can be dynamically dispatched via a vtable" is exactly my mental model for what kinds of traits are object-safe, but I agree this advice isn't necessarily good for new users coming from higher-level languages.
compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
Outdated
Show resolved
Hide resolved
compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| let concrete_impls = { |
There was a problem hiding this comment.
This block only seems to exists to narrow the scope of concrete_impls's mutability. To avoid rightward drift I would either not bother at all with "mut narrowing" (because the fn is rather short) or make use of shadowing instead:
let mut concrete_impls = Vec::new();
// ...
let concrete_impls = concrete_impls;There was a problem hiding this comment.
Actually, I'd rather you keep this a filter+collect instead of manually pushing. That's more idiomatic, avoids the whole mut/block/shadowing discussion and is probably more performant since Vec::from_iter has the chance to preallocate etc (via).
There was a problem hiding this comment.
Done, though note this isn't a filter + collect-- it's a collect with the (likely) possibility of early-return. I moved away from the iterator version because I think the literal return out of the function is more straightforward to read than the collect into an Option + return on None, and I'm skeptical that the performance of this impl collection is relevant.
| if has_non_lifetime_generics(tcx, *impl_id) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
"Convert to enum" suggestions are no longer generated when there exists a type-generic impl of the trait or an impl for dyn OtherTrait
It's hard for me to tell by the diff without testing your modifications locally which specific scenarios this is meant to target. So far, I found tests/ui/dyn-compatibility/almost-supertrait-associated-type.rs where we now suppress the mention of "PhantomData<T>".
I assume your motivation is to prevent situations where it's not clear that any generic parameters in that type don't actually correspond to the ones in scope at the primary highlight? E.g., the T in "PhantomData<T>" isn't Foo's T that's in scope in line 33 but "{impl at :43:1}"'s T.
If so, then I don't understand why you would exclude lifetime parameters — they are generally subject to the same scoping rules as non-lifetime ones.
And I'm not sure if it'd be worth fixing that right now in this PR? A lot of other diagnostics have that problem (e.g., E0599: Playground). Well, in bodies we usually instantiate all generic parameters with dummy infer vars for diagnostics which get printed as '_ / _ (but here we can have non-bodies as seen in the aforementioned UI test) and in some places we use /* … */ as a marker for placeholders.
(I'd love to print such types with a pseudo early binder like <T> PhantomData<T> but I doubt non-rustc-devs would appreciate that.)
If you're set on omitting such cases I would instead suggest you to write sth. similar to:
| if has_non_lifetime_generics(tcx, *impl_id) { | |
| return; | |
| } | |
| if !tcx.type_of(*impl_id).has_param() { | |
| return; | |
| } |
which does include lifetime parameters and allows you to get rid of your custom has_non_lifetime_generics (as an aside, there's also has_non_region_param). Semantically it's slightly different as it counts referenced parameters only instead of defined ones which would be more appropriate anyway for this case.
There was a problem hiding this comment.
The specific cases I was trying to avoid were suggestions on impls like impl<T: SomeTrait> SomeTrait for &mut T { .. }. For example, this playground example generates a note like:
= help: the following types implement the trait, consider defining an enum where each variant holds one of these types, implementing `SomeTrait` for this new enum and using it instead:
&mut T
std::boxed::Box<T>
u32
These suggestions are, in my experience, unlikely to be what is wanted.
More generally, traits with type-generic or const-generic impls are unlikely to be things that can be easily transformed into an enum, and these traits and impls are typically are written by more intermediate users who are already aware that they could instead use an enum.
I wanted to exclude lifetime parameters because it's common for lifetime parameters in impls to be conceptually "one type" (e.g. SomeTy<'_> is maybe something that makes sense to have as a case in an enum). I don't feel strongly about this.
I've switched to use tcx.type_of(*impl_id).no_bound_vars() which uses has_param as you suggested.
compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
E0038 now no longer mentions the term trait object anywhere which I feel is a bit too extreme of a change? The original criticism which lead to the change in terminology was about the terms object (contrary to the spelled out trait object) and object safe(-ty). We haven't abolished trait object as far as I'm aware.
There was a problem hiding this comment.
I agree that trait object could be useful terminology to introduce here and could give the user something to search for, but I think "dyn compatible" accomplishes a similar goal. I'm not sure where I'd add a mention of trait object here without introducing additional verbosity (this error message is already quite large and risks being noisy, IMO).
Note: the current error message actually doesn't refer to a trait object either:
error[E0038]: the trait `SomeTrait` cannot be made into an object
--> src/lib.rs:7:10
|
7 | fn f(_: &dyn SomeTrait) {}
| ^^^^^^^^^^^^^ `SomeTrait` cannot be made into an object
|
note: for a trait to be "dyn-compatible" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
It says "the trait ... cannot be made into an object", rather than "cannot be made into a trait object".
fmease
removed
the
S-waiting-on-review
fmease
added
the
S-waiting-on-author
This CL makes a number of small changes to dyn compatibility errors: - "object safety" has been renamed to "dyn-compatibility" throughout - "Convert to enum" suggestions are no longer generated when there exists a type-generic impl of the trait or an impl for `dyn OtherTrait` - Several error messages are reorganized for user readability Additionally, the dyn compatibility error creation code has been split out into functions. cc rust-lang#132713 cc rust-lang#133267
cramertj
force-pushed
the
rework-dyn-suggestions
branch
from
cf3816f to
63858be
Compare
cramertj
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This CL makes a number of small changes to dyn compatibility errors:
dyn OtherTraitAdditionally, the dyn compatibility error creation code has been split out into functions.
cc #132713
cc #133267
r? @compiler-errors