Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

silence mismatched types errors for implied projections #121863

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,31 +296,31 @@ fn check_item<'tcx>(tcx: TyCtxt<'tcx>, item: &'tcx hir::Item<'tcx>) -> Result<()
hir::ItemKind::Const(ty, ..) => {
check_item_type(tcx, def_id, ty.span, UnsizedHandling::Forbid)
}
hir::ItemKind::Struct(_, ast_generics) => {
hir::ItemKind::Struct(_, hir_generics) => {
let res = check_type_defn(tcx, item, false);
check_variances_for_type_defn(tcx, item, ast_generics);
check_variances_for_type_defn(tcx, item, hir_generics);
res
}
hir::ItemKind::Union(_, ast_generics) => {
hir::ItemKind::Union(_, hir_generics) => {
let res = check_type_defn(tcx, item, true);
check_variances_for_type_defn(tcx, item, ast_generics);
check_variances_for_type_defn(tcx, item, hir_generics);
res
}
hir::ItemKind::Enum(_, ast_generics) => {
hir::ItemKind::Enum(_, hir_generics) => {
let res = check_type_defn(tcx, item, true);
check_variances_for_type_defn(tcx, item, ast_generics);
check_variances_for_type_defn(tcx, item, hir_generics);
res
}
hir::ItemKind::Trait(..) => check_trait(tcx, item),
hir::ItemKind::TraitAlias(..) => check_trait(tcx, item),
// `ForeignItem`s are handled separately.
hir::ItemKind::ForeignMod { .. } => Ok(()),
hir::ItemKind::TyAlias(hir_ty, ast_generics) => {
hir::ItemKind::TyAlias(hir_ty, hir_generics) => {
if tcx.type_alias_is_lazy(item.owner_id) {
// Bounds of lazy type aliases and of eager ones that contain opaque types are respected.
// E.g: `type X = impl Trait;`, `type X = (impl Trait, Y);`.
let res = check_item_type(tcx, def_id, hir_ty.span, UnsizedHandling::Allow);
check_variances_for_type_defn(tcx, item, ast_generics);
check_variances_for_type_defn(tcx, item, hir_generics);
res
} else {
Ok(())
Expand Down Expand Up @@ -1277,25 +1277,26 @@ fn check_item_type(
})
}

#[instrument(level = "debug", skip(tcx, ast_self_ty, ast_trait_ref))]
#[instrument(level = "debug", skip(tcx, hir_self_ty, hir_trait_ref))]
fn check_impl<'tcx>(
tcx: TyCtxt<'tcx>,
item: &'tcx hir::Item<'tcx>,
ast_self_ty: &hir::Ty<'_>,
ast_trait_ref: &Option<hir::TraitRef<'_>>,
hir_self_ty: &hir::Ty<'_>,
hir_trait_ref: &Option<hir::TraitRef<'_>>,
) -> Result<(), ErrorGuaranteed> {
enter_wf_checking_ctxt(tcx, item.span, item.owner_id.def_id, |wfcx| {
match ast_trait_ref {
Some(ast_trait_ref) => {
match hir_trait_ref {
Some(hir_trait_ref) => {
// `#[rustc_reservation_impl]` impls are not real impls and
// therefore don't need to be WF (the trait's `Self: Trait` predicate
// won't hold).
let trait_ref = tcx.impl_trait_ref(item.owner_id).unwrap().instantiate_identity();
// Avoid bogus "type annotations needed `Foo: Bar`" errors on `impl Bar for Foo` in case
// other `Foo` impls are incoherent.
tcx.ensure().coherent_trait(trait_ref.def_id)?;
let trait_span = hir_trait_ref.path.span;
let trait_ref = wfcx.normalize(
ast_trait_ref.path.span,
trait_span,
Some(WellFormedLoc::Ty(item.hir_id().expect_owner().def_id)),
trait_ref,
);
Expand All @@ -1306,14 +1307,23 @@ fn check_impl<'tcx>(
wfcx.param_env,
wfcx.body_def_id,
trait_pred,
ast_trait_ref.path.span,
trait_span,
item,
);
for obligation in &mut obligations {
if obligation.cause.span != trait_span {
// We already have a better span.
continue;
}
if let Some(pred) = obligation.predicate.to_opt_poly_trait_pred()
&& pred.self_ty().skip_binder() == trait_ref.self_ty()
&& pred.skip_binder().self_ty() == trait_ref.self_ty()
{
obligation.cause.span = hir_self_ty.span;
}
if let Some(pred) = obligation.predicate.to_opt_poly_projection_pred()
&& pred.skip_binder().self_ty() == trait_ref.self_ty()
{
obligation.cause.span = ast_self_ty.span;
obligation.cause.span = hir_self_ty.span;
}
}
debug!(?obligations);
Expand All @@ -1327,7 +1337,7 @@ fn check_impl<'tcx>(
self_ty,
);
wfcx.register_wf_obligation(
ast_self_ty.span,
hir_self_ty.span,
Some(WellFormedLoc::Ty(item.hir_id().expect_owner().def_id)),
self_ty.into(),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1382,45 +1382,75 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {

#[extension(pub(super) trait InferCtxtPrivExt<'tcx>)]
impl<'tcx> TypeErrCtxt<'_, 'tcx> {
fn can_match_trait(
&self,
goal: ty::TraitPredicate<'tcx>,
assumption: ty::PolyTraitPredicate<'tcx>,
) -> bool {
if goal.polarity != assumption.polarity() {
return false;
}

let trait_goal = goal.trait_ref;
let trait_assumption = self.instantiate_binder_with_fresh_vars(
DUMMY_SP,
infer::BoundRegionConversionTime::HigherRankedType,
assumption.to_poly_trait_ref(),
);

self.can_eq(ty::ParamEnv::empty(), trait_goal, trait_assumption)
}

fn can_match_projection(
&self,
goal: ty::ProjectionPredicate<'tcx>,
assumption: ty::PolyProjectionPredicate<'tcx>,
) -> bool {
let assumption = self.instantiate_binder_with_fresh_vars(
DUMMY_SP,
infer::BoundRegionConversionTime::HigherRankedType,
assumption,
);

let param_env = ty::ParamEnv::empty();
self.can_eq(param_env, goal.projection_ty, assumption.projection_ty)
&& self.can_eq(param_env, goal.term, assumption.term)
}

// returns if `cond` not occurring implies that `error` does not occur - i.e., that
// `error` occurring implies that `cond` occurs.
fn error_implies(&self, cond: ty::Predicate<'tcx>, error: ty::Predicate<'tcx>) -> bool {
if cond == error {
return true;
}

// FIXME: It should be possible to deal with `ForAll` in a cleaner way.
let bound_error = error.kind();
let (cond, error) = match (cond.kind().skip_binder(), bound_error.skip_binder()) {
(
ty::PredicateKind::Clause(ty::ClauseKind::Trait(..)),
ty::PredicateKind::Clause(ty::ClauseKind::Trait(error)),
) => (cond, bound_error.rebind(error)),
_ => {
// FIXME: make this work in other cases too.
return false;
}
};

for pred in elaborate(self.tcx, std::iter::once(cond)) {
let bound_predicate = pred.kind();
if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(implication)) =
bound_predicate.skip_binder()
{
let error = error.to_poly_trait_ref();
let implication = bound_predicate.rebind(implication.trait_ref);
// FIXME: I'm just not taking associated types at all here.
// Eventually I'll need to implement param-env-aware
// `Γ₁ ⊦ φ₁ => Γ₂ ⊦ φ₂` logic.
let param_env = ty::ParamEnv::empty();
if self.can_sub(param_env, error, implication) {
debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implication);
return true;
}
}
if let Some(error) = error.to_opt_poly_trait_pred() {
self.enter_forall(error, |error| {
elaborate(self.tcx, std::iter::once(cond))
.filter_map(|implied| implied.to_opt_poly_trait_pred())
.any(|implied| {
let is_implied = self.can_match_trait(error, implied);
if is_implied {
debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implied);
}
is_implied
})
})
} else if let Some(error) = error.to_opt_poly_projection_pred() {
self.enter_forall(error, |error| {
elaborate(self.tcx, std::iter::once(cond))
.filter_map(|implied| implied.to_opt_poly_projection_pred())
.any(|implied| {
let is_implied = self.can_match_projection(error, implied);
if is_implied {
debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implied);
}
is_implied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(applies to can_match_trait as well)

instead of these four lines for debugging, change this to any(|implied| self.can_match_projection(error, implied)) and add #[instrument(level = "debug", skip(self), ret)] to these functions

})
})
} else {
false
}

false
}

#[instrument(skip(self), level = "debug")]
Expand Down
95 changes: 60 additions & 35 deletions compiler/rustc_trait_selection/src/traits/wf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,60 +223,87 @@ enum Elaborate {
None,
}

/// Points the cause span of a super predicate at the relevant associated type.
///
/// Given a trait impl item:
///
/// ```ignore (incomplete)
/// impl TargetTrait for TargetType {
/// type Assoc = SomeType;
/// }
/// ```
///
/// And a super predicate of `TargetTrait` that has any of the following forms:
///
/// 1. `<OtherType as OtherTrait>::Assoc == <TargetType as TargetTrait>::Assoc`
/// 2. `<<TargetType as TargetTrait>::Assoc as OtherTrait>::Assoc == OtherType`
/// 3. `<TargetType as TargetTrait>::Assoc: OtherTrait`
///
/// Replace the span of the cause with the span of the associated item:
///
/// ```ignore (incomplete)
/// impl TargetTrait for TargetType {
/// type Assoc = SomeType;
/// // ^^^^^^^^ this span
/// }
/// ```
///
/// Note that bounds that can be expressed as associated item bounds are **not**
/// super predicates. This means that form 2 and 3 from above are only relevant if
/// the [`GenericArgsRef`] of the projection type are not its identity arguments.
fn extend_cause_with_original_assoc_item_obligation<'tcx>(
tcx: TyCtxt<'tcx>,
trait_ref: ty::TraitRef<'tcx>,
item: Option<&hir::Item<'tcx>>,
cause: &mut traits::ObligationCause<'tcx>,
pred: ty::Predicate<'tcx>,
) {
debug!(
"extended_cause_with_original_assoc_item_obligation {:?} {:?} {:?} {:?}",
trait_ref, item, cause, pred
);
debug!(?item, ?cause, ?pred, "extended_cause_with_original_assoc_item_obligation");
let (items, impl_def_id) = match item {
Some(hir::Item { kind: hir::ItemKind::Impl(impl_), owner_id, .. }) => {
(impl_.items, *owner_id)
}
_ => return,
};
let fix_span =
|impl_item_ref: &hir::ImplItemRef| match tcx.hir().impl_item(impl_item_ref.id).kind {
hir::ImplItemKind::Const(ty, _) | hir::ImplItemKind::Type(ty) => ty.span,
_ => impl_item_ref.span,
};

let ty_to_impl_span = |ty: Ty<'_>| {
if let ty::Alias(ty::Projection, projection_ty) = ty.kind()
&& let Some(&impl_item_id) =
tcx.impl_item_implementor_ids(impl_def_id).get(&projection_ty.def_id)
&& let Some(impl_item) =
items.iter().find(|item| item.id.owner_id.to_def_id() == impl_item_id)
{
Some(tcx.hir().impl_item(impl_item.id).expect_type().span)
} else {
None
}
};

// It is fine to skip the binder as we don't care about regions here.
match pred.kind().skip_binder() {
ty::PredicateKind::Clause(ty::ClauseKind::Projection(proj)) => {
// The obligation comes not from the current `impl` nor the `trait` being implemented,
// but rather from a "second order" obligation, where an associated type has a
// projection coming from another associated type. See
// `tests/ui/associated-types/point-at-type-on-obligation-failure.rs` and
// `traits-assoc-type-in-supertrait-bad.rs`.
if let Some(ty::Alias(ty::Projection, projection_ty)) =
proj.term.ty().map(|ty| ty.kind())
&& let Some(&impl_item_id) =
tcx.impl_item_implementor_ids(impl_def_id).get(&projection_ty.def_id)
&& let Some(impl_item_span) = items
.iter()
.find(|item| item.id.owner_id.to_def_id() == impl_item_id)
.map(fix_span)
// Form 1: The obligation comes not from the current `impl` nor the `trait` being
// implemented, but rather from a "second order" obligation, where an associated
// type has a projection coming from another associated type.
// See `tests/ui/traits/assoc-type-in-superbad.rs` for an example.
if let Some(term_ty) = proj.term.ty()
&& let Some(impl_item_span) = ty_to_impl_span(term_ty)
{
cause.span = impl_item_span;
}

// Form 2: A projection obligation for an associated item failed to be met.
// We overwrite the span from above to ensure that a bound like
// `Self::Assoc1: Trait<OtherAssoc = Self::Assoc2>` gets the same
// span for both obligations that it is lowered to.
if let Some(impl_item_span) = ty_to_impl_span(proj.self_ty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you intentionally overwrite the span from Form1 here?

Copy link
Member Author

@lukas-code lukas-code Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not intentional. I just didn't think about the case where form 1 and form 2 both apply, but with different assoc items.

This can indeed happen and this PR changes the span for this case:

trait OtherTrait {
    type Assoc;
}

trait TargetTrait
where
    Self::Assoc1<()>: OtherTrait<Assoc = Self::Assoc2>
{
    type Assoc1<T>;
    type Assoc2;
}

impl OtherTrait for () {
    type Assoc = u8;
}

// ERROR: type mismatch resolving `<() as OtherTrait>::Assoc == u16`
impl TargetTrait for () {
    type Assoc1<T> = ();
//                   ^^ with the current state of this PR the span points here
    type Assoc2 = u16;
//                ^^^ before this PR the span points here
}

Ideally the error should probably be pointing at both of these, because either of them could be changed to fix it, but that would require changes to obligation spans in general. And pointing at either of these still seems to be better than just pointing at the trait.

But we also emit a special diagnostic for the "second order" / form 1 case, so that would be an argument for preferring the form 1 span over the form 2 span, i.e. reversing the order of my current implementation:

note: required by a bound in `TargetTrait`
  --> src/lib.rs:7:34
   |
5  | trait TargetTrait
   |       ----------- required by a bound in this trait
6  | where
7  |     Self::Assoc1<()>: OtherTrait<Assoc = Self::Assoc2>
   |                                  ^^^^^^^^^^^^^^^^^^^^ required by this bound in `TargetTrait`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 👍 don't have any strong opinion here and would need more time to read your comment to fully understand the impact. So I am fine with whatever you chose to do here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the error suppression work correctly, we have to ensure that when a bound like T::MyAssoc: Trait<OtherAssoc = U> is lowered to two obligations T::MyAssoc: Trait and <T::MyAssoc as Trait>::OtherAssoc == U then both obligations must get the same span. Otherwise, if we make an obligation like T::Assoc1::OtherAssoc == T::Assoc2 point at Assoc2 instead of Assoc1, then it's possible to construct an edge case where an error from a super projection is not suppressed due to having the wrong span:

trait Super<T> {
    type Assoc;
}

trait Sub<T>: Super<T, Assoc = T> {}

trait TargetTrait
where
    Self::Assoc1<()>: Sub<Self::Assoc2>
{
    type Assoc1<T>;
    type Assoc2;
}

impl Super<u16> for () {
    type Assoc = u8;
}

impl TargetTrait for u8 {
    type Assoc1<T> = ();
    type Assoc2 = u16;
}

What the error should look like:

error[E0277]: the trait bound `(): Sub<u16>` is not satisfied
  --> src/lib.rs:20:22
   |
20 |     type Assoc1<T> = ();
   |                      ^^ the trait `Sub<u16>` is not implemented for `()`, which is required by `<u8 as TargetTrait>::Assoc1<()>: Sub<<u8 as TargetTrait>::Assoc2>`
   |
help: this trait has no implementations, consider adding one
  --> src/lib.rs:4:1
   |
4  | trait Sub<T>: Super<T, Assoc = T> {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `TargetTrait`
  --> src/lib.rs:8:23
   |
6  | trait TargetTrait
   |       ----------- required by a bound in this trait
7  | where
8  |     Self::Assoc1<()>: Sub<Self::Assoc2>
   |                       ^^^^^^^^^^^^^^^^^ required by this bound in `TargetTrait`

What the error would looks like if we made the obligation u8::Assoc1::<()>::Assoc == u8::Assoc2 use the span of Assoc2 instead:

error[E0277]: the trait bound `(): Sub<u16>` is not satisfied
  --> src/lib.rs:20:22
   |
20 |     type Assoc1<T> = ();
   |                      ^^ the trait `Sub<u16>` is not implemented for `()`, which is required by `<u8 as TargetTrait>::Assoc1<()>: Sub<<u8 as TargetTrait>::Assoc2>`
   |
help: this trait has no implementations, consider adding one
  --> src/lib.rs:4:1
   |
4  | trait Sub<T>: Super<T, Assoc = T> {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `TargetTrait`
  --> src/lib.rs:8:23
   |
6  | trait TargetTrait
   |       ----------- required by a bound in this trait
7  | where
8  |     Self::Assoc1<()>: Sub<Self::Assoc2>
   |                       ^^^^^^^^^^^^^^^^^ required by this bound in `TargetTrait`

error[E0271]: type mismatch resolving `<() as Super<u16>>::Assoc == u16`
  --> src/lib.rs:21:19
   |
21 |     type Assoc2 = u16;
   |                   ^^^ type mismatch resolving `<() as Super<u16>>::Assoc == u16`
   |
note: expected this to be `u16`
  --> src/lib.rs:15:18
   |
15 |     type Assoc = u8;
   |                  ^^
note: required for `<u8 as TargetTrait>::Assoc1<()>` to implement `Sub<<u8 as TargetTrait>::Assoc2>`
  --> src/lib.rs:4:7
   |
4  | trait Sub<T>: Super<T, Assoc = T> {}
   |       ^^^
note: required by a bound in `TargetTrait`
  --> src/lib.rs:8:23
   |
6  | trait TargetTrait
   |       ----------- required by a bound in this trait
7  | where
8  |     Self::Assoc1<()>: Sub<Self::Assoc2>
   |                       ^^^^^^^^^^^^^^^^^ required by this bound in `TargetTrait`

So with that being the case and because Nobody Writes Code Like That Anyway[citation needed] I'd like to keep the current implementation, but add a comment and a test.

cause.span = impl_item_span;
}
}

ty::PredicateKind::Clause(ty::ClauseKind::Trait(pred)) => {
// An associated item obligation born out of the `trait` failed to be met. An example
// can be seen in `ui/associated-types/point-at-type-on-obligation-failure-2.rs`.
// Form 3: A trait obligation for an associated item failed to be met.
debug!("extended_cause_with_original_assoc_item_obligation trait proj {:?}", pred);
if let ty::Alias(ty::Projection, ty::AliasTy { def_id, .. }) = *pred.self_ty().kind()
&& let Some(&impl_item_id) = tcx.impl_item_implementor_ids(impl_def_id).get(&def_id)
&& let Some(impl_item_span) = items
.iter()
.find(|item| item.id.owner_id.to_def_id() == impl_item_id)
.map(fix_span)
{
if let Some(impl_item_span) = ty_to_impl_span(pred.self_ty()) {
cause.span = impl_item_span;
}
}
Expand Down Expand Up @@ -355,9 +382,7 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
traits::ObligationCauseCode::DerivedObligation,
);
}
extend_cause_with_original_assoc_item_obligation(
tcx, trait_ref, item, &mut cause, predicate,
);
extend_cause_with_original_assoc_item_obligation(tcx, item, &mut cause, predicate);
traits::Obligation::with_depth(tcx, cause, depth, param_env, predicate)
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ where
}

impl<T: Copy + std::ops::Deref> UnsafeCopy<'_, T> for T {
//~^ type mismatch resolving `<T as Deref>::Target == T`
type Item = T;
//~^ type mismatch resolving `<T as Deref>::Target == T`
}

pub fn main() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error[E0271]: type mismatch resolving `<T as Deref>::Target == T`
--> $DIR/hr-associated-type-projection-1.rs:13:33
--> $DIR/hr-associated-type-projection-1.rs:14:17
|
LL | impl<T: Copy + std::ops::Deref> UnsafeCopy<'_, T> for T {
| - ^^^^^^^^^^^^^^^^^ expected type parameter `T`, found associated type
| |
| expected this type parameter
| - expected this type parameter
LL | type Item = T;
| ^ expected type parameter `T`, found associated type
|
= note: expected type parameter `T`
found associated type `<T as Deref>::Target`
Expand Down
1 change: 0 additions & 1 deletion tests/ui/issues/issue-33941.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@ use std::collections::HashMap;
fn main() {
for _ in HashMap::new().iter().cloned() {} //~ ERROR expected `Iter<'_, _, _>` to be an iterator that yields `&_`, but it yields `(&_, &_)`
//~^ ERROR expected `Iter<'_, _, _>` to be an iterator that yields `&_`, but it yields `(&_, &_)`
//~| ERROR expected `Iter<'_, _, _>` to be an iterator that yields `&_`, but it yields `(&_, &_)`
}
12 changes: 1 addition & 11 deletions tests/ui/issues/issue-33941.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,6 @@ LL | for _ in HashMap::new().iter().cloned() {}
= note: required for `Cloned<std::collections::hash_map::Iter<'_, _, _>>` to implement `Iterator`
= note: required for `Cloned<std::collections::hash_map::Iter<'_, _, _>>` to implement `IntoIterator`

error[E0271]: expected `Iter<'_, _, _>` to be an iterator that yields `&_`, but it yields `(&_, &_)`
--> $DIR/issue-33941.rs:6:14
|
LL | for _ in HashMap::new().iter().cloned() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `(&_, &_)`, found `&_`
|
= note: expected tuple `(&_, &_)`
found reference `&_`
= note: required for `Cloned<std::collections::hash_map::Iter<'_, _, _>>` to implement `Iterator`

error: aborting due to 3 previous errors
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0271`.
Loading
Loading