Skip to content
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

Deprecate ClassfileAnnotation, introduce ConstantAnnotation #6143

Merged
merged 4 commits into from Dec 13, 2017

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Oct 20, 2017

Annotations that need to be written to the classfile in the
Java-compatible format need to be defined in Java.

Scala annotations extending ConstantAnnotation can only accept
compile-time constants as argument. The implementation represents
constant annotations using the assocs field in the AnnotationInfo,
just like for Java-defined annotations. The implementation for getting
the constants and building the assocs is the same as for Java
annotations, and the same restrictions apply: arguments need to be
passed as named arguments (except if there's a single argument defining
the value parameter). This is a bit of an ad-hoc restriction for
Scala-defined constant annotations, but changing it to accept positional
args went over my time budget (there's no easy way to reuse the code
that deals with named arguments in general).

Internally, we still add the StaticAnnotation parent to Java-defined
annotations (in the Classfile parser, Java source parser, runtime
reflection) to make sure that Java annotations are not only emitted to
the classfile in the Java format, but also pickled.

Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

Looks good. I got 99% through the review and then interrupted again.

@@ -1989,6 +1972,16 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
if (clazz.info.firstParent.typeSymbol == AnyValClass)
validateDerivedValueClass(clazz, body3)

if (!clazz.isTrait && clazz.isNonBottomSubClass(ConstantAnnotationClass)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the restriction on classfile annotations that they could not be inner classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see this is caught by https://github.com/scala/scala/pull/6143/files#diff-4eab1aad4533a31c10565971e90f73eaR3738 (else if (!annType.typeSymbol.isJavaDefined))

@adriaanm
Copy link
Contributor

adriaanm commented Dec 1, 2017

I tried to resolve the conflicts with the web editor (cool!), but the UI hasn't caught up?

@xeno-by
Copy link
Member

xeno-by commented Dec 1, 2017

@adriaanm Wow the web editor is pretty cool indeed 😍

@adriaanm
Copy link
Contributor

adriaanm commented Dec 1, 2017

Some by-name fails due to my incomplete merge?

!! 680 - neg/t6895.scala                           [expected compilation failure]
!! 687 - neg/t6895b.scala                          [expected compilation failure]
!!  62 - run/byname.scala                          [compilation failed]

byname.scala:55: error: type mismatch;
 found   : Int
 required: () => ?
val testC00R = testC00 _
               ^
byname.scala:58: error: type mismatch;
 found   : Int
 required: () => ?
val testC00RR = testC00() _
                       ^
two errors found

@lrytz
Copy link
Member Author

lrytz commented Dec 1, 2017

These test failures are in current 2.13.x, since the merge of #6092 / https://github.com/scala/scala/commits/8272151ac87628e2f1165547ff4b794f3226bbbc

In scala#6092, `-Xsource:2.13` was enabled by default. Between this PRs
parent and current 2.13.x, there were some changes that depended on
this flag (scala#5983, scala#6069).
Annotations that need to be written to the classfile in the
Java-compatible format need to be defined in Java.

Scala annotations extending ConstantAnnotation can only accept
compile-time constants as argument. The implementation represents
constant annotations using the `assocs` field in the AnnotationInfo,
just like for Java-defined annotations. The implementation for getting
the constants and building the `assocs` is the same as for Java
annotations, and the same restrictions apply: arguments need to be
passed as named arguments (except if there's a single argument defining
the `value` parameter). This is a bit of an ad-hoc restriction for
Scala-defined constant annotations, but changing it to accept positional
args went over my time budget (there's no easy way to reuse the code
that deals with named arguments in general).

Internally, we still add the `StaticAnnotation` parent to Java-defined
annotations (in the Classfile parser, Java source parser, runtime
reflection) to make sure that Java annotations are not only emitted to
the classfile in the Java format, but also pickled.
Also enforce the primary constructor to have a single argument list.

Clarify the Scaladoc of ConstantAnnotation.
@lrytz
Copy link
Member Author

lrytz commented Dec 8, 2017

ping @adriaanm

@retronym
Copy link
Member

retronym commented Jan 17, 2018

FTR, this change seems to be causing some temporary problems when working on the 2.13.x branch. The reference compiler, 2.13.0-M2, incorrectly typechecks @SerialVersionUID(0L) to put the arguments in args, rather than assocs, because SerialVersionID no longer extends ClassfileAnnotation. The backend then omits the static field, because the pattern match doesn't look at args.

This causes a failure in SerializationStabilityTest.

@lrytz
Copy link
Member Author

lrytz commented Jan 17, 2018

@retronym the failure only shows up when using the reference compiler with a current 2.13.x version of the library on the class- / sourcepath, not on CI where we bootstrap, correct? Would you like me to do something about it? M3 is coming out soon...

@retronym
Copy link
Member

retronym commented Jan 17, 2018

Yes, the problem appears specific to local development without bootstrapping.

I think we can just wait for the next release. I just wanted to note the problem for posterity on the off chance that it helps someone else avoid debugging it :)

With the benefit of hindsight, we might have been better to defer changing our annotations to the non-deprecated parent types until the next STARR update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants