Fixed bug in the disambiguation of f(foo='bar')...

Fixed bug in the disambiguation of f(foo='bar') style method calls in
the presence of overloading, parameterization, and by-name arguments.
Took the opportunity to clean things up a little bit. Closes SI-4592,
review by rytz.

7 files changed, 101 insertions, 53 deletions

diff --git a/src/compiler/scala/reflect/internal/AnnotationInfos.scala b/src/compiler/scala/reflect/internal/AnnotationInfos.scala
index 62b27c2daf..dcabdf0a27 100644
--- a/src/compiler/scala/reflect/internal/AnnotationInfos.scala
+++ b/src/compiler/scala/reflect/internal/AnnotationInfos.scala
@@ -118,17 +118,21 @@ trait AnnotationInfos extends api.AnnotationInfos { self: SymbolTable =>
       AnnotationInfo(atp, args.map(subs(_)), assocs).setPos(pos)
     }
 
-    // !!! when annotation arguments are not literal strings, but any sort of
-    // assembly of strings, there is a fair chance they will turn up here not as
+    def stringArg(index: Int) = constantAtIndex(index) map (_.stringValue)
+    def intArg(index: Int)    = constantAtIndex(index) map (_.intValue)
+    def symbolArg(index: Int) = argAtIndex(index) collect {
+      case Apply(fun, Literal(str) :: Nil) if fun.symbol == definitions.Symbol_apply =>
+        newTermName(str.stringValue)
+    }
+
+    // !!! when annotation arguments are not literals, but any sort of
+    // expression, there is a fair chance they will turn up here not as
     // Literal(const) but some arbitrary AST.
-    def stringArg(index: Int): Option[String] = if(args.size > index) Some(args(index) match {
-      case Literal(const) => const.stringValue
-      case x              => x.toString // should not be necessary, but better than silently ignoring an issue
-    }) else None
-
-    def intArg(index: Int): Option[Int] = if(args.size > index) Some(args(index)) collect {
-      case Literal(Constant(x: Int)) => x
-    } else None
+    def constantAtIndex(index: Int): Option[Constant] =
+      argAtIndex(index) collect { case Literal(x) => x }
+
+    def argAtIndex(index: Int): Option[Tree] =
+      if (index < args.size) Some(args(index)) else None
   }
 
   object AnnotationInfo extends AnnotationInfoExtractor
diff --git a/src/compiler/scala/reflect/internal/Symbols.scala b/src/compiler/scala/reflect/internal/Symbols.scala
index 3e80f1cdee..4789d69c69 100644
--- a/src/compiler/scala/reflect/internal/Symbols.scala
+++ b/src/compiler/scala/reflect/internal/Symbols.scala
@@ -437,6 +437,8 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     def hasBridgeAnnotation = hasAnnotation(BridgeClass)
     def deprecationMessage  = getAnnotation(DeprecatedAttr) flatMap (_ stringArg 0)
     def deprecationVersion  = getAnnotation(DeprecatedAttr) flatMap (_ stringArg 1)
+    def deprecatedParamName = getAnnotation(DeprecatedNameAttr) flatMap (_ symbolArg 0)
+
     // !!! when annotation arguments are not literal strings, but any sort of
     // assembly of strings, there is a fair chance they will turn up here not as
     // Literal(const) but some arbitrary AST.  However nothing in the compiler
