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

Fix #9970: Reconcile how Scala 3 and 2 decide whether a trait has $init$. #10530

Merged
merged 1 commit into from Dec 9, 2020

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented Nov 27, 2020

Scala 2 uses a very simple mechanism to detect traits that have or don't have a $init$ method:

  • At parsing time, a $init$ method is created if and only if there at least one member in the body that "requires" it.
  • $init$ are otherwise never created nor removed.
  • A call to T.$init$ in a subclass C is generated if and only if T.$init$ exists.

Scala 3 has a flags-based approach to the above:

  • At parsing time, a <init> method is always created for traits.
  • The NoInits flag is added if the body does not contain any member that "requires" an initialization.
  • In addition, the constructor receives the StableRealizable flag if the trait and all its base classes/traits have the NoInits flag. This is then used for purity analysis in the inliner.
  • The Constructors phase creates a $init$ method for traits that do not have the NoInits flag, and generates calls based on the same criteria.

Now, one might think that this is all fine, except it isn't when we mix Scala 2 and 3, and in particular when a Scala 2 class extend a Scala 3 trait.

Indeed, since Scala 3 always defines a <init> method in traits, which Scala 2 translates as $init$, Scala 2 would think that it always needs to emit a call to $init$ (even for traits where Scala 3 does not, in fact, emit a $init$ method in the end). This was mitigated in the TASTy reader of Scala 2 by removing $init$ if it has the STABLE flag (coming from StableRealizable).

This would have been fine if StableRealizable was present if and only if the owner trait has NoInits. But until this commit, that was not the case: a trait without initialization in itself, but extending a trait with initialization code, would have the flag NoInits but its constructor would not have the StableRealizable flag.

Therefore, this commit basically reconciles the NoInits and StableRealizable flags, so that Scala 2 understands whether or not a Scala 3 trait has a $init$ method. We also align those flags when reading from Scala 2 traits, so that Scala 3 also understands whether or not a Scala 2 trait has a $init$ trait.

This solves the compatibility issue between Scala 2 and Scala 3.

One detail remains. The attentive reader may have noticed the quotes in 'an element of the body that "requires" initialization'. Scala 2 and Scala 3 do not agree on what requires initialization: notably, Scala 2 thinks that a concrete def requires initialization, whereas Scala 3 knows that this is not the case. This is not an issue for synchronous interoperability between Scala 2 and 3, since each decides on its own which of its traits has a $init$ method, and communicates that fact to the other version.

However, this still poses an issue for "diachronic" compatibility: if a library defines a trait with a concrete def and is compiled by Scala 2 in version v1, that trait will have a $init$. When the library upgrades to Scala 3 in version v2, the trait will lose the $init$, which can break third-party subclasses.

This commit does not address this issue. There are two ways we could do so:

  • Precisely align the detection of whether a trait should have a $init$ method with Scala 2 (notably, adding one when there is a
    concrete def), or
  • Always emit an empty static $init$ method, even for traits that have the NoInits flag (but do not generate calls to them, nor have that affect whether or not subclasses are considered pure).

@anatoliykmetyuk anatoliykmetyuk linked an issue Nov 27, 2020 that may be closed by this pull request
@anatoliykmetyuk anatoliykmetyuk added this to the 3.0.0-RC1 milestone Nov 27, 2020
@sjrd
Copy link
Member Author

sjrd commented Nov 27, 2020

Well, I have no idea how to test this in a remotely automated way. See #9970 (comment)

What I did to locally test the original report was:

  • Temporarily reset the TASTy version to 24.0.

  • Compile the example snippet with dotty from sbt with > scalac TheSnippet.scala

  • Compile the following source:

    object MySimpleApp extends IOApp.Simple {
      def run: Unit = {
        println("hello")
        println(foo)
      }
    }
    

    with Scala 2.13.4 with $ scalac -cp .;C:\Users\sjrdo\Documents\Projets\dotty\library\..\out\bootstrap\scala3-library-bootstrapped\scala-3.0.0-RC1\scala3-library_3.0.0-RC1-3.0.0-RC1-bin-SNAPSHOT.jar -Ytasty-reader MySimpleApp.scala

  • Run it with scala -cp ... MySimpleApp -> it runs

  • Inspect with javap -cp . -c MySimpleApp$ -> it prints the expected result:

