accommodates pull request feedback

https://github.com/scala/scala/pull/2072

5 files changed, 68 insertions, 17 deletions

diff --git a/src/compiler/scala/reflect/reify/codegen/GenSymbols.scala b/src/compiler/scala/reflect/reify/codegen/GenSymbols.scala
index 0d795e3783..47c966ea24 100644
--- a/src/compiler/scala/reflect/reify/codegen/GenSymbols.scala
+++ b/src/compiler/scala/reflect/reify/codegen/GenSymbols.scala
@@ -1,6 +1,8 @@
 package scala.reflect.reify
 package codegen
 
+import scala.reflect.internal.Flags._
+
 trait GenSymbols {
   self: Reifier =>
 
@@ -100,8 +102,33 @@ trait GenSymbols {
     reifyIntoSymtab(binding.symbol) { sym =>
       if (reifyDebug) println("Free term" + (if (sym.isCapturedVariable) " (captured)" else "") + ": " + sym + "(" + sym.accurateKindString + ")")
       val name = newTermName(nme.REIFY_FREE_PREFIX + sym.name + (if (sym.isType) nme.REIFY_FREE_THIS_SUFFIX else ""))
-      // Flag <stable> is set here to make reified free value pass stability test during reflective compilation
-      if (!sym.isMutable) sym setFlag reflect.internal.Flags.STABLE
+      // We need to note whether the free value being reified is stable or not to guide subsequent reflective compilation.
+      // Here's why reflection compilation needs our help.
+      //
+      // When dealing with a tree, which contain free values, toolboxes extract those and wrap the entire tree in a Function
+      // having parameters defined for every free values in the tree. For example, evaluating
+      //
+      //   Ident(setTypeSignature(newFreeTerm("x", 2), <Int>))
+      //
+      // Will generate something like
+      //
+      //   object wrapper {
+      //     def wrapper(x: () => Int) = {
+      //       x()
+      //     }
+      //   }
+      //
+      // Note that free values get transformed into, effectively, by-name parameters. This is done to make sure
+      // that evaluation order is kept intact. And indeed, we cannot just evaluate all free values at once in order
+      // to obtain arguments for wrapper.wrapper, because if some of the free values end up being unused during evaluation,
+      // we might end up doing unnecessary calculations.
+      //
+      // So far, so good - we didn't need any flags at all. However, if the code being reified contains path-dependent types,
+      // we're in trouble, because valid code like `free.T` ends up being transformed into `free.apply().T`, which won't compile.
+      //
+      // To overcome this glitch, we note whether a given free term is stable or not (because vars can also end up being free terms).
+      // Then, if a free term is stable, we tell the compiler to treat `free.apply()` specially and assume that it's stable.
+      if (!sym.isMutable) sym setFlag STABLE
       if (sym.isCapturedVariable) {
         assert(binding.isInstanceOf[Ident], showRaw(binding))
         val capturedBinding = referenceCapturedVariable(sym)
diff --git a/src/compiler/scala/tools/reflect/ToolBoxFactory.scala b/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
index fdc790a920..c05c59d5ff 100644
--- a/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
+++ b/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
@@ -8,7 +8,7 @@ import scala.tools.nsc.typechecker.Modes
 import scala.tools.nsc.io.VirtualDirectory
 import scala.tools.nsc.interpreter.AbstractFileClassLoader
 import scala.tools.nsc.util.FreshNameCreator
-import scala.reflect.internal.Flags
+import scala.reflect.internal.Flags._
 import scala.reflect.internal.util.{BatchSourceFile, NoSourceFile, NoFile}
 import java.lang.{Class => jClass}
 import scala.compat.Platform.EOL
@@ -211,12 +211,9 @@ abstract class ToolBoxFactory[U <: JavaUniverse](val u: U) { factorySelf =>
 
           val meth = obj.moduleClass.newMethod(newTermName(wrapperMethodName))
           def makeParam(schema: (FreeTermSymbol, TermName)) = {
+            // see a detailed explanation of the STABLE trick in `GenSymbols.reifyFreeTerm`
             val (fv, name) = schema
-            /* Free term symbol `fv` can contain flag <stable> which was set by
-             * `scala.reflect.reify.codegen.GenSymbols.reifyFreeTerm` method.
-             * It is recovered here, so value parameter can pass `isExprSafeToInline`
-             * test in `scala.reflect.internal.TreeInfo`. */
-            meth.newValueParameter(name, newFlags = if (fv.hasStableFlag) Flags.STABLE else 0) setInfo appliedType(definitions.FunctionClass(0).tpe, List(fv.tpe.resultType))
+            meth.newValueParameter(name, newFlags = if (fv.hasStableFlag) STABLE else 0) setInfo appliedType(definitions.FunctionClass(0).tpe, List(fv.tpe.resultType))
           }
           meth setInfo MethodType(freeTerms.map(makeParam).toList, AnyClass.tpe)
           minfo.decls enter meth
diff --git a/src/reflect/scala/reflect/internal/TreeInfo.scala b/src/reflect/scala/reflect/internal/TreeInfo.scala
index f83106aaef..3040486076 100644
--- a/src/reflect/scala/reflect/internal/TreeInfo.scala
+++ b/src/reflect/scala/reflect/internal/TreeInfo.scala
@@ -89,18 +89,15 @@ abstract class TreeInfo {
       tree.symbol.isStable && isExprSafeToInline(qual)
     case TypeApply(fn, _) =>
       isExprSafeToInline(fn)
-    /* Special case for reified free terms. During reflective compilation,
-     * reified value recovered flag <stable> from free term and wrapped into a
-     * Function object, so it can pass stability check here to become a stable
-     * identifier.*/
     case Apply(Select(free @ Ident(_), nme.apply), _) if free.symbol.name endsWith nme.REIFY_FREE_VALUE_SUFFIX =>
+      // see a detailed explanation of this trick in `GenSymbols.reifyFreeTerm`
       free.symbol.hasStableFlag && isExprSafeToInline(free)
     case Apply(fn, List()) =>
-      /* Note: After uncurry, field accesses are represented as Apply(getter, Nil),
-       * so an Apply can also be pure.
-       * However, before typing, applications of nullary functional values are also
-       * Apply(function, Nil) trees. To prevent them from being treated as pure,
-       * we check that the callee is a method. */
+      // Note: After uncurry, field accesses are represented as Apply(getter, Nil),
+      // so an Apply can also be pure.
+      // However, before typing, applications of nullary functional values are also
+      // Apply(function, Nil) trees. To prevent them from being treated as pure,
+      // we check that the callee is a method.
       fn.symbol.isMethod && !fn.symbol.isLazy && isExprSafeToInline(fn)
     case Typed(expr, _) =>
       isExprSafeToInline(expr)
diff --git a/test/files/run/t6591_7.check b/test/files/run/t6591_7.check
new file mode 100644
index 0000000000..e21a3669b6
--- /dev/null
+++ b/test/files/run/t6591_7.check
@@ -0,0 +1,4 @@
+name = x, stable = true
+name = y, stable = true
+name = z, stable = false
+name = C, stable = true
diff --git a/test/files/run/t6591_7.scala b/test/files/run/t6591_7.scala
new file mode 100644
index 0000000000..b6c8d399a0
--- /dev/null
+++ b/test/files/run/t6591_7.scala
@@ -0,0 +1,26 @@
+import scala.reflect.runtime.universe._
+import scala.tools.reflect.Eval
+
+object Test extends App {
+  locally {
+    val x = 2
+    def y = 3
+    var z = 4
+    class C {
+      var w = 5
+      locally {
+        val expr = reify(x + y + z + w)
+        // blocked by SI-7103, though it's not the focus of this test
+        // therefore I'm just commenting out the evaluation
+        // println(expr.eval)
+        expr.tree.freeTerms foreach (ft => {
+          // blocked by SI-7104, though it's not the focus of this test
+          // therefore I'm just commenting out the call to typeSignature
+          // println(s"name = ${ft.name}, sig = ${ft.typeSignature}, stable = ${ft.isStable}")
+          println(s"name = ${ft.name}, stable = ${ft.isStable}")
+        })
+      }
+    }
+    new C()
+  }
+}
\ No newline at end of file