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

Mixin fields with trait setters shouldn't be JVM final #7028

Merged
merged 3 commits into from
Aug 16, 2018

Conversation

retronym
Copy link
Member

@retronym retronym commented Aug 9, 2018

Following on from #6425, also a release fence to the constructor of classes that mixin traits with val fields, and changed the backing JVM field to be non-final. This lets us comply with the JVM spec ("assignment to a final field must lexically scoped in the class constructor"), which is enforced under Classfile format 53

Fixes scala/scala-dev#408

Before:

defined class C

scala> :javap -private -c C Compiled from "<console>" public class
$line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T {
 private final int foo;

  public int foo();
   Code:
      0: aload_0
      1: getfield      #21                 // Field foo:I
      4: ireturn

  public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int);
   Code:
      0: aload_0
      1: iload_1
      2: putfield      #21                 // Field foo:I
      5: return

  public $line3.$read$$iw$$iw$C();
   Code:
      0: aload_0
      1: invokespecial #30                 // Method
java/lang/Object."<init>":()V
      4: aload_0
      5: invokestatic  #34                 // InterfaceMethod
$line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V
      8: return
}

The assignment to the final field foo has always contravened the JVM
spec, and this rule is enforced for any classfiles of format 53 and higher.

After this patch:

defined class C

scala> :javap -private -c C Compiled from "<console>" public class
$line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T {
 private int foo;

  public int foo();
   Code:
      0: aload_0
      1: getfield      #21                 // Field foo:I
      4: ireturn

  public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int);
   Code:
      0: aload_0
      1: iload_1
      2: putfield      #21                 // Field foo:I
      5: return

  public $line3.$read$$iw$$iw$C();
   Code:
      0: aload_0
      1: invokespecial #30                 // Method
java/lang/Object."<init>":()V
      4: aload_0
      5: invokestatic  #34                 // InterfaceMethod
$line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V
      8: getstatic     #40                 // Field
scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
     11: invokevirtual #43                 // Method
scala/runtime/ScalaRunTime$.releaseFence:()V
     14: return
}

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 9, 2018
Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

A few questions, mostly because I still haven't fully internalized the memory model :-)

mkAccessor(setter)(Assign(Select(This(clazz), fieldSel), castHack(Ident(setter.firstParam), fieldSel.info)))
}
case fieldSel =>
if (!fieldSel.hasFlag(MUTABLE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use a comment: If the field is mutable, it won't be final, so we can write to it in a setter. If it's not, we still need to initialize it, and make sure it's safely published. Since initialization is performed (lexically) outside of the constructor (in the trait setter), we have to make the field mutable starting with classfile format 53 (it was never allowed, but the verifier enforces this now).

Copy link
Member Author

Choose a reason for hiding this comment

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

updated with this comment

@@ -560,6 +567,7 @@ sealed abstract class List[+A]

final case class :: [+A](override val head: A, private[scala] var next: List[A @uncheckedVariance]) // sound because `next` is used only locally
extends List[A] {
ScalaRunTime.releaseFence()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment to the class that every time next is mutated and the resulting :: instance is published, you should first call releaseFence. Maybe, maybe, we can actually add a method publishNext or something to make this more clear.

@@ -215,6 +216,7 @@ final class Vector[+A] private[immutable] (private[collection] val startIndex: I
s.dirty = dirty
s.gotoPosWritable(focus, idx, focus ^ idx) // if dirty commit changes; go to new pos and prepare for writing
s.display0(idx & 31) = elem.asInstanceOf[AnyRef]
ScalaRunTime.releaseFence()
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to publish the mutation of dirty? Should we also do this in the constructor?

@@ -61,6 +62,7 @@ class ListBuffer[A]
// Avoids copying where possible.
override def toList: List[A] = {
aliased = nonEmpty
ScalaRunTime.releaseFence()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no fence for the other methods that mutate? I see they're private, but they are called from public methods, I assume?

@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.13.0-M5 Aug 9, 2018
@SethTisue SethTisue added the prio:blocker release blocker (used only by core team, only near release time) label Aug 15, 2018
@SethTisue
Copy link
Member

@retronym should we merge this for M5, or hold it for RC1?

Before:

```
scala> trait T { val foo = 24 }; class C extends T
defined trait T
defined class C

scala> :javap -private -c C
Compiled from "<console>"
public class $line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T {
  private final int foo;

  public int foo();
    Code:
       0: aload_0
       1: getfield      #21                 // Field foo:I
       4: ireturn

  public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int);
    Code:
       0: aload_0
       1: iload_1
       2: putfield      #21                 // Field foo:I
       5: return

  public $line3.$read$$iw$$iw$C();
    Code:
       0: aload_0
       1: invokespecial #30                 // Method java/lang/Object."<init>":()V
       4: aload_0
       5: invokestatic  #34                 // InterfaceMethod $line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V
       8: return
}
```

The assignment to the final field `foo` has always contravened the JVM spec,
and this rule is enforced for any classfiles of format 53 and higher.

After this patch:

```
scala> trait T { val foo = 24 }; class C extends T
defined trait T
defined class C

scala> :javap -private -c C
Compiled from "<console>"
public class $line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T {
  private int foo;

  public int foo();
    Code:
       0: aload_0
       1: getfield      #21                 // Field foo:I
       4: ireturn

  public void $line3$$read$$iw$$iw$T$_setter_$foo_$eq(int);
    Code:
       0: aload_0
       1: iload_1
       2: putfield      #21                 // Field foo:I
       5: return

  public $line3.$read$$iw$$iw$C();
    Code:
       0: aload_0
       1: invokespecial #30                 // Method java/lang/Object."<init>":()V
       4: aload_0
       5: invokestatic  #34                 // InterfaceMethod $line3/$read$$iw$$iw$T.$init$:(L$line3/$read$$iw$$iw$T;)V
       8: getstatic     #40                 // Field scala/runtime/ScalaRunTime$.MODULE$:Lscala/runtime/ScalaRunTime$;
      11: invokevirtual #43                 // Method scala/runtime/ScalaRunTime$.releaseFence:()V
      14: return
}
```
@retronym
Copy link
Member Author

retronym commented Aug 16, 2018

Rebased, this still shares the preliminary commits with #6425.

I think we should get this into M5.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Aug 16, 2018
@SethTisue SethTisue merged commit 69b307a into scala:2.13.x Aug 16, 2018
@SethTisue SethTisue added release-notes worth highlighting in next release notes and removed release-notes worth highlighting in next release notes labels Aug 22, 2018
sjrd added a commit to dotty-staging/dotty that referenced this pull request Nov 25, 2020
This is a forward port of relevant parts of the upstream PR
scala/scala#7028
sjrd added a commit to dotty-staging/dotty that referenced this pull request Nov 25, 2020
This is a forward port of relevant parts of the upstream PR
scala/scala#7028
sjrd added a commit to dotty-staging/dotty that referenced this pull request Nov 26, 2020
This is a forward port of relevant parts of the upstream PR
scala/scala#7028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:blocker release blocker (used only by core team, only near release time) release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants