From e2a17d9c8234bf620ba0109f9ab4bae822c20bf0 Mon Sep 17 00:00:00 2001 From: Eugene Burmako Date: Sat, 16 Feb 2013 23:42:09 +0100 Subject: [PATCH 1/2] resetAttrs now always erases This.tpe The symbol of This, if it points to a package class, isn't touched, just as usual, so that our Select(Select(Select(...))) => This(...) optimization works fine with attr reset. However the tpe is now erased, so that subsequent reflective compilation doesn't spuriously fail when seeing that some subtrees of a tree being compiled are typed. Erasing the tpe doesn't pose even a tiniest problem, because, as it can be seen in typedThis, type is trivially reconstructed from the symbol. --- src/compiler/scala/tools/nsc/ast/Trees.scala | 11 +++++++---- test/files/run/resetattrs-this.check | 1 + test/files/run/resetattrs-this.scala | 11 +++++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 test/files/run/resetattrs-this.check create mode 100644 test/files/run/resetattrs-this.scala diff --git a/src/compiler/scala/tools/nsc/ast/Trees.scala b/src/compiler/scala/tools/nsc/ast/Trees.scala index 2ad762fd5524..f83a9632f6d5 100644 --- a/src/compiler/scala/tools/nsc/ast/Trees.scala +++ b/src/compiler/scala/tools/nsc/ast/Trees.scala @@ -335,14 +335,17 @@ trait Trees extends scala.reflect.internal.Trees { self: Global => else tree case TypeApply(fn, args) if args map transform exists (_.isEmpty) => transform(fn) - case This(_) if tree.symbol != null && tree.symbol.isPackageClass => - tree case EmptyTree => tree case _ => val dupl = tree.duplicate - if (tree.hasSymbol && (!localOnly || (locals contains tree.symbol)) && !(keepLabels && tree.symbol.isLabel)) - dupl.symbol = NoSymbol + if (dupl.hasSymbol) { + val sym = dupl.symbol + val vetoScope = localOnly && !(locals contains sym) + val vetoLabel = keepLabels && sym.isLabel + val vetoThis = dupl.isInstanceOf[This] && sym.isPackageClass + if (!(vetoScope || vetoLabel || vetoThis)) dupl.symbol = NoSymbol + } dupl.tpe = null dupl } diff --git a/test/files/run/resetattrs-this.check b/test/files/run/resetattrs-this.check new file mode 100644 index 000000000000..27ba77ddaf61 --- /dev/null +++ b/test/files/run/resetattrs-this.check @@ -0,0 +1 @@ +true diff --git a/test/files/run/resetattrs-this.scala b/test/files/run/resetattrs-this.scala new file mode 100644 index 000000000000..12afa3d71211 --- /dev/null +++ b/test/files/run/resetattrs-this.scala @@ -0,0 +1,11 @@ +import scala.reflect.runtime.universe._ +import scala.reflect.runtime.{currentMirror => cm} +import scala.tools.reflect.ToolBox + +object Test extends App { + val tb = cm.mkToolBox() + val tree = Select(This(cm.staticPackage("scala").moduleClass), newTermName("Predef")) + val ttree = tb.typeCheck(tree) + val rttree = tb.resetAllAttrs(ttree) + println(tb.eval(rttree) == Predef) +} \ No newline at end of file From 3e7db2d189ca37203b39d2c028581a0ae1281823 Mon Sep 17 00:00:00 2001 From: Eugene Burmako Date: Tue, 26 Feb 2013 19:42:52 +0100 Subject: [PATCH 2/2] adds some comments to resetAttrs --- src/compiler/scala/tools/nsc/ast/Trees.scala | 46 +++++++++++++++++--- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/src/compiler/scala/tools/nsc/ast/Trees.scala b/src/compiler/scala/tools/nsc/ast/Trees.scala index f83a9632f6d5..0a1273757278 100644 --- a/src/compiler/scala/tools/nsc/ast/Trees.scala +++ b/src/compiler/scala/tools/nsc/ast/Trees.scala @@ -327,18 +327,54 @@ trait Trees extends scala.reflect.internal.Trees { self: Global => case tpt: TypeTree => if (tpt.original != null) transform(tpt.original) - else if (tpt.tpe != null && (tpt.wasEmpty || (tpt.tpe exists (tp => locals contains tp.typeSymbol)))) { - val dupl = tpt.duplicate - dupl.tpe = null - dupl + else { + val refersToLocalSymbols = tpt.tpe != null && (tpt.tpe exists (tp => locals contains tp.typeSymbol)) + val isInferred = tpt.wasEmpty + if (refersToLocalSymbols || isInferred) { + val dupl = tpt.duplicate + dupl.tpe = null + dupl + } else { + tpt + } } - else tree + // If one of the type arguments of a TypeApply gets reset to an empty TypeTree, then this means that: + // 1) It isn't empty now (tpt.tpe != null), but it was empty before (tpt.wasEmpty). + // 2) Thus, its argument got inferred during a preceding typecheck. + // 3) Thus, all its arguments were inferred (because scalac can only infer all or nothing). + // Therefore, we can safely erase the TypeApply altogether and have it inferred once again in a subsequent typecheck. + // UPD: Actually there's another reason for erasing a type behind the TypeTree + // is when this type refers to symbols defined in the tree being processed. + // These symbols will be erased, because we can't leave alive a type referring to them. + // Here we can only hope that everything will work fine afterwards. case TypeApply(fn, args) if args map transform exists (_.isEmpty) => transform(fn) case EmptyTree => tree case _ => val dupl = tree.duplicate + // Typically the resetAttrs transformer cleans both symbols and types. + // However there are exceptions when we cannot erase symbols due to idiosyncrasies of the typer. + // 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). + // 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. if (dupl.hasSymbol) { val sym = dupl.symbol val vetoScope = localOnly && !(locals contains sym)