Miscompilation: When a try expression mutating a var is lifted, incorrect code is generated

[smarter]

object Test {
  def main(args: Array[String]): Unit = {
    var x: String = "foo"
    identity(
      try {
        x = "bar"
      } catch {
        case ex =>
          throw ex
      }
    )
    println(x)
  }
}

This should print bar but it prints foo instead, because the generated code looks like this:

    def main(args: String[]): Unit = 
      {
        var x: String = "foo"
        identity(
          {
            this.liftedTree1(x)
          }
        )
        println(x)
      }
    private final def liftedTree1(x$1: String): 
      scala.runtime.BoxedUnit(scala.runtime.BoxedUnit.UNIT)
     = 
      try 
        {
          {
            x$1 = "bar"
            scala.runtime.BoxedUnit$#UNIT
          }
        }
       catch 
        {
          case ex @ _ => 
            throw ex
        }
  }

This is of course non-sensical (we're mutating a method parameter!) and is detected by the compiler under -Ycheck:all. Either LiftTry needs to be taught to handle vars, or it needs to be moved before CapturedVars.

[odersky]

I agree that LiftTry should be moved before CapturedVars. But then we run into this:

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

Since LazyVals and CapturedVars are in the same group we are out of choices. I looked at t2333.scala. The problem is that LazyVals generates this code:

        def b$lzyINIT1(): Int = 
          b$lzy1.synchronized[Int](
            if b$lzy1.initialized() then b$lzy1.value() else 
              b$lzy1.initialize(
                try 2 catch 
                  {
                    case _:Throwable => 0
                  }
              )
          )

and then LiftTry fails to lift out the try from the initialize call. The reason is that the question
whether to lift or not is determined by the context (i.e. where does the try appear?) and that
context is defined at the start of the miniphase group. At the start, it still looks like this:

lazy def b() = try ... 

which seems to indicate that no lifting is necessary. But once LazyVals did its job lifting is in fact necessary.

Possible remedies:

1. Make LiftTry aware that a try in a lazy def has to be lifted, no matter what, or
2. Tweak LazyVals to produce val x = try ... ; initialize(x) instead of initialize(try ... ).

Number (2) is the better fix, since it avoids unnecessary lifting.

[odersky]

Note that the algorithm for redesigned lazyvals #6979 would not have this problem. The initializing code is not passed as an argument there.