Early return from resolveOverloaded in case arguments are erroneous #10985
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
It seems isErroneous forces too much. cats fails in mysterious ways if it is called in resolveOverloaded, even if it always returns false. This commit uses an error detector instead that does not go via an accumulator, so that we have more control what gets visited.
We had a problem in handling using clauses that were both parameter dependent
and had default parameters. Example:
trait A:
type X
f(using a: A = defaultA, b: a.X)
If implicit search fails for `A`, we should search the second parameter with
`defaultA.X` as type. We searched instead with an error type (i.e. the type of
the missing argument for A). Sine error types are compatible with everything,
any implicit candidate would match. In the original test i5427 we had just one
closest nested candidate that was the right one, but once we add another
candidate with a different type next to the first one the test fails.
If the searched-for type has errors, fail implicit search immediately. Error types match everything, so we risk traversing many implicit candidates for nothing. This change uncovered the problem with parameter-dependent using clauses with default arguments that was fixed in the last commit.
There are two concepts that were quite close but not exactly the same: - isErroneous, used if a type has errors in any part of it. Usually tested to not show a type with errors in it in diagnostics. - unusableForInference, used if a type (or its alias) has errors that make the type too undiscriminating for overloading resolution or implicit search. Previously, we only used isErroneous in error diagnstics. Using it also for disabling implicit search and resolutions ran into problems. Hence, the new predicate unusableForInference.
bishabosha
approved these changes
Jan 7, 2021
| */ | ||
| def unusableForInference(using Context): Boolean = widenDealias match | ||
| case AppliedType(tycon, args) => tycon.unusableForInference || args.exists(_.unusableForInference) | ||
| case RefinedType(parent, _, rinfo) => parent.unusableForInference || rinfo.unusableForInference |
There was a problem hiding this comment.
should there be a case for RecType?
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.
Fixes #9344
Fixes #10983
This was more involved than I anticipated. I originally thought we could use
isErroneousto flag types as not fit for inference.But it turned out that was not exactly what we needed, and that in fact we need another predicate
unusableForInferenceinstead. This work also uncovered a failure to correctly treat parameter-dependent using clauses with defaults, which hasbeen fixed as well.