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

Wrong codegen transform for final val with literal type but not literal rhs inside trait #17549

Closed
pweisenburger opened this issue May 21, 2023 · 1 comment

Comments

@pweisenburger
Copy link
Contributor

pweisenburger commented May 21, 2023

Was: Scala.js backend crashes for final val with literal type inside trait

Compiler version

3.2.2, 3.3.0-RC3

Minimized code

The following code crashes the compiler when using the Scala.js backend:

trait T:
  final val x: 1 = ???

Similar for this example:

trait T:
  def f(): Unit = ()
  final val x: "test" = { f(); "test" }

To trigger the issue, the field needs to be in a trait, it has to be a final val, the field needs a literal type and the right-hand-side needs to different from just being the literal itself (e.g., it compiles for final val x: 1 = 1).

Output

dotty.tools.FatalError: Illegal tree in gen of genInterface(): private val x: Int
class = trait T extends Object {
  def <init>(): Unit = 
    {
      this.x = ???()
      ()
    }
  private val x: Int
  final def x(): Int = this.x
}
@pweisenburger pweisenburger added itype:bug itype:crash stat:needs triage Every issue needs to have an "area" and "itype" label labels May 21, 2023
@WojciechMazur WojciechMazur added area:scala.js and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels May 22, 2023
@sjrd
Copy link
Member

sjrd commented May 23, 2023

The problem is not Scala.js-related. Scala.js is actually sort of "friendlier" than Scala/JVM, because it crashes at compile-time, whereas the JVM crashes at run-time. If we elaborate the test a little bit more to something that should run:

trait T:
  final val x: 1 =
    println("T.x")
    1
end T

trait U:
  def x: Any

class C extends T with U

object Test:
  def main(args: Array[String]): Unit =
    val c = new C
    println(c.x)

    val u: U = c
    println(u.x)
  end main
end Test

we get the following on the JVM:

Exception in thread "main" java.lang.ClassFormatError: Illegal field modifiers in class T: 0x12
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
        at java.lang.ClassLoader.defineClass1(Native Method)
        at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
        at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
        at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
        at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
        at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
        at Test$.main(i17549.scala:14)
        at Test.main(i17549.scala)

@sjrd sjrd changed the title Scala.js backend crashes for final val with literal type inside trait Wrong codegen transform for final val with literal type but not literal rhs inside trait May 23, 2023
sjrd added a commit to dotty-staging/dotty that referenced this issue May 23, 2023
…s need storing.

The Memoize and Constructors have to work together and agree on
which `final val`s actually need storing in a field. Previously,
they used slightly different criteria: one on the result type, and
one on the rhs (with an additional Scala.js-specific eligibility
condition). That discrepancy resulted in the crash/wrong codegen
in the issue.

We now unify both: we avoid a field if and only if all of the
following apply:

* it is a `final val`,
* its result *type* is a `ConstantType`, and
* it is eligible according to Scala.js semantics.

In particular, there is no condition on the right-hand-side. We
avoid a field even if the right-hand-side has side effects. The
side effects are moved to the constructor regardless.

---

This introduces both progressions and regressions in the amount of
fields we generate. We can avoid fields even for side-effecting
rhs'es, as long as the result type is constant. On the other hand,
we now create a field for `final val`s with non-constant result
type, even if the rhs is a literal.

While the latter is a regression for Scala 3, it aligns with the
behavior of Scala 2. It also has the nice benefit that whether or
not a val has a field is now independent of its *implementation*,
and only dependent on its *API*. Overall, I think this is a
trade-off worth taking.

We could reintroduce that optimization in the future (but in
classes only; not in traits), if we really want to, although that
would require dedicated code.
smarter added a commit that referenced this issue May 23, 2023
…d storing. (#17560)

The Memoize and Constructors have to work together and agree on which
`final val`s actually need storing in a field. Previously, they used
slightly different criteria: one on the result type, and one on the rhs
(with an additional Scala.js-specific eligibility condition). That
discrepancy resulted in the crash/wrong codegen in the issue.

We now unify both: we avoid a field if and only if all of the following
apply:

* it is a `final val`,
* its result *type* is a `ConstantType`, and
* it is eligible according to Scala.js semantics.

In particular, there is no condition on the right-hand-side. We avoid a
field even if the right-hand-side has side effects. The side effects are
moved to the constructor regardless.

---

This introduces both progressions and regressions in the amount of
fields we generate. We can avoid fields even for side-effecting rhs'es,
as long as the result type is constant. On the other hand, we now create
a field for `final val`s with non-constant result type, even if the rhs
is a literal.

While the latter is a regression for Scala 3, it aligns with the
behavior of Scala 2. It also has the nice benefit that whether or not a
val has a field is now independent of its *implementation*, and only
dependent on its *API*. Overall, I think this is a trade-off worth
taking.

We could reintroduce that optimization in the future (but in classes
only; not in traits), if we really want to, although that would require
dedicated code.
G1ng3r pushed a commit to G1ng3r/dotty that referenced this issue Sep 10, 2023
…s need storing.

The Memoize and Constructors have to work together and agree on
which `final val`s actually need storing in a field. Previously,
they used slightly different criteria: one on the result type, and
one on the rhs (with an additional Scala.js-specific eligibility
condition). That discrepancy resulted in the crash/wrong codegen
in the issue.

We now unify both: we avoid a field if and only if all of the
following apply:

* it is a `final val`,
* its result *type* is a `ConstantType`, and
* it is eligible according to Scala.js semantics.

In particular, there is no condition on the right-hand-side. We
avoid a field even if the right-hand-side has side effects. The
side effects are moved to the constructor regardless.

---

This introduces both progressions and regressions in the amount of
fields we generate. We can avoid fields even for side-effecting
rhs'es, as long as the result type is constant. On the other hand,
we now create a field for `final val`s with non-constant result
type, even if the rhs is a literal.

While the latter is a regression for Scala 3, it aligns with the
behavior of Scala 2. It also has the nice benefit that whether or
not a val has a field is now independent of its *implementation*,
and only dependent on its *API*. Overall, I think this is a
trade-off worth taking.

We could reintroduce that optimization in the future (but in
classes only; not in traits), if we really want to, although that
would require dedicated code.
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

3 participants