silence mismatched types errors for implied projections #121863

lukas-code · 2024-03-01T17:26:29Z

Currently, if a trait bound is not satisfied, then we suppress any errors for the trait's supertraits not being satisfied, but still report errors for super projections not being satisfied.

For example:

trait Super {
    type Assoc;
}
trait Sub: Super<Assoc = ()> {}

Before this PR, if T: Sub is not satisfied, then errors for T: Super are suppressed, but errors for <T as Super>::Assoc == () are still shown. This PR makes it so that errors about super projections not being satisfied are also suppressed.

The errors are only suppressed if the span of the trait obligation matches the span of the super predicate obligation to avoid silencing error that are not related. This PR removes some differences between the spans of supertraits and super projections to make the suppression work correctly.

This PR fixes the majority of the diagnostics fallout when making Thin a supertrait of Sized (in a future PR).
cc #120354 (comment)
cc @lcnr

rustbot · 2024-03-01T17:26:38Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

lcnr

r=me after nits

lcnr · 2024-03-04T10:39:24Z

compiler/rustc_trait_selection/src/traits/wf.rs

            {
                cause.span = impl_item_span;
            }
+
+            // Form 2: A projection obligation for an associated item failed to be met.
+            if let Some(impl_item_span) = ty_to_impl_span(proj.self_ty()) {


you intentionally overwrite the span from Form1 here?

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`

🤔 👍 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

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.

lcnr · 2024-03-04T10:45:09Z

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

+                elaborate(self.tcx, std::iter::once(cond))
+                    .filter_map(|implied| implied.to_opt_poly_projection_pred())
+                    .any(|implied| {
+                        self.enter_forall(implied, |implied| {


you should instantiate cond with existentials here, not forall. You want to prove cond -> error (forall<T> cond<T> => forall<U> error<U>). This holds if forall<U> exists<T> cond<T> eq errorU> holds

lcnr · 2024-03-04T10:46:59Z

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

+                    // 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);


can you also use self.enter_forall(error, ...) here and manually instantiate implied with infer vars?

Isn't that exactly what can_sub already does for PolyTraitRefs?

yes, that's something I dislike though. It's subtle and hard to understand, I've already seem other people accidentally using sub inside of the trait system for non-binders because that's what they've seen used. To prevent bugs when using Ty<'tcx> instead of Binder<Ty<'tcx>>, I prefer manually instantiating binders everywhere.

lcnr · 2024-03-07T11:59:05Z

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

+                        let is_implied = self.can_match_projection(error, implied);
+                        if is_implied {
+                            debug!("error_implies: {:?} -> {:?} -> {:?}", cond, error, implied);
+                        }
+                        is_implied


(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

rustbot · 2024-03-07T12:22:36Z

Type relation code was changed

cc @compiler-errors, @lcnr

lcnr · 2024-03-07T13:02:21Z

compiler/rustc_infer/src/infer/relate/higher_ranked.rs

@@ -77,7 +77,7 @@ impl<'tcx> InferCtxt<'tcx> {
        // that name placeholders created in this function. Nested goals from type relations can
        // also contain placeholders created by this function.
        let value = self.enter_forall_and_leak_universe(forall);
-        debug!("?value");


lcnr · 2024-03-07T13:21:27Z

@bors r+ rollup

bors · 2024-03-07T13:21:30Z

📌 Commit db48b93 has been approved by lcnr

It is now in the queue for this repository.

…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#121863 (silence mismatched types errors for implied projections) - rust-lang#122043 (Apply `EarlyBinder` only to `TraitRef` in `ImplTraitHeader`) - rust-lang#122066 (Add proper cfgs for struct HirIdValidator used only with debug-assert) - rust-lang#122104 (Rust is a proper name: rust → Rust) - rust-lang#122110 (Make `x t miri` respect `MIRI_TEMP`) - rust-lang#122114 (Make not finding core a fatal error) - rust-lang#122115 (Cancel parsing ever made during recovery) - rust-lang#122123 (Don't require specifying unrelated assoc types when trait alias is in `dyn` type) - rust-lang#122126 (Fix `tidy --bless` on ̶X̶e̶n̶i̶x̶ Windows) - rust-lang#122129 (Set `RustcDocs` to only run on host) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#121863 - lukas-code:silence-mismatched-super-projections, r=lcnr silence mismatched types errors for implied projections Currently, if a trait bound is not satisfied, then we suppress any errors for the trait's supertraits not being satisfied, but still report errors for super projections not being satisfied. For example: ```rust trait Super { type Assoc; } trait Sub: Super<Assoc = ()> {} ``` Before this PR, if `T: Sub` is not satisfied, then errors for `T: Super` are suppressed, but errors for `<T as Super>::Assoc == ()` are still shown. This PR makes it so that errors about super projections not being satisfied are also suppressed. The errors are only suppressed if the span of the trait obligation matches the span of the super predicate obligation to avoid silencing error that are not related. This PR removes some differences between the spans of supertraits and super projections to make the suppression work correctly. This PR fixes the majority of the diagnostics fallout when making `Thin` a supertrait of `Sized` (in a future PR). cc rust-lang#120354 (comment) cc `@lcnr`

make `Thin` a supertrait of `Sized` This is a follow-up to rust-lang#120354 to address rust-lang#120354 (comment) and remove the "fallback impl" hack from `<T as Pointee>::Metadata` normalization in both trait solvers. We make `Thin` and therefore `Pointee<Metadata = ()>` a supertrait of `Sized`, such that every (implicit or explicit) `T: Sized` bound now also requires `T: Thin` and `<T as Pointee>::Metadata == ()`. These new bounds will be used when normalizing `<T as Pointee>::Metadata` instead of a hacky builtin impl that only applies for type parameters and aliases and overlaps with other builtin impls. Note that [RFC 2580](https://rust-lang.github.io/rfcs/2580-ptr-meta.html), which introduced `Pointee` and `Thin`, lists these unresolved questions: > Should `Thin` be added as a supertrait of `Sized`? Or could it ever make sense to have fat pointers to statically-sized types? For now, they are answered with yes and no respectively. If we end up deciding that we do want types that are `Sized + !Thin`, then a possible alternative is to add make `Thin` a defaulted trait bound that can be relaxed with `T: ?Thin` and remove `Thin` as supertrait from `Sized` before the stabilization of `feature(ptr_metadata)`. --- The removal of the "fallback impl" also allows us to always project to the shallow struct tail in the old solver, making the implementation in the old solver almost identical to the new solver. This is required to make `<Wrapper<T> as Pointee>::Metadata` work correctly in the presence of trivial bounds, for example if we know `[T]: Thin`, then we should also know `Wrapper<T>: Thin`. Lastly, this PR implements some diagnostics changes to hide errors about `` type mismatch resolving `<T as Pointee>::Metadata == ()` `` when the actual error comes from `T: Sized`. This is a continuation of rust-lang#121863. r? `@lcnr`

rustbot assigned fmease Mar 1, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 1, 2024

lcnr reviewed Mar 4, 2024

View reviewed changes

fmease assigned lcnr and unassigned fmease Mar 4, 2024

Lukas Markeffsky added 5 commits March 4, 2024 21:06

add test for assoc type mismatch in supertrait

6fa58be

adjust obligation spans for super projections

189e784

rename ast_* to hir_* in wfcheck

443c816

suppress fulfillment errors for super projections

aa55f6d

address review feedback

6bd970d

lukas-code force-pushed the silence-mismatched-super-projections branch from 3a98e5e to 6bd970d Compare March 4, 2024 20:23

lcnr reviewed Mar 7, 2024

View reviewed changes

improve debug logging

db48b93

lcnr reviewed Mar 7, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2024

GuillaumeGomez mentioned this pull request Mar 7, 2024

Rollup of 10 pull requests #122139

Merged

bors merged commit 0e37648 into rust-lang:master Mar 7, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 7, 2024

lukas-code deleted the silence-mismatched-super-projections branch March 7, 2024 17:31

lukas-code mentioned this pull request Mar 15, 2024

make Thin a supertrait of Sized #122553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

silence mismatched types errors for implied projections #121863

silence mismatched types errors for implied projections #121863

lukas-code commented Mar 1, 2024

rustbot commented Mar 1, 2024

lcnr left a comment

lcnr Mar 4, 2024

lukas-code Mar 4, 2024 •

edited

Loading

lcnr Mar 4, 2024

lukas-code Mar 4, 2024

lcnr Mar 4, 2024

lcnr Mar 4, 2024

lukas-code Mar 4, 2024

lcnr Mar 4, 2024 •

edited

Loading

lcnr Mar 7, 2024

rustbot commented Mar 7, 2024

lcnr Mar 7, 2024

lcnr commented Mar 7, 2024

bors commented Mar 7, 2024

silence mismatched types errors for implied projections #121863

silence mismatched types errors for implied projections #121863

Conversation

lukas-code commented Mar 1, 2024

rustbot commented Mar 1, 2024

lcnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukas-code Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Mar 7, 2024

Choose a reason for hiding this comment

lcnr commented Mar 7, 2024

bors commented Mar 7, 2024

lukas-code Mar 4, 2024 •

edited

Loading

lcnr Mar 4, 2024 •

edited

Loading