Support @Repeatable Java annotations.#6846
Merged
lrytz merged 1 commit intoscala:2.12.xfrom Jun 27, 2018
Merged
Conversation
Contributor
|
Thank you, it's originally reported by me. |
lrytz
reviewed
Jun 25, 2018
Member
lrytz
left a comment
There was a problem hiding this comment.
Looks good, thanks! Some existing tests need updating.
| annots | ||
| .map(_.transformArgs(transformTrees)) | ||
| .groupBy(_.symbol) | ||
| .flatMap((groupRepeatableAnnotations _).tupled)(collection.breakOut) |
Member
There was a problem hiding this comment.
let's do this using 2.13 compatible API :-)
hrhino
commented
Jun 26, 2018
| | (Int @TypeAnn_0("a") @TypeAnn_0("b")) | ||
| | => (String @TypeAnn_0("x") @TypeAnn_0("y")) | ||
| |) { | ||
| | type Meh = Any@TypeAnn_0("p")@TypeAnn_0("q") |
Contributor
Author
There was a problem hiding this comment.
I had val meh: Any @ ..., but typer strips them due to not being <: TypeConstraint, so I'm not sure what to do. I wonder if Java annotations with @Target(ElementType.TYPE_USE) should be considered typely constraining. I wonder if anyone, anywhere, cares.
Member
There was a problem hiding this comment.
That's fine, no action required (here) :-)
hrhino
commented
Jun 26, 2018
| ) | ||
| ) | ||
|
|
||
| override def transformArgs(f: List[Tree] => List[Tree]) = this |
Contributor
Author
There was a problem hiding this comment.
urk.
[error] * method transformArgs(scala.Function1)scala.reflect.runtime.JavaMirrors#JavaMirror#JavaAnnotationProxy in class scala.reflect.runtime.JavaMirrors#JavaMirror#JavaAnnotationProxy does not have a correspondent in other version
Is this an acceptable MiMa exception to add?
EDIT: please give me forgiveness, not permission.
lrytz
reviewed
Jun 27, 2018
| def groupRepeatableAnnotations(sym: Symbol, anns: List[AnnotationInfo]): List[AnnotationInfo] = | ||
| if (!(sym isSubClass ClassfileAnnotationClass)) anns else anns match { | ||
| case single :: Nil => anns | ||
| case multiple => // groupBy won't give us `Nil`s |
Contributor
Author
There was a problem hiding this comment.
Not stale, just out of place, like a slice of Limburger on a McDonald's happy meal patty. It was meant to refer to this method being used in groupBy a few lines up.
Member
There was a problem hiding this comment.
Ah sorry, i searched for groupBy in the diff, but missed it.
| | (Int @TypeAnn_0("a") @TypeAnn_0("b")) | ||
| | => (String @TypeAnn_0("x") @TypeAnn_0("y")) | ||
| |) { | ||
| | type Meh = Any@TypeAnn_0("p")@TypeAnn_0("q") |
Member
There was a problem hiding this comment.
That's fine, no action required (here) :-)
| checkTypeRefBounds(tp, tree) | ||
| } | ||
| private def doTypeTraversal(tree: Tree)(f: Type => Unit) = if (!inPattern) tree.tpe foreach f | ||
| private def applyRefchecksToAnnotations(tree: Tree): Tree = { |
Member
There was a problem hiding this comment.
Why does this method return a tree now? It seems all callsites are in statement position.
Contributor
Author
There was a problem hiding this comment.
I had thought I may have needed to return a different tree, rather than just mutating it. I was wrong. It's Unitary now.
Currently, duplicate classfile annotations cause a runtime crash when the JVM sees them (due to a call to `getAnnotations` or the like). Do instead exactly what Java does (since JEP-120): if the annotation type is (meta-)annotated with `@Repeatable`, wrap the annotations in an array and annotate the original element with a new annotation of the type given by `Repeatable#value`. It is now an error to have multiple annotations on the same tree with the same `typeSymbol` if the symbol is a classfile annotation. Fixes scala/bug#9529.
lrytz
approved these changes
Jun 27, 2018
Contributor
|
Thank you . |
retronym
added a commit
to retronym/scala
that referenced
this pull request
Nov 14, 2018
Regressed in scala#6846, which added support for encoding repeated annotations. Test failure before replacing `groupBy` with `LinkedHashMap`: ``` $ sbt junit/testOnly scala.tools.nsc.DeterminismTest ... java.lang.AssertionError: assertion failed: Difference detected between recompiling List(b.scala, Annot1.java) Run: jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330 $ jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330 diff --git a/Test.class.asm b/Test.class.asm index 98bfd80..a056f9a 100644 --- a/Test.class.asm +++ b/Test.class.asm @@ -4,10 +4,10 @@ // compiled from: b.scala - @LAnnot2;(value=java.lang.Object.class) - @LAnnot1;(value="foo") + @LAnnot2;(value=java.lang.Object.class) + @Lscala/reflect/ScalaSignature;(bytes="\u0006\u0001u1AAA\u0002\u0001\r!)Q\u0002\u0001C\u0001\u001d\u0009!A+Z:u\u0015\u0005!\u0011a\u0002\u001ff[B$\u0018PP\u0002\u0001'\u0009\u0001q\u0001\u0005\u0002\u0009\u00175\u0009\u0011BC\u0001\u000b\u0003\u0015\u00198-\u00197b\u0013\u0009a\u0011B\u0001\u0004B]f\u0014VMZ\u0001\u0007y%t\u0017\u000e\u001e \u0015\u0003=\u0001\"\u0001\u0005\u0001\u000e\u0003\rAC\u0001\u0001\n\u0016-A\u0011\u0001cE\u0005\u0003)\r\u0011a!\u00118o_R\u0014\u0014!\u0002<bYV,7%A\u0004)\u0009\u0001ARc\u0007\u0009\u0003!eI!AG\u0002\u0003\r\u0005sgn\u001c;2C\u0005a\u0012a\u00014p_\u0002") ``` WIP stabilize annotation ordering
retronym
added a commit
to retronym/scala
that referenced
this pull request
Nov 19, 2018
Regressed in scala#6846, which added support for encoding repeated annotations. Test failure before replacing `groupBy` with `LinkedHashMap`: ``` $ sbt junit/testOnly scala.tools.nsc.DeterminismTest ... java.lang.AssertionError: assertion failed: Difference detected between recompiling List(b.scala, Annot1.java) Run: jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330 $ jardiff -r /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/reference814657788418452571 /var/folders/tz/p8vd07wn7wxck3b9v54grlzw0000gp/T/recompileOutput4882243280168823330 diff --git a/Test.class.asm b/Test.class.asm index 98bfd80..a056f9a 100644 --- a/Test.class.asm +++ b/Test.class.asm @@ -4,10 +4,10 @@ // compiled from: b.scala - @LAnnot2;(value=java.lang.Object.class) - @LAnnot1;(value="foo") + @LAnnot2;(value=java.lang.Object.class) + @Lscala/reflect/ScalaSignature;(bytes="\u0006\u0001u1AAA\u0002\u0001\r!)Q\u0002\u0001C\u0001\u001d\u0009!A+Z:u\u0015\u0005!\u0011a\u0002\u001ff[B$\u0018PP\u0002\u0001'\u0009\u0001q\u0001\u0005\u0002\u0009\u00175\u0009\u0011BC\u0001\u000b\u0003\u0015\u00198-\u00197b\u0013\u0009a\u0011B\u0001\u0004B]f\u0014VMZ\u0001\u0007y%t\u0017\u000e\u001e \u0015\u0003=\u0001\"\u0001\u0005\u0001\u000e\u0003\rAC\u0001\u0001\n\u0016-A\u0011\u0001cE\u0005\u0003)\r\u0011a!\u00118o_R\u0014\u0014!\u0002<bYV,7%A\u0004)\u0009\u0001ARc\u0007\u0009\u0003!eI!AG\u0002\u0003\r\u0005sgn\u001c;2C\u0005a\u0012a\u00014p_\u0002") ``` WIP stabilize annotation ordering
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Currently, duplicate classfile annotations cause a runtime crash when the JVM sees them (due to a call to
getAnnotationsor the like). Do instead exactly what Java does (since JEP-120): if the annotation type is (meta-)annotated with@Repeatable, wrap the annotations in an array and annotate the original element with a new annotation of the type given byRepeatable#value.It is now an error to have multiple annotations on the same tree with the same
typeSymbolif the symbol is a classfile annotation.Fixes scala/bug#9529.