Fixes #1856: mimic Scalac sematic of recursive lazy vals.

[DarkDimius]

No description provided.

[DarkDimius]

This change affects only non-threadsafe lazy vals. The change can be boined down to whether lazy vals do:

flag  = true
value_lzy = rhs

or

value_lzy = rhs
flag = true

This difference wouldn't matter if lazy val wasn't recursive, but in case it is - it does matter.

Dotty used to the first approach, and my(undocumented) rationale was that this ensures that lazy val is only assigned once. The second one instead may assign multiple times making lazy val observably change value multiple times. Note that those values can only be observed if some code runs between the first rhs is assigned and the last one is, as in the following example:

object Test {
  var count: Int = 0
  def getLzy = {
   val read = lzy
   println("Lzy has been read as: " +read)
   read
 }

  lazy val lzy: Int = {
    if (count < 10) {
      println(s"Iteration $count")
      count += 1
      getLzy + 1
    } else 42
  }

  def main(args: Array[String]): Unit = {
    println(lzy)
    getLzy
  }
}

with the new Dotty behavior(and the Scalac one) this test prints:

Iteration 0
Iteration 1
Iteration 2
Iteration 3
Iteration 4
Iteration 5
Iteration 6
Iteration 7
Iteration 8
Iteration 9
Lzy has been read as: 42
Lzy has been read as: 43
Lzy has been read as: 44
Lzy has been read as: 45
Lzy has been read as: 46
Lzy has been read as: 47
Lzy has been read as: 48
Lzy has been read as: 49
Lzy has been read as: 50
Lzy has been read as: 51
52
Lzy has been read as: 52

As you can see in the test and output, you can observe multiple different values of a lazy val.
In the previous behavior, you were only able to observe 2 values: uninitialized and final.
I find the Dotty behavior(initialize once) better then the one in Scalac(initialize as many times as you want), but it would be very tricky for users to find this changed behaviour in their codebase.

Note that Lazy Vals Sip considers lazy vals with cycles to be errorneous code and does not address them. Dotty's thread safe lazy vals would wait forever on such a test, which I believe is better, as they would otherwise publish multiple values in a threaded environment.

[DarkDimius]

ping @odersky @smarter for input.
Also, @adriaanm, you may be interested to have a look.

[DarkDimius]

Continuing discussion on migration: as rewrite tool would stamp @volatile on every lazy val,
and the new thread-safe lazy vals always deadlock in this scenario, there will be no silent changes in behavior and users will have a reliable way to find their circular lazy vals(by having a look on thread stacks)

[smarter]

Thanks for looking into this! I think for now we should go with the scalac behavior since bootstrapping dotty relies on that and I'd like to get my full bootstrapping tests in (see #1856). But after merging this we should keep an issue open to consider alternative semantics.

as rewrite tool would stamp @volatile on every lazy val, and the new thread-safe lazy vals always deadlock in this scenario, there will be no silent changes in behavior

That alleviates one of my concern, but if lazy vals can potentially return null then they cannot have non-nullable types which is going to be very annoying. My other concern is that it is not clear at all how to fix Dotty to not rely on recursive lazy vals (the workarounds I mentioned in #1856 are not a good solution).

@DarkDimius: Maybe we should measure the performance impact of throwing an exception when a lazy val is called recursively?

[smarter]

Random thought: when a lazy val is initialized twice, throw an exception only if it's initialized to a different value each time, otherwise don't do anything. This way you can be sure that the value you read from your lazy val is the final, correct value.

[DarkDimius]

@smarter, for your curiosity, last commit implements the change that you've proposed.
It allows to initialize the same lazy val twice, as long as it initializes to the same value, or the old value was zero of respected type.

I quickly measured how much it would influence bytecode size: +50 bytes for every non-volatile lazy val. I don't think it's worth it though, for the same reason:

as rewrite tool would stamp @volatile on every lazy val, and the new thread-safe lazy vals always deadlock in this scenario, there will be no silent changes in behavior

[smarter]

Maybe we could have an @reentrant lazy val or something like that where we allow recursion? This way we can get dotty to bootstrap correctly, but this still doesn't solve the issue with non-nullable types for regular lazy val

[odersky]

I agree that recursive lazy vals should be an error - ideally there would be a way to report such an error, maybe controlled by an option, if it's too expensive to do by default.

Is there no way we can avoid the cycle in Dotty bootstrap?

[odersky]

I agree that recursive lazy vals should be an error - ideally there would be a way to report such an error, maybe controlled by an option, if it's too expensive to do by default.

Is there no way we can avoid the cycle in Dotty bootstrap?

[odersky]

It seems to me there is an easy way to avoid the problem in the bootstrap. Don't use a lazy val in ImportInfo. Use a cache var and a getter that implements the necessary logic.

[smarter]

Use a cache var and a getter that implements the necessary logic.

Yes, that works, though I had to replace both lazy val sym in ImportInfo and lazy val DottyPredefModuleRef in Definitions to get all the tests to pass, and I'm worried that the same issue might crop up for some other lazy val, but this should be enough to let me finish my sbt-based bootstrapping PR which is my main focus right now.

[odersky]

@smarter Great! Let's do that.

[odersky]

So in the light of the discussion this PR should be closed, right?

[smarter]

Sure.

[Blaisorblade]

EDIT: moved to #1856.