Skip to content

No static $init$ in pure trait extending impure trait #9970

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

Closed
bishabosha opened this issue Oct 8, 2020 · 18 comments · Fixed by #10530
Closed

No static $init$ in pure trait extending impure trait #9970

bishabosha opened this issue Oct 8, 2020 · 18 comments · Fixed by #10530

Comments

@bishabosha
Copy link
Member

Minimized code

trait IOApp {
  protected val foo = 23

  def run(args: List[String]): Int

  final def main(args: Array[String]): Unit = {
    sys.exit(run(args.toList))
  }

}
object IOApp {
  trait Simple extends IOApp {
    def run: Unit

    final def run(args: List[String]): Int = {
      run
      0
    }
  }
}

Output

IOApp compiled by dotty 0.27.0-RC1:

public static interface IOApp.Simple
extends IOApp {
    public void run();

    public static int run$(IOApp.Simple $this, List args) {
        return $this.run((List<String>)args);
    }

    @Override
    default public int run(List<String> args) {
        this.run();
        return 0;
    }
}

Expectation

IOApp compiled by Scala 2.13.3:

public static interface IOApp.Simple
extends IOApp {
    public void run();

    public static /* synthetic */ int run$(IOApp.Simple $this, List args) {
        return $this.run((List<String>)args);
    }

    @Override
    default public int run(List<String> args) {
        this.run();
        return 0;
    }

    public static void $init$(IOApp.Simple $this) {
    }
}

This means we can't use cats-effect-3 (compiled by dotty) in scala 2.13, as extending IOApp.Simple expects the static IOApp.Simple.$init$ method to exist

@bishabosha
Copy link
Member Author

Additionally I guess classes extending IOApp.Simple should be calling IOApp.Simple.$init$(this)

@sjrd
Copy link
Member

sjrd commented Oct 8, 2020

The Mixin and Constructors phase trust the NoInits flag to know whether or not a $init$ method should be emitted/called.

That would mean that the computation of the NoInits flag for IOApp.Simple is not looking at the parents. That is done in Namer and Scala2Unpickler, which I'm not familiar with. I suggest assigning to someone who knows these areas.

@odersky
Copy link
Contributor

odersky commented Oct 10, 2020

Hmm. But there are no initializers; the $init$method is empty. So NoInits works as intended.

@sjrd
Copy link
Member

sjrd commented Oct 10, 2020

There is no initializer in Simple itself, but there is one in its super trait IOApp. So Simple should inherit the fact that it needs an init.

@bishabosha
Copy link
Member Author

its not that simple because in this case IOApp.Simple should not get an initialiser:

trait IOApp {
  val foo = 23
}
object IOApp {
  trait Simple extends IOApp
}

@bishabosha
Copy link
Member Author

i refer to my comment above that when IOApp and Simple defines no methods Simple should not get an init

@sjrd
Copy link
Member

sjrd commented Nov 26, 2020

Some notes about my investigation so far:

  • In scalac, there is no equivalent of the NoInits flag. Instead, a trait either has a $init$ method or it doesn't have one at all. This is the only thing that distinguishes a trait with or without initialization. A call to a super T.$init$ method is generated if and only if T.$init$ exists.
  • The decision to generate that method is taken during parsing, based on the body of the template:
  • In dotty, NoInits has a dual purpose:
    • to identify classes and traits whose constructors are pure (for elimination in inlining, I believe)
    • and, for traits, to determine whether or not they should receive a $init$ method

For Scala 2 interop, we need to generate a $init$ and calls to it if and only if Scala 2 does/would do so. Or, at the very least,

  • we need to generate a $init$ if Scala 2 would do so (because Scala 2 classes inheriting our trait will want to call it)
  • we must not call a $init$ if Scala 2 would not do so (because we would attempt to call a non-existing method)

We may, if necessary:

  • generate more $init$ methods, as long as we don't call them
  • omit to call a $init$ method even if it exists, if we know that it's a no-op

I don't think those "may"s would help the implementation, though. So I would stick with generate-and-call $init$ if and only if Scala 2 does.

That means that we sometimes must not generate an $init$ even though a supertrait has a non-pure constructor. This directly contradicts the first purpose of NoInits in dotty: for inlining and its eliminations to be sound, such a trait must not have NoInits, but we still must not generate (definitely not call) a $init$.

Consequently, we need to decouple the generation of $init$ from the NoInits flag.

@sjrd
Copy link
Member

sjrd commented Nov 26, 2020

@bishabosha What does the Tasty reader in 2.13 do about this? Does it try to ignore the $init$ methods coming from TASTy at all when the trait should not have an init?

@bishabosha
Copy link
Member Author

bishabosha commented Nov 26, 2020

@bishabosha What does the Tasty reader in 2.13 do about this? Does it try to ignore the $init$ methods coming from TASTy at all when the trait should not have an init?

@sjrd Currently in Scala 2, any <init> method owned by a trait is renamed to $init$ and will only be entered in scope if it is [Edit: not] stable

@sjrd
Copy link
Member

sjrd commented Nov 26, 2020

Thanks. What do you mean by "if it is stable"? I was not aware that a constructor could ever be stable.

@bishabosha
Copy link
Member Author

Thanks. What do you mean by "if it is stable"? I was not aware that a constructor could ever be stable.

just if you print out the tasty, you will see that the constructor has the stable flag

@bishabosha
Copy link
Member Author

Sorry, I actually meant that if the constructor has a stable flag then it will not be entered

@sjrd
Copy link
Member

sjrd commented Nov 27, 2020

Oh OK. So yes, apparently class and mixin constructors receive the StableRealizable flag (aka Stable in TASTy) in Namer when their owner satisfied isNoInitsClass. Thanks. I'm not sure what I'm going to do with all that yet, but I think I have everything I need now.

@sjrd
Copy link
Member

sjrd commented Nov 27, 2020

@bishabosha How could I write a test for the original issue? Is my only option to write a scripted test, or do we have better now?

@bishabosha
Copy link
Member Author

do we have better now?

I think thats the only option, without creating your own test framework

@bishabosha
Copy link
Member Author

bishabosha commented Nov 27, 2020

Alternatively, I can propose a bytecode test/java reflection test where you curate some examples based on what scala 2 does, without comparing to scala 2 code

@sjrd
Copy link
Member

sjrd commented Nov 27, 2020

Alternatively, I can propose a bytecode test/java reflection test where you curate some examples based on what scala 2 does, without comparing to scala 2 code

That wouldn't work in the solution that I have, because eventually I found a way not to completely align with what Scala 2 does, while still correctly communicate to Scala 2 what to do with our traits.

sjrd added a commit to dotty-staging/dotty that referenced this issue Nov 27, 2020
…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 added a commit to dotty-staging/dotty that referenced this issue Nov 27, 2020
…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
Copy link
Member

sjrd commented Nov 27, 2020

Ah! I can't even write a scripted test, because I would need to use Scala 2.13.4 with -Ytasty-reader, but that doesn't work because the TASTy has format has already changed since Scala 2.13.4 😬 I'm not even sure how I can possibly test it by hand once, to check whether a future version of the TASTy reader would do the right thing, with my changes.

sjrd added a commit to dotty-staging/dotty that referenced this issue Nov 28, 2020
…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 added a commit to dotty-staging/dotty that referenced this issue Dec 1, 2020
…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 removed the help wanted label Dec 1, 2020
sjrd added a commit to dotty-staging/dotty that referenced this issue Dec 9, 2020
…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 added a commit that referenced this issue Dec 9, 2020
…cala2

Fix #9970: Reconcile how Scala 3 and 2 decide whether a trait has $init$.
@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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants