Represent Java annotations as interfaces so they can be extended, and disallow various misuses of them #16260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smarter
force-pushed
the
proper-java-annots
branch
2 times, most recently
from
d28e23f to
30e4aff
Compare
sjrd
requested changes
Nov 2, 2022
tests/run/repeatable/Test_1.scala
Outdated
Comment on lines
14
to
15
| object Test { | ||
| def main(args: Array[String]) = { |
There was a problem hiding this comment.
Are the changes in this file intended? Were they temporarily done to be compilable by Scala 2?
There was a problem hiding this comment.
Are the changes in this file intended?
The addition of new is intended, the other ones weren't and I removed them. I've updated the commit message of the corresponding commit to explain why this was needed:
This change is not fully backwards source compatible: this is illustrated
by the diffs in tests/run/repeatable/Test_1.scala:
-@FirstLevel_0(Array(Plain_0(4), Plain_0(5)))
+@FirstLevel_0(Array(new Plain_0(4), new Plain_0(5)))
Here, FirstLevel_0 takes an array of `Plain_0` annotations as arguments, and in
previous releases of Scala 3 we could put `Plain_0(4)` in this array without
`new`. This is because the compiler generates a "constructor proxy" apply method
for classes, but this no longer works since `Plain_0` is now a trait. While we
could potentially tweak the constructor proxy logic to handle this case, it
seems simpler to require a `new` here, both because Scala 2 does it too and
because it ensures that user code that inspects the annotation tree does not
have to deal with constructor proxies.
The Trait and Abstract flags are not supposed to be set together, and we don't set Abstract when deserializing Java interfaces from classfiles so we shouldn't do it when reading them from source code either. While we're at it, this commit also drops Flags.Trait which is implied by Flags.JavaInterface.
…ion,Annotation} This compiler fiction isn't necessary to handle them as annotations and could lead to runtime crashes when upcasting an annotation value.
Previously we treated Java annotations as if they were classes, like Scala
annotations. For example, given
@interface Ann { int foo(); }
we pretended it was defined as:
abstract class Ann(foo: Int) extends java.lang.annotation.Annotation { def foo(): Int }
We take advantage of this to type annotation trees as if they were
new calls, for example `@Ann(1)` is typed as `new Ann(1)`.
Pretending that annotations are classes is fine most of the time and matches
what Scala 2.12 did, but it's problematic because the JVM treats annotations as
interfaces. In practice this was only an issue with code trying to extend
Java annotations, which would either be rejected at compile-time or miscompiled
before this commit.
This commit switches our representation of annotations to be trait-based
instead:
trait Ann(foo: Int) extends java.lang.annotation.Annotation { def foo(): Int }
Classes are then free to extend annotations using the same pattern as in Scala 2.13:
class Foo extends Ann {val annotationType = classOf[Retention]; def foo(): Int = 1}
Notice that we still pretend these traits have constructors, this lets us type
annotation trees in much the same way as before, and crucially it means that
macros that depended on the exact tree shape of annotation trees can continue to
work, as demonstrated by the annot-java-tree test extracted from wartremover.
To prevent miscompilation issues, we disallow passing arguments to the
annotation constructor in `extends` clause.
This change is not fully backwards source compatible: this is illustrated
by the diffs in tests/run/repeatable/Test_1.scala:
-@FirstLevel_0(Array(Plain_0(4), Plain_0(5)))
+@FirstLevel_0(Array(new Plain_0(4), new Plain_0(5)))
Here, FirstLevel_0 takes an array of `Plain_0` annotations as arguments, and in
previous releases of Scala 3 we could put `Plain_0(4)` in this array without
`new`. This is because the compiler generates a "constructor proxy" apply method
for classes, but this no longer works since `Plain_0` is now a trait. While we
could potentially tweak the constructor proxy logic to handle this case, it
seems simpler to require a `new` here, both because Scala 2 does it too and
because it ensures that user code that inspects the annotation tree does not
have to deal with constructor proxies.
The treatment of default arguments to annotations stays unchanged from 85cd1cf.
Fixes scala#5690. Fixes scala#12840. Fixes scala#14199.
A Java interface that extends java.lang.annotation.Annotation might not be a Java annotation, so to prevent false positives we need to keep track of whether an interface is also an annotation when parsing Java sources and bytecode.
smarter
force-pushed
the
proper-java-annots
branch
from
30e4aff to
a685a4f
Compare
sjrd
approved these changes
Nov 14, 2022
nicolasstucki
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 14, 2023
Flag added in scala#16260
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by the work of hrhino in Scala 2 (scala/scala#6869).