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

releaseFence needs to be added at the end of constructors of classes that contain mixin fields #9424

Closed
smarter opened this issue Jul 23, 2020 · 4 comments · Fixed by #10493

Comments

@smarter
Copy link
Member

smarter commented Jul 23, 2020

Since #8652, fields in classes coming from traits are no longer marked final because they're set outside of the class constructor. When this was changed in scalac, they also had to add calls to releaseFence at the end of these constructors, this is justified with 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).

from scala/scala#7028, which we should port.

Tentatively assigning to @sjrd since unfortunately for him he has become our resident mixin expert :).

@smarter
Copy link
Member Author

smarter commented Jul 23, 2020

Also relevant is scala/scala@5e109ff from scala/scala#7270, but tha's only an optimization (if I understand correctly) and one that depends on #9181

@sjrd
Copy link
Member

sjrd commented Jul 27, 2020

I won't have time to do that in the three weeks. If someone else wants to pick it up, I suggest to do that in Mixin. In transformTemplate, for a non-trait class, if we process any trait setter in that class, then add an additional statement at the end of the new Template's body that calls releaseFence(). Then Constructors will naturally move that statement to the body of the constructor.

@smarter smarter added this to the 3.0.0-M1 milestone Aug 30, 2020
@anatoliykmetyuk
Copy link
Contributor

Is it still a blocker for 3.0.0-M1? @smarter ?

@smarter
Copy link
Member Author

smarter commented Oct 21, 2020

We can move it to M2

@bishabosha bishabosha modified the milestones: 3.0.0-M1, 3.0.0-M2 Oct 27, 2020
@nicolasstucki nicolasstucki modified the milestones: 3.0.0-M2, 3.0.0-RC1 Nov 20, 2020
sjrd added a commit to dotty-staging/dotty that referenced this issue 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 issue 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 issue Nov 26, 2020
This is a forward port of relevant parts of the upstream PR
scala/scala#7028
smarter added a commit that referenced this issue Nov 26, 2020
Fix #9424: Add `releaseFence()` call when mixing in a trait `val`.
@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