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.

Sign up for GitHub

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

Jump to bottom

silence mismatched types errors for implied projections #121863

Merged
merged 6 commits into from
Mar 7, 2024
Merged

silence mismatched types errors for implied projections #121863

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6fa58be
add test for assoc type mismatch in supertrait
Feb 29, 2024
189e784
adjust obligation spans for super projections
Feb 8, 2024
443c816
rename `ast_*` to `hir_*` in wfcheck
Mar 1, 2024
aa55f6d
suppress fulfillment errors for super projections
Mar 1, 2024
6bd970d
address review feedback
Mar 4, 2024
db48b93
improve debug logging
Mar 7, 2024
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
79 changes: 51 additions & 28 deletions compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,6 +1382,41 @@ 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 {
Expand All @@ -1390,39 +1425,27 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
}

if let Some(error) = error.to_opt_poly_trait_pred() {
elaborate(self.tcx, std::iter::once(cond))
.filter_map(|implied| implied.to_opt_poly_trait_pred())
.any(|implied| {
if error.polarity() != implied.polarity() {
return false;
}
let error = error.to_poly_trait_ref();
let implied = implied.to_poly_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();
let is_implied = self.can_sub(param_env, error, implied);
if is_implied {
debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implied);
}
is_implied
})
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| {
self.enter_forall(implied, |implied| {
let param_env = ty::ParamEnv::empty();
let is_implied =
self.can_eq(param_env, error.projection_ty, implied.projection_ty)
&& self.can_eq(param_env, error.term, implied.term);
if is_implied {
debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implied);
}
is_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 {
Expand Down
7 changes: 5 additions & 2 deletions compiler/rustc_trait_selection/src/traits/wf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ enum Elaborate {
///
/// 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`
/// 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:
Expand Down Expand Up @@ -292,6 +292,9 @@ fn extend_cause_with_original_assoc_item_obligation<'tcx>(
}

// 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 •
edited
Loading

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;
}
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/trait-bounds/super-assoc-mismatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,28 @@ impl BoundOnGat for u8 {
fn trivial_bound() where (): Sub {}
//~^ ERROR the trait bound `(): Sub` is not satisfied

// The following is an edge case where the unsatisfied projection predicate
// `<<u8 as MultiAssoc>::Assoc1<()> as SuperGeneric<u16>>::Assoc == <u8 as MultiAssoc>::Assoc2`
// contains both associated types of `MultiAssoc`. To suppress the error about the unsatisfied
// super projection, the error's span must be equal to the span of the unsatisfied trait error.
trait SuperGeneric<T> {
type Assoc;
}
trait SubGeneric<T>: SuperGeneric<T, Assoc = T> {}
trait MultiAssoc
where
Self::Assoc1<()>: SubGeneric<Self::Assoc2>
{
type Assoc1<T>;
type Assoc2;
}
impl SuperGeneric<u16> for () {
type Assoc = u8;
}
impl MultiAssoc for u8 {
type Assoc1<T> = ();
//~^ ERROR the trait bound `(): SubGeneric<u16>` is not satisfied
type Assoc2 = u16;
}

fn main() {}
22 changes: 21 additions & 1 deletion tests/ui/trait-bounds/super-assoc-mismatch.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,26 @@ LL | trait Sub: Super<Assoc = u16> {}
= help: see issue #48214
= help: add `#![feature(trivial_bounds)]` to the crate attributes to enable

error: aborting due to 5 previous errors
error[E0277]: the trait bound `(): SubGeneric<u16>` is not satisfied
--> $DIR/super-assoc-mismatch.rs:55:22
|
LL | type Assoc1<T> = ();
| ^^ the trait `SubGeneric<u16>` is not implemented for `()`, which is required by `<u8 as MultiAssoc>::Assoc1<()>: SubGeneric<<u8 as MultiAssoc>::Assoc2>`
|
help: this trait has no implementations, consider adding one
--> $DIR/super-assoc-mismatch.rs:43:1
|
LL | trait SubGeneric<T>: SuperGeneric<T, Assoc = T> {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `MultiAssoc`
--> $DIR/super-assoc-mismatch.rs:46:23
|
LL | trait MultiAssoc
| ---------- required by a bound in this trait
LL | where
LL | Self::Assoc1<()>: SubGeneric<Self::Assoc2>
| ^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `MultiAssoc`

error: aborting due to 6 previous errors

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