From 528d0f029ab17dfbc9bac80b3bb33d60220c467e Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 9 Oct 2024 01:54:09 +0200 Subject: [PATCH 1/3] Make overload pruning based on result types less aggressive `adaptByResult` was introduced in 2015 in 54835b6fb19ab0758c7503fb6f0e990ee4c25491 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. --- .../src/dotty/tools/dotc/typer/Applications.scala | 10 +++++++--- tests/pos/i21410.scala | 12 ++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 tests/pos/i21410.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 96590cb84544..08c738cd7808 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -2118,16 +2118,20 @@ trait Applications extends Compatibility { def resolveOverloaded(alts: List[TermRef], pt: Type)(using Context): List[TermRef] = record("resolveOverloaded") - /** Is `alt` a method or polytype whose result type after the first value parameter + /** Is `alt` a method or polytype whose approximated result type after the first value parameter * section conforms to the expected type `resultType`? If `resultType` * is a `IgnoredProto`, pick the underlying type instead. + * + * Using approximated result types is necessary to avoid false negatives + * due to incomplete type inference such as in tests/pos/i21410.scala. */ def resultConforms(altSym: Symbol, altType: Type, resultType: Type)(using Context): Boolean = resultType.revealIgnored match { case resultType: ValueType => altType.widen match { - case tp: PolyType => resultConforms(altSym, instantiateWithTypeVars(tp), resultType) - case tp: MethodType => constrainResult(altSym, tp.resultType, resultType) + case tp: PolyType => resultConforms(altSym, tp.resultType, resultType) + case tp: MethodType => + constrainResult(altSym, wildApprox(tp.resultType), resultType) case _ => true } case _ => true diff --git a/tests/pos/i21410.scala b/tests/pos/i21410.scala new file mode 100644 index 000000000000..c3ba3ea862bc --- /dev/null +++ b/tests/pos/i21410.scala @@ -0,0 +1,12 @@ +class A +object Test: + type F[X] <: Any = X match + case A => Int + + def foo[T](x: String): T = ??? + def foo[U](x: U): F[U] = ??? + + val x1 = foo(A()) + val y: Int = x1 + + val x2: Int = foo(A()) // error From f7259cf77e053b00068bd5dca124e9a3013489dd Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 10 Oct 2024 19:41:58 +0200 Subject: [PATCH 2/3] Make `Applications#resolveMapped` preserve annotations 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 --- .../src/dotty/tools/dotc/typer/Applications.scala | 1 + tests/pos/i21410c.scala | 11 +++++++++++ 2 files changed, 12 insertions(+) create mode 100644 tests/pos/i21410c.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 08c738cd7808..93dddd5a3a1c 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -2503,6 +2503,7 @@ trait Applications extends Compatibility { if t.exists && alt.symbol.exists then val (trimmed, skipped) = trimParamss(t.stripPoly, alt.symbol.rawParamss) val mappedSym = alt.symbol.asTerm.copy(info = t) + mappedSym.annotations = alt.symbol.annotations mappedSym.rawParamss = trimmed val (pre, totalSkipped) = mappedAltInfo(alt.symbol) match case Some((pre, prevSkipped)) => diff --git a/tests/pos/i21410c.scala b/tests/pos/i21410c.scala new file mode 100644 index 000000000000..21f69fec20fa --- /dev/null +++ b/tests/pos/i21410c.scala @@ -0,0 +1,11 @@ +class AppliedPIso[A, B]() +case class User(age: Int) + +object Test: + extension [From, To](from: From) + def focus(): AppliedPIso[From, From] = ??? + transparent inline def focus(inline lambda: (From => To)): Any = ??? + + + val u = User(1) + val ap: AppliedPIso[User, User] = u.focus(_.age) // error From 32ac2e62f9b18aecf0064236a7ce6c60a1bbf579 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Sat, 12 Oct 2024 10:41:03 +0200 Subject: [PATCH 3/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. --- .../src/dotty/tools/dotc/typer/Applications.scala | 13 ++++++++++--- tests/pos/i21410b.scala | 10 ++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 tests/pos/i21410b.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 93dddd5a3a1c..3e29aa5ee28a 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -2122,8 +2122,8 @@ trait Applications extends Compatibility { * section conforms to the expected type `resultType`? If `resultType` * is a `IgnoredProto`, pick the underlying type instead. * - * Using approximated result types is necessary to avoid false negatives - * due to incomplete type inference such as in tests/pos/i21410.scala. + * Using an approximated result type is necessary to avoid false negatives + * due to incomplete type inference such as in tests/pos/i21410.scala and tests/pos/i21410b.scala. */ def resultConforms(altSym: Symbol, altType: Type, resultType: Type)(using Context): Boolean = resultType.revealIgnored match { @@ -2131,7 +2131,14 @@ trait Applications extends Compatibility { altType.widen match { case tp: PolyType => resultConforms(altSym, tp.resultType, resultType) case tp: MethodType => - constrainResult(altSym, wildApprox(tp.resultType), resultType) + val wildRes = wildApprox(tp.resultType) + + class ResultApprox extends AvoidWildcardsMap: + // Avoid false negatives by approximating to a lower bound + variance = -1 + + val approx = ResultApprox()(wildRes) + constrainResult(altSym, approx, resultType) case _ => true } case _ => true diff --git a/tests/pos/i21410b.scala b/tests/pos/i21410b.scala new file mode 100644 index 000000000000..a17ad59bc59e --- /dev/null +++ b/tests/pos/i21410b.scala @@ -0,0 +1,10 @@ +object Test: + def foo[T](x: Option[T]): T = ??? + def foo[T <: Tuple](x: T): Tuple.Map[T, List] = ??? + + val tup: (Int, String) = (1, "") + + val x = foo(tup) + val y: (List[Int], List[String]) = x + + val x2: (List[Int], List[String]) = foo(tup) // error