bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Refactor dyn-compatibility error and suggestions

Browse source 
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
This commit is contained in:
Taylor Cramer 2024-11-20 14:19:36 -08:00
parent b2728d5426
commit d00d4dfe0d
175 changed files with 1338 additions and 1102 deletions
Download patch file Download diff file Expand all files Collapse all files

209
compiler/rustc_trait_selection/src/error_reporting/traits/mod.rs
View file

@ -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"));
err.span_label(span, format!("`{trait_str}` is not dyn compatible"));
attempt_dyn_to_impl_suggestion(tcx, hir_id, &mut err);
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,
);
}
}
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,7 +477,7 @@ 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);
@ -500,16 +485,12 @@ pub fn report_dyn_incompatibility<'tcx>(
// 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>(
}
}
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()
// 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
} 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 externally_visible {
err.note(format!(
"`{trait_str}` can 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",
));
}
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);
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)
} else {
false
};
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}` 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",
));
}
}
/// 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,
);
}

1
compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs
View file

@ -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)),