Use root obligation on E0277 for some cases by estebank · Pull Request #121826 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

When encountering trait bound errors that satisfy some heuristics that tell us that the relevant trait for the user comes from the root obligation and not the current obligation, we use the root predicate for the main message.

This allows to talk about "X doesn't implement Pattern<'_>" over the most specific case that just happened to fail, like "char doesn't implement Fn(&mut char)" in
tests/ui/traits/suggest-dereferences/root-obligation.rs

The heuristics are:

• the type of the leaf predicate is (roughly) the same as the type from the root predicate, as a proxy for "we care about the root"
• the leaf trait and the root trait are different, so as to avoid talking about &mut T: Trait and instead remain talking about T: Trait instead
• the root trait is not Unsize, as to avoid talking about it in tests/ui/coercion/coerce-issue-49593-box-never.rs.

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

Fix #79359, fix #119983, fix #118779, cc #118415 (the suggestion needs to change), cc #121398 (doesn't fix the underlying issue).

r? @pnkfelix

rustbot has assigned @pnkfelix.
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

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.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

Mar 1, 2024

Choose a reason for hiding this comment

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

This error, in the future (not this PR) it should actually be:

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Pattern<'_>` is not implemented for `&char`
   |                             |
   |                             required by a bound introduced by this call
   |
