merge ambiguity errors that blame the same inference variable - #159593
merge ambiguity errors that blame the same inference variable#159593Albab-Hasan wants to merge 3 commits into
Conversation
rustbot
added
S-waiting-on-review
|
looks vaguely good to me, gonna reassign though as I don't have the capacity to properly review this rn @rustbot reroll |
This comment has been minimized.
This comment has been minimized.
rustbot
removed
the
S-waiting-on-review
rustbot
added
the
S-waiting-on-review
|
@jackh726 hello, just wanted to follow up on the pr. sorry if this is bothering you |
|
@rustbot reroll |
| LL - x.foo(); | ||
| LL + <Vec<T> as Foo>::foo(&x); | ||
| | | ||
|
|
There was a problem hiding this comment.
I haven't dug at all into the change itself yet, but e.g. this diff seems unfortunate. Is it possible to keep mentioning the impls and whatnot here?
This comment has been minimized.
This comment has been minimized.
578ea93 to
ca47683
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
multiple `FulfillmentErrorCode::Ambiguity` obligations on the same inference variable only ever reported the first — the rest got dropped after `set_tainted_by_errors` fired. grouped them by sub-unification-table root and fold Trait/Projection predicates from the group into notes on the primary. uses the sub root not the raw TyVid so variables unified by a pending Coerce obligation correctly land in the same group. fixes rust-lang#103911
ca47683 to
e27cb9b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| help: consider specifying a concrete type for the type parameter `S` | ||
| | | ||
| LL | println!("{}", 23u64.test(xs.iter().sum::</* Type */>())); | ||
| | ++++++++++++++ |
There was a problem hiding this comment.
sorry for being slow to review! very swamped with other stuff right now :s
dropping this kind of thing from the output seems unfortunate too. From my understanding of reading the PR description of:
the rest got dropped after set_tainted_by_errors fired
my feeling is that we should generally only be adding information to errors in this PR, not dropping additional errors we were emitting before. Mmmmayyybe it's fine to drop errors like this that actually are from the same inference variable, but the information on these dropped errors should probably migrate to an actually emitted error.
There was a problem hiding this comment.
np, im just glad this pr is getting reviewed lol :) rlly appreciate the review!
agree, that shouldnt be dropped. errors blaming a different span keep their own error now, and the merged ones carry their notes over.
btw not to be the github profile lurker here but the fractals on ur website are lowkey insane
an error pointing at a different expression labels that expression and suggests how to annotate it, and a note on the merged diagnostic carries none of that, so it kept getting dropped. such errors are reported on their own again, and the bounds behind the errors that do get merged are now explained on the merged diagnostic.
6 participants
multiple
FulfillmentErrorCode::Ambiguityobligations on the same inference variable only ever reported the first. the rest got dropped afterset_tainted_by_errorsfired. grouped them by sub-unification-table root and fold Trait/Projection predicates from the group into notes on the primary. uses the sub root not the raw TyVid so variables unified by a pending Coerce obligation correctly land in the same group.fixes #103911
r? @lcnr