scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.83k stars 1.05k forks source link

Stack overflow when summoning `Mirror` for local class #20416

Open mrdziuban opened 4 months ago

mrdziuban commented 4 months ago

Compiler version

3.3.3, 3.4.2, and the latest nightly 3.5.0-RC1-bin-20240514-7c9aae3-NIGHTLY

Minimized code

https://scastie.scala-lang.org/mrdziuban/6oBpwuegRvOjFsuhXKbfNA

def test(): Unit = {
  case class A()
  object A {
    val mirror = summon[scala.deriving.Mirror.ProductOf[A]]
  }
  println(A.mirror)
}

test()

Output

java.lang.StackOverflowError
    at Playground$A$3$.<init>(main.scala:5)
    at Playground$.A$lzyINIT1$1(main.scala:5)
    at Playground$.Playground$$$_$A$2(main.scala:5)
    at Playground$A$3$.<init>(main.scala:6)
    at Playground$.A$lzyINIT1$1(main.scala:5)
    at Playground$.Playground$$$_$A$2(main.scala:5)
    at Playground$A$3$.<init>(main.scala:6)
    at Playground$.A$lzyINIT1$1(main.scala:5)
    at Playground$.Playground$$$_$A$2(main.scala:5)
    at Playground$A$3$.<init>(main.scala:6)
    ...

Expectation

Summoning the Mirror should work and not cause an error

bishabosha commented 4 months ago

the reason this happens is because you are initialising A in an infinite loop, the Mirror of class A is object A, so you are trying to store A in a field in its own body.

exactly like you did this:

def foo() =
  object Foo:
    val mirror = Foo
  println(Foo.mirror)

foo()
mrdziuban commented 4 months ago

I see, thanks for explaining @bishabosha. Why does it work for non-local classes though?

bishabosha commented 4 months ago

the solution here could be a linter that checks for these kinds of cases. (maybe @liufengyun knows)?

mrdziuban commented 4 months ago

For context, this came up in circe/circe#2263. It seems like disallowing cases like this would also mean derivation would no longer be possible, which feels less than ideal.

bishabosha commented 4 months ago

can use a lazy val?

mrdziuban commented 4 months ago

Yeah using a lazy val works as a workaround but in the context of derivation there's no way to make sure all downstream users do so

bishabosha commented 4 months ago

well is it possible to delay the evaluation of the mirror in the circe deriving code since you are retaining the mirror?

mrdziuban commented 4 months ago

That sounds promising, but I've tried a number of things and can't get it working. The only fix I've found so far is twofold:

  1. Mark the mirror param here as inline -- https://github.com/circe/circe/blob/v0.14.7/modules/core/shared/src/main/scala-3/io/circe/derivation/ConfiguredDecoder.scala#L222
  2. Construct the ConfiguredDecoder in the inline method rather than proxying to ConfiguredDecoder.of

Both of these have their own issues though:

  1. This requires downgrading Scala to 3.2.2, which in turn requires downgrading sbt and cats. In Scala 3.3 and later, the inline mirror results in errors like this:
        -- [E083] Type Error: /Users/matt/circe/modules/core/shared/src/main/scala-3/io/circe/derivation/ConfiguredDecoder.scala:223:39
    223 |    ConfiguredDecoder.of[A](constValue[mirror.MirroredLabel], decoders[A], summonLabels[mirror.MirroredElemLabels])
        |                                       ^^^^^^
        |(mirror : scala.deriving.Mirror.Of[A]) is not a valid type prefix, since it is not an immutable path
  2. This was purposely changed recently to avoid generating a new class file per call to derived -- https://github.com/circe/circe/pull/2230

Here's my WIP branch in case you see anything I might be missing: https://github.com/circe/circe/compare/series/0.14.x...mblink:fix-2263

mrdziuban commented 4 months ago

I'm far from an expert in interpreting how this might be related, but in case it helps someone else here's the output and diff of compiling a top-level vs. local class with -Xprint:genBCode

top-level-class-output.txt local-class-output.txt

// top-level-class.scala
case object A {
  val mirror = summon[scala.deriving.Mirror.ProductOf[A.type]]
}
// local-class.scala
def test(): Unit = {
  case object A {
    val mirror = summon[scala.deriving.Mirror.ProductOf[A.type]]
  }
}

I can follow why there's a stack overflow with the local version -- during the initialization of the lazy val, the class constructor calls rs$line$1$$$_$A$1, which calls A$lzyINIT1$1, which calls the class constructor, etc.

