Skip to content

Fix parsing Java annotation values #11809

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

Merged
merged 5 commits into from
Aug 19, 2021

Conversation

noti0na1
Copy link
Member

The annotation arguments were not parsed correctly; since the literal LiteralT returns was always the annotaion id string.

@noti0na1
Copy link
Member Author

@smarter It there a way to test the annotation values read by the compiler?

@noti0na1 noti0na1 requested a review from smarter March 18, 2021 18:46
@smarter
Copy link
Member

smarter commented Mar 18, 2021

You could write a macro that calls getAnnotation

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to the parser look OK to me, but it would be nice to have a test.

@noti0na1
Copy link
Member Author

@smarter @odersky It is tricky to write a test to checking the values of annotations in the compiler.
I don't know how to write a macro which calls the getAnnotation function in the compiler.

@smarter
Copy link
Member

smarter commented Mar 25, 2021

I don't know how to write a macro which calls the getAnnotation function in the compiler.

getAnnotation is available in the library as part of tasty-reflect, /cc @nicolasstucki could you help @noti0na1 come up with a testcase?

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Apr 27, 2021

@noti0na1 What was the code where you saw that? Which annotation was it?

@noti0na1
Copy link
Member Author

@nicolasstucki Some of my code break the typing of annotations in Java. During debugging, I noticed the arguments from all annotations (in Java file) are not parsed correctly.

If you print out the tree, you may see the wrong arguments?

@dwijnand
Copy link
Member

dwijnand commented Jul 7, 2021

@noti0na1 could you provide a code example showing what happened and what you expected. That would form the basis for writing a test case for this - it's a little hard to understand by reverse engineering your patch here.

@SethTisue
Copy link
Member

Does this affect runtime annotations as well as compile-time-only annotations? A runtime annotation might be easier to construct a test case around.

@smarter
Copy link
Member

smarter commented Jul 7, 2021

@SethTisue this PR changes the JavaParser so it affects how the scala compiler (and therefore macros) sees Java annotations coming from .java file, but it has no impact on what annotations are kept at runtime.

@noti0na1
Copy link
Member Author

noti0na1 commented Jul 8, 2021

@dwijnand

An example would be any Java class with some annotations:

class Test {
  @SuppressWarnings("unchecked")
  void f() {}
}

I found this bug because I was trying to print some trees in the compiler, and I noticed the parsed arguments were wrong.

The compiler doesn't use the annotation arguments in Java files currently, so it is difficult to write a test.
We can use a macro to show the annotations, but I don't know how to do it.

@anatoliykmetyuk
Copy link
Contributor

@noti0na1 here's a macro that will give you annotations of the first method of stuff.Bar class:

mcr.scala

package stuff

import scala.quoted.*

inline def mcr: Any = ${ mcrImpl }
def mcrImpl(using Quotes): Expr[Any] =
  import quotes.reflect.*
  println(Symbol.requiredClass("stuff.Bar").declaredMethods.head.annotations)
  val tree = '{()}
  tree

Test.scala

package stuff

trait Bar:
  @SuppressWarnings(Array("unchecked"))
  def f(x: Int) = ???

@main def Test = mcr

It successfully obtains annotations for types from the Scala sources. However, for a class coming from a Java source, it doesn't find any annotations. Maybe you can come up with something based on the example above.

@noti0na1
Copy link
Member Author

I add a test using the macro created by @anatoliykmetyuk , we can see from the output the arguments are correctly parsed now ("a", "b", "c" and "d").

@smarter Could you review the PR again? Thanks.

@noti0na1 noti0na1 requested a review from smarter August 19, 2021 05:35
@smarter smarter merged commit 408e6d2 into scala:master Aug 19, 2021
@smarter smarter deleted the fix-annot-arg-value branch August 19, 2021 09:16
olsdavis pushed a commit to olsdavis/dotty that referenced this pull request Apr 4, 2022
@Kordyjan Kordyjan added this to the 3.1.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants