Recursively calling a lazy val is broken, prevents bootstrapping tests from passing

[smarter]

I don't know if any of this is specified but:

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

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

With scalac:

Iteration 0
Iteration 1
Iteration 2
Iteration 3
Iteration 4
Iteration 5
Iteration 6
Iteration 7
Iteration 8
Iteration 9
42

With dotty:

Iteration 0
0

This is not just a theoretical problem, I hit this while trying to get the sbt-based bootstrapping tests to work, the lazy val was https://github.com/lampepfl/dotty/blob/119725799675ee00d6d3374771765b88e4de67bc/compiler/src/dotty/tools/dotc/typer/ImportInfo.scala#L30, the cycle involves looking up the root DottyPredef import while completing the import (stacktrace: https://gist.github.com/smarter/b2126f895a8458506e58710c93bc9342). I assume that this is not a loop with the Scala2-compiled dotty because we carefully avoid cycles in various ways.

[smarter]

The spec is super vague: http://www.scala-lang.org/files/archive/spec/2.11/05-classes-and-objects.html#lazy

Attempting to access a lazy value during its initialization might lead to looping behavior.

[odersky]

I think the spec permits the new behavior (and it is actually more useful than the old one), so I would opt for disabling the test.

…

On Sat, Dec 24, 2016 at 4:16 AM, Guillaume Martres ***@***.*** > wrote: The spec is super-vague: http://www.scala-lang.org/ files/archive/spec/2.11/05-classes-and-objects.html#lazy Attempting to access a lazy value during its initialization might lead to looping behavior. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwlVrbOKavOwIJWRxgVmCSYW7R4fTPZks5rLGR7gaJpZM4LVHdu> . {"api_version":"1.0","publisher":{"api_key":" 05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity": {"external_key":"github/lampepfl/dotty","title":" lampepfl/dotty","subtitle":"GitHub repository","main_image_url":" https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88- 11e6-95fc-7290892c7bb5.png","avatar_image_url":"https:// cloud.githubusercontent.com/assets/143418/15842166/ 7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/lampepfl/dotty"}}," ***@***.*** in #1856: The spec is super-vague: http://www.scala-lang.org/files/archive/spec/2.11/05- classes-and-objects.html#lazy\r\n\u003e Attempting to access a lazy value during its initialization might lead to looping behavior."}],"action":{"name":"View Issue","url":"https://github. com/lampepfl/dotty/issues/1856#issuecomment-269059401"}}}

-- Prof. Martin Odersky LAMP/IC, EPFL

[smarter]

so I would opt for disabling the test.

The test in question is tasty_bootstrap so I can't really disable it, however I can also fix the bug by rewriting SourceFile in java and adding an explicit null check in the code, see the last commits of https://github.com/dotty-staging/dotty/commits/fix/bootstrap-1856
(If I add the null check without rewriting SourceFile in java I get a cyclic import error involving scala.annotation.Annotation).

I think the spec permits the new behavior (and it is actually more useful than the old one)

It does, but consider that this means that the same code will silently have different semantics depending on whether it was compiled with scalac or dotty. At the very least, this should be clearly documented somewhere. I would be more confortable with a difference in semantics if it was obvious (for example, the recursive call could throw an exception instead of returning null).

[smarter]

Also, consider what this means for non-nullable types, are we going to require that lazy vals always need to have a nullable type?

[smarter]

I've hit this again, this time while compiling compileStdLib with a bootstrapped partest, see: dotty-staging@cf4fdf6
The change to PlainPrinter is especially nasty: defn.UnqualifiedOwnerTypes is indirectly going to call defn.DottyPredefModuleRef, if we're in the middle of computing defn.DottyPredefModuleRef this is going to return null and since defn.UnqualifiedOwnerTypes is a lazy val, this means that it will be incorrectly set forever. So I have to check if we're currently computing defn.DottyPredefModuleRef (by comparing it against null) before calling defn.UnqualifiedOwnerTypes. This completely breaks abstraction layers.

[DarkDimius]

I did make a fix that changes behavior, but it has substantial side effects that I'd prefer to discuss.
Lets continue discussion in #1892

[Blaisorblade]

SIP-20 considers circular initializations erroneous code. That would be the "principled" approach. Why not just throw an exception then always? Other semantics are not something you'd call lazy vals.

The latest variant of #1892 still allowed observable changes for lazy vals if the first initialization produces a zero for the expected type. In comparison, having three states (initial; initialization started, no calls allowed till initialization completed; initialization completed), like in SIP-20, avoids this issue.
(EDIT: I've written more on this but summarized it away).

[DarkDimius]

Checking for circular initialization reliably would require substantial modifications to the scheme, and those would introduce slowdowns. In particular, the current non-@volatile lazy vals scheme doesn't introduce a single memory barrier.

As the actual critical blocker bug is now fixed, I'm closing this one. Feel free to open a separate issues for topics discussed here.