SI-6028 Avoid needless symbol renaming in lambdalift.

Preserve names of all referenced free vars. Only the proxy symbols
have the fresh names.

The resulting natural beauty is evident in the diff of t6028.check.

This subsumes the treatment in 0e170e4b that ensured named parameter
calls cannot see mangled names; pos/t6028 confirms as much.

1 files changed, 55 insertions, 30 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
index fad41ae98d..bee5aa5f4f 100644
--- a/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
+++ b/src/compiler/scala/tools/nsc/transform/LambdaLift.scala
@@ -56,6 +56,31 @@ abstract class LambdaLift extends InfoTransform {
     /** The set of symbols that need to be renamed. */
     private val renamable = newSymSet
 
+    /**
+     * The new names for free variables proxies. If we simply renamed the
+     * free variables, we would transform:
+     * {{{
+     *   def closure(x: Int) = { () => x }
+     * }}}
+     *
+     * To:
+     * {{{
+     *   def closure(x$1: Int) = new anonFun$1(this, x$1)
+     *   class anonFun$1(outer$: Outer, x$1: Int) { def apply() => x$1 }
+     * }}}
+     *
+     * This is fatally bad for named arguments (0e170e4b), extremely impolite to tools
+     * reflecting on the method parameter names in the generated bytecode (SI-6028),
+     * and needlessly bothersome to anyone using a debugger.
+     *
+     * Instead, we transform to:
+     * {{{
+     *   def closure(x: Int) = new anonFun$1(this, x)
+     *   class anonFun$1(outer$: Outer, x$1: Int) { def apply() => x$1 }
+     * }}}
+     */
+    private val proxyNames       = mutable.HashMap[Symbol, Name]()
+
     // (trait, name) -> owner
     private val localTraits      = mutable.HashMap[(Symbol, Name), Symbol]()
     // (owner, name) -> implClass
@@ -117,15 +142,6 @@ abstract class LambdaLift extends InfoTransform {
           if (!ss(sym)) {
             ss addEntry sym
             renamable addEntry sym
-            beforePickler {
-              // The param symbol in the MethodType should not be renamed, only the symbol in scope. This way,
-              // parameter names for named arguments are not changed. Example: without cloning the MethodType,
-              //     def closure(x: Int) = { () => x }
-              // would have the signature
-              //     closure: (x$1: Int)() => Int
-              if (sym.isParameter && sym.owner.info.paramss.exists(_ contains sym))
-                sym.owner modifyInfo (_ cloneInfo sym.owner)
-            }
             changedFreeVars = true
             debuglog("" + sym + " is free in " + enclosure);
             if (sym.isVariable) sym setFlag CAPTURED
@@ -215,24 +231,26 @@ abstract class LambdaLift extends InfoTransform {
 
       def renameSym(sym: Symbol) {
         val originalName = sym.name
+        sym setName newName(sym)
+        debuglog("renaming in %s: %s => %s".format(sym.owner.fullLocationString, originalName, sym.name))
+      }
+
+      def newName(sym: Symbol): Name = {
+        val originalName = sym.name
         def freshen(prefix: String): Name =
           if (originalName.isTypeName) unit.freshTypeName(prefix)
           else unit.freshTermName(prefix)
 
-        val newName: Name = (
-          if (sym.isAnonymousFunction && sym.owner.isMethod) {
-            freshen(sym.name + nme.NAME_JOIN_STRING + sym.owner.name + nme.NAME_JOIN_STRING)
-          } else {
-            // SI-5652 If the lifted symbol is accessed from an inner class, it will be made public. (where?)
-            //         Generating a a unique name, mangled with the enclosing class name, avoids a VerifyError
-            //         in the case that a sub-class happens to lifts out a method with the *same* name.
-            val name = freshen(sym.name + nme.NAME_JOIN_STRING)
-            if (originalName.isTermName && !sym.enclClass.isImplClass && calledFromInner(sym)) nme.expandedName(name, sym.enclClass)
-            else name
-          }
-        )
-        sym setName newName
-        debuglog("renaming in %s: %s => %s".format(sym.owner.fullLocationString, originalName, sym.name))
+        if (sym.isAnonymousFunction && sym.owner.isMethod) {
+          freshen(sym.name + nme.NAME_JOIN_STRING + sym.owner.name + nme.NAME_JOIN_STRING)
+        } else {
+          // SI-5652 If the lifted symbol is accessed from an inner class, it will be made public. (where?)
+          //         Generating a a unique name, mangled with the enclosing class name, avoids a VerifyError
+          //         in the case that a sub-class happens to lifts out a method with the *same* name.
+          val name = freshen(sym.name + nme.NAME_JOIN_STRING)
+          if (originalName.isTermName && !sym.enclClass.isImplClass && calledFromInner(sym)) nme.expandedName(name, sym.enclClass)
+          else name
+        }
       }
 
       /** Rename a trait's interface and implementation class in coordinated fashion.
@@ -245,6 +263,8 @@ abstract class LambdaLift extends InfoTransform {
         debuglog("renaming impl class in step with %s: %s => %s".format(traitSym, originalImplName, implSym.name))
       }
 
+      val allFree: Set[Symbol] = free.values.flatMap(_.iterator).toSet
+
       for (sym <- renamable) {
         // If we renamed a trait from Foo to Foo$1, we must rename the implementation
         // class from Foo$class to Foo$1$class.  (Without special consideration it would
@@ -252,7 +272,9 @@ abstract class LambdaLift extends InfoTransform {
         // under us, and there's no reliable link between trait symbol and impl symbol,
         // we have maps from ((trait, name)) -> owner and ((owner, name)) -> impl.
         localTraits remove ((sym, sym.name)) match {
-          case None        => renameSym(sym)
+          case None        =>
+            if (allFree(sym)) proxyNames(sym) = newName(sym)
+            else renameSym(sym)
           case Some(owner) =>
             localImplClasses remove ((owner, sym.name)) match {
               case Some(implSym)  => renameTrait(sym, implSym)
@@ -267,7 +289,8 @@ abstract class LambdaLift extends InfoTransform {
           debuglog("free var proxy: %s, %s".format(owner.fullLocationString, freeValues.toList.mkString(", ")))
           proxies(owner) =
             for (fv <- freeValues.toList) yield {
-              val proxy = owner.newValue(fv.name, owner.pos, newFlags) setInfo fv.info
+              val proxyName = proxyNames.getOrElse(fv, fv.name)
+              val proxy = owner.newValue(proxyName, owner.pos, newFlags) setInfo fv.info
               if (owner.isClass) owner.info.decls enter proxy
               proxy
             }
@@ -280,9 +303,9 @@ abstract class LambdaLift extends InfoTransform {
         if (enclosure eq NoSymbol) throw new IllegalArgumentException("Could not find proxy for "+ sym.defString +" in "+ sym.ownerChain +" (currentOwner= "+ currentOwner +" )")
         debuglog("searching for " + sym + "(" + sym.owner + ") in " + enclosure + " " + enclosure.logicallyEnclosingMember)
 
-        val ps = (proxies get enclosure.logicallyEnclosingMember).toList.flatten filter (_.name == sym.name)
-        if (ps.isEmpty) searchIn(enclosure.skipConstructor.owner)
-        else ps.head
+        val proxyName = proxyNames.getOrElse(sym, sym.name)
+        val ps = (proxies get enclosure.logicallyEnclosingMember).toList.flatten find (_.name == proxyName)
+        ps getOrElse searchIn(enclosure.skipConstructor.owner)
       }
       debuglog("proxy %s from %s has logical enclosure %s".format(
         sym.debugLocationString,
@@ -501,8 +524,10 @@ abstract class LambdaLift extends InfoTransform {
     }
 
     override def transformUnit(unit: CompilationUnit) {
-      computeFreeVars
-      afterOwnPhase(super.transformUnit(unit))
+      computeFreeVars()
+      afterOwnPhase {
+        super.transformUnit(unit)
+      }
       assert(liftedDefs.isEmpty, liftedDefs.keys mkString ", ")
     }
   } // class LambdaLifter