Skip to content

Commit

Permalink
Rollup merge of #133392 - compiler-errors:object-sup, r=lcnr
Browse files Browse the repository at this point in the history 
Fix ICE when multiple supertrait substitutions need assoc but only one is provided

Dyn traits must have all of their associated types constrained either by:
1. writing them in the dyn trait itself as an associated type bound, like `dyn Iterator<Item = u32>`,
2. A supertrait bound, like `trait ConstrainedIterator: Iterator<Item = u32> {}`, then you may write `dyn ConstrainedIterator` which doesn't need to mention `Item`.

However, the object type lowering code did not consider the fact that there may be multiple supertraits with different substitutions, so it just used the associated type's *def id* as a key for keeping track of which associated types are missing:

https://github.com/rust-lang/rust/blob/1fc691e6ddc24506b5234d586a5c084eb767f1ad/compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs#L131

This means that we can have missing associated types when there are mutliple supertraits with different substitutions and only one of them is constrained, like:

```rust
trait Sup<T> {
    type Assoc: Default;
}

impl<T: Default> Sup<T> for () {
    type Assoc = T;
}
impl<T: Default, U: Default> Dyn<T, U> for () {}

trait Dyn<A, B>: Sup<A, Assoc = A> + Sup<B> {}
```

The above example allows you to name `<dyn Dyn<i32, u32> as Sup<u32>>::Assoc` even though it is not possible to project since it's neither constrained by a manually written projection bound or a supertrait bound. This successfully type-checks, but leads to a codegen ICE since we are not able to project the associated type.

This PR fixes the validation for checking that a dyn trait mentions all of its associated type bounds. This is theoretically a breaking change, since you could technically use that `dyn Dyn<A, B>` type mentionedin the example above without actually *projecting* to the bad associated type, but I don't expect it to ever be relevant to a user since it's almost certainly a bug. This is corroborated with the crater results[^crater], which show no failures[^unknown].

Crater: #133392 (comment)

Fixes #133388

[^crater]: I cratered this originally with #133397, which is a PR that is stacked on top, then re-ran crater with just the failures from that PR.
[^unknown]: If you look at the crater results, it shows all of the passes as "unknown". I believe this is a crater bug, since looking at the results manually shows them as passes.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Dec 14, 2024
2 parents ecfa09a + 43e2fd5 commit 67278cd
Show file tree
Hide file tree
Showing 10 changed files with 184 additions and 163 deletions.
52 changes: 28 additions & 24 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/dyn_compatibility.rs
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
use rustc_errors::codes::*;
use rustc_errors::struct_span_code_err;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_lint_defs::builtin::UNUSED_ASSOCIATED_TYPE_BOUNDS;
use rustc_middle::span_bug;
use rustc_middle::ty::fold::BottomUpFolder;
use rustc_middle::ty::{
self, DynKind, ExistentialPredicateStableCmpExt as _, Ty, TyCtxt, TypeFoldable,
TypeVisitableExt, Upcast,
};
use rustc_span::{ErrorGuaranteed, Span};
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span};
use rustc_trait_selection::error_reporting::traits::report_dyn_incompatibility;
use rustc_trait_selection::traits::{self, hir_ty_lowering_dyn_compatibility_violations};
use rustc_type_ir::elaborate::ClauseWithSupertraitSpan;
Expand Down Expand Up @@ -128,8 +127,9 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
}
}

let mut associated_types: FxIndexMap<Span, FxIndexSet<DefId>> = FxIndexMap::default();
let mut needed_associated_types = FxIndexSet::default();

