Overloading: always prefer non-varargs to varargs

We recently merged scala#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:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#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`).

Author: smarter

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`.

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 <T> int foo3(T x) { return 1; }
+  public static <T> int foo3(T... x) { return 2; }
+
+  public static <T> int foo4(T x) { return 1; }
+  public static <T> 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;
+  }
+}

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
+  }
+}

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)
 }