Skip to content

Make overload pruning based on result types less aggressive #21744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

smarter
Copy link
Member

@smarter smarter commented Oct 9, 2024

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 #21410.

@dwijnand
Copy link
Member

dwijnand commented Oct 9, 2024

Want to add your Tuple.Map example, as another case?

@smarter
Copy link
Member Author

smarter commented Oct 10, 2024

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

@smarter smarter marked this pull request as ready for review October 23, 2024 09:13
@smarter smarter requested a review from dwijnand October 23, 2024 09:13
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@smarter smarter merged commit 93af7b8 into scala:main Feb 24, 2025
29 checks passed
@smarter smarter deleted the i21410-2 branch February 24, 2025 14:18
@tgodzik
Copy link
Contributor

tgodzik commented Mar 3, 2025

Is it possible to port this to LTS, any opinions? @smarter @dwijnand

@WojciechMazur
Copy link
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 darrenjw/scala-smfsb

https://github.com/VirtusLab/community-build3/actions/runs/13631644315/job/38100526097

Other project that might be related can be found in this report in Projects with last successful builds using Scala 3.7.0-RC1-bin-20250219-345b2da-NIGHTLY section

@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 11, 2025
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
WojciechMazur added a commit that referenced this pull request Apr 8, 2025
mbovel added a commit that referenced this pull request Apr 11, 2025
Fix #22724

We still need to instantiate `TypeParamRef`s with `TypeVar`s for
`PolyType`s when checking whether result conforms.
mbovel pushed a commit to mbovel/dotty that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overloading resolution incorrectly drops alternative with match type result when a target type is provided
4 participants