let principal_span = regular_traits.first().map_or(DUMMY_SP, |info| info.bottom().1);
let regular_traits_refs_spans = trait_bounds
.into_iter()
.filter(|(trait_ref, _)| !tcx.trait_is_auto(trait_ref.def_id()));
Expand All @@ -145,13 +145,18 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
let bound_predicate = pred.kind();
match bound_predicate.skip_binder() {
ty::PredicateKind::Clause(ty::ClauseKind::Trait(pred)) => {
let pred = bound_predicate.rebind(pred);
associated_types.entry(original_span).or_default().extend(
tcx.associated_items(pred.def_id())
// FIXME(negative_bounds): Handle this correctly...
let trait_ref =
tcx.anonymize_bound_vars(bound_predicate.rebind(pred.trait_ref));
needed_associated_types.extend(
tcx.associated_items(trait_ref.def_id())
.in_definition_order()
.filter(|item| item.kind == ty::AssocKind::Type)
.filter(|item| !item.is_impl_trait_in_trait())
.map(|item| item.def_id),
// If the associated type has a `where Self: Sized` bound,
// we do not need to constrain the associated type.
.filter(|item| !tcx.generics_require_sized_self(item.def_id))
.map(|item| (item.def_id, trait_ref)),
);
}
ty::PredicateKind::Clause(ty::ClauseKind::Projection(pred)) => {
Expand Down Expand Up @@ -201,26 +206,25 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
// So every `Projection` clause is an `Assoc = Foo` bound. `associated_types` contains all associated
// types's `DefId`, so the following loop removes all the `DefIds` of the associated types that have a
// corresponding `Projection` clause
for def_ids in associated_types.values_mut() {
for (projection_bound, span) in &projection_bounds {
let def_id = projection_bound.item_def_id();
def_ids.swap_remove(&def_id);
if tcx.generics_require_sized_self(def_id) {
tcx.emit_node_span_lint(
UNUSED_ASSOCIATED_TYPE_BOUNDS,
hir_id,
*span,
crate::errors::UnusedAssociatedTypeBounds { span: *span },
);
}
for (projection_bound, span) in &projection_bounds {
let def_id = projection_bound.item_def_id();
let trait_ref = tcx.anonymize_bound_vars(
projection_bound.map_bound(|p| p.projection_term.trait_ref(tcx)),
);
needed_associated_types.swap_remove(&(def_id, trait_ref));
if tcx.generics_require_sized_self(def_id) {
tcx.emit_node_span_lint(
UNUSED_ASSOCIATED_TYPE_BOUNDS,
hir_id,
*span,
crate::errors::UnusedAssociatedTypeBounds { span: *span },
);
}
// If the associated type has a `where Self: Sized` bound, we do not need to constrain the associated
// type in the `dyn Trait`.
def_ids.retain(|def_id| !tcx.generics_require_sized_self(def_id));
}

if let Err(guar) = self.check_for_required_assoc_tys(
associated_types,
principal_span,
needed_associated_types,
potential_assoc_types,
hir_trait_bounds,
) {
Expand Down
244 changes: 117 additions & 127 deletions compiler/rustc_hir_analysis/src/hir_ty_lowering/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -721,51 +721,42 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
/// emit a generic note suggesting using a `where` clause to constraint instead.
pub(crate) fn check_for_required_assoc_tys(
&self,
associated_types: FxIndexMap<Span, FxIndexSet<DefId>>,
principal_span: Span,
missing_assoc_types: FxIndexSet<(DefId, ty::PolyTraitRef<'tcx>)>,
potential_assoc_types: Vec<usize>,
trait_bounds: &[hir::PolyTraitRef<'_>],
) -> Result<(), ErrorGuaranteed> {
if associated_types.values().all(|v| v.is_empty()) {
if missing_assoc_types.is_empty() {
return Ok(());
}

let tcx = self.tcx();
// FIXME: Marked `mut` so that we can replace the spans further below with a more
// appropriate one, but this should be handled earlier in the span assignment.
let associated_types: FxIndexMap<Span, Vec<_>> = associated_types
// FIXME: This logic needs some more care w.r.t handling of conflicts
let missing_assoc_types: Vec<_> = missing_assoc_types
.into_iter()
.map(|(span, def_ids)| {
(span, def_ids.into_iter().map(|did| tcx.associated_item(did)).collect())
})
.map(|(def_id, trait_ref)| (tcx.associated_item(def_id), trait_ref))
.collect();
let mut names: FxIndexMap<String, Vec<Symbol>> = Default::default();
let mut names: FxIndexMap<_, Vec<Symbol>> = Default::default();
let mut names_len = 0;

// Account for things like `dyn Foo + 'a`, like in tests `issue-22434.rs` and
// `issue-22560.rs`.
let mut trait_bound_spans: Vec<Span> = vec![];
let mut dyn_compatibility_violations = Ok(());
for (span, items) in &associated_types {
if !items.is_empty() {
trait_bound_spans.push(*span);
}
for assoc_item in items {
let trait_def_id = assoc_item.container_id(tcx);
names.entry(tcx.def_path_str(trait_def_id)).or_default().push(assoc_item.name);
names_len += 1;

let violations =
dyn_compatibility_violations_for_assoc_item(tcx, trait_def_id, *assoc_item);
if !violations.is_empty() {
dyn_compatibility_violations = Err(report_dyn_incompatibility(
tcx,
*span,
None,
trait_def_id,
&violations,
)
.emit());
}
for (assoc_item, trait_ref) in &missing_assoc_types {
names.entry(trait_ref).or_default().push(assoc_item.name);
names_len += 1;

let violations =
dyn_compatibility_violations_for_assoc_item(tcx, trait_ref.def_id(), *assoc_item);
if !violations.is_empty() {
dyn_compatibility_violations = Err(report_dyn_incompatibility(
tcx,
principal_span,
None,
trait_ref.def_id(),
&violations,
)
.emit());
}
}

Expand Down Expand Up @@ -813,6 +804,7 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
.into_iter()
.map(|(trait_, mut assocs)| {
assocs.sort();
let trait_ = trait_.print_trait_sugared();
format!("{} in `{trait_}`", match &assocs[..] {
[] => String::new(),
[only] => format!("`{only}`"),
Expand All @@ -826,10 +818,9 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
names.sort();
let names = names.join(", ");

trait_bound_spans.sort();
let mut err = struct_span_code_err!(
self.dcx(),
trait_bound_spans,
principal_span,
E0191,
"the value of the associated type{} {} must be specified",
pluralize!(names_len),
Expand All @@ -839,81 +830,83 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
let mut types_count = 0;
let mut where_constraints = vec![];
let mut already_has_generics_args_suggestion = false;
for (span, assoc_items) in &associated_types {
let mut names: UnordMap<_, usize> = Default::default();
for item in assoc_items {
types_count += 1;
*names.entry(item.name).or_insert(0) += 1;
}
let mut dupes = false;
let mut shadows = false;
for item in assoc_items {
let prefix = if names[&item.name] > 1 {
let trait_def_id = item.container_id(tcx);
dupes = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else if bound_names.get(&item.name).is_some_and(|x| x != &item) {
let trait_def_id = item.container_id(tcx);
shadows = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else {
String::new()
};

let mut is_shadowed = false;

if let Some(assoc_item) = bound_names.get(&item.name)
&& assoc_item != &item
{
is_shadowed = true;
let mut names: UnordMap<_, usize> = Default::default();
for (item, _) in &missing_assoc_types {
types_count += 1;
*names.entry(item.name).or_insert(0) += 1;
}
let mut dupes = false;
let mut shadows = false;
for (item, trait_ref) in &missing_assoc_types {
let prefix = if names[&item.name] > 1 {
let trait_def_id = trait_ref.def_id();
dupes = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else if bound_names.get(&item.name).is_some_and(|x| *x != item) {
let trait_def_id = trait_ref.def_id();
shadows = true;
format!("{}::", tcx.def_path_str(trait_def_id))
} else {
String::new()
};

let rename_message =
if assoc_item.def_id.is_local() { ", consider renaming it" } else { "" };
err.span_label(
tcx.def_span(assoc_item.def_id),
format!("`{}{}` shadowed here{}", prefix, item.name, rename_message),
);
}
let mut is_shadowed = false;

let rename_message = if is_shadowed { ", consider renaming it" } else { "" };
if let Some(assoc_item) = bound_names.get(&item.name)
&& *assoc_item != item
{
is_shadowed = true;

if let Some(sp) = tcx.hir().span_if_local(item.def_id) {
err.span_label(
sp,
format!("`{}{}` defined here{}", prefix, item.name, rename_message),
);
}
let rename_message =
if assoc_item.def_id.is_local() { ", consider renaming it" } else { "" };
err.span_label(
tcx.def_span(assoc_item.def_id),
format!("`{}{}` shadowed here{}", prefix, item.name, rename_message),
);
}
if potential_assoc_types.len() == assoc_items.len() {
// When the amount of missing associated types equals the number of
// extra type arguments present. A suggesting to replace the generic args with
// associated types is already emitted.
already_has_generics_args_suggestion = true;
} else if let (Ok(snippet), false, false) =
(tcx.sess.source_map().span_to_snippet(*span), dupes, shadows)
{
let types: Vec<_> =
assoc_items.iter().map(|item| format!("{} = Type", item.name)).collect();
let code = if snippet.ends_with('>') {
// The user wrote `Trait<'a>` or similar and we don't have a type we can
// suggest, but at least we can clue them to the correct syntax
// `Trait<'a, Item = Type>` while accounting for the `<'a>` in the
// suggestion.
format!("{}, {}>", &snippet[..snippet.len() - 1], types.join(", "))
} else if in_expr_or_pat {
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
// least we can clue them to the correct syntax `Iterator::<Item = Type>`.
format!("{}::<{}>", snippet, types.join(", "))
} else {
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
// least we can clue them to the correct syntax `Iterator<Item = Type>`.
format!("{}<{}>", snippet, types.join(", "))
};
suggestions.push((*span, code));
} else if dupes {
where_constraints.push(*span);

let rename_message = if is_shadowed { ", consider renaming it" } else { "" };

if let Some(sp) = tcx.hir().span_if_local(item.def_id) {
err.span_label(
sp,
format!("`{}{}` defined here{}", prefix, item.name, rename_message),
);
}
}
if potential_assoc_types.len() == missing_assoc_types.len() {
// When the amount of missing associated types equals the number of
// extra type arguments present. A suggesting to replace the generic args with
// associated types is already emitted.
already_has_generics_args_suggestion = true;
} else if let (Ok(snippet), false, false) =
(tcx.sess.source_map().span_to_snippet(principal_span), dupes, shadows)
{
let types: Vec<_> = missing_assoc_types
.iter()
.map(|(item, _)| format!("{} = Type", item.name))
.collect();
let code = if snippet.ends_with('>') {
// The user wrote `Trait<'a>` or similar and we don't have a type we can
// suggest, but at least we can clue them to the correct syntax
// `Trait<'a, Item = Type>` while accounting for the `<'a>` in the
// suggestion.
format!("{}, {}>", &snippet[..snippet.len() - 1], types.join(", "))
} else if in_expr_or_pat {
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
// least we can clue them to the correct syntax `Iterator::<Item = Type>`.
format!("{}::<{}>", snippet, types.join(", "))
} else {
// The user wrote `Iterator`, so we don't have a type we can suggest, but at
// least we can clue them to the correct syntax `Iterator<Item = Type>`.
format!("{}<{}>", snippet, types.join(", "))
};
suggestions.push((principal_span, code));
} else if dupes {
where_constraints.push(principal_span);
}

let where_msg = "consider introducing a new type parameter, adding `where` constraints \
using the fully-qualified path to the associated types";
if !where_constraints.is_empty() && suggestions.is_empty() {
Expand All @@ -924,32 +917,29 @@ impl<'tcx> dyn HirTyLowerer<'tcx> + '_ {
}
if suggestions.len() != 1 || already_has_generics_args_suggestion {
// We don't need this label if there's an inline suggestion, show otherwise.
for (span, assoc_items) in &associated_types {
let mut names: FxIndexMap<_, usize> = FxIndexMap::default();
for item in assoc_items {
types_count += 1;
*names.entry(item.name).or_insert(0) += 1;
}
let mut label = vec![];
for item in assoc_items {
let postfix = if names[&item.name] > 1 {
let trait_def_id = item.container_id(tcx);
format!(" (from trait `{}`)", tcx.def_path_str(trait_def_id))
} else {
String::new()
};
label.push(format!("`{}`{}", item.name, postfix));
}
if !label.is_empty() {
err.span_label(
*span,
format!(
"associated type{} {} must be specified",
pluralize!(label.len()),
label.join(", "),
),
);
}
let mut names: FxIndexMap<_, usize> = FxIndexMap::default();
for (item, _) in &missing_assoc_types {
types_count += 1;
*names.entry(item.name).or_insert(0) += 1;
}
let mut label = vec![];
for (item, trait_ref) in &missing_assoc_types {
let postfix = if names[&item.name] > 1 {
format!(" (from trait `{}`)", trait_ref.print_trait_sugared())
} else {
String::new()
};
label.push(format!("`{}`{}", item.name, postfix));
}
if !label.is_empty() {
err.span_label(
principal_span,
format!(
"associated type{} {} must be specified",
pluralize!(label.len()),
label.join(", "),
),
);
}
}
suggestions.sort_by_key(|&(span, _)| span);
Expand Down
Loading

0 comments on commit 67278cd

Please sign in to comment.