Turn mismatch given errors into given not found errors #8611
Conversation
|
How can I reproduce the test failure? I’ve run |
|
My guess is that there's some non-determinism in the order in which we look for implicits, which leads to non-deterministic error output. |
|
I doubt this is an improvement. At least the previous strategy was intentional. The idea is to always provide the most specific information. If there is just one candidate that could plausibly match, and it doesn't in the end, I want to find out why by getting a type mismatch error. "Implicit not found" is much less helpful. |
|
@odersky I don’t completely agree: it’s true that it’s better to provide the most precise information as possible to the user, but in our case I think the information we provide is useless: if the candidate can’t be typed, then it should not be a candidate in the first place. But maybe there are other kinds of mismatch that could be worth reporting? Do we agree at least that inferring |
Yes, that makes sense! |
julienrf
force-pushed
the
replace-implicit-mismatch-with-implicit-not-found
branch
from
da29c0a to
d072148
Compare
| if (adapted.show == "$conforms[Nothing]" || adapted.show == "<:<.refl[Nothing]") | ||
| NoMatchingImplicitsFailure | ||
| else | ||
| SearchFailure(adapted.withType(new MismatchedImplicit(ref, pt, argument))) |
There was a problem hiding this comment.
Note that there are (and there were) no tests to exercise this code path.
julienrf
force-pushed
the
replace-implicit-mismatch-with-implicit-not-found
branch
2 times, most recently
from
307d51b to
43876a4
Compare
julienrf
force-pushed
the
replace-implicit-mismatch-with-implicit-not-found
branch
2 times, most recently
from
2aface6 to
63605ca
Compare
tests/neg/missing-implicit3.check
Outdated
| | .. | ||
| | I found: | ||
| | | ||
| | scala.math.Ordering.ordered[A](/* missing */implicitly[scala.math.Ordering.AsComparable[B]]) |
There was a problem hiding this comment.
Could you use some locally defined implicits instead of depending on what is in the standard library? It makes the test more stable in case the library changes, and it seems that even when the library don't change our output is not stable based on earlier failures of this PR (this is something we should try to fix, but it might be hard).
There was a problem hiding this comment.
Sure I can do that. On the other hand, please be aware that by creating artificial test scenarios we might simplify the reality and miss bugs/limitations.
This is what motivated this PR: the existing test suite for import suggestions was only based on artificial scenarios and we didn’t know that the output of the compiler was not good on some real world cases.
There was a problem hiding this comment.
Fixed and squashed.
…[Nothing]` and `<:<.refl[Nothing]` Fixes scala#8053 When a given candidate turns out to not be applicable we used to report that there is a mismatch between the candidate type and the expected type. The goal is to report an error as precise as possible to the user. However, in some cases showing the inferred candidate brings no value to the user, especially when the candidate is uninhabited. I’ve changed the failure to be a `NoMatchingImplicitsFailure` instead of a mismatch when `$conforms[Nothing]` or `<:<.refl` are inferred because these values are uninhabited and might be confusing for end users.
julienrf
force-pushed
the
replace-implicit-mismatch-with-implicit-not-found
branch
from
63605ca to
d525c59
Compare
Fixes #8053
When a given candidate turns out to not be applicable we used to report that
there is a mismatch between the candidate type and the expected type.
However, I believe that if there is a type mismatch, then the candidate should
not be a candidate in the first place.
I’ve changed the failure to be a
NoMatchingImplicitsFailureinstead of a mismatch.Hopefully this won’t have a negative impact. I’ve added a couple of tests that used to
report a mismatch for the "candidate"
$conforms[Nothing]and they now report thatno given could be found. One existing test was broken by my changes. This test was
very similar to the ones described in #8053, so I’ve changed the test expectation.