From e0d66a2492af93730812ca6c8388bae65a320659 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Thu, 3 Sep 2020 18:41:46 +0200 Subject: [PATCH] Overloading: always prefer non-varargs to varargs We recently merged #9601 which unified our handling of `Object` coming from Java methods, an unintended consequence of that change is that some existing Java APIs can no longer be called without running into ambiguity errors, for example log4j defines two overloads for `Logger.error`: (x: String, y: Object): Unit (x: String, y: Object*): Unit previously we translated `Object` to `Any` but left `Object*` alone, now they're both treated the same way (translated to a special alias of `Object`) and so neither method ends up being more specific than the other, so `error("foo: {}, 1)` is now ambiguous. Clearly the problem lies with how we handle varargs in overloading resolution, but this has been a source of issues for years with no clear resolution: - https://github.com/scala/bug/issues/8342 - https://github.com/scala/bug/issues/4728 - https://github.com/scala/bug/issues/8344 - #6230 This PR cuts the Gordian knot by simply declaring that non-varargs methods are always more specific than varargs. This has several advantages: - It's an easy rule to remember - It matches what Java does (see https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2) - It avoids unnecessary wrapping of arguments The downside is that it doesn't match what Scala 2 does, but our current behavior isn't a perfect match either (also it seems that Scala 2 handles Java varargs and Scala varargs differently in overloading resolution which is another source of complexity best avoided, see `tests/run/overload_repeated`). Fixes #9688, supercedes #6230. --- .../dotty/tools/dotc/typer/Applications.scala | 72 ++++++++++--------- tests/run/overload_repeated/A_1.java | 23 ++++++ tests/run/overload_repeated/B_2.scala | 31 ++++++++ tests/run/t8197.scala | 6 +- 4 files changed, 96 insertions(+), 36 deletions(-) create mode 100644 tests/run/overload_repeated/A_1.java create mode 100644 tests/run/overload_repeated/B_2.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index e95e6b5b274f..28ec45d1c3d3 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1452,48 +1452,54 @@ trait Applications extends Compatibility { /** Is alternative `alt1` with type `tp1` as specific as alternative * `alt2` with type `tp2` ? * - * 1. A method `alt1` of type (p1: T1, ..., pn: Tn)U is as specific as `alt2` + * 1. A non-varargs method is always as specific as a varargs method. + * 2. A varargs method is never as specific as a non-varargs method. + * 3. A method `alt1` of type (p1: T1, ..., pn: Tn)U is as specific as `alt2` * if `alt2` is applicable to arguments (p1, ..., pn) of types T1,...,Tn * or if `alt1` is nullary. - * 2. A polymorphic member of type [a1 >: L1 <: U1, ..., an >: Ln <: Un]T is as + * 4. A polymorphic member of type [a1 >: L1 <: U1, ..., an >: Ln <: Un]T is as * specific as `alt2` of type `tp2` if T is as specific as `tp2` under the * assumption that for i = 1,...,n each ai is an abstract type name bounded * from below by Li and from above by Ui. - * 3. A member of any other type `tp1` is: + * 5. A member of any other type `tp1` is: * a. always as specific as a method or a polymorphic method. * b. as specific as a member of any other type `tp2` if `tp1` is compatible * with `tp2`. */ - def isAsSpecific(alt1: TermRef, tp1: Type, alt2: TermRef, tp2: Type): Boolean = trace(i"isAsSpecific $tp1 $tp2", overload) { tp1 match { - case tp1: MethodType => // (1) - val formals1 = - if (tp1.isVarArgsMethod && tp2.isVarArgsMethod) tp1.paramInfos.map(_.repeatedToSingle) - else tp1.paramInfos - isApplicableMethodRef(alt2, formals1, WildcardType) || - tp1.paramInfos.isEmpty && tp2.isInstanceOf[LambdaType] - case tp1: PolyType => // (2) - inContext(ctx.fresh.setExploreTyperState()) { - // Fully define the PolyType parameters so that the infos of the - // tparams created below never contain TypeRefs whose underling types - // contain uninstantiated TypeVars, this could lead to cycles in - // `isSubType` as a TypeVar might get constrained by a TypeRef it's - // part of. - val tp1Params = tp1.newLikeThis(tp1.paramNames, tp1.paramInfos, defn.AnyType) - fullyDefinedType(tp1Params, "type parameters of alternative", alt1.symbol.span) - - val tparams = newTypeParams(alt1.symbol, tp1.paramNames, EmptyFlags, tp1.instantiateParamInfos(_)) - isAsSpecific(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2) - } - case _ => // (3) - tp2 match { - case tp2: MethodType => true // (3a) - case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a) - case tp2: PolyType => // (3b) - explore(isAsSpecificValueType(tp1, constrained(tp2).resultType)) - case _ => // (3b) - isAsSpecificValueType(tp1, tp2) - } - }} + def isAsSpecific(alt1: TermRef, tp1: Type, alt2: TermRef, tp2: Type): Boolean = trace(i"isAsSpecific $tp1 $tp2", overload) { + val tp1IsVarArgs = tp1.isVarArgsMethod + val tp2IsVarArgs = tp2.isVarArgsMethod + if !tp1IsVarArgs && tp2IsVarArgs then true // (1) + else if tp1IsVarArgs && !tp2IsVarArgs then false // (2) + else tp1 match + case tp1: MethodType => // (3) + val formals1 = + if tp1IsVarArgs && tp2IsVarArgs then tp1.paramInfos.map(_.repeatedToSingle) + else tp1.paramInfos + isApplicableMethodRef(alt2, formals1, WildcardType) || + tp1.paramInfos.isEmpty && tp2.isInstanceOf[LambdaType] + case tp1: PolyType => // (4) + inContext(ctx.fresh.setExploreTyperState()) { + // Fully define the PolyType parameters so that the infos of the + // tparams created below never contain TypeRefs whose underling types + // contain uninstantiated TypeVars, this could lead to cycles in + // `isSubType` as a TypeVar might get constrained by a TypeRef it's + // part of. + val tp1Params = tp1.newLikeThis(tp1.paramNames, tp1.paramInfos, defn.AnyType) + fullyDefinedType(tp1Params, "type parameters of alternative", alt1.symbol.span) + + val tparams = newTypeParams(alt1.symbol, tp1.paramNames, EmptyFlags, tp1.instantiateParamInfos(_)) + isAsSpecific(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2) + } + case _ => // (5) + tp2 match + case tp2: MethodType => true // (5a) + case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (5a) + case tp2: PolyType => // (5b) + explore(isAsSpecificValueType(tp1, constrained(tp2).resultType)) + case _ => // (5b) + isAsSpecificValueType(tp1, tp2) + } /** Test whether value type `tp1` is as specific as value type `tp2`. * Let's abbreviate this to `tp1 <:s tp2`. diff --git a/tests/run/overload_repeated/A_1.java b/tests/run/overload_repeated/A_1.java new file mode 100644 index 000000000000..c891eaff6920 --- /dev/null +++ b/tests/run/overload_repeated/A_1.java @@ -0,0 +1,23 @@ +class A_1 { + public static int foo1(String x) { return 1; } + public static int foo1(Object... x) { return 2; } + + public static int foo2(Object x) { return 1; } + public static int foo2(Object... x) { return 2; } + + public static int foo3(T x) { return 1; } + public static int foo3(T... x) { return 2; } + + public static int foo4(T x) { return 1; } + public static int foo4(T x, T... y) { return 2; } + + public static boolean check() { + // Java prefers non-varargs to varargs: + // https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2 + return + foo1("") == 1 && + foo2("") == 1 && + foo3("") == 1 && + foo4("") == 1; + } +} diff --git a/tests/run/overload_repeated/B_2.scala b/tests/run/overload_repeated/B_2.scala new file mode 100644 index 000000000000..6ee606943be0 --- /dev/null +++ b/tests/run/overload_repeated/B_2.scala @@ -0,0 +1,31 @@ +object Test { + def bar1(x: Any) = 1 + def bar1(x: String*) = 2 + + def bar2(x: Any) = 1 + def bar2(x: Any*) = 2 + + def bar3[T](x: T): Int = 1 + def bar3[T](x: T*): Int = 2 + + def bar4[T](x: T): Int = 1 + def bar4[T](x: T, xs: T*): Int = 2 + + def main(args: Array[String]): Unit = { + // Java, Scala 2 and Dotty all agree that Java varargs are + // less specific than Java non-varargs: + assert(A_1.check()) + assert(A_1.foo1("") == 1) // Same as in Java and Scala 2 + assert(A_1.foo2("") == 1) // Same as in Java and Scala 2 + assert(A_1.foo3("") == 1) // Same as in Java and Scala 2 + assert(A_1.foo4("") == 1) // Same as in Java and Scala 2 + + // ... but Scala 2 seems to treat Scala varargs specially + // (or maybe it's Object coming from Java which is treated + // specially), in Dotty this doesn't make a difference: + assert(bar1("") == 1) // ambiguous in Scala 2 + assert(bar2("") == 1) // same in Scala 2 + assert(bar3("") == 1) // same in Scala 2 + assert(bar4("") == 1) // same in Scala 2 + } +} diff --git a/tests/run/t8197.scala b/tests/run/t8197.scala index 27212cbbd3c2..d3e37644d952 100644 --- a/tests/run/t8197.scala +++ b/tests/run/t8197.scala @@ -10,7 +10,7 @@ class Foo(val x: A = null) { object Test extends App { // both constructors of `Foo` are applicable. Overloading resolution - // will eliminate the alternative that uses a default argument, therefore - // the vararg constructor is chosen. - assert((new Foo).x != null) + // used to select the varargs alternative, but we now always + // prefer non-varargs to varargs overloads. + assert((new Foo).x == null) }