Compiled from "MySimpleApp.scala"
public final class MySimpleApp$ implements IOApp$Simple {
  public static final MySimpleApp$ MODULE$;

  private static int foo;

  public static {};
    Code:
       0: new           #2                  // class MySimpleApp$
       3: dup
       4: invokespecial #19                 // Method "<init>":()V
       7: putstatic     #21                 // Field MODULE$:LMySimpleApp$;
      10: getstatic     #21                 // Field MODULE$:LMySimpleApp$;
      13: invokestatic  #25                 // InterfaceMethod IOApp.$init$:(LIOApp;)V
      16: return

  public final int run(scala.collection.immutable.List<java.lang.String>);
    Code:
       0: aload_0
       1: aload_1
       2: invokestatic  #33                 // InterfaceMethod IOApp$Simple.run$:(LIOApp$Simple;Lscala/collection/immutable/List;)I
       5: ireturn

  public final void main(java.lang.String[]);
    Code:
       0: aload_0
       1: aload_1
       2: invokestatic  #41                 // InterfaceMethod IOApp.main$:(LIOApp;[Ljava/lang/String;)V
       5: return

  public int foo();
    Code:
       0: getstatic     #45                 // Field foo:I
       3: ireturn

  public void IOApp$_setter_$foo_$eq(int);
    Code:
       0: iload_1
       1: putstatic     #45                 // Field foo:I
       4: return

  public void run();
    Code:
       0: getstatic     #53                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
       3: ldc           #55                 // String hello
       5: invokevirtual #59                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
       8: getstatic     #53                 // Field scala/Predef$.MODULE$:Lscala/Predef$;
      11: aload_0
      12: invokevirtual #61                 // Method foo:()I
      15: invokestatic  #67                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      18: invokevirtual #59                 // Method scala/Predef$.println:(Ljava/lang/Object;)V
      21: return

  private MySimpleApp$();
    Code:
       0: aload_0
       1: invokespecial #68                 // Method java/lang/Object."<init>":()V
       4: return
}

Note in particular that there is no call to IOApp$Simple.$init$ in the constructor MySimpleApp$() at the end of transcript.


I welcome any suggestion how to add a test for this, but in the meantime I will mark this as ready for review.

@sjrd sjrd marked this pull request as ready for review November 27, 2020 15:18
@nicolasstucki
Copy link
Contributor

Maybe we can add tests in compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala

@sjrd
Copy link
Member Author

sjrd commented Nov 27, 2020

No, that wouldn't test what the issue was about. The problem in that specific case was not the bytecode, but the Tasty signatures interpreted by the Tasty reader in Scala 2. The bytecode emitted by Scala 3 hasn't changed a bit in this specific case.

@sjrd sjrd force-pushed the reconcile-noinits-with-scala2 branch 2 times, most recently from 75d19ef to c2f35b8 Compare December 1, 2020 08:16
@sjrd
Copy link
Member Author

sjrd commented Dec 1, 2020

I added a TASTy Inspector test, as suggested by Nicolas yesterday.

Copy link
Member

@bishabosha bishabosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my review I compiled the two examples that were tricky from #9970 with this commit, I then took the latest scala 2.13.x commit, changed the tasty version to 26.0, and compiled and ran the following apps, in-turn, consuming ioapp:

// for the IOApp example with no `run`
object Foo extends IOApp.Simple {}

object Main extends App {
  println(Foo)
}
// for the more complex example that defines `run`
object Main extends IOApp.Simple {
  def run = println("Foo")
}

