Retain parameter annotations in the generation of mixin phase forwarding methods -- PR choice 2 #24893
Conversation
…ding methods
This PR fixes issue scala#22991 by retaining parameter annotations in mixin phase
generation of forwarder methods.
Therefore, a trait method parameter annotated with Java RetentionPolicy.RUNTIME
annotation is retained in the subclasses .class file in same named method/parameter
for Java reflection use.
This restores the behavior of Scala 2 for such RUNTIME annotated paramters.
Closes ticket scala#22991
|
Where to add or not annotations is a bit outside my area of expertise. Someone should evaluate the two alternative PRs and compare with what Scala 2 does. Then do the closest to the Scala 2 behavior. |
| vpms.addAnnotations(annots) | ||
| forwarderSym.setParamss(vparams :: Nil) | ||
| end if | ||
| transformFollowing(DefDef(forwarderSym, forwarderRhsFn(meth))) |
There was a problem hiding this comment.
This was over my head, but it felt like a lot of work.
I pushed a slightly different PR. If that flies, I'll add you as co-author.
There was a problem hiding this comment.
I looked at the PR you mention and I like it. I was not able to clone this repo to try out the test but I assume the test still passes. No problem and appreciate the co-author. I figured my PR here was just a starting point and I have some learning to do if I were to become a further contributor. Thanks.
som-snytt
left a comment
som-snytt
left a comment
There was a problem hiding this comment.
Let's see if the 3rd time was a charm. Feedback welcome!
| @@ -0,0 +1,9 @@ | |||
| class Blah extends scala.annotation.StaticAnnotation | |||
|
|
|||
| trait Barly { | |||
There was a problem hiding this comment.
I will definitely use Barly in future!
|
I forgot that the assignment was to compare Scala 2. Can I get an extension? I saw the Scala 3 comment that params are kept up-to-date until erasure, but I did not understand it. (I expected "fidelity"; but I'll look again tomorrow, or your today.) |
| @@ -0,0 +1,17 @@ | |||
|
|
|||
There was a problem hiding this comment.
It needs to skip scalajs because of java reflection. My version will also fail!
// scalajs: --skip
There was a problem hiding this comment.
sounds good. I didnt understand the issue with scala-js testing.
3 participants
Note -- This is one of two PR choices that fix the issue. Consider this one as second cut with alternate set of changes.
Fixes #22991