Rollup merge of rust-lang#133372 - cramertj:rework-dyn-suggestions, r=fmease

Refactor dyn-compatibility error and suggestions

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

r? `@compiler-errors`

Author: matthiaskrgr

compiler/rustc_error_codes/src/error_codes/E0038.md

@@ -264,15 +264,15 @@ trait Foo {
 ### Trait contains associated constants
 
 Just like static functions, associated constants aren't stored on the method
-table. If the trait or any subtrait contain an associated constant, they cannot
-be made into an object.
+table. If the trait or any subtrait contain an associated constant, they are not
+dyn compatible.
 
 ```compile_fail,E0038
 trait Foo {
     const X: i32;
 }
 
-impl Foo {}
+impl dyn Foo {}
 ```
 
 A simple workaround is to use a helper method instead:

compiler/rustc_feature/src/removed.rs

@@ -163,7 +163,7 @@ declare_features! (
     /// then removed. But there was no utility storing it separately, so now
     /// it's in this list.
     (removed, no_stack_check, "1.0.0", None, None),
-    /// Allows making `dyn Trait` well-formed even if `Trait` is not dyn-compatible (object safe).
+    /// Allows making `dyn Trait` well-formed even if `Trait` is not dyn compatible (object safe).
     /// Renamed to `dyn_compatible_for_dispatch`.
     (removed, object_safe_for_dispatch, "1.83.0", Some(43561),
      Some("renamed to `dyn_compatible_for_dispatch`")),

compiler/rustc_feature/src/unstable.rs

@@ -272,7 +272,7 @@ declare_features! (
     (unstable, doc_notable_trait, "1.52.0", Some(45040)),
     /// Allows using the `may_dangle` attribute (RFC 1327).
     (unstable, dropck_eyepatch, "1.10.0", Some(34761)),
-    /// Allows making `dyn Trait` well-formed even if `Trait` is not dyn-compatible[^1].
+    /// Allows making `dyn Trait` well-formed even if `Trait` is not dyn compatible[^1].
     /// In that case, `dyn Trait: Trait` does not hold. Moreover, coercions and
     /// casts in safe Rust to `dyn Trait` for such a `Trait` is also forbidden.
     ///

compiler/rustc_hir_analysis/src/coherence/mod.rs

@@ -185,7 +185,7 @@ fn check_object_overlap<'tcx>(
     // check for overlap with the automatic `impl Trait for dyn Trait`
     if let ty::Dynamic(data, ..) = trait_ref.self_ty().kind() {
         // This is something like `impl Trait1 for Trait2`. Illegal if
-        // Trait1 is a supertrait of Trait2 or Trait2 is not dyn-compatible.
+        // Trait1 is a supertrait of Trait2 or Trait2 is not dyn compatible.
 
         let component_def_ids = data.iter().flat_map(|predicate| {
             match predicate.skip_binder() {

compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs

@@ -433,34 +433,19 @@ pub fn report_dyn_incompatibility<'tcx>(
         hir::Node::Item(item) => Some(item.ident.span),
         _ => None,
     });
+
     let mut err = struct_span_code_err!(
         tcx.dcx(),
         span,
         E0038,
-        "the {} `{}` cannot be made into an object",
+        "the {} `{}` is not dyn compatible",
         tcx.def_descr(trait_def_id),
         trait_str
     );
-    err.span_label(span, format!("`{trait_str}` cannot be made into an object"));
-
-    if let Some(hir_id) = hir_id
-        && let hir::Node::Ty(ty) = tcx.hir_node(hir_id)
-        && let hir::TyKind::TraitObject([trait_ref, ..], ..) = ty.kind
-    {
-        let mut hir_id = hir_id;
-        while let hir::Node::Ty(ty) = tcx.parent_hir_node(hir_id) {
-            hir_id = ty.hir_id;
-        }
-        if tcx.parent_hir_node(hir_id).fn_sig().is_some() {
-            // Do not suggest `impl Trait` when dealing with things like super-traits.
-            err.span_suggestion_verbose(
-                ty.span.until(trait_ref.span),
-                "consider using an opaque type instead",
-                "impl ",
-                Applicability::MaybeIncorrect,
-            );
-        }
-    }
+    err.span_label(span, format!("`{trait_str}` is not dyn compatible"));
+
+    attempt_dyn_to_impl_suggestion(tcx, hir_id, &mut err);
+
     let mut reported_violations = FxIndexSet::default();
     let mut multi_span = vec![];
     let mut messages = vec![];
@@ -475,7 +460,7 @@ pub fn report_dyn_incompatibility<'tcx>(
         if reported_violations.insert(violation.clone()) {
             let spans = violation.spans();
             let msg = if trait_span.is_none() || spans.is_empty() {
-                format!("the trait cannot be made into an object because {}", violation.error_msg())
+                format!("the trait is not dyn compatible because {}", violation.error_msg())
             } else {
                 format!("...because {}", violation.error_msg())
             };
@@ -492,24 +477,20 @@ pub fn report_dyn_incompatibility<'tcx>(
     let has_multi_span = !multi_span.is_empty();
     let mut note_span = MultiSpan::from_spans(multi_span.clone());
     if let (Some(trait_span), true) = (trait_span, has_multi_span) {
-        note_span.push_span_label(trait_span, "this trait cannot be made into an object...");
+        note_span.push_span_label(trait_span, "this trait is not dyn compatible...");
     }
     for (span, msg) in iter::zip(multi_span, messages) {
         note_span.push_span_label(span, msg);
     }
     // FIXME(dyn_compat_renaming): Update the URL.
     err.span_note(
         note_span,
-        "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>",
+        "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>",
     );
 
     // Only provide the help if its a local trait, otherwise it's not actionable.
     if trait_span.is_some() {
-        let mut reported_violations: Vec<_> = reported_violations.into_iter().collect();
-        reported_violations.sort();
-
         let mut potential_solutions: Vec<_> =
             reported_violations.into_iter().map(|violation| violation.solution()).collect();
         potential_solutions.sort();
@@ -520,68 +501,116 @@ pub fn report_dyn_incompatibility<'tcx>(
         }
     }
 
+    attempt_dyn_to_enum_suggestion(tcx, trait_def_id, &*trait_str, &mut err);
+
+    err
+}
+
+/// Attempt to suggest converting the `dyn Trait` argument to an enumeration
+/// over the types that implement `Trait`.
+fn attempt_dyn_to_enum_suggestion(
+    tcx: TyCtxt<'_>,
+    trait_def_id: DefId,
+    trait_str: &str,
+    err: &mut Diag<'_>,
+) {
     let impls_of = tcx.trait_impls_of(trait_def_id);
-    let impls = if impls_of.blanket_impls().is_empty() {
-        impls_of
-            .non_blanket_impls()
-            .values()
-            .flatten()
-            .filter(|def_id| {
-                !matches!(tcx.type_of(*def_id).instantiate_identity().kind(), ty::Dynamic(..))
-            })
-            .collect::<Vec<_>>()
-    } else {
-        vec![]
-    };
-    let externally_visible = if !impls.is_empty()
-        && let Some(def_id) = trait_def_id.as_local()
+
+    if !impls_of.blanket_impls().is_empty() {
+        return;
+    }
+
+    let concrete_impls: Option<Vec<Ty<'_>>> = impls_of
+        .non_blanket_impls()
+        .values()
+        .flatten()
+        .map(|impl_id| {
+            // Don't suggest conversion to enum if the impl types have type parameters.
+            // It's unlikely the user wants to define a generic enum.
+            let Some(impl_type) = tcx.type_of(*impl_id).no_bound_vars() else { return None };
+
+            // Obviously unsized impl types won't be usable in an enum.
+            // Note: this doesn't use `Ty::is_trivially_sized` because that function
+            // defaults to assuming that things are *not* sized, whereas we want to
+            // fall back to assuming that things may be sized.
+            match impl_type.kind() {
+                ty::Str | ty::Slice(_) | ty::Dynamic(_, _, ty::DynKind::Dyn) => {
+                    return None;
+                }
+                _ => {}
+            }
+            Some(impl_type)
+        })
+        .collect();
+    let Some(concrete_impls) = concrete_impls else { return };
+
+    const MAX_IMPLS_TO_SUGGEST_CONVERTING_TO_ENUM: usize = 9;
+    if concrete_impls.is_empty() || concrete_impls.len() > MAX_IMPLS_TO_SUGGEST_CONVERTING_TO_ENUM {
+        return;
+    }
+
+    let externally_visible = if let Some(def_id) = trait_def_id.as_local() {
         // We may be executing this during typeck, which would result in cycle
         // if we used effective_visibilities query, which looks into opaque types
         // (and therefore calls typeck).
-        && tcx.resolutions(()).effective_visibilities.is_exported(def_id)
-    {
-        true
+        tcx.resolutions(()).effective_visibilities.is_exported(def_id)
     } else {
         false
     };
-    match &impls[..] {
-        [] => {}
-        _ if impls.len() > 9 => {}
-        [only] if externally_visible => {
-            err.help(with_no_trimmed_paths!(format!(
-                "only type `{}` is seen to implement the trait in this crate, consider using it \
-                 directly instead",
-                tcx.type_of(*only).instantiate_identity(),
-            )));
-        }
-        [only] => {
-            err.help(with_no_trimmed_paths!(format!(
-                "only type `{}` implements the trait, consider using it directly instead",
-                tcx.type_of(*only).instantiate_identity(),
-            )));
-        }
-        impls => {
-            let types = impls
-                .iter()
-                .map(|t| {
-                    with_no_trimmed_paths!(format!("  {}", tcx.type_of(*t).instantiate_identity(),))
-                })
-                .collect::<Vec<_>>();
-            err.help(format!(
-                "the following types implement the trait, consider defining an enum where each \
-                 variant holds one of these types, implementing `{}` for this new enum and using \
-                 it instead:\n{}",
-                trait_str,
-                types.join("\n"),
-            ));
-        }
+
+    if let [only_impl] = &concrete_impls[..] {
+        let within = if externally_visible { " within this crate" } else { "" };
+        err.help(with_no_trimmed_paths!(format!(
+            "only type `{only_impl}` implements `{trait_str}`{within}; \
+            consider using it directly instead."
+        )));
+    } else {
+        let types = concrete_impls
+            .iter()
+            .map(|t| with_no_trimmed_paths!(format!("  {}", t)))
+            .collect::<Vec<String>>()
+            .join("\n");
+
+        err.help(format!(
+            "the following types implement `{trait_str}`:\n\
+             {types}\n\
+             consider defining an enum where each variant holds one of these types,\n\
+             implementing `{trait_str}` for this new enum and using it instead",
+        ));
     }
+
     if externally_visible {
         err.note(format!(
-            "`{trait_str}` can be implemented in other crates; if you want to support your users \
+            "`{trait_str}` may be implemented in other crates; if you want to support your users \
              passing their own types here, you can't refer to a specific type",
         ));
     }
+}
 
-    err
+/// Attempt to suggest that a `dyn Trait` argument or return type be converted
+/// to use `impl Trait`.
+fn attempt_dyn_to_impl_suggestion(tcx: TyCtxt<'_>, hir_id: Option<hir::HirId>, err: &mut Diag<'_>) {
+    let Some(hir_id) = hir_id else { return };
+    let hir::Node::Ty(ty) = tcx.hir_node(hir_id) else { return };
+    let hir::TyKind::TraitObject([trait_ref, ..], ..) = ty.kind else { return };
+
+    // Only suggest converting `dyn` to `impl` if we're in a function signature.
+    // This ensures that we don't suggest converting e.g.
+    //   `type Alias = Box<dyn DynIncompatibleTrait>;` to
+    //   `type Alias = Box<impl DynIncompatibleTrait>;`
+    let Some((_id, first_non_type_parent_node)) =
+        tcx.hir().parent_iter(hir_id).find(|(_id, node)| !matches!(node, hir::Node::Ty(_)))
+    else {
+        return;
+    };
+    if first_non_type_parent_node.fn_sig().is_none() {
+        return;
+    }
+
+    err.span_suggestion_verbose(
+        ty.span.until(trait_ref.span),
+        "consider using an opaque type instead",
+        "impl ",
+        Applicability::MaybeIncorrect,
+    );
 }

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

@@ -53,7 +53,6 @@ fn dyn_compatibility_violations(
 ) -> &'_ [DynCompatibilityViolation] {
     debug_assert!(tcx.generics_of(trait_def_id).has_self);
     debug!("dyn_compatibility_violations: {:?}", trait_def_id);
-
     tcx.arena.alloc_from_iter(
         elaborate::supertrait_def_ids(tcx, trait_def_id)
             .flat_map(|def_id| dyn_compatibility_violations_for_trait(tcx, def_id)),

tests/rustdoc-ui/invalid_const_in_lifetime_position.rs

@@ -8,4 +8,4 @@ fn f<'a>(arg : Box<dyn X<Y<1> = &'a ()>>) {}
 //~| ERROR associated type takes 0 generic arguments but 1 generic argument
 //~| ERROR associated type takes 1 lifetime argument but 0 lifetime arguments
 //~| ERROR associated type takes 0 generic arguments but 1 generic argument
-//~| ERROR trait `X` cannot be made into an object
+//~| ERROR trait `X` is not dyn compatible

tests/rustdoc-ui/invalid_const_in_lifetime_position.stderr

@@ -92,17 +92,18 @@ LL |     type Y<'a>;
    |          ^
    = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`
 
-error[E0038]: the trait `X` cannot be made into an object
+error[E0038]: the trait `X` is not dyn compatible
   --> $DIR/invalid_const_in_lifetime_position.rs:4:20
    |
 LL | fn f<'a>(arg : Box<dyn X<Y<1> = &'a ()>>) {}
-   |                    ^^^^^^^^^^^^^^^^^^^^ `X` cannot be made into an object
+   |                    ^^^^^^^^^^^^^^^^^^^^ `X` is not dyn compatible
    |
-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>
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> $DIR/invalid_const_in_lifetime_position.rs:2:10
    |
 LL | trait X {
-   |       - this trait cannot be made into an object...
+   |       - this trait is not dyn compatible...
 LL |     type Y<'a>;
    |          ^ ...because it contains the generic associated type `Y`
    = help: consider moving `Y` to another trait

tests/rustdoc-ui/issues/ice-generic-type-alias-105742.rs

@@ -5,8 +5,8 @@ use std::ops::Index;
 pub fn next<'a, T>(s: &'a mut dyn SVec<Item = T, Output = T>) {
     //~^ expected 1 lifetime argument
     //~| expected 1 generic argument
-    //~| the trait `SVec` cannot be made into an object
-    //~| `SVec` cannot be made into an object
+    //~| the trait `SVec` is not dyn compatible
+    //~| `SVec` is not dyn compatible
     //~| missing generics for associated type `SVec::Item`
     //~| missing generics for associated type `SVec::Item`
     let _ = s;

tests/rustdoc-ui/issues/ice-generic-type-alias-105742.stderr

@@ -294,19 +294,20 @@ help: add missing generic argument
 LL |     Output = <Self as SVec>::Item> as SVec>::Item<T>,
    |                                                  +++
 
-error[E0038]: the trait `SVec` cannot be made into an object
+error[E0038]: the trait `SVec` is not dyn compatible
   --> $DIR/ice-generic-type-alias-105742.rs:5:35
    |
 LL | pub fn next<'a, T>(s: &'a mut dyn SVec<Item = T, Output = T>) {
-   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ `SVec` cannot be made into an object
+   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^ `SVec` is not dyn compatible
    |
-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>
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> $DIR/ice-generic-type-alias-105742.rs:15:17
    |
 LL |    pub trait SVec: Index<
    |  ____________----__^
    | |            |
-   | |            this trait cannot be made into an object...
+   | |            this trait is not dyn compatible...
 LL | |      <Self as SVec>::Item,
 ...  |
 LL | |/     Output = <Index<<Self as SVec>::Item,