Merge pull request #910 from adriaanm/redo-847-ticket-6028

SI-6028 Avoid needless symbol renaming in lambdalift

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