Other notable differences (to me at least) are:

  1. case object A is represented as a module case class in the top-level version as opposed to a case class in the local version
  2. The class constructor takes a LazyRef representing an instance of the class itself
  3. val mirror is represented as a private <static> val in the top-level version as opposed to a private val in the local version
diff --git a/Users/matt/Desktop/top-level-class-output.txt b/Users/matt/Desktop/local-class-output.txt
index c8c1fa23f6b..906c223349f 100644
--- a/Users/matt/Desktop/top-level-class-output.txt
+++ b/Users/matt/Desktop/local-class-output.txt
@@ -7,17 +7,33 @@ package <empty> {
       }
     private def writeReplace(): Object =
       new scala.runtime.ModuleSerializationProxy(classOf[rs$line$1])
-    final lazy module case <static> val A: rs$line$1$A = new rs$line$1$A()
+    def test(): Unit =
+      {
+        lazy var A$lzy1: scala.runtime.LazyRef = new scala.runtime.LazyRef()
+        ()
+      }
+    private final def A$lzyINIT1$1(A$lzy1$1: scala.runtime.LazyRef):
+      rs$line$1$A$2 =
+      A$lzy1$1.synchronized[rs$line$1$A$2](
+        (if A$lzy1$1.initialized() then A$lzy1$1.value() else
+          A$lzy1$1.initialize(new rs$line$1$A$2(A$lzy1$1))).asInstanceOf[
+          rs$line$1$A$2]
+      )
+    final lazy case def rs$line$1$$$_$A$1(A$lzy1$2: scala.runtime.LazyRef):
+      rs$line$1$A$2 =
+      (if A$lzy1$2.initialized() then A$lzy1$2.value() else
+        this.A$lzyINIT1$1(A$lzy1$2)).asInstanceOf[rs$line$1$A$2]
   }
-  final module case class rs$line$1$A extends Object, Product,
+  private final case class rs$line$1$A$2 extends Object, Product,
     java.io.Serializable, scala.deriving.Mirror.Mirror$Singleton {
-    def <init>(): Unit =
+    def <init>(A$lzy1$3: scala.runtime.LazyRef): Unit =
       {
         super()
-        mirror =
+        this.mirror =
           {
             val x$proxy1: scala.deriving.Mirror.Mirror$Singleton =
-              A:scala.deriving.Mirror.Mirror$Singleton
+              rs$line$1.this.rs$line$1$$$_$A$1(A$lzy1$3):
+                scala.deriving.Mirror.Mirror$Singleton
             x$proxy1
           }
         ()
@@ -28,12 +44,10 @@ package <empty> {
       super[Product].productElementNames()
     def fromProduct(p: Product): scala.deriving.Mirror.Mirror$Singleton =
       super[Singleton].fromProduct(p)
-    private def writeReplace(): Object =
-      new scala.runtime.ModuleSerializationProxy(classOf[rs$line$1$A])
     override def hashCode(): Int = 65
     override def toString(): String = "A"
     override def canEqual(that: Object): Boolean =
-      that.isInstanceOf[rs$line$1$A]
+      that.isInstanceOf[rs$line$1$A$2]
     override def productArity(): Int = 0
     override def productPrefix(): String = "A"
     override def productElement(n: Int): Object =
@@ -48,8 +62,8 @@ package <empty> {
           case val x2: Int = n
           throw new IndexOutOfBoundsException(Int.box(n).toString())
         }
-    private <static> val mirror: scala.deriving.Mirror.Mirror$Singleton
-    def mirror(): scala.deriving.Mirror.Mirror$Singleton = mirror
+    private val mirror: scala.deriving.Mirror.Mirror$Singleton
+    def mirror(): scala.deriving.Mirror.Mirror$Singleton = this.mirror
     def fromProduct(p: Product): Object = this.fromProduct(p)
   }
   final lazy module val rs$line$1: rs$line$1 = new rs$line$1()
liufengyun commented 4 months ago

the solution here could be a linter that checks for these kinds of cases. (maybe @liufengyun knows)?

Non-termination is not captured by the initialization checker, as we know accurately checking non-termination is impossible.

To soundly check non-termination, the checker would report many false positives, which will be annoying.

To address the problem above and related problems with simple non-termination, it seems an unsound check of blatant non-terminations that only report true positives will be useful in practice.

/cc: @olhotak @EnzeXing