From 05fb4208c68d194a8575d42852ee1c792a328267 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Aug 2015 22:10:47 -0700 Subject: [PATCH 1/6] Make changeOwnerAfter more robust wrt NotDefinedHere errors It can happen that changeOwner sees symbols that are not defined yet at the phase where the method is run. Such symbols should be ignored. --- src/dotty/tools/dotc/ast/tpd.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dotty/tools/dotc/ast/tpd.scala b/src/dotty/tools/dotc/ast/tpd.scala index 989cae2168f8..21a52e1f8c9c 100644 --- a/src/dotty/tools/dotc/ast/tpd.scala +++ b/src/dotty/tools/dotc/ast/tpd.scala @@ -641,7 +641,8 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { def traverse(tree: Tree)(implicit ctx: Context) = tree match { case tree: DefTree => val sym = tree.symbol - if (sym.denot(ctx.withPhase(trans)).owner == from) { + val prevDenot = sym.denot(ctx.withPhase(trans)) + if (prevDenot.validFor.containsPhaseId(trans.id) && prevDenot.owner == from) { val d = sym.copySymDenotation(owner = to) d.installAfter(trans) d.transformAfter(trans, d => if (d.owner eq from) d.copySymDenotation(owner = to) else d) @@ -651,7 +652,7 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { traverseChildren(tree) } } - traverser.traverse(tree) + traverser.traverse(tree)(ctx.withMode(Mode.FutureDefsOK)) tree } From b30843c59757ccaaf00c6733bf81f15eb911e6c2 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Aug 2015 18:50:43 -0700 Subject: [PATCH 2/6] Fix miniphase assembly. There were two architectural errors here, which confused TreeTransforms and MiniPhases and which caused "NotDefinedHere" on transformFollowing: 1. TreeTransforms should not have idx fields, MiniPhases have them.2 2. TreeTransformers initialize arrays of MiniPhases not TreeTransforms. --- src/dotty/tools/dotc/core/Decorators.scala | 2 +- src/dotty/tools/dotc/core/Phases.scala | 18 +++++++-------- .../tools/dotc/transform/TreeChecker.scala | 2 +- .../tools/dotc/transform/TreeTransform.scala | 23 +++++++++---------- test/test/transform/TreeTransformerTest.scala | 10 ++++---- 5 files changed, 26 insertions(+), 29 deletions(-) diff --git a/src/dotty/tools/dotc/core/Decorators.scala b/src/dotty/tools/dotc/core/Decorators.scala index d90b959ab9bc..60c019bce733 100644 --- a/src/dotty/tools/dotc/core/Decorators.scala +++ b/src/dotty/tools/dotc/core/Decorators.scala @@ -135,7 +135,7 @@ object Decorators { */ implicit class PhaseListDecorator(val names: List[String]) extends AnyVal { def containsPhase(phase: Phase): Boolean = phase match { - case phase: TreeTransformer => phase.transformations.exists(trans => containsPhase(trans.phase)) + case phase: TreeTransformer => phase.miniPhases.exists(containsPhase) case _ => names exists { name => name == "all" || { diff --git a/src/dotty/tools/dotc/core/Phases.scala b/src/dotty/tools/dotc/core/Phases.scala index 42b03484e670..24eb19cd5ca8 100644 --- a/src/dotty/tools/dotc/core/Phases.scala +++ b/src/dotty/tools/dotc/core/Phases.scala @@ -109,11 +109,9 @@ object Phases { assert(false, s"Only tree transforms can be squashed, ${phase.phaseName} can not be squashed") } } - val transforms = filteredPhaseBlock.asInstanceOf[List[MiniPhase]].map(_.treeTransform) val block = new TreeTransformer { - override def phaseName: String = transformations.map(_.phase.phaseName).mkString("TreeTransform:{", ", ", "}") - - override def transformations: Array[TreeTransform] = transforms.toArray + override def phaseName: String = miniPhases.map(_.phaseName).mkString("TreeTransform:{", ", ", "}") + override def miniPhases: Array[MiniPhase] = filteredPhaseBlock.asInstanceOf[List[MiniPhase]].toArray } prevPhases ++= filteredPhaseBlock.map(_.getClazz) block @@ -145,7 +143,7 @@ object Phases { val flatPhases = collection.mutable.ListBuffer[Phase]() phasess.foreach(p => p match { - case t: TreeTransformer => flatPhases ++= t.transformations.map(_.phase) + case t: TreeTransformer => flatPhases ++= t.miniPhases case _ => flatPhases += p }) @@ -173,11 +171,11 @@ object Phases { val phase = phasess(i) phase match { case t: TreeTransformer => - val transforms = t.transformations - transforms.foreach{ x => - checkRequirements(x.phase) - x.phase.init(this, nextPhaseId)} - t.init(this, transforms.head.phase.id, transforms.last.phase.id) + val miniPhases = t.miniPhases + miniPhases.foreach{ phase => + checkRequirements(phase) + phase.init(this, nextPhaseId)} + t.init(this, miniPhases.head.id, miniPhases.last.id) case _ => phase.init(this, nextPhaseId) checkRequirements(phase) diff --git a/src/dotty/tools/dotc/transform/TreeChecker.scala b/src/dotty/tools/dotc/transform/TreeChecker.scala index a1847e456a2a..6ae76548046f 100644 --- a/src/dotty/tools/dotc/transform/TreeChecker.scala +++ b/src/dotty/tools/dotc/transform/TreeChecker.scala @@ -103,7 +103,7 @@ class TreeChecker extends Phase with SymTransformer { private def previousPhases(phases: List[Phase])(implicit ctx: Context): List[Phase] = phases match { case (phase: TreeTransformer) :: phases1 => - val subPhases = phase.transformations.map(_.phase) + val subPhases = phase.miniPhases val previousSubPhases = previousPhases(subPhases.toList) if (previousSubPhases.length == subPhases.length) previousSubPhases ::: previousPhases(phases1) else previousSubPhases diff --git a/src/dotty/tools/dotc/transform/TreeTransform.scala b/src/dotty/tools/dotc/transform/TreeTransform.scala index 62cd5dbe93bd..7a2280baa34e 100644 --- a/src/dotty/tools/dotc/transform/TreeTransform.scala +++ b/src/dotty/tools/dotc/transform/TreeTransform.scala @@ -63,9 +63,6 @@ object TreeTransforms { def treeTransformPhase: Phase = phase.next - /** id of this treeTransform in group */ - var idx: Int = _ - def prepareForIdent(tree: Ident)(implicit ctx: Context) = this def prepareForSelect(tree: Select)(implicit ctx: Context) = this def prepareForThis(tree: This)(implicit ctx: Context) = this @@ -137,10 +134,10 @@ object TreeTransforms { def transform(tree: Tree)(implicit ctx: Context, info: TransformerInfo): Tree = info.group.transform(tree, info, 0) /** Transform subtree using all transforms following the current one in this group */ - def transformFollowingDeep(tree: Tree)(implicit ctx: Context, info: TransformerInfo): Tree = info.group.transform(tree, info, idx + 1) + def transformFollowingDeep(tree: Tree)(implicit ctx: Context, info: TransformerInfo): Tree = info.group.transform(tree, info, phase.idx + 1) /** Transform single node using all transforms following the current one in this group */ - def transformFollowing(tree: Tree)(implicit ctx: Context, info: TransformerInfo): Tree = info.group.transformSingle(tree, idx + 1) + def transformFollowing(tree: Tree)(implicit ctx: Context, info: TransformerInfo): Tree = info.group.transformSingle(tree, phase.idx + 1) def atGroupEnd[T](action : Context => T)(implicit ctx: Context, info: TransformerInfo) = { val last = info.transformers(info.transformers.length - 1) @@ -152,6 +149,9 @@ object TreeTransforms { trait MiniPhase extends Phase { thisPhase => def treeTransform: TreeTransform + /** id of this mini phase in group */ + var idx: Int = _ + /** List of names of phases that should have finished their processing of all compilation units * before this phase starts */ @@ -159,7 +159,7 @@ object TreeTransforms { protected def mkTreeTransformer = new TreeTransformer { override def phaseName: String = thisPhase.phaseName - override def transformations = Array(treeTransform) + override def miniPhases = Array(thisPhase) } override def run(implicit ctx: Context): Unit = { @@ -197,7 +197,6 @@ object TreeTransforms { @sharable val NoTransform = new TreeTransform { def phase = unsupported("phase") - idx = -1 } type Mutator[T] = (TreeTransform, T, Context) => TreeTransform @@ -474,7 +473,7 @@ object TreeTransforms { /** A group of tree transforms that are applied in sequence during the same phase */ abstract class TreeTransformer extends Phase { - def transformations: Array[TreeTransform] + def miniPhases: Array[MiniPhase] override def run(implicit ctx: Context): Unit = { val curTree = ctx.compilationUnit.tpdTree @@ -549,10 +548,10 @@ object TreeTransforms { val prepForStats: Mutator[List[Tree]] = (trans, trees, ctx) => trans.prepareForStats(trees)(ctx) val prepForUnit: Mutator[Tree] = (trans, tree, ctx) => trans.prepareForUnit(tree)(ctx) - val initialTransformationsCache = transformations.zipWithIndex.map { - case (transform, id) => - transform.idx = id - transform + val initialTransformationsCache = miniPhases.zipWithIndex.map { + case (miniPhase, id) => + miniPhase.idx = id + miniPhase.treeTransform } val initialInfoCache = new TransformerInfo(initialTransformationsCache, new NXTransformations(initialTransformationsCache), this) diff --git a/test/test/transform/TreeTransformerTest.scala b/test/test/transform/TreeTransformerTest.scala index fadc44ab9495..df6175735d05 100644 --- a/test/test/transform/TreeTransformerTest.scala +++ b/test/test/transform/TreeTransformerTest.scala @@ -20,7 +20,7 @@ class TreeTransformerTest extends DottyTest { init(ctx, ctx.period.firstPhaseId, ctx.period.lastPhaseId) } val transformer = new TreeTransformer { - override def transformations = Array(new EmptyTransform) + override def miniPhases = Array(new EmptyTransform) override def phaseName: String = "test" } @@ -42,7 +42,7 @@ class TreeTransformerTest extends DottyTest { init(ctx, ctx.period.firstPhaseId, ctx.period.lastPhaseId) } val transformer = new TreeTransformer { - override def transformations = Array(new ConstantTransform) + override def miniPhases = Array(new ConstantTransform) override def phaseName: String = "test" } @@ -72,7 +72,7 @@ class TreeTransformerTest extends DottyTest { init(ctx, ctx.period.firstPhaseId, ctx.period.lastPhaseId) } val transformer = new TreeTransformer { - override def transformations = Array(new Transformation) + override def miniPhases = Array(new Transformation) override def phaseName: String = "test" @@ -119,7 +119,7 @@ class TreeTransformerTest extends DottyTest { init(ctx, ctx.period.firstPhaseId, ctx.period.lastPhaseId) } val transformer = new TreeTransformer { - override def transformations = Array(new Transformation1, new Transformation2) + override def miniPhases = Array(new Transformation1, new Transformation2) override def phaseName: String = "test" } @@ -187,7 +187,7 @@ class TreeTransformerTest extends DottyTest { init(ctx, ctx.period.firstPhaseId, ctx.period.lastPhaseId) } val transformer = new TreeTransformer { - override def transformations = Array(new Transformation1, new Transformation2) + override def miniPhases = Array(new Transformation1, new Transformation2) override def phaseName: String = "test" } From edaa193aac1d7880a81cd6a836fe3d7c3d652d45 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 14 Aug 2015 18:57:34 -0700 Subject: [PATCH 3/6] Fix CaputuredVars/LiftTry interaction. CapturedVars introduced an assignment that could cause a try to be executed with a non-empty stack, even after LiftTry had already run. We now avoid this by introducing a temporary variable. --- src/dotty/tools/dotc/core/Definitions.scala | 9 ++++-- .../tools/dotc/transform/CapturedVars.scala | 32 +++++++++++++++---- tests/run/liftedTry.scala | 12 ++++++- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/dotty/tools/dotc/core/Definitions.scala b/src/dotty/tools/dotc/core/Definitions.scala index 502b42987982..fcd9ef224f10 100644 --- a/src/dotty/tools/dotc/core/Definitions.scala +++ b/src/dotty/tools/dotc/core/Definitions.scala @@ -592,13 +592,16 @@ class Definitions { } /** The classes for which a Ref type exists. */ - lazy val refClasses: collection.Set[Symbol] = ScalaNumericValueClasses + BooleanClass + ObjectClass + lazy val refClassKeys: collection.Set[Symbol] = ScalaNumericValueClasses + BooleanClass + ObjectClass lazy val refClass: Map[Symbol, Symbol] = - refClasses.map(rc => rc -> ctx.requiredClass(s"scala.runtime.${rc.name}Ref")).toMap + refClassKeys.map(rc => rc -> ctx.requiredClass(s"scala.runtime.${rc.name}Ref")).toMap lazy val volatileRefClass: Map[Symbol, Symbol] = - refClasses.map(rc => rc -> ctx.requiredClass(s"scala.runtime.Volatile${rc.name}Ref")).toMap + refClassKeys.map(rc => rc -> ctx.requiredClass(s"scala.runtime.Volatile${rc.name}Ref")).toMap + + lazy val boxedRefClasses: collection.Set[Symbol] = + refClassKeys.flatMap(k => Set(refClass(k), volatileRefClass(k))) def wrapArrayMethodName(elemtp: Type): TermName = { val cls = elemtp.classSymbol diff --git a/src/dotty/tools/dotc/transform/CapturedVars.scala b/src/dotty/tools/dotc/transform/CapturedVars.scala index 86cf80073b75..0f1a60282f8a 100644 --- a/src/dotty/tools/dotc/transform/CapturedVars.scala +++ b/src/dotty/tools/dotc/transform/CapturedVars.scala @@ -92,14 +92,34 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer { thisTransfo else id } + /** If assignment is to a boxed ref type, e.g. + * + * intRef.elem = expr + * + * rewrite using a temporary var to + * + * val ev$n = expr + * intRef.elem = ev$n + * + * That way, we avoid the problem that `expr` might contain a `try` that would + * run on a non-empty stack (which is illegal under JVM rules). Note that LiftTry + * has already run before, so such `try`s would not be eliminated. + * + * Also: If the ref type lhs is followed by a cast (can be an artifact of nested translation), + * drop the cast. + */ override def transformAssign(tree: Assign)(implicit ctx: Context, info: TransformerInfo): Tree = { - val lhs1 = tree.lhs match { - case TypeApply(Select(qual @ Select(qual2, nme.elem), nme.asInstanceOf_), _) => - assert(captured(qual2.symbol)) - qual - case _ => tree.lhs + def recur(lhs: Tree): Tree = lhs match { + case TypeApply(Select(qual, nme.asInstanceOf_), _) => + val Select(_, nme.elem) = qual + recur(qual) + case Select(_, nme.elem) if defn.boxedRefClasses.contains(lhs.symbol.maybeOwner) => + val tempDef = transformFollowing(SyntheticValDef(ctx.freshName("ev$").toTermName, tree.rhs)) + transformFollowing(Block(tempDef :: Nil, cpy.Assign(tree)(lhs, ref(tempDef.symbol)))) + case _ => + tree } - cpy.Assign(tree)(lhs1, tree.rhs) + recur(tree.lhs) } } } diff --git a/tests/run/liftedTry.scala b/tests/run/liftedTry.scala index 2e4c25c2b690..0d956fc1f39c 100644 --- a/tests/run/liftedTry.scala +++ b/tests/run/liftedTry.scala @@ -12,10 +12,20 @@ object Test { foo(try 3 catch handle) - def main(args: Array[String]) = { + def main(args: Array[String]): Unit = { assert(x == 1) assert(foo(2) == 2) assert(foo(try raise(3) catch handle) == 3) + Tr.foo } } + +object Tr { + def fun(a: Int => Unit) = a(2) + def foo: Int = { + var s = 1 + s = try {fun(s = _); 3} catch{ case ex: Throwable => s = 4; 5 } + s + } +} From 4ff06368cda585e3a2d04af084a8fe09662e55f0 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 15 Aug 2015 11:16:35 -0700 Subject: [PATCH 4/6] Add a local val to lifted try to make sure owners are still legal. Checks the hypothesis that lifting a try may safely move expressions into a ValDef owned by a new temp var. --- tests/run/liftedTry.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/run/liftedTry.scala b/tests/run/liftedTry.scala index 0d956fc1f39c..5ff4add6dee3 100644 --- a/tests/run/liftedTry.scala +++ b/tests/run/liftedTry.scala @@ -25,7 +25,7 @@ object Tr { def fun(a: Int => Unit) = a(2) def foo: Int = { var s = 1 - s = try {fun(s = _); 3} catch{ case ex: Throwable => s = 4; 5 } + s = try {fun(s = _); 3} catch{ case ex: Throwable => val x = 4; s = x; 5 } s } } From 2cbf29e40af2494cc4a685b6c1e683d95bde95af Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 19 Aug 2015 14:12:17 -0700 Subject: [PATCH 5/6] Tweak to installAfter installAfter now overwrites denotaton also when the first denotation of a symbol is defined after the current phase. Previously, a new denotation with a last valid phase before a first valid phase was created. --- src/dotty/tools/dotc/core/Denotations.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/core/Denotations.scala b/src/dotty/tools/dotc/core/Denotations.scala index fc97fb32b05c..16a151e89ddd 100644 --- a/src/dotty/tools/dotc/core/Denotations.scala +++ b/src/dotty/tools/dotc/core/Denotations.scala @@ -680,7 +680,7 @@ object Denotations { // println(s"installing $this after $phase/${phase.id}, valid = ${current.validFor}") // printPeriods(current) this.validFor = Period(ctx.runId, targetId, current.validFor.lastPhaseId) - if (current.validFor.firstPhaseId == targetId) + if (current.validFor.firstPhaseId >= targetId) replaceDenotation(current) else { // insert this denotation after current From 5a36e2f20bb5dd9cb04a1e8010280995c6305a39 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 19 Aug 2015 14:12:49 -0700 Subject: [PATCH 6/6] ChangeOwnerAfter should also change owners of denotations defined later. --- src/dotty/tools/dotc/ast/tpd.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dotty/tools/dotc/ast/tpd.scala b/src/dotty/tools/dotc/ast/tpd.scala index 21a52e1f8c9c..5334bc45975f 100644 --- a/src/dotty/tools/dotc/ast/tpd.scala +++ b/src/dotty/tools/dotc/ast/tpd.scala @@ -642,7 +642,7 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo { case tree: DefTree => val sym = tree.symbol val prevDenot = sym.denot(ctx.withPhase(trans)) - if (prevDenot.validFor.containsPhaseId(trans.id) && prevDenot.owner == from) { + if (prevDenot.owner == from) { val d = sym.copySymDenotation(owner = to) d.installAfter(trans) d.transformAfter(trans, d => if (d.owner eq from) d.copySymDenotation(owner = to) else d)