Refactor dyn-compatibility error and suggestions

[cramertj]

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 #132713
cc #133267

r? @compiler-errors

[rustbot]

HIR ty lowering was modified

cc @fmease

[cramertj]

cc @compiler-errors

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!

[rustbot]

Some changes occurred in src/tools/cargo

cc @ehuss

[fmease]

You've touched several git submodules. See https://rustc-dev-guide.rust-lang.org/git.html#i-changed-a-submodule-by-accident

[traviscross]

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:

rust-lang/lang-team#286

My grammatical intuition is that in most uses it should be unhyphenated, as with "object safety", "trait object", or "forward compatible".

[cramertj]

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]

Updated!

[cramertj]

You've touched several git submodules. See https://rustc-dev-guide.rust-lang.org/git.html#i-changed-a-submodule-by-accident

@fmease Thanks for the link! I should make sure my search-and-replace tooling doesn't recurse into submodules by accident :)

[fmease]

I'm sorry for the long wait! Thanks for working on this! I approve the hyphenation change and I think the new diagnostic message for E0038 might be the right way to go. I do have some comments.

[fmease]

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).

[fmease]

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;).)

[cramertj]

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.

[fmease]

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;

[fmease]

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).

[cramertj]

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.

[fmease]

"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:

Suggested change

	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.

[cramertj]

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.

[fmease]

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.

[cramertj]

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".

[traviscross]

Perhaps it would be more clear in any case to say:

error[E0038]: the trait `SomeTrait` cannot be used with `dyn`

Such verbiage would match the theory on which we adopted "dyn compatible". That is, traits that can be used with dyn are dyn compatible. So if a trait is not dyn compatible, then we'd say it cannot be used with dyn.