Fixes incorrect handling of TraitRefs when emitting suggestions. #90819
Conversation
This file contains 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
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
b-naber
reviewed
Nov 12, 2021
| @@ -808,14 +801,34 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { | |||
|
|
|||
| if let ObligationCauseCode::ImplDerivedObligation(obligation) = &*code { | |||
| let expected_trait_ref = obligation.parent_trait_ref; | |||
| let new_imm_trait_ref = poly_trait_ref | |||
| let found_ty = expected_trait_ref.self_ty().skip_binder(); | |||
| let imm_borrowed_found_ty = self.tcx.mk_imm_ref(self.tcx.lifetimes.re_static, found_ty); | |||
There was a problem hiding this comment.
Why can we use a static lifetime here? Is it just that the actual lifetime is irrelevant for the diagnostics and we're fine with any lifetime?
There was a problem hiding this comment.
Mostly, yeah. This will indeed lead to wrong diagnostics in some cases, but changing this to an inference variable would yield more diagnostics that are not necessarily any more right. The "correct" solution here I think is to change this to an inference variable and then also check that we don't generate a requirement like '?A: 'free, but I'm not sure what the best way to do that is.
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
petrochenkov
added
S-waiting-on-author
|
@rustbot label +S-waiting-on-review -S-waiting-on-author |
rustbot
added
S-waiting-on-review
petrochenkov
added
S-waiting-on-author
JakobDegen
force-pushed
the
issue-90804
branch
from
1d92061 to
4ef0ed4
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
petrochenkov
added
S-waiting-on-author
…ggest references When suggesting references, substitutions were being forgotten and some types were misused. This led to at least one ICE and other incorrectly emitted diagnostics. This has been fixed; in some cases this leads to diagnostics changing, and tests have been adjusted.
JakobDegen
force-pushed
the
issue-90804
branch
from
4ef0ed4 to
d58d52a
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Thanks! |
|
📌 Commit d58d52a has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit d58d52a with merge f5d19184ac83088d001d5d6bae0d17ea6bf5ccea... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Nov 14, 2021
Fixes incorrect handling of TraitRefs when emitting suggestions. Closes rust-lang#90804 , although there were more issues here that were hidden by the thing that caused this ICE. Underlying problem was that substitutions were being thrown out, which not only leads to an ICE but also incorrect diagnostics. On top of that, in some cases the self types from the root obligations were being mixed in with those from derived obligations. This makes a couple diagnostics arguable worse ("`B<C>` does not implement `Copy`" instead of "`C` does not implement `Copy`") but the worse diagnostics are at least still correct and that downside is in my opinion clearly outweighed by the benefits of fixing the ICE and unambiguously wrong diagnostics.
|
⌛ Testing commit d58d52a with merge 5cc47571b463ee445d6666e9fa2457a7fbe8f689... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Nov 15, 2021
Fixes incorrect handling of TraitRefs when emitting suggestions. Closes rust-lang#90804 , although there were more issues here that were hidden by the thing that caused this ICE. Underlying problem was that substitutions were being thrown out, which not only leads to an ICE but also incorrect diagnostics. On top of that, in some cases the self types from the root obligations were being mixed in with those from derived obligations. This makes a couple diagnostics arguable worse ("`B<C>` does not implement `Copy`" instead of "`C` does not implement `Copy`") but the worse diagnostics are at least still correct and that downside is in my opinion clearly outweighed by the benefits of fixing the ICE and unambiguously wrong diagnostics.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Nov 15, 2021
Fixes incorrect handling of TraitRefs when emitting suggestions. Closes rust-lang#90804 , although there were more issues here that were hidden by the thing that caused this ICE. Underlying problem was that substitutions were being thrown out, which not only leads to an ICE but also incorrect diagnostics. On top of that, in some cases the self types from the root obligations were being mixed in with those from derived obligations. This makes a couple diagnostics arguable worse ("`B<C>` does not implement `Copy`" instead of "`C` does not implement `Copy`") but the worse diagnostics are at least still correct and that downside is in my opinion clearly outweighed by the benefits of fixing the ICE and unambiguously wrong diagnostics.
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Nov 16, 2021
Fixes incorrect handling of TraitRefs when emitting suggestions. Closes rust-lang#90804 , although there were more issues here that were hidden by the thing that caused this ICE. Underlying problem was that substitutions were being thrown out, which not only leads to an ICE but also incorrect diagnostics. On top of that, in some cases the self types from the root obligations were being mixed in with those from derived obligations. This makes a couple diagnostics arguable worse ("`B<C>` does not implement `Copy`" instead of "`C` does not implement `Copy`") but the worse diagnostics are at least still correct and that downside is in my opinion clearly outweighed by the benefits of fixing the ICE and unambiguously wrong diagnostics.
This was referenced Nov 16, 2021
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 16, 2021
Rollup of 8 pull requests Successful merges: - rust-lang#86455 (check where-clause for explicit `Sized` before suggesting `?Sized`) - rust-lang#90801 (Normalize both arguments of `equate_normalized_input_or_output`) - rust-lang#90803 (Suggest `&str.chars()` on attempt to `&str.iter()`) - rust-lang#90819 (Fixes incorrect handling of TraitRefs when emitting suggestions.) - rust-lang#90910 (fix getting the discriminant of a zero-variant enum) - rust-lang#90925 (rustc_mir_build: reorder bindings) - rust-lang#90928 (Use a different server for checking clock drift) - rust-lang#90936 (Add a regression test for rust-lang#80772) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #90804 , although there were more issues here that were hidden by the thing that caused this ICE.
Underlying problem was that substitutions were being thrown out, which not only leads to an ICE but also incorrect diagnostics. On top of that, in some cases the self types from the root obligations were being mixed in with those from derived obligations.
This makes a couple diagnostics arguable worse ("
B<C>does not implementCopy" instead of "Cdoes not implementCopy") but the worse diagnostics are at least still correct and that downside is in my opinion clearly outweighed by the benefits of fixing the ICE and unambiguously wrong diagnostics.