From 4bc3fa102768e78b194fd6a594f4b87d29e4efbf Mon Sep 17 00:00:00 2001 From: Paul Phillips Date: Mon, 10 Dec 2012 00:27:21 -0800 Subject: Eliminated some sources of tree sharing. Tracking shared trees led to various perpetrators, the simplest of which are addressed herein. More consideration will be required: we need to approach the problem with sufficient command to assure both that trees are only shared when safe (which might without architectural changes be "never") but also that we do not duplicate definition trees unless it is appropriate. Why do we care about tree sharing? Sometimes, a lot of the time even, you can get away with sharing trees - but that's also why it's responsible for all kinds of trouble. If the compiler would break obviously and immediately then we wouldn't be doing it. The danger of sharing is that one piece of an AST may undergo a transformation or mutation and an unrelated piece of the AST will be partially dragged into the change. The danger has become more urgent with the arrival of macros. The first step in preventing tree sharing mishaps is to reduce the amount the compiler does so whatever is left is a lot easier to see. As a happy accident, it will also fix bugs. --- src/compiler/scala/tools/nsc/ast/TreeDSL.scala | 20 +++++++++++--------- .../scala/tools/nsc/transform/ExtensionMethods.scala | 13 +++++++------ src/compiler/scala/tools/nsc/transform/Mixin.scala | 9 +++++---- 3 files changed, 23 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/ast/TreeDSL.scala b/src/compiler/scala/tools/nsc/ast/TreeDSL.scala index 3129748e9f..e3bf562a2c 100644 --- a/src/compiler/scala/tools/nsc/ast/TreeDSL.scala +++ b/src/compiler/scala/tools/nsc/ast/TreeDSL.scala @@ -30,19 +30,17 @@ trait TreeDSL { def returning[T](x: T)(f: T => Unit): T = util.returning(x)(f) object LIT extends (Any => Literal) { + def typed(x: Any) = apply(x) setType ConstantType(Constant(x)) def apply(x: Any) = Literal(Constant(x)) def unapply(x: Any) = condOpt(x) { case Literal(Constant(value)) => value } } - // You might think these could all be vals, but empirically I have found that - // at least in the case of UNIT the compiler breaks if you re-use trees. - // However we need stable identifiers to have attractive pattern matching. - // So it's inconsistent until I devise a better way. - val TRUE = LIT(true) - val FALSE = LIT(false) - val ZERO = LIT(0) - def NULL = LIT(null) - def UNIT = LIT(()) + // Boring, predictable trees. + def TRUE = LIT typed true + def FALSE = LIT typed false + def ZERO = LIT(0) + def NULL = LIT(null) + def UNIT = LIT(()) // for those preferring boring, predictable lives, without the thrills of tree-sharing // (but with the perk of typed trees) @@ -106,6 +104,10 @@ trait TreeDSL { def DOT(sym: Symbol) = SelectStart(Select(target, sym)) /** Assignment */ + // !!! This method is responsible for some tree sharing, but a diligent + // reviewer pointed out that we shouldn't blindly duplicate these trees + // as there might be DefTrees nested beneath them. It's not entirely + // clear how to proceed, so for now it retains the non-duplicating behavior. def ===(rhs: Tree) = Assign(target, rhs) /** Methods for sequences **/ diff --git a/src/compiler/scala/tools/nsc/transform/ExtensionMethods.scala b/src/compiler/scala/tools/nsc/transform/ExtensionMethods.scala index 717c4b627b..77e7e013ab 100644 --- a/src/compiler/scala/tools/nsc/transform/ExtensionMethods.scala +++ b/src/compiler/scala/tools/nsc/transform/ExtensionMethods.scala @@ -232,12 +232,13 @@ abstract class ExtensionMethods extends Transform with TypingTransformers { override def transformStats(stats: List[Tree], exprOwner: Symbol): List[Tree] = super.transformStats(stats, exprOwner) map { - case md @ ModuleDef(_, _, _) if extensionDefs contains md.symbol => - val defns = extensionDefs(md.symbol).toList map (member => - atOwner(md.symbol)(localTyper.typedPos(md.pos.focus)(member)) - ) - extensionDefs -= md.symbol - deriveModuleDef(md)(tmpl => deriveTemplate(tmpl)(_ ++ defns)) + case md @ ModuleDef(_, _, _) => + val extraStats = extensionDefs remove md.symbol match { + case Some(defns) => defns.toList map (defn => atOwner(md.symbol)(localTyper.typedPos(md.pos.focus)(defn.duplicate))) + case _ => Nil + } + if (extraStats.isEmpty) md + else deriveModuleDef(md)(tmpl => deriveTemplate(tmpl)(_ ++ extraStats)) case stat => stat } diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala index 571b3aeefc..4ecc1e01db 100644 --- a/src/compiler/scala/tools/nsc/transform/Mixin.scala +++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala @@ -1035,16 +1035,17 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL { } // if class is not a trait add accessor definitions else if (!clazz.isTrait) { + // This needs to be a def to avoid sharing trees + def accessedRef = accessedReference(sym) if (sym.hasAccessorFlag && (!sym.isDeferred || sym.hasFlag(lateDEFERRED))) { // add accessor definitions addDefDef(sym, { - val accessedRef = accessedReference(sym) if (sym.isSetter) { if (isOverriddenSetter(sym)) UNIT else accessedRef match { - case Literal(_) => accessedRef - case _ => - val init = Assign(accessedRef, Ident(sym.firstParam)) + case ref @ Literal(_) => ref + case ref => + val init = Assign(ref, Ident(sym.firstParam)) val getter = sym.getter(clazz) if (!needsInitFlag(getter)) init -- cgit v1.2.3