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
SI-4788 Respect RetentionPolicy of Java annotations #3459
Conversation
Review by @retronym and @xeno-by please, especially regarding:
Comments and opinions appreciated. |
private def isRuntimeVisible(annot: AnnotationInfo) = { | ||
annot.atp.typeSymbol.annotations | ||
.find(annot => annot.matches(AnnotationRetentionAttr)) | ||
.exists(annotRet => annotRet.assocs.contains((TermName("value") -> LiteralAnnotArg(Constant(AnnotationRetentionPolicyRuntimeValue))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use nme.value
.
Every call to TermName(...)
has to intern, that's why we cache commonly used names. Additionally, it's good to be able to find all usages of nme.value
and find spots in the compiler that use that name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@retronym Added tests. |
PLS REBUILD/pr-scala@ec9b8c9 |
(kitty-note-to-self: ignore 34027767) |
annot.matches(ClassfileAnnotationClass) && | ||
retentionPolicyOf(annot) != AnnotationRetentionPolicySourceValue && | ||
annot.args.isEmpty && | ||
!annot.matches(DeprecatedAttr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, so DeprecatedAttr is java-defined?
No, but deprecation needs to be treated differently, see http://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.15.
Yes, the code feels like it has too many checks here, I'll investigate.
PLS REBUILD/pr-scala@4d698ca |
(kitty-note-to-self: ignore 34145783) |
For god's sake ... is stuff broken again? |
Looks like a connectivity problem to Github. If it persists, I'll contact the sysadmin at EPFL to see if its on their network. PLS REBUILD ALL |
(kitty-note-to-self: ignore 34155799) |
PLS REBUILD ALL |
(kitty-note-to-self: ignore 34279298) |
Sigh
|
When compiling with
I have absolutely no idea what's happening here. |
@retronym Any idea what's happening here? |
Build yourself a new locker to workaround:
You might also need to rebase this onto master. |
@retronym Mhhh, do you mean versions.properties? |
Also closes SI-5420 and SI-5948.
Build.properties is an optional file that can contain local ant properties. On Monday, February 10, 2014, soc notifications@github.com wrote:
|
@soc: what's the status of this? |
@gkossakowski I'm slowly losing my mind. With @retronym's change, it at least compiles locally, but now suddenly the compiler pretends that the elements annotated with Scala annotations don't exist anymore.
|
Huh, that's strange indeed. In any case, I think we'll need to push this to 2.11.1 (if the change retains binary compatibility) or 2.12. Please reopen once we are done with 2.11.0 RCs. |
@gkossakowski Alright, no problem. I will look into it as soon as trunk is a bit more stable again (doesn't make sense if I get it to run locally, but PR validation still fails ...) |
Also closes SI-5420 and SI-5948.