Merge pull request #3485 from xeno-by/topic/reset-all-attrs

kills resetAllAttrs

11 files changed, 47 insertions, 71 deletions

diff --git a/src/compiler/scala/reflect/macros/contexts/Typers.scala b/src/compiler/scala/reflect/macros/contexts/Typers.scala
index cd3db74016..c1ab17027f 100644
--- a/src/compiler/scala/reflect/macros/contexts/Typers.scala
+++ b/src/compiler/scala/reflect/macros/contexts/Typers.scala
@@ -46,9 +46,7 @@ trait Typers {
     universe.analyzer.inferImplicit(tree, viewTpe, true, callsiteTyper.context, silent, withMacrosDisabled, pos, (pos, msg) => throw TypecheckException(pos, msg))
   }
 
-  def resetAllAttrs(tree: Tree): Tree = universe.resetAllAttrs(universe.duplicateAndKeepPositions(tree))
+  def resetLocalAttrs(tree: Tree): Tree = universe.resetAttrs(universe.duplicateAndKeepPositions(tree))
 
-  def resetLocalAttrs(tree: Tree): Tree = universe.resetLocalAttrs(universe.duplicateAndKeepPositions(tree))
-
-  def untypecheck(tree: Tree): Tree = universe.resetLocalAttrs(universe.duplicateAndKeepPositions(tree))
+  def untypecheck(tree: Tree): Tree = resetLocalAttrs(tree)
 }
diff --git a/src/compiler/scala/reflect/reify/Reifier.scala b/src/compiler/scala/reflect/reify/Reifier.scala
index ad0632f93e..6b0a0ee8d7 100644
--- a/src/compiler/scala/reflect/reify/Reifier.scala
+++ b/src/compiler/scala/reflect/reify/Reifier.scala
@@ -86,7 +86,7 @@ abstract class Reifier extends States
           throw new Error("reifee %s of type %s is not supported".format(reifee, if (reifee == null) "null" else reifee.getClass.toString))
       }
 
-      // todo. why do we resetAllAttrs?
+      // todo. why do we reset attrs?
       //
       // typically we do some preprocessing before reification and
       // the code emitted/moved around during preprocessing is very hard to typecheck, so we leave it as it is
@@ -109,19 +109,11 @@ abstract class Reifier extends States
       //
       // todo. this is a common problem with non-trivial macros in our current macro system
       // needs to be solved some day