diff --git a/src/compiler/scala/tools/nsc/typechecker/Infer.scala b/src/compiler/scala/tools/nsc/typechecker/Infer.scala
index 7305cef34f..495594da5e 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Infer.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Infer.scala
@@ -755,7 +755,8 @@ trait Infer {
       val argtpes1 = argtpes map {
         case NamedType(name, tp) => // a named argument
           var res = tp
-          val pos = params.indexWhere(p => (p.name == name || deprecatedName(p) == Some(name)) && !p.isSynthetic)
+          val pos = params.indexWhere(p => paramMatchesName(p, name) && !p.isSynthetic)
+
           if (pos == -1) {
             if (positionalAllowed) { // treat assignment as positional argument
               argPos(index) = index
@@ -1634,6 +1635,54 @@ trait Infer {
       }
     }
 
+    @inline private def wrapTypeError(expr: => Boolean): Boolean =
+      try expr
+      catch { case _: TypeError => false }
+
+    // Checks against the name of the parameter and also any @deprecatedName.
+    private def paramMatchesName(param: Symbol, name: Name) =
+      param.name == name || param.deprecatedParamName.exists(_ == name)
+
+    // Check the first parameter list the same way.
+    private def methodMatchesName(method: Symbol, name: Name) = method.paramss match {
+      case ps :: _  => ps exists (p => paramMatchesName(p, name))
+      case _        => false
+    }
+
+    private def resolveOverloadedMethod(argtpes: List[Type], eligible: List[Symbol]) = {
+      // If there are any foo=bar style arguments, and any of the overloaded
+      // methods has a parameter named `foo`, then only those methods are considered.
+      val namesOfArgs = argtpes collect { case NamedType(name, _) => name }
+      val namesMatch = (
+        if (namesOfArgs.isEmpty) Nil
+        else eligible filter { m =>
+          namesOfArgs forall { name =>
+            methodMatchesName(m, name)
+          }
+        }
+      )
+
+      if (namesMatch.nonEmpty) namesMatch
+      else if (eligible.isEmpty || eligible.tail.isEmpty) eligible
+      else eligible filter { alt =>
+        // for functional values, the `apply` method might be overloaded
+        val mtypes = followApply(alt.tpe) match {
+          case OverloadedType(_, alts) => alts map (_.tpe)
+          case t                       => List(t)
+        }
+        // Drop those that use a default; keep those that use vararg/tupling conversion.
+        mtypes exists (t =>
+          !t.typeSymbol.hasDefaultFlag && {
+            compareLengths(t.params, argtpes) < 0 ||  // tupling (*)
+            hasExactlyNumParams(t, argtpes.length)    // same nb or vararg
+          }
+        )
+        // (*) more arguments than parameters, but still applicable: tupling conversion works.
+        //     todo: should not return "false" when paramTypes = (Unit) no argument is given
+        //     (tupling would work)
+      }
+    }
+
     /** Assign <code>tree</code> the type of an alternative which is applicable
      *  to <code>argtpes</code>, and whose result type is compatible with `pt`.
      *  If several applicable alternatives exist, drop the alternatives which use
@@ -1656,40 +1705,21 @@ trait Infer {
           debuglog("infer method alt "+ tree.symbol +" with alternatives "+
                 (alts map pre.memberType) +", argtpes = "+ argtpes +", pt = "+ pt)
 
-          var allApplicable = alts filter (alt =>
-            // TODO: this will need to be re-written once we substitute throwing exceptions
-            // with generating error trees. We wrap this applicability in try/catch because of #4457.
-            try {isApplicable(undetparams, followApply(pre.memberType(alt)), argtpes, pt)} catch {case _: TypeError => false})
-
-          //log("applicable: "+ (allApplicable map pre.memberType))
-
-          if (varArgsOnly)
-            allApplicable = allApplicable filter (alt => isVarArgsList(alt.tpe.params))
-
-          // if there are multiple, drop those that use a default
-          // (keep those that use vararg / tupling conversion)
-          val applicable =
-            if (allApplicable.lengthCompare(1) <= 0) allApplicable
-            else allApplicable filter (alt => {
-              val mtypes = followApply(alt.tpe) match {
-                // for functional values, the `apply` method might be overloaded
-                case OverloadedType(_, alts) => alts map (_.tpe)
-                case t                       => List(t)
-              }
-              mtypes exists (t =>
-                compareLengths(t.params, argtpes) < 0 ||  // tupling (*)
-                hasExactlyNumParams(t, argtpes.length)    // same nb or vararg
-              )
-              // (*) more arguments than parameters, but still applicable: tuplig conversion works.
-              //     todo: should not return "false" when paramTypes = (Unit) no argument is given
-              //     (tupling would work)
-            })
+          val applicable = resolveOverloadedMethod(argtpes, {
+            alts filter { alt =>
+              // TODO: this will need to be re-written once we substitute throwing exceptions
+              // with generating error trees. We wrap this applicability in try/catch because of #4457.
+              wrapTypeError(isApplicable(undetparams, followApply(pre.memberType(alt)), argtpes, pt)) &&
+              (!varArgsOnly || isVarArgsList(alt.tpe.params))
+            }
+          })
 
-          def improves(sym1: Symbol, sym2: Symbol) =
-//            util.trace("improve "+sym1+sym1.locationString+" on "+sym2+sym2.locationString)(
+          def improves(sym1: Symbol, sym2: Symbol) = {
+            // util.trace("improve "+sym1+sym1.locationString+" on "+sym2+sym2.locationString)
             sym2 == NoSymbol || sym2.isError || sym2.hasAnnotation(BridgeClass) ||
             isStrictlyMoreSpecific(followApply(pre.memberType(sym1)),
                                    followApply(pre.memberType(sym2)), sym1, sym2)
+          }
 
           val best = ((NoSymbol: Symbol) /: applicable) ((best, alt) =>
             if (improves(alt, best)) alt else best)
diff --git a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
index a5c5aa5320..bf7cc72fab 100644
--- a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
@@ -21,8 +21,13 @@ trait NamesDefaults { self: Analyzer =>
   val defaultParametersOfMethod =
     perRunCaches.newWeakMap[Symbol, Set[Symbol]]() withDefaultValue Set()
 
-  case class NamedApplyInfo(qual: Option[Tree], targs: List[Tree],
-                            vargss: List[List[Tree]], blockTyper: Typer)
+  case class NamedApplyInfo(
+    qual:       Option[Tree],
+    targs:      List[Tree],
+    vargss:     List[List[Tree]],
+    blockTyper: Typer
+  ) { }
+
   val noApplyInfo = NamedApplyInfo(None, Nil, Nil, null)
 
   def nameOf(arg: Tree) = arg match {
@@ -558,17 +563,11 @@ trait NamesDefaults { self: Analyzer =>
       val p = rest.head
       if (!p.isSynthetic) {
         if (p.name == name) return (i, None)
-        if (deprecatedName(p) == Some(name)) return (i, Some(p.name))
+        if (p.deprecatedParamName == Some(name)) return (i, Some(p.name))
       }
       i += 1
       rest = rest.tail
     }
     (-1, None)
   }
-
-  def deprecatedName(sym: Symbol): Option[Name] =
-    sym.getAnnotation(DeprecatedNameAttr).map(ann => (ann.args(0): @unchecked) match {
-      case Apply(fun, Literal(str) :: Nil) if (fun.symbol == Symbol_apply) =>
-        newTermName(str.stringValue)
-    })
 }
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index f305644837..7ae390a992 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1795,10 +1795,10 @@ trait Typers extends Modes with Adaptations {
       if (phase.id <= currentRun.typerPhase.id) {
         val allParams = meth.paramss.flatten
         for (p <- allParams) {
-          deprecatedName(p).foreach(n => {
-            if (allParams.exists(p1 => p1.name == n || (p != p1 && deprecatedName(p1) == Some(n))))
+          for (n <- p.deprecatedParamName) {
+            if (allParams.exists(p1 => p1.name == n || (p != p1 && p1.deprecatedParamName.exists(_ == n))))
               error(p.pos, "deprecated parameter name "+ n +" has to be distinct from any other parameter name (deprecated or not).")
-          })
+          }
         }
       }
 
diff --git a/test/files/run/bug4592.check b/test/files/run/bug4592.check
new file mode 100644
index 0000000000..e133386482
--- /dev/null
+++ b/test/files/run/bug4592.check
@@ -0,0 +1,3 @@
+3.14
+3.14
+3.14
diff --git a/test/files/run/bug4592.scala b/test/files/run/bug4592.scala
new file mode 100644
index 0000000000..d1666d84d7
--- /dev/null
+++ b/test/files/run/bug4592.scala
@@ -0,0 +1,10 @@
+object Test {
+  def repeat[T](count: Int = 1, x: Boolean = true)(thunk: => T) : T = (0 until count).map(_ => thunk).last
+  def repeat[T](thunk: => T) : T = repeat()(thunk)
+
+  def main(args: Array[String]): Unit = {
+    println(repeat(3.14))
+    println(repeat(count=5)(3.14))
+    println(repeat(count=5,x=false)(3.14))
+  }
+}