there were no crashes. I think the assumption that entering $init$ causes it to be called is correct

…s $init$.

Scala 2 uses a very simple mechanism to detect traits that have or
don't have a `$init$` method:

* At parsing time, a `$init$` method is created if and only if
  there at least one member in the body that "requires" it.
* `$init$` are otherwise never created nor removed.
* A call to `T.$init$` in a subclass `C` is generated if and only
  if `T.$init$` exists.

Scala 3 has a flags-based approach to the above:

* At parsing time, a `<init>` method is always created for traits.
* The `NoInits` flag is added if the body does not contain any
  member that "requires" an initialization.
* In addition, the constructor receives the `StableRealizable` flag
  if the trait *and all its base classes/traits* have the `NoInits`
  flag. This is then used for purity analysis in the inliner.
* The `Constructors` phase creates a `$init$` method for traits
  that do not have the `NoInits` flag, and generates calls based on
  the same criteria.

Now, one might think that this is all fine, except it isn't when we
mix Scala 2 and 3, and in particular when a Scala 2 class extend a
Scala 3 trait.

Indeed, since Scala 3 *always* defines a `<init>` method in traits,
which Scala 2 translates as `$init$`, Scala 2 would think that it
always needs to emit a call to `$init$` (even for traits where
Scala 3 does not, in fact, emit a `$init$` method in the end).
This was mitigated in the TASTy reader of Scala 2 by removing
`$init$` if it has the `STABLE` flag (coming from
`StableRealizable`).

This would have been fine if `StableRealizable` was present if and
only if the owner trait has `NoInits`. But until this commit, that
was not the case: a trait without initialization in itself, but
extending a trait with initialization code, would have the flag
`NoInits` but its constructor would not have the `StableRealizable`
flag.

Therefore, this commit basically reconciles the `NoInits` and
`StableRealizable` flags, so that Scala 2 understands whether or
not a Scala 3 trait has a `$init$` method. We also align those
flags when reading from Scala 2 traits, so that Scala 3 also
understands whether or not a Scala 2 trait has a `$init$` trait.

This solves the compatibility issue between Scala 2 and Scala 3.

One detail remains. The attentive reader may have noticed the
quotes in 'an element of the body that "requires" initialization'.
Scala 2 and Scala 3 do not agree on what requires initialization:
notably, Scala 2 thinks that a concrete `def` requires
initialization, whereas Scala 3 knows that this is not the case.
This is not an issue for synchronous interoperability between Scala
2 and 3, since each decides on its own which of its traits has a
`$init$` method, and communicates that fact to the other version.

However, this still poses an issue for "diachronic" compatibility:
if a library defines a trait with a concrete `def` and is compiled
by Scala 2 in version v1, that trait will have a `$init$`. When the
library upgrades to Scala 3 in version v2, the trait will *lose*
the `$init$`, which can break third-party subclasses.

This commit does not address this issue. There are two ways we
could do so:

* Precisely align the detection of whether a trait should have a
  `$init$` method with Scala 2 (notably, adding one when there is a
  concrete `def`), or
* *Always* emit an empty static `$init$` method, even for traits
  that have the `NoInits` flag (but do not generate calls to them,
  nor have that affect whether or not subclasses are considered
  pure).
@sjrd sjrd force-pushed the reconcile-noinits-with-scala2 branch from c2f35b8 to 471fcd9 Compare December 9, 2020 10:12
@nicolasstucki nicolasstucki requested review from smarter and removed request for nicolasstucki December 9, 2020 10:42
@nicolasstucki nicolasstucki removed their assignment Dec 9, 2020
@sjrd sjrd merged commit f70ee5f into scala:master Dec 9, 2020
@sjrd sjrd deleted the reconcile-noinits-with-scala2 branch December 9, 2020 11:46
@Kordyjan Kordyjan modified the milestones: 3.0.0-M3, 3.0.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No static $init$ in pure trait extending impure trait
6 participants