note: required by a bound in `core::str::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

instead of

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

This comment has been minimized.

Choose a reason for hiding this comment

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

is it intended that we treat types with lifetimes differently if their lifetimes differ? We'll just have fewer cases where we switch to the root obligation, but I'm not sure whether it'll miss out on cases where we'd want to use the root.

I'll give it a more thorough look next week, but if you have ideas here, maybe add some tests where the self type has lifetimes

Choose a reason for hiding this comment

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

We could use can_eq instead here.

Choose a reason for hiding this comment

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

Yea good point. If that shows some tests that are affected, good, otherwise we need to write some

Choose a reason for hiding this comment

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

None of new tests were affected (after I checked for no escaping vars). I'll have to think of a case where lifetime divergence is the issue but that doesn't manifest as an OutlivesPredicate clause.

Choose a reason for hiding this comment

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

I haven't read the implementation or the most recent Zulip comments and just saw this stderr diff, so sorry if this has been discussed already.

Is this intentional? Shouldn't we still use the #[rustc_on_unimplemented]/#[diagnostic::on_unimplemented] message here despite the derived/root obligation changes?

Choose a reason for hiding this comment

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

It is somewhat intentional. When you have T: Sized then you want T to have a compile time known size. When you have T: Foo, Foo: Sized, I think it makes sense to go back to "T does not implement Sized", it's a subtle difference of "what does the user care about".

Choose a reason for hiding this comment

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

Also, I do want to do a full refactor of this logic, too many ad-hoc hacks have piled beyond maintainability (in no small part due to me), but now that we know what the output we want to keep and change is, it will be easier to clean this up in a way that makes sense. This PR is about putting the output piece closer to that "ideal end state" in anticipation of that work.

When encountering trait bound errors that satisfy some heuristics that tell us that the relevant trait for the user comes from the root obligation and not the current obligation, we use the root predicate for the main message.

This allows to talk about "X doesn't implement Pattern<'_>" over the most specific case that just happened to fail, like "char doesn't implement Fn(&mut char)" in tests/ui/traits/suggest-dereferences/root-obligation.rs

The heuristics are:

• the type of the leaf predicate is (roughly) the same as the type from the root predicate, as a proxy for "we care about the root"
• the leaf trait and the root trait are different, so as to avoid talking about &mut T: Trait and instead remain talking about T: Trait instead
• the root trait is not Unsize, as to avoid talking about it in tests/ui/coercion/coerce-issue-49593-box-never.rs.

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

Fix rust-lang#79359, fix rust-lang#119983, fix rust-lang#118779, cc rust-lang#118415 (the suggestion needs to change).

… methods on non-Iterator

error[E0599]: no method named `map` found for struct `Vec<bool>` in the current scope
  --> $DIR/vec-on-unimplemented.rs:3:23
   |
LL |     vec![true, false].map(|v| !v).collect::<Vec<_>>();
   |                       ^^^ `Vec<bool>` is not an iterator
   |
help: call `.into_iter()` first
   |
LL |     vec![true, false].into_iter().map(|v| !v).collect::<Vec<_>>();
   |                       ++++++++++++

We used to provide some help through rustc_on_unimplemented on non-impl Trait and non-type-params, but this lets us get rid of some otherwise unnecessary conditions in the annotation on Iterator.

📌 Commit 40f9dcc has been approved by oli-obk

It is now in the queue for this repository.

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 4, 2024

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

Mar 4, 2024

…r=oli-obk

Use root obligation on E0277 for some cases

When encountering trait bound errors that satisfy some heuristics that tell us that the relevant trait for the user comes from the root obligation and not the current obligation, we use the root predicate for the main message.

This allows to talk about "X doesn't implement Pattern<'_>" over the most specific case that just happened to fail, like "char doesn't implement Fn(&mut char)" in tests/ui/traits/suggest-dereferences/root-obligation.rs

The heuristics are:

• the type of the leaf predicate is (roughly) the same as the type from the root predicate, as a proxy for "we care about the root"
• the leaf trait and the root trait are different, so as to avoid talking about &mut T: Trait and instead remain talking about T: Trait instead
• the root trait is not Unsize, as to avoid talking about it in tests/ui/coercion/coerce-issue-49593-box-never.rs.

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

Fix rust-lang#79359, fix rust-lang#119983, fix rust-lang#118779, cc rust-lang#118415 (the suggestion needs to change), cc rust-lang#121398 (doesn't fix the underlying issue).

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 4, 2024

…llaumeGomez

Rollup of 9 pull requests

Successful merges:

• rust-lang#120976 (constify a couple thread_local statics)
• rust-lang#121576 (Convert the rest of the visitors to use VisitorResult)
• rust-lang#121826 (Use root obligation on E0277 for some cases)
• rust-lang#121928 (Extract an arguments struct for Builder::then_else_break)
• rust-lang#121958 (Fix redundant import errors for preload extern crate)
• rust-lang#121959 (Removing absolute path in proc-macro)
• rust-lang#121968 (Don't run test_get_os_named_thread on win7)
• rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.)
• rust-lang#121978 (Fix duplicated path in the "not found dylib" error)

r? @ghost @rustbot modify labels: rollup

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 5, 2024

…r=oli-obk

Use root obligation on E0277 for some cases

When encountering trait bound errors that satisfy some heuristics that tell us that the relevant trait for the user comes from the root obligation and not the current obligation, we use the root predicate for the main message.

This allows to talk about "X doesn't implement Pattern<'_>" over the most specific case that just happened to fail, like "char doesn't implement Fn(&mut char)" in tests/ui/traits/suggest-dereferences/root-obligation.rs

The heuristics are:

• the type of the leaf predicate is (roughly) the same as the type from the root predicate, as a proxy for "we care about the root"
• the leaf trait and the root trait are different, so as to avoid talking about &mut T: Trait and instead remain talking about T: Trait instead
• the root trait is not Unsize, as to avoid talking about it in tests/ui/coercion/coerce-issue-49593-box-never.rs.

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

Fix rust-lang#79359, fix rust-lang#119983, fix rust-lang#118779, cc rust-lang#118415 (the suggestion needs to change), cc rust-lang#121398 (doesn't fix the underlying issue).

This was referenced

Mar 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

…iaskrgr

Rollup of 10 pull requests

Successful merges:

• rust-lang#121213 (Add an example to demonstrate how Rc::into_inner works)
• rust-lang#121262 (Add vector time complexity)
• rust-lang#121287 (Clarify/add must_use message for Rc/Arc/Weak::into_raw.)
• rust-lang#121664 (Adjust error yield/await lowering)
• rust-lang#121826 (Use root obligation on E0277 for some cases)
• rust-lang#121838 (Use the correct logic for nested impl trait in assoc types)
• rust-lang#121913 (Don't panic when waiting on poisoned queries)
• rust-lang#121987 (pattern analysis: abort on arity mismatch)
• rust-lang#121993 (Avoid using unnecessary queries when printing the query stack in panics)
• rust-lang#121997 (interpret/cast: make more matches on FloatTy properly exhaustive)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

Rollup merge of rust-lang#121826 - estebank:e0277-root-obligation-2, r=oli-obk

Use root obligation on E0277 for some cases

When encountering trait bound errors that satisfy some heuristics that tell us that the relevant trait for the user comes from the root obligation and not the current obligation, we use the root predicate for the main message.

This allows to talk about "X doesn't implement Pattern<'_>" over the most specific case that just happened to fail, like "char doesn't implement Fn(&mut char)" in tests/ui/traits/suggest-dereferences/root-obligation.rs

The heuristics are:

• the type of the leaf predicate is (roughly) the same as the type from the root predicate, as a proxy for "we care about the root"
• the leaf trait and the root trait are different, so as to avoid talking about &mut T: Trait and instead remain talking about T: Trait instead
• the root trait is not Unsize, as to avoid talking about it in tests/ui/coercion/coerce-issue-49593-box-never.rs.

error[E0277]: the trait bound `&char: Pattern<'_>` is not satisfied
  --> $DIR/root-obligation.rs:6:38
   |
