Support for parsing annotation arguments from java #11117
Conversation
|
I won't have time to review this PR this week, @liufengyun : do you think you could have a look? |
No problem, I'll do it. |
|
Some of the code in this PR appears to be adapted from Scala 2 code in https://github.com/scala/scala/blob/2.13.x/src/compiler/scala/tools/nsc/javac/JavaParsers.scala (the relevant scala 2 PRs are scala/scala#8781 and scala/scala#8982). This should be indicated in the corresponding commit messages for proper attribution. |
|
Attribution added and ported work squashed into a single commit. |
| @@ -81,6 +81,12 @@ object Typer { | |||
| */ | |||
| private val DroppedEmptyArgs = new Property.Key[Unit] | |||
|
|
|||
|
|
|||
| /** Marker context property that indicates that typer is resolving types for arguments of | |||
| * an annotation defined in Java. This means that T can appear in positions where array[T] is expected. | |||
There was a problem hiding this comment.
typo: array[T] -> Array[T]?
The text is not easy to understand. Maybe add a small code example?
| val arg1 = fallback match { | ||
| case Some(f) => tryAlternatively { typed(tree.arg, pt) } { f } | ||
| case _ => typed(tree.arg, pt) | ||
| } |
There was a problem hiding this comment.
Maybe merge this pattern match with the one above, to make the logic more clear.
There was a problem hiding this comment.
I've done it that way because I wanted to avoid repeating typed(tree.arg, pt) three times, as it may be a source of errors when the logic would be changed. On the other hand, merging those two patterns is much more readable.
| } | ||
| if (c.isJava) c.fresh.setProperty(JavaAnnotationArg, ()) | ||
| else c |
There was a problem hiding this comment.
If c.isJava implies JavaAnnotationArg, do we still need the property JavaAnnotationArg?
There was a problem hiding this comment.
My logic here was that JavaAnnotationArg implies that c.isJava is true and that we are currently typechecking annotation arguments.
If it is guaranteed that isJava being true inside of typedNamedArg implies that we are typechecking annotation arguments, then indeed it may be redundant. But I'm not sure if it is guaranteed now and it stays guaranteed in the future.
There was a problem hiding this comment.
But I'm not sure if it is guaranteed now and it stays guaranteed in the future.
I think it's safe to assume this if you leave a comment explaining the assumption in the code, we can always adapt the code if Java somehow changes, but I wouldn't worry too much about it.
* A port of fixes from scala/scala#8781 with improvements. * Some of improvements come from scala/scala#8982. * There are small changes to typer to allow for single elements of any type T be used where array of T is expected inside of java annotations arguments as it is correct in java and is used in 2 files in scala standard library.
| val arg1 = typed(tree.arg, pt) | ||
| val arg1 = if (ctx.property(JavaAnnotationArg).isDefined) { | ||
| pt match { | ||
| case AppliedType(a, typ :: Nil) if (a.isRef(defn.ArrayClass)) => |
There was a problem hiding this comment.
What about reducing the outer if/else by moving the condition to the guard here?
Co-authored-by: Fengyun Liu <liu@fengy.me>
No description provided.