Improve multi argument list overload resolution #22740
Conversation
`resolveOverloaded1` starts with `narrowMostSpecific(candidates)` which compares the alternatives based on the 1st argument list. If more than one result if found, we can continue with the 2nd argument list. But we do so with the original candidates, rather than with the result of `narrowMostSpecific`. This can lead to choosing an alternative which is not the most specific, by now disregarding the 1st argument list. In i120053, the 1st pass correctly eliminated the 3rd `def ^^^`, but could not resolve between the 1st two (having the same argument list). The 2nd pass then disregarded this and restarted the comparison based on the 2nd argument list alone, which incorrectly yielded the 3rd option. The change is simply using the initial result of `narrowMostSpecific` in the recursive resolution based on subsequent argument lists. I'm not sure however if the same changes should apply to the rest of the cases attempting further narrowing ?
…in 3.6 This is similar to the warnings for changes in given preference. In this case, however, the changes only affect a part of disambiguation used relatively late in the process, as a "last resort" disambiguation mechanism. We can therefore accept running resolution independently with both schemes in these cases to detect and report changes. Having a source position for the warning messages requires passing the tree source position to resolveOverloaded from the Typer. It could previously be avoided this since any potential error in overloading could be determined from its result. Clearly, this cannot be done for the new warnings, although I am open to an alternative design.
Note that in tests/neg/multiparamlist-overload-3.7 Test2 we now get an error in both Parts as intended. But they are, however, different ones: Part1 is a TypeMismatch Error, whereas Part2 is a NoMatchingOverload Error. This is due to the fact that `resolveOverloaded` will first find the candidate alternatives by considering only the 1st parameter list and commit early if there is a single one, e.g. Test2.Part1. If not, we recursively continue with the found alternatives and recompute the candidate alternatives based on the 2nd parameter list, which may rule out all of them, and hence lead to a different message.
With the warnings about the changes enabled in 3.7 and 3.8-migration
used for the two trials: without and with implicits and conversions and analogously parametrize `narrowByNextParamClause`
before starting to narrow most specific
It will need to be at the top level with the new proposal anyway
`resolvedMapped` applies `resolveOverloaded(resolve)`, _not_ `resolve` directly. This benefits from the insertion of implicit parameters, apply methods, etc. But there are still some adaptations, e.g. auto-tupling, that are not performed at this stage. In those cases, it is possible that we find that no alternatives are applicable. So we fallback to the `alts` we had before considering the next parameter clause. Resolution will succeed (only) if narrowMostSpecific finds an unambiguous alternative by considering (only) the prior argument lists, after which adaptation can be performed. See tests/run/tupled-function-extension-method.scala for an example. We only do this for resolveCandidates and not resolveOverloaded2, because in the latter causes some cases where there are indeed no good alternatives to be reported as ambiguous instead, which is unideal for error messages.
For cases which are now detected as unambiguous!
|
@WojciechMazur can we run the open community build again? It would be good to have a comparison between:
|
|
I've scheduled the builds, will let you know with the results |
|
Total execution times: There's 1716 projects in the build. The big difference between 3.6 and other is caused by timeout (6h) in one project https://github.com/rolang/google-rest-api-codegen build logs, normal execution in other builds takes ~2m There are in total 43 warnings related to |
The new Scheme, from 3.7:
This is an alternative of #20054. But the changes are much more involved, so it is not feasible to report warnings for changes in resolution without running the entire process twice. For that reason, this PR only do so in the migration version preceding the one with the changes. Since that does not entail doing a full minor with warnings, I have moved back the changes to apply in
3.7. The hope is that, even though this PR has more significant changes, it should match more with the intended prioritization. We can always decide to push back to 3.8 again depending on the results with the community build.Fixes #20053