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

Stack overflow when summoning Mirror for local class #20416

Open
mrdziuban opened this issue May 15, 2024 · 10 comments
Open

Stack overflow when summoning Mirror for local class #20416

mrdziuban opened this issue May 15, 2024 · 10 comments

Comments

@mrdziuban
Copy link

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
Copy link
Member

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
Copy link
Author

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

@bishabosha
Copy link
Member

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

@mrdziuban
Copy link
Author

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
Copy link
Member

can use a lazy val?

@mrdziuban
Copy link
Author

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
Copy link
Member

bishabosha commented May 15, 2024

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

@mrdziuban
Copy link
Author

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 -- Move anonymous classes out of inlined code and into constructor methods circe/circe#2230

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

@Gedochao Gedochao removed the stat:needs triage Every issue needs to have an "area" and "itype" label label May 17, 2024
@mrdziuban
Copy link
Author

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
Copy link
Contributor

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

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

4 participants