LL |         .filter(|c| "aeiou".contains(c))
   |                             -------- ^ the trait `Fn<(char,)>` is not implemented for `&char`, which is required by `&char: Pattern<'_>`
   |                             |
   |                             required by a bound introduced by this call
   |
   = note: required for `&char` to implement `FnOnce<(char,)>`
   = note: required for `&char` to implement `Pattern<'_>`
note: required by a bound in `core::str::<impl str>::contains`
  --> $SRC_DIR/core/src/str/mod.rs:LL:COL
help: consider dereferencing here
   |
LL |         .filter(|c| "aeiou".contains(*c))
   |                                      +

Fix rust-lang#79359, fix rust-lang#119983, fix rust-lang#118779, cc rust-lang#118415 (the suggestion needs to change), cc rust-lang#121398 (doesn't fix the underlying issue).

compiler-errors added a commit to compiler-errors/rust that referenced this pull request

Jun 13, 2024

…=jackh726

Harmonize using root or leaf obligation in trait error reporting

When rust-lang#121826 changed the error reporting to use root obligation and not the leafmost obligation, it didn't actually make sure that all the other diagnostics helper functions used the right obligation.

Specifically, when reporting similar impl candidates we are looking for impls of the root obligation, but trying to match them against the trait ref of the leaf obligation.

This does a few other miscellaneous changes. There's a lot more clean-up that could be done here, but working with this code is really grief-inducing due to how messy it has become over the years. Someone really needs to show it love. 😓

r? @estebank

Fixes rust-lang#126129

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Jun 13, 2024

…=jackh726

Harmonize using root or leaf obligation in trait error reporting

When rust-lang#121826 changed the error reporting to use root obligation and not the leafmost obligation, it didn't actually make sure that all the other diagnostics helper functions used the right obligation.

Specifically, when reporting similar impl candidates we are looking for impls of the root obligation, but trying to match them against the trait ref of the leaf obligation.

This does a few other miscellaneous changes. There's a lot more clean-up that could be done here, but working with this code is really grief-inducing due to how messy it has become over the years. Someone really needs to show it love. 😓

r? @estebank

Fixes rust-lang#126129

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Jun 13, 2024

…=jackh726

Harmonize using root or leaf obligation in trait error reporting

When rust-lang#121826 changed the error reporting to use root obligation and not the leafmost obligation, it didn't actually make sure that all the other diagnostics helper functions used the right obligation.

Specifically, when reporting similar impl candidates we are looking for impls of the root obligation, but trying to match them against the trait ref of the leaf obligation.

This does a few other miscellaneous changes. There's a lot more clean-up that could be done here, but working with this code is really grief-inducing due to how messy it has become over the years. Someone really needs to show it love. 😓

r? @estebank

Fixes rust-lang#126129

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jun 13, 2024

Rollup merge of rust-lang#126142 - compiler-errors:trait-ref-split, r=jackh726

Harmonize using root or leaf obligation in trait error reporting

When rust-lang#121826 changed the error reporting to use root obligation and not the leafmost obligation, it didn't actually make sure that all the other diagnostics helper functions used the right obligation.

Specifically, when reporting similar impl candidates we are looking for impls of the root obligation, but trying to match them against the trait ref of the leaf obligation.

This does a few other miscellaneous changes. There's a lot more clean-up that could be done here, but working with this code is really grief-inducing due to how messy it has become over the years. Someone really needs to show it love. 😓

r? @estebank

Fixes rust-lang#126129

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.