Make overload pruning based on result types less aggressive#21744
Merged
smarter merged 3 commits intoscala:mainfrom Feb 24, 2025
Merged
Make overload pruning based on result types less aggressive#21744smarter merged 3 commits intoscala:mainfrom
smarter merged 3 commits intoscala:mainfrom
Conversation
Member
|
Want to add your |
Member
Author
|
The only test failure is in monocle: [error] -- Error: /__w/scala3/scala3/community-build/community-projects/Monocle/core/shared/src/test/scala-3.x/monocle/focus/ComposedFocusTest.scala:42:62
[error] 42 | val addressLens: AppliedLens[User, Address] = elise.focus(_.address)
[error] | ^^^^^^^^^
[error] |too many arguments for method focus in trait AppliedFocusSyntax: (): monocle.AppliedIso[From, From]
[error] one error found
[error] (coreJVM / Test / compileIncremental) Compilation failed
[error] Total time: 22 s, completed Oct 10, 2024 2:52:16 AM |
dwijnand
requested changes
Oct 28, 2024
Member
dwijnand
left a comment
There was a problem hiding this comment.
Basically LGTM to merge, but 1 question about the isInlineable change.
`adaptByResult` was introduced in 2015 in 54835b6 as a last step in overloading resolution: > Take expected result type into account more often for overloading resolution > > Previously, the expected result type of a FunProto type was ignored and taken into > account only in case of ambiguities. arrayclone-new.scala shows that this is not enough. > In a case like > > val x: Array[Byte] = Array(1, 2) > > we typed 1, 2 to be Int, so overloading resulution would give the Array.apply of > type (Int, Int*)Array[Int]. But that's a dead end, since Array[Int] is not a subtype > of Array[Byte]. > > This commit proposes the following modified rule for overloading resulution: > > A method alternative is applicable if ... (as before), and if its result type > is copmpatible with the expected type of the method application. > > The commit does not pre-select alternatives based on comparing with the expected > result type. I tried that but it slowed down typechecking by a factor of at least 4. > Instead, we proceed as usual, ignoring the result type except in case of > ambiguities, but check whether the result of overloading resolution has a > compatible result type. If that's not the case, we filter all alternatives > for result type compatibility and try again. In i21410.scala this means we end up checking: F[?U] <:< Int (where ?U is unconstrained, because the check is done without looking at the argument types) The problem is that the subtype check returning false does not mean that there is no instantiation of `?U` that would make this check return true, just that type inference was not able to come up with one. This could happen for any number of reason but commonly will happen with match types since inference cannot do much with them. We cannot avoid this by taking the argument types into account, because this logic was added precisely to handle cases where the argument types mislead you because adaptation isn't taken into account. Instead, we can approximate type variables in the result type to trade false negatives for false positives which should be less problematic here. Fixes scala#21410.
Overloading may create a temporary symbol via `Applications#resolveMapped`, but before this commit, this symbol did not carry the annotations from the original symbol. This meant in particular that `isInlineable` would always return false for them. This matters because during the course of overloading resolution we might call `ProtoTypes.Compatibility#constrainResult` which special-cases transparent inline methods. Fixes a regression in Monocle introduced in the previous commit. wip
The changes two commits ago were not enough to handle i21410b.scala because we
end up checking:
Tuple.Map[WildcardType(...), List] <: (List[Int], List[String])
which fails because a match type with a wildcard argument apparently only gets
reduced when the match type case is not parameterized.
To handle this more generally we use AvoidWildcardsMap to remove wildcards from
the result type, but since we want to prevent false negatives we start with
`variance = -1` to get a lower-bound instead of an upper-bound.
dwijnand
approved these changes
Feb 24, 2025
Contributor
Contributor
|
We have a couple of project that started to fail due this change. I don't yet have a minimization, but I'll create a dedicated issue as soon as I'll have one. Here are the logs project-wise bisect of https://github.com/VirtusLab/community-build3/actions/runs/13631644315/job/38100526097 Other project that might be related can be found in this report in |
This was referenced Mar 5, 2025
Closed
noti0na1
added a commit
to dotty-staging/dotty
that referenced
this pull request
Mar 17, 2025
WojciechMazur
added a commit
to WojciechMazur/dotty
that referenced
this pull request
Apr 8, 2025
…cala#21744)" This reverts commit 93af7b8, reversing changes made to 7d79c56.
WojciechMazur
added a commit
that referenced
this pull request
Apr 8, 2025
…in 3.7.0 (#22940) This reverts #21744 in 3.7.0-RC2 as per #22713 (comment) discussion
mbovel
pushed a commit
to mbovel/dotty
that referenced
this pull request
Apr 14, 2025
tgodzik
pushed a commit
to scala/scala3-lts
that referenced
this pull request
Apr 27, 2025
[Cherry-picked 00e92b1]
tgodzik
added a commit
to scala/scala3-lts
that referenced
this pull request
Apr 28, 2025
Backport "Fix scala#22724: Revert the PolyType case in scala#21744" to 3.3 LTS
WojciechMazur
added a commit
to WojciechMazur/dotty
that referenced
this pull request
May 22, 2025
…cala#21744)" This reverts commit 93af7b8, reversing changes made to 7d79c56.
WojciechMazur
added a commit
that referenced
this pull request
May 22, 2025
WojciechMazur
added a commit
to WojciechMazur/dotty
that referenced
this pull request
Jun 9, 2025
…cala#21744)" This reverts commit 93af7b8, reversing changes made to 7d79c56.
tgodzik
pushed a commit
that referenced
this pull request
Jun 10, 2025
tgodzik
pushed a commit
to scala/scala3-lts
that referenced
this pull request
Jun 19, 2025
…cala#21744)" in main (scala#23331) Revert scala#21744 which already was reverted in 3.7.0 and 3.7.1 due to introduced bugs. Resolves scala#22713 [Cherry-picked a183140]
tgodzik
pushed a commit
to scala/scala3-lts
that referenced
this pull request
Jun 20, 2025
…cala#21744)" in main (scala#23331) Revert scala#21744 which already was reverted in 3.7.0 and 3.7.1 due to introduced bugs. Resolves scala#22713 [Cherry-picked a183140]
tgodzik
added a commit
to scala/scala3-lts
that referenced
this pull request
Jun 20, 2025
Backport "Revert "Make overload pruning based on result types less aggressive (scala#21744)" in main" to 3.3 LTS
This was referenced Jun 23, 2025
WojciechMazur
added a commit
that referenced
this pull request
Jun 23, 2025
tgodzik
pushed a commit
to scala/scala3-lts
that referenced
this pull request
Jun 30, 2025
Adds regression tests for scala#23237 to prevent reintroducing regression from scala#21744 [Cherry-picked 0a4b6ec]
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
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.
adaptByResultwas introduced in 2015 in54835b6 as a last step in overloading resolution:
In i21410.scala this means we end up checking:
The problem is that the subtype check returning false does not mean that there is no instantiation of
?Uthat would make this check return true, just that type inference was not able to come up with one. This could happen for any number of reason but commonly will happen with match types since inference cannot do much with them.We cannot avoid this by taking the argument types into account, because this logic was added precisely to handle cases where the argument types mislead you because adaptation isn't taken into account. Instead, we can approximate type variables in the result type to trade false negatives for false positives which should be less problematic here.
Fixes #21410.