Skip to content

@transient lazy val does not get marked as ACC_TRANSIENT #15149

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
DavidGoodenough opened this issue May 9, 2022 · 6 comments · Fixed by #15296
Closed

@transient lazy val does not get marked as ACC_TRANSIENT #15149

DavidGoodenough opened this issue May 9, 2022 · 6 comments · Fixed by #15296
Assignees
Milestone

Comments

@DavidGoodenough
Copy link

DavidGoodenough commented May 9, 2022

Compiler version

3.1.0 and 3.1.1

Minimized code

class Test() {
        @transient lazy val lazyVal = "lazy"
        @transient val ordinaryVal = "ordinary"
        }

Output

javap generates:-

  public static final long OFFSET$0;
    descriptor: J
    flags: (0x0019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL

  public long 0bitmap$1;
    descriptor: J
    flags: (0x0001) ACC_PUBLIC

  public java.lang.String lazyVal$lzy1;
    descriptor: Ljava/lang/String;
    flags: (0x0001) ACC_PUBLIC

  private final transient java.lang.String ordinaryVal;
    descriptor: Ljava/lang/String;
    flags: (0x0092) ACC_PRIVATE, ACC_FINAL, ACC_TRANSIENT

Expectation

0bitmap$1 and lazyVal$lzy1 should both have flag ACC_TRANSIENT.

I am also confused when reading SIP-20 (version 6) as to what OFFSET$0 does - it does not appear in https://dotty.epfl.ch/docs/reference/changed-features/lazy-vals-init.html equivalent code (how could it, an ACC_STATIC can not be directly be directly created in Scala). But it does appear in the code produced by 3.1.0 and 3.1.1. Being ACC_STATIC it can not also be ACC_TRANSIENT, but I am unsure whether the code as currently implemented would survive serialisation or any equivalent code following the same rules for what get processed.

@DavidGoodenough DavidGoodenough added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels May 9, 2022
@megri
Copy link
Contributor

megri commented May 11, 2022

There is a test for this in https://github.com/lampepfl/dotty/blob/7c446ce8b2bf7031a6cf223b5f6a2782c1543016/tests/run/serialization-new.scala#L469-L495 but from what I can tell it actually doesn't verify that the lazy vals aren't serialized.

@szymon-rd szymon-rd self-assigned this May 12, 2022
@szymon-rd
Copy link
Contributor

szymon-rd commented May 12, 2022

I am also confused when reading SIP-20 (version 6) as to what OFFSET$0 does

It seems that OFFSET$0 is used to store offset of the lazy val state field in the class:

      85: getstatic     #28                 // Field scala/runtime/LazyVals$.MODULE$:Lscala/runtime/LazyVals$;
      88: aload_0
      89: getstatic     #35                 // Field OFFSET$0:J
      92: iconst_0
      93: iconst_0
      94: invokevirtual #61                 // Method scala/runtime/LazyVals$.setFlag:(Ljava/lang/Object;JII)V

In the SIP it's the bitmap_offset field. Therefore, we probably do not need to think about it in the context of this problem.

@szymon-rd szymon-rd added area:backend and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels May 12, 2022
@szymon-rd
Copy link
Contributor

It seems that the fields are serialized in the test as well. After modifying the test to this:

  @transient lazy val a1 = { 
    println("Recalculating a1")
    1
  }
  @transient private lazy val a2 = { 
    println("Recalculating a2")
    2
  }

The output is:

Recalculating a1
1
Recalculating a2
2
1
2

So the fields are not recalculated, they are deserialized.

@szymon-rd
Copy link
Contributor

Ok so it seems that in the LazyVals transform class, while emitting synthetic fields & methods, the transient annotation is lost. It's a little bit tricky to fix, as there is also the bitmap that is keeping the state of lazy vals. Fix for this would touch the same code as #14545. This does not only affect the @transient annotation, but also @volatile and runtime retention annotations:

import java.lang.Deprecated

class Test() {
        @Deprecated @volatile @transient lazy val lazyVal = "lazy"
        @Deprecated @volatile @transient lazy val lazyVal2 = "lazy"
        @Deprecated @volatile @transient val ordinaryVal = "ordinary"
        }

Compiles to:

  public long 0bitmap$1;
    descriptor: J
    flags: (0x0001) ACC_PUBLIC

  public java.lang.String lazyVal$lzy1;
    descriptor: Ljava/lang/String;
    flags: (0x0001) ACC_PUBLIC

  public java.lang.String lazyVal2$lzy1;
    descriptor: Ljava/lang/String;
    flags: (0x0001) ACC_PUBLIC

  private final volatile transient java.lang.String ordinaryVal;
    descriptor: Ljava/lang/String;
    flags: (0x00d2) ACC_PRIVATE, ACC_FINAL, ACC_VOLATILE, ACC_TRANSIENT
    RuntimeVisibleAnnotations:
      0: #13()
        java.lang.Deprecated

It seems that it's not addressed in the rework in #14545, but I may be missing something so I'd like to tag @olsdavis as he is the author of most commits in this PR. If that's the case, then my suggestions are:

  • Forward the @transient (and other annotations) to generated fields for lazy vals.
  • Create separate bitmap for transient lazy vals, and mark it as @transient as well - we would have separate bitmap$i and bitmap_transient$i.

That should cover the transient case. However, volatile is a separate issue as it should probably also affect the bitmap - maybe we should introduce another bitmap or make all bitmaps volatile by default?

@smarter

@DavidGoodenough
Copy link
Author

The problem is still present in 3.2.0. Any idea when this might get fixed?

@szymon-rd
Copy link
Contributor

It's going to be fixed in:
#15296
I will probably merge it soon and it should be made experimental in 3.2.2, and default in 3.3.0.

szymon-rd added a commit that referenced this issue Oct 28, 2022
Cleaned up version of #14545

Implementing the new lazy vals scheme mentioned in
#6979, fixing
#7140

New PR to avoid force pushing to main branch of the previous author.

Fixes #15149 as well
Applied #14780
@Kordyjan Kordyjan added this to the 3.2.2 milestone Aug 1, 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.

4 participants