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

Avoid overeager completion of Java annotations in classfile parser #15185

Merged
merged 2 commits into from
May 23, 2022

Conversation

griggt
Copy link
Contributor

@griggt griggt commented May 13, 2022

Require observing the ScalaSig attribute in a classfile before scanning for Scala pickling annotations, which aligns with the behavior of the Scala 2 classfile parser.

Before this commit, all classfiles, including those produced by the Java compiler and compilers of other JVM languages, were scanned for Scala pickling annotations. In certain situations (as in tests/pos/i15166), this is problematic, as the denotation of an annotation symbol defined as a Java inner class may be forced before the inner class table is populated and setClassInfo is called on the class root.

We avoid this situation by only performing the pickling annotation scan for those classfiles having the ScalaSig attribute, i.e. those produced by the Scala 2 compiler.

We also drop support for pickling TASTy using the classfile annotations

  • scala.annotation.internal.TASTYSignature and
  • scala.annotation.internal.TASTYLongSignature

These were never used by the compiler, there are no plans for future use, and preserving support would complicate this fix.

Fixes #15166

@griggt
Copy link
Contributor Author

griggt commented May 18, 2022

The issue in #15166 does not occur with the Scala 2 compiler because it does not perform the scan for pickle signature annotations without first observing the ScalaSig attribute: https://github.com/scala/scala/blob/7952071b88a85e2502e9ecd4e8bd57040d9448f5/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala#L1135

As an alternative to the fix here in 1df17bb, we could potentially do the same, but we currently have

https://github.com/lampepfl/dotty/blob/965f164cc364e83efa5a27d29e0243912114869b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala#L1013-L1020

and so we'd also have to gate the checks for TASTYSignatureAnnot and TASTYLongSignatureAnnot. As far I can tell from the commit history (using git log -S / -G), we've never actually emitted these annotations. Was this some form of future-proofing?

We could drop support for those, or if used in the future, could we stipulate that we would also need to emit a corresponding attribute (ScalaSig or maybe a new TASTYSig)?

@smarter
Copy link
Member

smarter commented May 18, 2022

As far I can tell from the commit history (using git log -S / -G), we've never actually emitted these annotations. Was this some form of future-proofing?

No, those look like abandoned experiments that can safely be removed.

griggt added 2 commits May 18, 2022 22:12
When scanning for Scala pickling annotations, all annotations in all
classfiles (including those produced by the Java compiler) are considered.

Before this commit, a Type and Symbol were created for each discovered
annotation and comparared to the known Scala signature annotation types.
In certain situations (as in tests/pos/i15166), this is problematic, as
the denotation of an annotation symbol defined as a Java inner class may
be forced before the inner class table is populated and setClassInfo is
called on the class root.

Comparing names (as strings) rather than type symbols avoids this situation.

Fixes scala#15166
An alternative fix for scala#15166, which aligns with the behavior of the
Scala 2 classfile parser.

Before this commit, all classfiles, including those produced by the Java
compiler and compilers of other JVM languages, were scanned for Scala
pickling annotations. In certain situations (as in tests/pos/i15166),
this is problematic, as the denotation of an annotation symbol defined as
a Java inner class may be forced before the inner class table is populated
and setClassInfo is called on the class root.

We avoid this situation by only performing the pickling annotation scan for
those classfiles having the `ScalaSig` attribute, i.e. those produced by
the Scala 2 compiler.

We also drop support for pickling TASTy using the classfile annotations
  `scala.annotation.internal.TASTYSignature` and
  `scala.annotation.internal.TASTYLongSignature`.
These were never used by the compiler, there are no plans for future use,
and preserving support would complicate this fix.
@griggt griggt changed the title Avoid overeager completion of annotations in classfile parser Avoid overeager completion of Java annotations in classfile parser May 19, 2022
@griggt griggt added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label May 19, 2022
@griggt griggt marked this pull request as ready for review May 19, 2022 06:21
@smarter smarter merged commit 324516b into scala:main May 23, 2022
@michelou
Copy link
Contributor

@griggt 🎉

@griggt griggt deleted the fix-15166 branch May 23, 2022 16:00
@griggt griggt added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels May 23, 2022
@griggt griggt added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels May 31, 2022
@Kordyjan Kordyjan added this to the 3.2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in ClassfileParser
4 participants