Merge pull request #5099 from retronym/ticket/9390

SI-9390 Emit local defs that don't capture this as static

17 files changed, 192 insertions, 33 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Constructors.scala b/src/compiler/scala/tools/nsc/transform/Constructors.scala
index 636fb08b89..971a55f763 100644
--- a/src/compiler/scala/tools/nsc/transform/Constructors.scala
+++ b/src/compiler/scala/tools/nsc/transform/Constructors.scala
@@ -715,6 +715,9 @@ abstract class Constructors extends Statics with Transform with ast.TreeDSL {
           primaryConstrBody.expr)
       })
 
+      if (omittableAccessor.exists(_.isOuterField) && !constructorStats.exists(_.exists { case i: Ident if i.symbol.isOuterParam => true; case _ => false}))
+        primaryConstructor.symbol.updateAttachment(OuterArgCanBeElided)
+
       val constructors = primaryConstructor :: auxConstructors
 
       // Unlink all fields that can be dropped from class scope
diff --git a/src/compiler/scala/tools/nsc/transform/Delambdafy.scala b/src/compiler/scala/tools/nsc/transform/Delambdafy.scala
index 1dfc1330c6..2dd8def53e 100644
--- a/src/compiler/scala/tools/nsc/transform/Delambdafy.scala
+++ b/src/compiler/scala/tools/nsc/transform/Delambdafy.scala
@@ -225,6 +225,7 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
       }
     }
 
+
     private def transformFunction(originalFunction: Function): Tree = {
       val target = targetMethod(originalFunction)
       assert(target.hasFlag(Flags.STATIC))
@@ -272,10 +273,21 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
           val Template(parents, self, body) = super.transform(deriveTemplate(tree)(_.mapConserve(pretransform)))
           Template(parents, self, body ++ boxingBridgeMethods)
         } finally boxingBridgeMethods.clear()
+      case dd: DefDef if dd.symbol.isLiftedMethod && !dd.symbol.isDelambdafyTarget =>
+        // SI-9390 emit lifted methods that don't require a `this` reference as STATIC
+        // delambdafy targets are excluded as they are made static by `transformFunction`.
+        if (!dd.symbol.hasFlag(STATIC) && !methodReferencesThis(dd.symbol))
+          dd.symbol.setFlag(STATIC)
+        super.transform(tree)
+      case Apply(fun, outer :: rest) if shouldElideOuterArg(fun.symbol, outer) =>
+        val nullOuter = gen.mkZero(outer.tpe)
+        treeCopy.Apply(tree, transform(fun), nullOuter :: transformTrees(rest))
       case _ => super.transform(tree)
     }
   } // DelambdafyTransformer
 
+  private def shouldElideOuterArg(fun: Symbol, outerArg: Tree): Boolean =
+    fun.isConstructor && treeInfo.isQualifierSafeToElide(outerArg) && fun.hasAttachment[OuterArgCanBeElided.type]
 
   // A traverser that finds symbols used but not defined in the given Tree
   // TODO freeVarTraverser in LambdaLift does a very similar task. With some
@@ -326,19 +338,28 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
 
     // recursively find methods that refer to 'this' directly or indirectly via references to other methods
     // for each method found add it to the referrers set
