Skip to content
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

vals in a nested singleton object may be initialized to null #16152

Closed
matil019 opened this issue Oct 7, 2022 · 11 comments
Closed

vals in a nested singleton object may be initialized to null #16152

matil019 opened this issue Oct 7, 2022 · 11 comments

Comments

@matil019
Copy link

matil019 commented Oct 7, 2022

Compiler version

3.0.0, 3.1.3, 3.2.0 and 3.2.2-RC1-bin-20221006-a6a3385-NIGHTLY

Minimized code

final case class Foo(name: String)

object Foo {
  object Foos {
    val Name1 = Foo("Apple")
    val Name2 = Foo("Banana")
    val Name3 = Foo("Cherry")
    // Using explicit `new`s prevents the issue
    // val Name1 = new Foo("Apple")
    // val Name2 = new Foo("Banana")
    // val Name3 = new Foo("Cherry")
  }

  val allFoos: Seq[Foo] = {
    import Foos._
    Seq(Name1, Name2, Name3)
  }
}

object Main {
  def main(args: Array[String]): Unit = {
    println(Foo.Foos.Name1)
    println(Foo.allFoos)
    // Accessing in the reverse order (or not accessing Name1 at all) prevents the issue
    // println(Foo.allFoos)
    // println(Foo.Foos.Name1)
  }
}

Output

Foo(Apple)
List(null, null, null)

Expectation

The output should be:

Foo(Apple)
List(Foo(Apple), Foo(Banana), Foo(Cherry))

Replacing any of the commented part of the program above prevents the issue. Also, Scala 2.13.9 produces the expected output without any modification. So, this is a behavior change between 2.13 and 3.

@matil019 matil019 added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 7, 2022
@som-snytt
Copy link
Contributor

Scala 2 lacks "universal apply" and rewrites the "case apply" to "new" in refchecks. I think these init issues are a fact of life?

I would expect (?) -Ysafe-init to tell me something.

Also for the record, I can never remember -Ysafe-init. I start with -Ycheck-init and then spend five minutes searching the output of -Y and sometimes -X too.

@matil019
Copy link
Author

matil019 commented Oct 8, 2022

Unfortunately, -Ysafe-init has no effect (no compilation warnings/errors).

@matil019
Copy link
Author

matil019 commented Oct 8, 2022

It turned out that Scala 2.13 has the same problem when a handmade factory method is used instead:

final case class Bar(name: String)

object Bar {
  object Bars {
    val Name1 = Bar.make("Apple")
    val Name2 = Bar.make("Banana")
    val Name3 = Bar.make("Cherry")
  }

  def make(name: String): Bar = Bar(name)

  val allBars: Seq[Bar] = {
    import Bars._
    Seq(Name1, Name2, Name3)
  }
}

object Main2 {
  def main(args: Array[String]): Unit = {
    println(Bar.Bars.Name1)
    println(Bar.allBars)
  }
}

Output (2.13.9):

Bar(Apple)
List(null, null, null)

I was not able to find an existing bug report for this in scala/bug.

@som-snytt
Copy link
Contributor

Sample Scala 2 issue

scala/bug#3342

@liufengyun
Copy link
Contributor

Related: #11262 #9176

Currently -Ysafe-init is not capable of handling static objects.

@liufengyun liufengyun added area:initialization and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 8, 2022
@bertlebee
Copy link

@odersky how is this not a bug? surely this is not the expected behaviour. I guess fixing a bug would be an enhancement, but that doesn't mean this isn't broken behaviour.

@som-snytt
Copy link
Contributor

@robmwalsh maybe a 3rd party linter could say, "This is the expected behavior which you may not have expected."

(Personally, I am on the fence. I'd prefer the language lock down cases which are obvious from a "platform" perspective.)

@odersky
Copy link
Contributor

odersky commented Dec 28, 2022

@robmwalsh It is very much the expected behavior according to the rules for initialization. The reason Scala-2 does not have the behavior is that Scala-2 silently rewrites a case class apply into a constructor call, thereby skipping the initialization of the enclosing object. This is surprising behavior, because it changes initialization order in unforeseen ways, even though it happens to be beneficial in this case.

So the only thing we should do is diagnose better that this is not initialization safe.

@bertlebee
Copy link

@robmwash It is very much the expected behavior according to the rules for initialization. The reason Scala-2 does not have the behavior is that Scala-2 silently rewrites a case class apply into a constructor call, thereby skipping the initialization of the enclosing object. This is surprising behavior, because it changes initialization order in unforeseen ways, even though it happens to be beneficial in this case.

So the only thing we should do is diagnose better that this is not initialization safe.

Fair enough I guess. These initialization shenanigans are going to be a real sticking point for new users. I'm very used to the behavior from scala 2 which has never caused me any real trouble. I stumbled across this issue after getting stung by #16456 (the 2nd stack overflow case) and it's cost me another couple of days of digging through the compiler trying to figure out the cause. Diagnosing better would very much be appreciated. I've raised #16593 which will help with crash diagnosis in general, though I'm not sure about this one in particular since it will still compile (I don't actually think it should compile if it's unsound)

@liufengyun
Copy link
Contributor

So the only thing we should do is diagnose better that this is not initialization safe.

FYI, @olhotak and I are still investigating a solution to this problem (One of previous attempts #12698). The challenge here is to have a solution that is simple, expressive, fast, and sound. If things go smoothly, we will propose a PR in the coming month(s).

liufengyun added a commit that referenced this issue Jun 16, 2023
The problem is illustrated by the example below:

``` Scala
class Foo(val opposite: Foo)
case object A extends Foo(B)     // A -> B
case object B extends Foo(A)     // B -> A
```
The check aims to be simple for programmers to understand, expressive,
fast, and sound.

The check is centered around two design ideas: (1) initialization-time
irrelevance; (2) partial ordering.

The check enforces the principle of _initialization-time irrelevance_,
which means that the time when a static object is initialized should not
change program semantics. For that purpose, it enforces the following
rule:

> **The initialization of a static object should not directly or
indirectly read or write mutable state owned by another static object**.

This principle not only puts the initialization of static objects on a
solid foundation but also avoids whole-program analysis.

Partial ordering means that the initialization dependencies of static
objects form a directed-acyclic graph (DAG). No cycles with length
bigger than 1 allowed --- which might lead to deadlocks in the presence
of concurrency and strong coupling & subtle contracts between objects.

Related Issues:

#16152
#9176
#11262
scala/bug#9312
scala/bug#9115
scala/bug#9261
scala/bug#5366
scala/bug#9360
@liufengyun
Copy link
Contributor

This can now be checked with the experimental option: -Ysafe-init-global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants