Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make overload pruning based on result types less aggressive #21744

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make overload pruning based on result types less aggressive #21744

wants to merge 3 commits into from

Commits on Oct 22, 2024

  1. Make overload pruning based on result types less aggressive 

    `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.
    @smarter
    smarter committed Oct 22, 2024
    Configuration menu
    Copy the full SHA
    fe6d690 View commit details
    Browse the repository at this point in the history

  2. Fix isInlineable check to work during overloading resolution 

    Overloading may create temporary symbols via `Applications#resolveMapped`, these
symbols do not carry the annotations from the original symbols which means the
`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.
    @smarter
    smarter committed Oct 22, 2024
    Configuration menu
    Copy the full SHA
    fbd55a4 View commit details
    Browse the repository at this point in the history

  3. Avoid even more false negatives in overload pruning 

    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 committed Oct 22, 2024
    Configuration menu
    Copy the full SHA
    2b5cf1e View commit details
    Browse the repository at this point in the history