Fix #5773: Apply more context info to avoid ambiguous implicits #5836
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
In implicit search, if both contextual and type search fail, pick the more significant error of the two searches to report. - ambiguous trumps other errors - otherwise, errors with larger computed results trump errors with smaller ones.
dottybot
reviewed
Feb 3, 2019
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Add" instead of "Added")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
odersky
changed the title
smarter
added a commit
to lampepfl/scala
that referenced
this pull request
Feb 3, 2019
See scala/scala3#5836 for more information.
smarter
added a commit
to lampepfl/scala
that referenced
this pull request
Feb 3, 2019
See scala/scala3#5836 for more information.
smarter
added a commit
to lampepfl/scala
that referenced
this pull request
Feb 3, 2019
See scala/scala3#5836 for more information.
A subtype test `A <:< P` where `P` is a prototype was implemented as `P.isMatchedBy(A)`. `isMatchedBy` did not keep the constraint which meant that information was lost instead of being propagated.
Don't cache the result of an unforced typedArgs if it is a WildcardType. We might come back later and want the forced version.
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
Drop all IgnoredProto parts of the expected type before inferring the arguments of an implicit method. That way, we propagate all available info by `constrainResult` into the current constraint before doing the implicit search. This prevents ambiguity errors. See pos/i5773.scala.
When doping a deepenProto, strip all levels of IgnoredProto, not just one.
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/5836/ to see the changes. Benchmarks is based on merging with master (03887b7) |
If an inferred argument is ambiguous, try to deepen the prototyoe, and if that succeeds, try again.
|
Some improvements of type inference now make this work. And it seems they also help performance (a bit). In a nutshell:
|
odersky
changed the title
smarter
approved these changes
Feb 4, 2019
| // So we'd get a "inconsistent: no typevars were added to committable constraint" | ||
| // assertion failure in `constrained`. To do better, we'd have to change the | ||
| // constraint handling architecture so that some type parameters are committable | ||
| // and others are not. But that's a whole different ballgame. |
There was a problem hiding this comment.
Could we add a pending test that demonstrates the current limitation here ?
There was a problem hiding this comment.
I don't have a test for this (but agree it would be good to have one).
Co-Authored-By: odersky <[email protected]>
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.
In the absence of a proper fix for #5773 (which is problematic since it would affect implicit search oder),
we still can give a better error message.
EDIT: We now have a full fix, but it's still good to have the better error messages.