Skip to content

Fix #7843: Move LiftTry back to before CapturedVars #7846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class Compiler {
new InterceptedMethods, // Special handling of `==`, `|=`, `getClass` methods
new Getters, // Replace non-private vals and vars with getter defs (fields are added later)
new ElimByName, // Expand by-name parameter references
new LiftTry, // Put try expressions that might execute on non-empty stacks into their own methods
new CollectNullableFields, // Collect fields that can be nulled out after use in lazy initialization
new ElimOuterSelect, // Expand outer selections
new AugmentScala2Traits, // Augments Scala2 traits with additional members needed for mixin composition.
Expand All @@ -105,8 +106,7 @@ class Compiler {
// Note: constructors changes decls in transformTemplate, no InfoTransformers should be added after it
new FunctionalInterfaces, // Rewrites closures to implement @specialized types of Functions.
new Instrumentation, // Count closure allocations under -Yinstrument-closures
new GetClass, // Rewrites getClass calls on primitive types.
new LiftTry) :: // Put try expressions that might execute on non-empty stacks into their own methods their implementations
new GetClass) :: // Rewrites getClass calls on primitive types.
List(new LinkScala2Impls, // Redirect calls to trait methods defined by Scala 2.x, so that they now go to
new LambdaLift, // Lifts out nested functions to class scope, storing free variables in environments
// Note: in this mini-phase block scopes are incorrect. No phases that rely on scopes should be here
Expand Down
3 changes: 3 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/CapturedVars.scala
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class CapturedVars extends MiniPhase with IdentityDenotTransformer { thisPhase =
/** the following two members override abstract members in Transform */
val phaseName: String = "capturedVars"

override def runsAfterGroupsOf: Set[String] = Set(LiftTry.name)
// lifting tries changes what variables are considered to be captured

private[this] var Captured: Store.Location[collection.Set[Symbol]] = _
private def captured(implicit ctx: Context) = ctx.store(Captured)

Expand Down
19 changes: 12 additions & 7 deletions compiler/src/dotty/tools/dotc/transform/LiftTry.scala
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@ import util.Store
class LiftTry extends MiniPhase with IdentityDenotTransformer { thisPhase =>
import ast.tpd._

val phaseName: String = "liftTry"

// See tests/run/t2333.scala for an example where running after the group of LazyVals matters
override def runsAfterGroupsOf: Set[String] = Set(LazyVals.name)
val phaseName: String = LiftTry.name

private var NeedLift: Store.Location[Boolean] = _
private def needLift(implicit ctx: Context): Boolean = ctx.store(NeedLift)
Expand All @@ -48,9 +45,15 @@ class LiftTry extends MiniPhase with IdentityDenotTransformer { thisPhase =>
liftingCtx(true)

override def prepareForValDef(tree: ValDef)(implicit ctx: Context): Context =
if (!tree.symbol.exists ||
tree.symbol.isSelfSym ||
tree.symbol.owner == ctx.owner.enclosingMethod) ctx
if !tree.symbol.exists
|| tree.symbol.isSelfSym
|| tree.symbol.owner == ctx.owner.enclosingMethod
&& !tree.symbol.is(Lazy)
// The current implementation wraps initializers of lazy vals in
// calls to an initialize method, which means that a `try` in the
// initializer needs to be lifted. Note that the new scheme proposed
// in #6979 would avoid this.
then ctx
else liftingCtx(true)

override def prepareForAssign(tree: Assign)(implicit ctx: Context): Context =
Expand All @@ -75,3 +78,5 @@ class LiftTry extends MiniPhase with IdentityDenotTransformer { thisPhase =>
}
else tree
}
object LiftTry with
val name = "liftTry"
4 changes: 4 additions & 0 deletions tests/run/i7843.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
done outside try: false
setting done to true
done in catch clause: true
done
34 changes: 34 additions & 0 deletions tests/run/i7843.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import scala.concurrent._, duration._

object Test {
implicit val ec: scala.concurrent.ExecutionContext = scala.concurrent.ExecutionContext.global
val thunkFuture: Future[() => String] = Future.successful(() => throw new RuntimeException("Whoops"))

final def shouldTerminate: Future[Unit] =
thunkFuture.flatMap { thunk =>
def fold() = {
var done = false
while (!done) {
println(s"done outside try: $done")
try {
thunk()
} catch {
case _: Throwable =>
println("setting done to true")
done = true
println(s"done in catch clause: $done")
}
}
}

Future {
fold()
}
}


def main(args: Array[String]): Unit = {
Await.result(shouldTerminate, Duration.Inf)
println("done")
}
}