-    private def refersToThis(symbol: Symbol): Boolean =
-      (thisReferringMethods contains symbol) ||
-        (liftedMethodReferences(symbol) exists refersToThis) && {
-          // add it early to memoize
-          debuglog(s"$symbol indirectly refers to 'this'")
-          thisReferringMethods += symbol
-          true
+    private def refersToThis(symbol: Symbol): Boolean = {
+      var seen = mutable.Set[Symbol]()
+      def loop(symbol: Symbol): Boolean = {
+        if (seen(symbol)) false
+        else {
+          seen += symbol
+          (thisReferringMethods contains symbol) ||
+            (liftedMethodReferences(symbol) exists loop) && {
+              // add it early to memoize
+              debuglog(s"$symbol indirectly refers to 'this'")
+              thisReferringMethods += symbol
+              true
+            }
         }
+      }
+      loop(symbol)
+    }
 
     private var currentMethod: Symbol = NoSymbol
 
     override def traverse(tree: Tree) = tree match {
-      case DefDef(_, _, _, _, _, _) if tree.symbol.isDelambdafyTarget =>
+      case DefDef(_, _, _, _, _, _) if tree.symbol.isDelambdafyTarget || tree.symbol.isLiftedMethod =>
         // we don't expect defs within defs. At this phase trees should be very flat
         if (currentMethod.exists) devWarning("Found a def within a def at a phase where defs are expected to be flattened out.")
         currentMethod = tree.symbol
@@ -349,6 +370,12 @@ abstract class Delambdafy extends Transform with TypingTransformers with ast.Tre
         // They'll be of the form {(args...) => this.anonfun(args...)}
         // but we do need to make note of the lifted body method in case it refers to 'this'
         if (currentMethod.exists) liftedMethodReferences(currentMethod) += targetMethod(fun)
+      case Apply(sel @ Select(This(_), _), args) if sel.symbol.isLiftedMethod =>
+        if (currentMethod.exists) liftedMethodReferences(currentMethod) += sel.symbol
+        super.traverseTrees(args)
+      case Apply(fun, outer :: rest) if shouldElideOuterArg(fun.symbol, outer) =>
+        super.traverse(fun)
+        super.traverseTrees(rest)
       case This(_) =>
         if (currentMethod.exists && tree.symbol == currentMethod.enclClass) {
           debuglog(s"$currentMethod directly refers to 'this'")
diff --git a/src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala b/src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala
index 3d6fad4238..411ff6b9be 100644
--- a/src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala
+++ b/src/compiler/scala/tools/nsc/transform/ExplicitOuter.scala
@@ -67,8 +67,6 @@ abstract class ExplicitOuter extends InfoTransform
     result
   }
 
-  private val innerClassConstructorParamName: TermName = newTermName("arg" + nme.OUTER)
-
   class RemoveBindingsTransformer(toRemove: Set[Symbol]) extends Transformer {
     override def transform(tree: Tree) = tree match {
       case Bind(_, body) if toRemove(tree.symbol) => super.transform(body)
@@ -169,7 +167,7 @@ abstract class ExplicitOuter extends InfoTransform
 
       val paramsWithOuter =
         if (sym.isClassConstructor && isInner(sym.owner)) // 1
-          sym.newValueParameter(innerClassConstructorParamName, sym.pos).setInfo(sym.owner.outerClass.thisType) :: params
+          sym.newValueParameter(nme.OUTER_ARG, sym.pos).setInfo(sym.owner.outerClass.thisType) :: params
         else params
 
       if ((resTpTransformed ne resTp) || (paramsWithOuter ne params)) MethodType(paramsWithOuter, resTpTransformed)
diff --git a/src/reflect/scala/reflect/internal/StdAttachments.scala b/src/reflect/scala/reflect/internal/StdAttachments.scala
index ef95b38843..76e34153c9 100644
--- a/src/reflect/scala/reflect/internal/StdAttachments.scala
+++ b/src/reflect/scala/reflect/internal/StdAttachments.scala
@@ -71,4 +71,9 @@ trait StdAttachments {
   abstract class InlineAnnotatedAttachment
   case object NoInlineCallsiteAttachment extends InlineAnnotatedAttachment
   case object InlineCallsiteAttachment extends InlineAnnotatedAttachment
+
+  /** Attached to a local class that has its outer field elided. A `null` constant may be passed
+    * in place of the outer parameter, can help callers to avoid capturing the outer instance.
+    */
+  case object OuterArgCanBeElided extends PlainAttachment
 }
diff --git a/src/reflect/scala/reflect/internal/StdNames.scala b/src/reflect/scala/reflect/internal/StdNames.scala
index 20a485469d..4f5a545c95 100644
--- a/src/reflect/scala/reflect/internal/StdNames.scala
+++ b/src/reflect/scala/reflect/internal/StdNames.scala
@@ -365,6 +365,7 @@ trait StdNames {
     val MODULE_INSTANCE_FIELD: NameType    = NameTransformer.MODULE_INSTANCE_NAME  // "MODULE$"
     val OUTER: NameType                    = "$outer"
     val OUTER_LOCAL: NameType              = OUTER.localName
+    val OUTER_ARG: NameType                = "arg" + OUTER
     val OUTER_SYNTH: NameType              = "<outer>" // emitted by virtual pattern matcher, replaced by outer accessor in explicitouter
     val ROOTPKG: NameType                  = "_root_"
     val SELECTOR_DUMMY: NameType           = "<unapply-selector>"
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index ba195363c1..6f2d8d802a 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -914,6 +914,9 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
     /** Is this symbol an accessor method for outer? */
     final def isOuterField = isArtifact && (unexpandedName == nme.OUTER_LOCAL)
 
+    /** Is this symbol an outer parameter in a constructor */
+    final def isOuterParam = isParameter && owner.isConstructor && (name == nme.OUTER_ARG || name == nme.OUTER)
+
     /** Does this symbol denote a stable value, ignoring volatility?
      *
      * Stability and volatility are checked separately to allow volatile paths in patterns that amount to equality checks. SI-6815
diff --git a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
index 28222cf9a7..0a90a141d3 100644
--- a/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
+++ b/src/reflect/scala/reflect/runtime/JavaUniverseForce.scala
@@ -45,6 +45,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     this.SubpatternsAttachment
     this.NoInlineCallsiteAttachment
     this.InlineCallsiteAttachment
+    this.OuterArgCanBeElided
     this.noPrint
     this.typeDebug
     this.Range
diff --git a/test/files/jvm/t9105.check b/test/files/jvm/t9105.check
index 48439ee004..9447e0cf29 100644
--- a/test/files/jvm/t9105.check
+++ b/test/files/jvm/t9105.check
@@ -1,18 +1,8 @@
-#partest -Ydelambdafy:inline
-(class C$$anonfun$1$A$1,class C$$anonfun$1,null)
-(class C$$anonfun$1$B$1,class C$$anonfun$1,private final java.lang.Object C$$anonfun$1.m$1())
-(class C$$anonfun$1$C$1,class C$$anonfun$1,null)
-(class C$$anonfun$1$$anonfun$2$D$1,class C$$anonfun$1$$anonfun$2,null)
-(class C$$anonfun$met$1$E$1,class C$$anonfun$met$1,null)
-(class C$$anonfun$met$1$F$1,class C$$anonfun$met$1,private final java.lang.Object C$$anonfun$met$1.m$2())
-(class C$$anonfun$met$1$G$1,class C$$anonfun$met$1,null)
-(class C$$anonfun$met$1$$anonfun$3$H$1,class C$$anonfun$met$1$$anonfun$3,null)
-#partest !-Ydelambdafy:inline
 (class C$A$1,class C,null)
-(class C$B$1,class C,private final java.lang.Object C.m$1())
+(class C$B$1,class C,private static final java.lang.Object C.m$1())
 (class C$C$1,class C,null)
 (class C$D$1,class C,null)
 (class C$E$1,class C,public scala.Function0 C.met())
-(class C$F$1,class C,private final java.lang.Object C.m$2())
+(class C$F$1,class C,private static final java.lang.Object C.m$2())
 (class C$G$1,class C,public scala.Function0 C.met())
 (class C$H$1,class C,public scala.Function0 C.met())
diff --git a/test/files/run/t5652.check b/test/files/run/t5652.check
index a0fb6fe9b4..7c65ba6698 100644
--- a/test/files/run/t5652.check
+++ b/test/files/run/t5652.check
@@ -1,7 +1,7 @@
-public default int T1.T1$$g$1()
 public default int T1.f0()
 public default void T1.$init$()
-public final int A1.A1$$g$2()
+public static int T1.T1$$g$1()
 public int A1.f1()
-public final int A2.A2$$g$1()
+public static final int A1.A1$$g$2()
 public int A2.f2()
+public static final int A2.A2$$g$1()
diff --git a/test/files/run/t5652b.check b/test/files/run/t5652b.check
index ca9d0a74f0..0f4290796f 100644
--- a/test/files/run/t5652b.check
+++ b/test/files/run/t5652b.check
@@ -1,4 +1,4 @@
-private final int A1.g$1()
+private static final int A1.g$1()
 public int A1.f1()
-private final int A2.g$1()
+private static final int A2.g$1()
 public int A2.f2()
diff --git a/test/files/run/t5652c.check b/test/files/run/t5652c.check
index 3b889e066d..5a6d535f02 100644
--- a/test/files/run/t5652c.check
+++ b/test/files/run/t5652c.check
@@ -1,6 +1,6 @@
-public final int A1.A1$$g$1()
-public final int A1.A1$$g$2()
 public int A1.f1()
 public int A1.f2()
+public static final int A1.A1$$g$1()
+public static final int A1.A1$$g$2()
 1
 2
diff --git a/test/files/run/t9390.scala b/test/files/run/t9390.scala
new file mode 100644
index 0000000000..8d7e1be557
--- /dev/null
+++ b/test/files/run/t9390.scala
@@ -0,0 +1,67 @@
+class C {
+  def methodLift1 = {
+    def isEven(c: Int) = c % 2 == 0
+    val f: Int => Boolean = isEven
+    f
+  }
+  def methodLift2 = {
+    def isEven(c: Int) = c % 2 == 0
+    def isEven0(c: Int) = isEven(c)
+    val f: Int => Boolean = isEven0
+    f
+  }
+
+  def methodLift3 = {
+    def isEven(c: Int) = {toString; c % 2 == 0}
+    def isEven0(c: Int) = isEven(c)
+    val f: Int => Boolean = isEven0
+    f
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val c = new C
+
+    {
+      val f = c.methodLift1
+      assert(f(0))
+      assert(!f(1))
+      val f1 = serializeDeserialize(f)
+      assert(f1(0))
+      assert(!f1(1))
+    }
+
+
+    {
+      val f = c.methodLift2
+      assert(f(0))
+      assert(!f(1))
+      val f1 = serializeDeserialize(f)
+      assert(f1(0))
+      assert(!f1(1))
+    }
+
+    {
+      val f = c.methodLift3
+      assert(f(0))
+      assert(!f(1))
+      try {
+        serializeDeserialize(this)
+        assert(false)
+      } catch {
+        case _: java.io.NotSerializableException =>
+          // expected, the closure in methodLift3 must capture C which is not serializable
+      }
+    }
+  }
+
+  def serializeDeserialize[T <: AnyRef](obj: T): T = {
+    import java.io._
+    val buffer = new ByteArrayOutputStream
+    val out = new ObjectOutputStream(buffer)
+    out.writeObject(obj)
+    val in = new ObjectInputStream(new ByteArrayInputStream(buffer.toByteArray))
+    in.readObject.asInstanceOf[T]
+  }
+}
diff --git a/test/files/run/t9390b.scala b/test/files/run/t9390b.scala
new file mode 100644
index 0000000000..439e21e0a0
--- /dev/null
+++ b/test/files/run/t9390b.scala
@@ -0,0 +1,31 @@
+class C { // C is not serializable
+  def foo = (x: Int) => (y: Int) => x + y
+  def bar = (x: Int) => (y: Int) => {toString; x + y}
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    val c = new C
+    val f = c.foo
+    assert(f(1)(2) == 3)
+    val f1 = serializeDeserialize(f)
+    assert(f1(1)(2) == 3)
+
+    try {
+      serializeDeserialize(c.bar)
+      assert(false)
+    } catch {
+      case _: java.io.NotSerializableException =>
+        // expected, lambda transitively refers to this
+    }
+  }
+
+  def serializeDeserialize[T <: AnyRef](obj: T): T = {
+    import java.io._
+    val buffer = new ByteArrayOutputStream
+    val out = new ObjectOutputStream(buffer)
+    out.writeObject(obj)
+    val in = new ObjectInputStream(new ByteArrayInputStream(buffer.toByteArray))
+    in.readObject.asInstanceOf[T]
+  }
+}
diff --git a/test/files/run/t9390c.scala b/test/files/run/t9390c.scala
new file mode 100644
index 0000000000..db39da57cd
--- /dev/null
+++ b/test/files/run/t9390c.scala
@@ -0,0 +1,21 @@
+class C { // C is not serializable
+  def foo = {
+    { (x: Any) => new Object {} }
+  }
+}
+object Test {
+  def main(args: Array[String]): Unit = {
+    val c = new C
+    val f = c.foo
+    val f1 = serializeDeserialize(f)
+  }
+
+  def serializeDeserialize[T <: AnyRef](obj: T): T = {
+    import java.io._
+    val buffer = new ByteArrayOutputStream
+    val out = new ObjectOutputStream(buffer)
+    out.writeObject(obj)
+    val in = new ObjectInputStream(new ByteArrayInputStream(buffer.toByteArray))
+    in.readObject.asInstanceOf[T]
+  }
+}
diff --git a/test/files/run/t9390d.scala b/test/files/run/t9390d.scala
new file mode 100644
index 0000000000..3c5de3abf7
--- /dev/null
+++ b/test/files/run/t9390d.scala
@@ -0,0 +1,12 @@
+class C { // C is not serializable
+  def foo: () => Any = {
+    { () => class UseOuterInConstructor { C.this.toString }; new UseOuterInConstructor : Any}
+  }
+}
+object Test {
+  def main(args: Array[String]): Unit = {
+    val c = new C
+    val f = c.foo
+    f() // Doesn't NPE, as we didn't elide the outer instance in the constructor call.
+  }
+}
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
index 6161dc7b73..024cf0c416 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlineWarningTest.scala
@@ -113,7 +113,7 @@ class InlineWarningTest extends BytecodeTesting {
 
     val warn =
       """M::f()I is annotated @inline but could not be inlined:
-        |The callee M::f()I contains the instruction INVOKESPECIAL M.nested$1 ()I
+        |The callee M::f()I contains the instruction INVOKESTATIC M.nested$1 ()I
         |that would cause an IllegalAccessError when inlined into class N""".stripMargin
 
     var c = 0
@@ -140,7 +140,7 @@ class InlineWarningTest extends BytecodeTesting {
 
     val warn =
       """M::f(Lscala/Function1;)I could not be inlined:
-        |The callee M::f(Lscala/Function1;)I contains the instruction INVOKESPECIAL M.nested$1 ()I
+        |The callee M::f(Lscala/Function1;)I contains the instruction INVOKESTATIC M.nested$1 ()I
         |that would cause an IllegalAccessError when inlined into class N""".stripMargin
 
     var c = 0
diff --git a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
index 7234659a1d..02cd632af1 100644
--- a/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
+++ b/test/junit/scala/tools/nsc/backend/jvm/opt/InlinerTest.scala
@@ -1141,7 +1141,7 @@ class InlinerTest extends BytecodeTesting {
 
     val warn =
       """C::h()I is annotated @inline but could not be inlined:
-        |The callee C::h()I contains the instruction INVOKESPECIAL C.f$1 ()I
+        |The callee C::h()I contains the instruction INVOKESTATIC C.f$1 ()I
         |that would cause an IllegalAccessError when inlined into class D.""".stripMargin
 
     val List(c, d) = compile(code, allowMessage = _.msg contains warn)