-      // maybe try `resetLocalAttrs` once the dust settles
-      var importantSymbols = Set[Symbol](
-        NothingClass, AnyClass, SingletonClass, PredefModule, ScalaRunTimeModule, TypeCreatorClass, TreeCreatorClass, MirrorClass,
-        ApiUniverseClass, JavaUniverseClass, ReflectRuntimePackage, runDefinitions.ReflectRuntimeCurrentMirror)
-      importantSymbols ++= importantSymbols map (_.companionSymbol)
-      importantSymbols ++= importantSymbols map (_.moduleClass)
-      importantSymbols ++= importantSymbols map (_.linkedClassOfClass)
-      def isImportantSymbol(sym: Symbol): Boolean = sym != null && sym != NoSymbol && importantSymbols(sym)
-      val untyped = resetAllAttrs(result, leaveAlone = {
+      // upd. a new hope: https://groups.google.com/forum/#!topic/scala-internals/TtCTPlj_qcQ
+      val untyped = resetAttrs(result, leaveAlone = {
         case ValDef(_, u, _, _) if u == nme.UNIVERSE_SHORT => true
         case ValDef(_, m, _, _) if m == nme.MIRROR_SHORT => true
         case tree if symtab.syms contains tree.symbol => true
-        case tree if isImportantSymbol(tree.symbol) => true
         case _ => false
       })
 
diff --git a/src/compiler/scala/reflect/reify/codegen/GenTrees.scala b/src/compiler/scala/reflect/reify/codegen/GenTrees.scala
index f6b3c42ca9..b082796757 100644
--- a/src/compiler/scala/reflect/reify/codegen/GenTrees.scala
+++ b/src/compiler/scala/reflect/reify/codegen/GenTrees.scala
@@ -140,7 +140,7 @@ trait GenTrees {
         if (sym == NoSymbol) {
           // this sometimes happens, e.g. for binds that don't have a body
           // or for untyped code generated during previous phases
-          // (see a comment in Reifiers about the latter, starting with "why do we resetAllAttrs?")
+          // (see a comment in Reifiers about the latter, starting with "why do we reset attrs?")
           mirrorCall(nme.Ident, reify(name))
         }
         else if (!sym.isLocalToReifee) {
diff --git a/src/compiler/scala/tools/nsc/ast/Trees.scala b/src/compiler/scala/tools/nsc/ast/Trees.scala
index 24bce0636d..7a7d4ac0b2 100644
--- a/src/compiler/scala/tools/nsc/ast/Trees.scala
+++ b/src/compiler/scala/tools/nsc/ast/Trees.scala
@@ -176,13 +176,24 @@ trait Trees extends scala.reflect.internal.Trees { self: Global =>
     }
   }
 
-  /** resets symbol and tpe fields in a tree, @see ResetAttrs
-   */
-//  def resetAllAttrs[A<:Tree](x:A): A = { new ResetAttrsTraverser().traverse(x); x }
-//  def resetLocalAttrs[A<:Tree](x:A): A = { new ResetLocalAttrsTraverser().traverse(x); x }
-
-  def resetAllAttrs(x: Tree, leaveAlone: Tree => Boolean = null): Tree = new ResetAttrs(false, leaveAlone).transform(x)
-  def resetLocalAttrs(x: Tree, leaveAlone: Tree => Boolean = null): Tree = new ResetAttrs(true, leaveAlone).transform(x)
+  // Finally, noone resetAllAttrs it anymore, so I'm removing it from the compiler.
+  // Even though it's with great pleasure I'm doing that, I'll leave its body here to warn future generations about what happened in the past.
+  //
+  // So what actually happened in the past is that we used to have two flavors of resetAttrs: resetAllAttrs and resetLocalAttrs.
+  // resetAllAttrs destroyed all symbols and types in the tree in order to reset its state to something suitable for retypechecking
+  // and/or embedding into bigger trees / different lexical scopes. (Btw here's some background on why people would want to use
+  // reset attrs in the first place: https://groups.google.com/forum/#!topic/scala-internals/TtCTPlj_qcQ).
+  //
+  // However resetAllAttrs was more of a poison than of a treatment, because along with locally defined symbols that are the cause
+  // for almost every or maybe even every case of tree corruption, it erased external bindings that sometimes could not be restored.
+  // This is how we came up with resetLocalAttrs that left external bindings alone, and that was a big step forward.
+  // Then slowly but steadily we've evicted all usages of resetAllAttrs from our codebase in favor of resetLocalAttrs
+  // and have been living happily ever after.
+  //
+  // def resetAllAttrs(x: Tree, leaveAlone: Tree => Boolean = null): Tree = new ResetAttrs(localOnly = false, leaveAlone).transform(x)
+
+  /** @see ResetAttrs */
+  def resetAttrs(x: Tree, leaveAlone: Tree => Boolean = null): Tree = new ResetAttrs(leaveAlone).transform(x)
 
   /** A transformer which resets symbol and tpe fields of all nodes in a given tree,
    *  with special treatment of:
@@ -193,7 +204,7 @@ trait Trees extends scala.reflect.internal.Trees { self: Global =>
    *
    *  (bq:) This transformer has mutable state and should be discarded after use
    */
-  private class ResetAttrs(localOnly: Boolean, leaveAlone: Tree => Boolean = null, keepLabels: Boolean = false) {
+  private class ResetAttrs(leaveAlone: Tree => Boolean = null) {
     // this used to be based on -Ydebug, but the need for logging in this code is so situational
     // that I've reverted to a hard-coded constant here.
     val debug = false
@@ -277,29 +288,18 @@ trait Trees extends scala.reflect.internal.Trees { self: Global =>
                 // vetoXXX local variables declared below describe the conditions under which we cannot erase symbols.
                 //
                 // The first reason to not erase symbols is the threat of non-idempotency (SI-5464).
-                // Here we take care of labels (SI-5562) and references to package classes (SI-5705).
+                // Here we take care of references to package classes (SI-5705).
                 // There are other non-idempotencies, but they are not worked around yet.
                 //
-                // The second reason has to do with the fact that resetAttrs itself has limited usefulness.
-                //
-                // First of all, why do we need resetAttrs? Gor one, it's absolutely required to move trees around.
-                // One cannot just take a typed tree from one lexical context and transplant it somewhere else.
-                // Most likely symbols defined by those trees will become borked and the compiler will blow up (SI-5797).
-                // To work around we just erase all symbols and types and then hope that we'll be able to correctly retypecheck.
-                // For ones who're not affected by scalac Stockholm syndrome, this might seem to be an extremely naive fix, but well...
-                //
-                // Of course, sometimes erasing everything won't work, because if a given identifier got resolved to something
-                // in one lexical scope, it can get resolved to something else.
-                //
-                // What do we do in these cases? Enter the workaround for the workaround: resetLocalAttrs, which only destroys
-                // locally defined symbols, but doesn't touch references to stuff declared outside of a given tree.
-                // That's what localOnly and vetoScope are for.
+                // The second reason has to do with the fact that resetAttrs needs to be less destructive.
+                // Erasing locally-defined symbols is useful to prevent tree corruption, but erasing external bindings is not,
+                // therefore we want to retain those bindings, especially given that restoring them can be impossible
+                // if we move these trees into lexical contexts different from their original locations.
                 if (dupl.hasSymbol) {
                   val sym = dupl.symbol
-                  val vetoScope = localOnly && !(locals contains sym)
-                  val vetoLabel = keepLabels && sym.isLabel
+                  val vetoScope = !(locals contains sym)
                   val vetoThis = dupl.isInstanceOf[This] && sym.isPackageClass
-                  if (!(vetoScope || vetoLabel || vetoThis)) dupl.symbol = NoSymbol
+                  if (!(vetoScope || vetoThis)) dupl.symbol = NoSymbol
                 }
                 dupl.clearType()
             }
@@ -308,10 +308,9 @@ trait Trees extends scala.reflect.internal.Trees { self: Global =>
     }
 
     def transform(x: Tree): Tree = {
-      if (localOnly)
       new MarkLocals().traverse(x)
 
-      if (localOnly && debug) {
+      if (debug) {
         assert(locals.size == orderedLocals.size)
         val msg = orderedLocals.toList filter {_ != NoSymbol} map {"  " + _} mkString EOL
         trace("locals (%d total): %n".format(orderedLocals.size))(msg)
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 645f267a21..632e25aa2e 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -1205,7 +1205,7 @@ trait Namers extends MethodSynthesis {
      * flag.
      */
     private def addDefaultGetters(meth: Symbol, ddef: DefDef, vparamss: List[List[ValDef]], tparams: List[TypeDef], overriddenSymbol: => Symbol) {
-      val DefDef(_, _, rtparams0, rvparamss0, _, _) = resetLocalAttrs(ddef.duplicate)
+      val DefDef(_, _, rtparams0, rvparamss0, _, _) = resetAttrs(ddef.duplicate)
       // having defs here is important to make sure that there's no sneaky tree sharing
       // in methods with multiple default parameters
       def rtparams = rtparams0.map(_.duplicate)
@@ -1292,7 +1292,7 @@ trait Namers extends MethodSynthesis {
                     return // fix #3649 (prevent crash in erroneous source code)
                 }
               }
-              val ClassDef(_, _, rtparams, _) = resetLocalAttrs(cdef.duplicate)
+              val ClassDef(_, _, rtparams, _) = resetAttrs(cdef.duplicate)
               defTparams = rtparams.map(rt => copyTypeDef(rt)(mods = rt.mods &~ (COVARIANT | CONTRAVARIANT)))
               nmr
             }
diff --git a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
index 46ff98875f..6a4df415ae 100644
--- a/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala
@@ -173,7 +173,7 @@ trait NamesDefaults { self: Analyzer =>
           // setSymbol below is important because the 'selected' function might be overloaded. by
           // assigning the correct method symbol, typedSelect will just assign the type. the reason
           // to still call 'typed' is to correctly infer singleton types, SI-5259.
-          val selectPos = 
+          val selectPos =
             if(qual.pos.isRange && baseFun.pos.isRange) qual.pos.union(baseFun.pos).withStart(Math.min(qual.pos.end, baseFun.pos.end))
             else baseFun.pos
           val f = blockTyper.typedOperator(Select(newQual, selected).setSymbol(baseFun1.symbol).setPos(selectPos))
@@ -287,7 +287,7 @@ trait NamesDefaults { self: Analyzer =>
             }
             else {
               // TODO In 83c9c764b, we tried to a stable type here to fix SI-7234. But the resulting TypeTree over a
-              //      singleton type without an original TypeTree fails to retypecheck after a resetLocalAttrs (SI-7516),
+              //      singleton type without an original TypeTree fails to retypecheck after a resetAttrs (SI-7516),
               //      which is important for (at least) macros.
               arg.tpe
             }
@@ -310,7 +310,7 @@ trait NamesDefaults { self: Analyzer =>
               new ChangeOwnerTraverser(context.owner, sym) traverse arg // fixes #4502
               if (repeated) arg match {
                 case WildcardStarArg(expr) => expr
-                case _                     => blockTyper typed gen.mkSeqApply(resetLocalAttrs(arg))
+                case _                     => blockTyper typed gen.mkSeqApply(resetAttrs(arg))
               } else arg
             }
           Some(atPos(body.pos)(ValDef(sym, body).setType(NoType)))
diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index f7684b93af..2125e281f0 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1414,7 +1414,7 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
       case TypeRef(pre, sym, args) =>
         tree match {
           case tt: TypeTree if tt.original == null => // SI-7783 don't warn about inferred types
-                                                      // FIXME: reconcile this check with one in resetAllAttrs
+                                                      // FIXME: reconcile this check with one in resetAttrs
           case _ => checkUndesiredProperties(sym, tree.pos)
         }
         if(sym.isJavaDefined)
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 101e1526fe..10fe530445 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -832,7 +832,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
                 else tpr.typed(withImplicitArgs, mode, pt)
               }
               orElse { _ =>
-                val resetTree = resetLocalAttrs(original)
+                val resetTree = resetAttrs(original)
                 debuglog(s"fallback on implicits: ${tree}/$resetTree")
                 val tree1 = typed(resetTree, mode)
                 // Q: `typed` already calls `pluginsTyped` and `adapt`. the only difference here is that
@@ -2307,7 +2307,8 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
           context.scope.unlink(ldef.symbol)
           val sym2 = namer.enterInScope(
             context.owner.newLabel(ldef.name, ldef.pos) setInfo MethodType(List(), restpe))
-          val rhs2 = typed(resetAllAttrs(ldef.rhs), restpe)
+          val LabelDef(_, _, rhs1) = resetAttrs(ldef)
+          val rhs2 = typed(rhs1, restpe)
           ldef.params foreach (param => param setType param.symbol.tpe)
           deriveLabelDef(ldef)(_ => rhs2) setSymbol sym2 setType restpe
         }
@@ -2589,7 +2590,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
         //
         //         Well behaved trees satisfy the property:
         //
-        //         typed(tree) == typed(resetLocalAttrs(typed(tree))
+        //         typed(tree) == typed(resetAttrs(typed(tree))
         //
         //         Trees constructed without low-level symbol manipulation get this for free;
         //         references to local symbols are cleared by `ResetAttrs`, but bind to the
diff --git a/src/compiler/scala/tools/nsc/typechecker/Unapplies.scala b/src/compiler/scala/tools/nsc/typechecker/Unapplies.scala
index ffac29b4b8..cc2d9141ce 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Unapplies.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Unapplies.scala
@@ -52,13 +52,13 @@ trait Unapplies extends ast.TreeDSL {
   }
 
   private def constrParamss(cdef: ClassDef): List[List[ValDef]] = {
-    val ClassDef(_, _, _, Template(_, _, body)) = resetLocalAttrs(cdef.duplicate)
+    val ClassDef(_, _, _, Template(_, _, body)) = resetAttrs(cdef.duplicate)
     val DefDef(_, _, _, vparamss, _, _) = treeInfo firstConstructor body
     vparamss
   }
 
   private def constrTparamsInvariant(cdef: ClassDef): List[TypeDef] = {
-    val ClassDef(_, _, tparams, _) = resetLocalAttrs(cdef.duplicate)
+    val ClassDef(_, _, tparams, _) = resetAttrs(cdef.duplicate)
     val tparamsInvariant = tparams.map(tparam => copyTypeDef(tparam)(mods = tparam.mods &~ (COVARIANT | CONTRAVARIANT)))
     tparamsInvariant
   }
diff --git a/src/compiler/scala/tools/reflect/ToolBox.scala b/src/compiler/scala/tools/reflect/ToolBox.scala
index 4c1bc794bc..02a458214f 100644
--- a/src/compiler/scala/tools/reflect/ToolBox.scala
+++ b/src/compiler/scala/tools/reflect/ToolBox.scala
@@ -71,12 +71,6 @@ trait ToolBox[U <: scala.reflect.api.Universe] {
    */
   def inferImplicitView(tree: u.Tree, from: u.Type, to: u.Type, silent: Boolean = true, withMacrosDisabled: Boolean = false, pos: u.Position = u.NoPosition): u.Tree
 
-  /** Recursively resets symbols and types in a given tree.
-   *  WARNING: Don't use this API, go for [[untypecheck]] instead.
-   */
-  @deprecated("Use `tb.untypecheck` instead", "2.11.0")
-  def resetAllAttrs(tree: u.Tree): u.Tree
-
   /** Recursively resets locally defined symbols and types in a given tree.
    *  WARNING: Don't use this API, go for [[untypecheck]] instead.
    */
diff --git a/src/compiler/scala/tools/reflect/ToolBoxFactory.scala b/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
index 4a8c91bd1b..541a915adb 100644
--- a/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
+++ b/src/compiler/scala/tools/reflect/ToolBoxFactory.scala
@@ -236,7 +236,7 @@ abstract class ToolBoxFactory[U <: JavaUniverse](val u: U) { factorySelf =>
                   NoPosition))
           trace("wrapped: ")(showAttributed(moduledef, true, true, settings.Yshowsymkinds.value))
 
-          val cleanedUp = resetLocalAttrs(moduledef)
+          val cleanedUp = resetAttrs(moduledef)
           trace("cleaned up: ")(showAttributed(cleanedUp, true, true, settings.Yshowsymkinds.value))
           cleanedUp.asInstanceOf[ModuleDef]
         }
@@ -385,18 +385,10 @@ abstract class ToolBoxFactory[U <: JavaUniverse](val u: U) { factorySelf =>
       uitree
     }
 
-    def resetAllAttrs(tree: u.Tree): u.Tree = withCompilerApi { compilerApi =>
-      import compilerApi._
-      val ctree: compiler.Tree = importer.importTree(tree)
-      val ttree: compiler.Tree = compiler.resetAllAttrs(ctree)
-      val uttree = exporter.importTree(ttree)
-      uttree
-    }
-
     def resetLocalAttrs(tree: u.Tree): u.Tree = withCompilerApi { compilerApi =>
       import compilerApi._
       val ctree: compiler.Tree = importer.importTree(tree)
-      val ttree: compiler.Tree = compiler.resetLocalAttrs(ctree)
+      val ttree: compiler.Tree = compiler.resetAttrs(ctree)
       val uttree = exporter.importTree(ttree)
       uttree
     }