diff options
author | Paul Phillips <paulp@improving.org> | 2012-12-10 00:27:21 -0800 |
---|---|---|
committer | Paul Phillips <paulp@improving.org> | 2012-12-12 10:57:48 -0800 |
commit | 4bc3fa102768e78b194fd6a594f4b87d29e4efbf (patch) | |
tree | f82e50f1370eee618ac44bd1e0afcb695c47aba4 /src | |
parent | b26f12d4b116799e8860ddfd27ad398bc0c80b6a (diff) | |
download | scala-4bc3fa102768e78b194fd6a594f4b87d29e4efbf.tar.gz scala-4bc3fa102768e78b194fd6a594f4b87d29e4efbf.tar.bz2 scala-4bc3fa102768e78b194fd6a594f4b87d29e4efbf.zip |
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.
Diffstat (limited to 'src')
-rw-r--r-- | src/compiler/scala/tools/nsc/ast/TreeDSL.scala | 20 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/transform/ExtensionMethods.scala | 13 | ||||
-rw-r--r-- | src/compiler/scala/tools/nsc/transform/Mixin.scala | 9 |
3 files changed, 23 insertions, 19 deletions
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 |