Provide overlapping types for coherence errors #29962
Conversation
aturon
force-pushed
the
coherence-errors
branch
from
fc2a26e to
6649910
Compare
| impl2_def_id: DefId) | ||
| -> bool | ||
| /// If there are types that satisfy both impls, returns a `TraitRef` | ||
| /// with those types substituted. |
There was a problem hiding this comment.
So the thing that makes me wary here is that, because of the probe, this trait reference may include unresolved type variables that have since been popped. Basically, it's ok to print it out, but it's not ok to do much else with it, since it contains the equivalent of dangling references. I think instead of using resolve_type_vars_if_possible, we should consider using infer::freshen, which also resolves type variables, but which replaces unresolved type variables with a FreshTy. (Possibly not the best names...) One problem though is that freshen also replaces regions with 'static which, at least if they are printed out, could be pretty confusing.
I guess another, easier, option would be to move the probe call outside of overlapping_impls and into check_if_impls_overlap. Never mind, let's do that instead. :)
|
Also, we should add a test for the new message I think. I hate tests that encode error messages, but it seems like there is non-trivial logic here, so we'd like to test that the types we expect are being printed. |
|
☔ The latest upstream changes (presumably #29960) made this pull request unmergeable. Please resolve the merge conflicts. |
aturon
force-pushed
the
coherence-errors
branch
from
6649910 to
1a6746a
Compare
|
Test added. And it turned out the |
aturon
force-pushed
the
coherence-errors
branch
3 times, most recently
from
c95405d to
21bd4dc
Compare
|
@aturon seems good, but travis is unhappy -- still, I see no clear reason why this is the case. Perhaps it is bogus. |
|
@bors r+ let's see what bors thinks. |
|
📌 Commit 21bd4dc has been approved by |
|
Travis failure is due to |
|
⌛ Testing commit 21bd4dc with merge 3ab1ee8... |
|
💔 Test failed - auto-mac-64-nopt-t |
aturon
force-pushed
the
coherence-errors
branch
from
21bd4dc to
bc33dd7
Compare
Currently, a coherence error based on overlapping impls simply mentions the trait, and points to the two conflicting impls: ``` error: conflicting implementations for trait `Foo` ``` With this commit, the error will include all input types to the trait (including the `Self` type) after unification between the overlapping impls. In other words, the error message will provide feedback with full type details, like: ``` error: conflicting implementations of trait `Foo<u32>` for type `u8`: ``` When the `Self` type for the two impls unify to an inference variable, it is elided in the output, since "for type `_`" is just noise in that case. Closes rust-lang#23980
|
@bors: r=nmatsakis |
|
📌 Commit bc33dd7 has been approved by |
Currently, a coherence error based on overlapping impls simply mentions the trait, and points to the two conflicting impls: ``` error: conflicting implementations for trait `Foo` ``` With this commit, the error will include all input types to the trait (including the `Self` type) after unification between the overlapping impls. In other words, the error message will provide feedback with full type details, like: ``` error: conflicting implementations of trait `Foo<u32>` for type `u8`: ``` When the `Self` type for the two impls unify to an inference variable, it is elided in the output, since "for type `_`" is just noise in that case. Closes #23980 r? @nikomatsakis
Currently, a coherence error based on overlapping impls simply mentions
the trait, and points to the two conflicting impls:
With this commit, the error will include all input types to the
trait (including the
Selftype) after unification between theoverlapping impls. In other words, the error message will provide
feedback with full type details, like:
When the
Selftype for the two impls unify to an inference variable,it is elided in the output, since "for type
_" is just noise in thatcase.
Closes #23980
r? @nikomatsakis