-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Actively remove Scala 2 pickles and emit synthetic TASTy attribute for copied stdlib .class files #24846
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
Conversation
| def validRange: String = { | ||
| val min = TastyVersion(major, 0, 0) | ||
| val max = if (experimental == 0) this else TastyVersion(major, minor - 1, 0) | ||
| val extra = Option.when(experimental > 0)(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's compiled using Scala 2.12. For the sake of correctness we're adding tasty sources to unamanged sources of sbt build. See project/build.sbt changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, linking to my comment here #24846 (comment)
project/ScalaLibraryPlugin.scala
Outdated
| var stamps = analysis.stamps | ||
|
|
||
| val classDir = (Compile / classDirectory).value | ||
| val sourceDir = (LocalProject("scala-library-nonbootstrapped") / sourceDirectory).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the plugin know about scala-library-nonbootstrapped? It shouldn't.
And even if it were to know about it, why doesn't it know about scala-library-bootstrapped too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've rewritten this part so that sourceDir is not required at all. We now identify .java sources based on the Classfile Source attribute
| assert(tastyFile.exists(), s"TASTY file $tastyFile does not exist for $relativePath") | ||
|
|
||
| // Only add TASTY attribute if this is the primary class (class path equals base path) | ||
| // Inner classes, companion objects ($), anonymous classes ($$anon), etc. don't get TASTY attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember this honestly but it doesn't seem like a little detail. This is part of the specification of what attributes generated classfiles contain. Where is this specification written?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, it's based on the observations and outputs of compilation. Partially it's also based on the CodeGen logic:
scala3/compiler/src/dotty/tools/backend/jvm/CodeGen.scala
Lines 61 to 71 in d673395
| val mainClassNode = genClass(cd, unit) | |
| val mirrorClassNode = | |
| if !sym.isTopLevelModuleClass then null | |
| else if sym.companionClass == NoSymbol then genMirrorClass(sym, unit) | |
| else | |
| report.log(s"No mirror class for module with linked class: ${sym.fullName}", NoSourcePosition) | |
| null | |
| if sym.isClass then | |
| val tastyAttrNode = if (mirrorClassNode ne null) mirrorClassNode else mainClassNode | |
| genTastyAndSetAttributes(sym, tastyAttrNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need the answer to this question before blindly committing to a design. That's why #24180 is delayed, I still have more questions than answers. See this too: #24846 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize behaviour based on the compiler implementation, since there is no dedicated specification:
- Only one class per top-level definition gets the TASTy attribute - either the main class or the mirror class, never both
- TASTy pickling only iterates over
topLevelClasses(defined inTreeInfo.scala), not nested classes - Nested/inner classes are stored inside their parent's
.tastyfile, not in separate files - The TASTy attribute contains only a 16-byte UUID pointing to the
.tastyfile - if there's no separate.tastyfile, there's nothing to point to
Verification from ClassfileParser.scala:
- If a
.classfile has a TASTy attribute, the compiler expects a corresponding.tastyfile - If the UUID doesn't match, it warns about sync issues
Based on that current behavior is consistent with the design that one .tasty file corresponds to one top-level class and contains all nested definitions within it.
TASTy for companion module is store in companion class. Even if we'd define just an object foo a mirror ``foo.classwould be created and it would contain TASTy attribute, not thefoo$.class`
There is no TASTy attribute for anonynous classes e.g. foo$anon$1.class.
Since specialized class is basically a new anonymous class it should not define TASTY attribute either.
If in the future we'd decide that TASTy should be also stored for synthetics then it can be easily added in the future. Right now TASTy attrs have been added only to the .class files that should have produced TASTy right now. We can add additional validation to ensure that's the case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional assertions were added in 9178964
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WojciechMazur's analysis is correct.
In addition: all .class files produced by a Scala compiler (2 or 3, primary class or not) must have the Scala attribute. You might want to add that to the checks you perform. However, you shouldn't need to add/remove it since they will be present from the Scala 2 compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch and it seems the Scala 2 compiler might have a bug - there are 15 .class files that miss Scala attribute:
[error] (scala-library-nonbootstrapped / Compile / packageBin) java.lang.AssertionError: assertion failed: JAR scala-library-3.8.1-RC1-bin-SNAPSHOT-nonbootstrapped.jar contains 15 class files without 'Scala' attribute:
[error] - scala/Tuple1.class
[error] - scala/Tuple2.class
[error] - scala/collection/DoubleStepper.class
[error] - scala/collection/IntStepper.class
[error] - scala/collection/LongStepper.class
[error] - scala/collection/Stepper.class
[error] - scala/collection/immutable/DoubleVectorStepper.class
[error] - scala/collection/immutable/IntVectorStepper.class
[error] - scala/collection/immutable/LongVectorStepper.class
[error] - scala/collection/immutable/Range.class
[error] - scala/jdk/DoubleAccumulator.class
[error] - scala/jdk/IntAccumulator.class
[error] - scala/jdk/LongAccumulator.class
[error] - scala/runtime/NonLocalReturnControl.class
[error] - scala/util/Sorting.classThese seems to match the list of specialized classes we copy. It appears that we have following attributes:
- in Tuple1.class : ScalaInlineInfo, ScalaSig (no Scala)
- in Tuple1$.class: ScalaInlineInfo, Scala
- in Tuple2$mcCC$sp.class: ScalaInlineInfo, Scala
Unless ScalaSig was a special case for Scala attribute it seems to not match the spec.
Since Scala attribute is empty we can easily add to the copied files
| else super.visitAnnotation(desc, visible) | ||
| } | ||
| reader.accept(visitor, 0) | ||
| // Only add TASTY attribute for the primary class (not for inner/nested classes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then there is the question that we don't have any precedence for: should we add the attribute in specialized classes? I honestly don't know because I don't know what is the purpose of that attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, becouse these are not "primary". In the same way as we apparently don't add TASTY attribute for the companion module class or there is not TASTy for synthetic anonymous class
Right now the TASTy attribute would be added only to the sources that would naturally contain TASTy attribute if compiled using Scala 3:
tastyUUID for scala/Tuple1.class: 00a339b792cd7eac008660ea168725f3
tastyUUID for scala/Tuple2.class: 000ca8e2dce9c19a00f363ecefcdbec5
tastyUUID for scala/collection/DoubleStepper.class: 00703d7c2e7ffcb700b287d999ad1595
tastyUUID for scala/collection/IntStepper.class: 009766856a8859b700c67a980eacefc3
tastyUUID for scala/collection/LongStepper.class: 00d2dfa3133310b7003d929d583684a5
tastyUUID for scala/collection/Stepper.class: 00bb7f046916eaa3007b9a1b982043f1
tastyUUID for scala/collection/immutable/DoubleVectorStepper.class: 00b8ae9a9f4f11b7002260fd37125644
tastyUUID for scala/collection/immutable/IntVectorStepper.class: 00409db0061756b7003367ff5f067f44
tastyUUID for scala/collection/immutable/LongVectorStepper.class: 0066415ac82ecfb700456af85e1a7744
tastyUUID for scala/collection/immutable/Range.class: 0032837a8789b8f500aa3b2437445bb4
tastyUUID for scala/jdk/DoubleAccumulator.class: 004dee438fc6f8de000762cc7744fa8b
tastyUUID for scala/jdk/IntAccumulator.class: 0090e4b14fdf4cdf00831888bf948aa1
tastyUUID for scala/jdk/LongAccumulator.class: 005598d1fa676cda00a901fcddc8f4c8
tastyUUID for scala/runtime/NonLocalReturnControl.class: 00b0cf8ea56508cb00420efcdf3acd38
tastyUUID for scala/util/Sorting.class: 009ef6b0ffdffbbd00988e02c758b8c8| found | ||
| } | ||
|
|
||
| def validateNoScala2Pickles(jar: File): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't validate that ScalaSig and ScalaInlineInfo attributes were removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted it so that both of these are now validated
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just depend on the latest published version? Is it even possible to do such a thing between Scala 3 and Scala 2.12? Do we need to wait for sbt 2 to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't depend on it since that would require upgrade to sbt 2 which is not possible right now (e.g. MiMa plugin is not cross compiled yet) or to introduce cross compilation of tasty project (which again would require changes due to classess in stdlib that were stubbed in this PR_
sbt 1.11 already enables -Xsource:3 by default, so most of sources should compile (based on syntax) unless there are changes in stdlib (like mentioned Option.when introduced in 2.13).
Concept of depending on sources in the build definitions is frequently used in Scala.js and Scala Native altough it's more complicated there (it's used to bootstrap sbt plugins and whole toolchain that are later referenced from inside the build)
I bealive we should be able to switch for regular dependencies after upgrading to sbt 2 in hopefully 4-6 months (fingers crossed it would be ready by that time, support for mima was merged but awaits for release lightbend-labs/mima#878)
| private class TastyAttribute(val uuid: Array[Byte]) extends Attribute("TASTY") { | ||
| override def write(classWriter: ClassWriter, code: Array[Byte], codeLength: Int, maxStack: Int, maxLocals: Int): ByteVector = { | ||
| val bv = new ByteVector(uuid.length) | ||
| bv.putByteArray(uuid, 0, uuid.length) | ||
| bv | ||
| } | ||
| } | ||
| /** Custom ASM Attribute for reading TASTY attributes from class files */ | ||
| private class TastyAttributeReader extends Attribute("TASTY") { | ||
| var uuid: Option[Array[Byte]] = None | ||
|
|
||
| override def read(classReader: ClassReader, offset: Int, length: Int, charBuffer: Array[Char], codeOffset: Int, labels: Array[Label]): Attribute = { | ||
| val attr = new TastyAttributeReader() | ||
| if (length == 16) { | ||
| val bytes = new Array[Byte](16) | ||
| for (i <- 0 until 16) { | ||
| bytes(i) = classReader.readByte(offset + i).toByte | ||
| } | ||
| attr.uuid = Some(bytes) | ||
| } | ||
| attr | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we collapse them into a single class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially yes, but would be confusing as one is used only to extract attribute and other one to write it. I've refactored it so that reader one is locall to the function which extracts these
…lass files produced by javac based on the source attributes
… removed that would be missing in unpatched sources compiled using Scala 3
|
@sjrd Please take a look at this PR, you should be the most oriented in how these attributes are used. We need a decision if we can backport this to 3.8.0-RC6 or make RC5 stable. The issue seemed to be workaround by JetBrains on their side (see ticket) but it would be better to not have a "special" versions requiring dedicated fixed by the tools. |
…ch specialized classes to add missing attribute (Scala 2 compiler bug?)
Fixes #24161
Based on and superseeds #24180
We're now actively removing Scala 2 pickle annotations and emit synthetic TASTY attributes (for primary classes). Both of validations are now checked at build time