SI-9393 fix modifiers of ClassBTypes for Java annotations #4638

lrytz · 2015-07-16T07:41:53Z

The Scala classfile and java source parsers make Java annotation
classes (which are actually interfaces at the classfile level) look
like Scala annotation classes:

the INTERFACE / ABSTRACT flags are not added
scala.annotation.Annotation is added as superclass
scala.annotation.ClassfileAnnotation is added as interface

This makes type-checking @annot uniform, whether it is defined in Java
or Scala.

This is a hack that leads to various bugs (SI-9393, SI-9400). Instead
the type-checking should be special-cased for Java annotations.

This commit fixes SI-9393 and a part of SI-9400, but it's still easy
to generate invalid classfiles. Restores the annotations that were
disabled in #4621. I'd like to leave these assertions in: they
are valuable and helped uncovering the issue being fixed here.

A new flag ANNOTATION is introduced for Java annotation ClassSymbols,
similar to the existing ENUM flag. When building ClassBTypes for
Java annotations, the flags, superclass and interfaces are recovered
to represent the situation in the classfile.

Cleans up and documents the flags space in the area of "late" and
"anti" flags.

The test for SI-9393 is extended to test both the classfile and the
java source parser.

lrytz · 2015-07-16T07:47:04Z

review by @retronym. opinions welcome: is it too much of a workaround?

retronym · 2015-07-17T02:39:16Z

I'm fine with the approach of repairing this in the backend for now, but agree that a special case in the typer would be worth trying.

I'd like to see your (overdue) commentary surrounding flags turned into a unit test, exercising this sort of thing:

scala> def sym = NoSymbol.newTermSymbol(nme.EMPTY)
sym: $r.intp.global.TermSymbol

scala> def withFlagMask[A](mask: Long)(body: => A): A = enteringPhase(new Phase(NoPhase) { override def flagMask = mask; def name = ""; def run() = () })(body)
withFlagMask: [A](mask: Long)(body: => A)A

scala> withFlagMask(InitialFlags)(sym.setFlag(PRIVATE).setFlag(notPRIVATE).isPrivate)
res13: Boolean = true

scala> withFlagMask(AllFlags)(sym.setFlag(PRIVATE).setFlag(notPRIVATE).isPrivate)
res14: Boolean = false

scala> withFlagMask(InitialFlags | notPRIVATE (sym.setFlag(PRIVATE).setFlag(notPRIVATE).isPrivate)
res16: Boolean = false

scala> withFlagMask(InitialFlags)(sym.setFlag(lateDEFERRED).isDeferred)
res21: Boolean = false

scala> withFlagMask(InitialFlags | lateDEFERRED)(sym.setFlag(lateDEFERRED).isDeferred)
res22: Boolean = true

It might also be worth noting that the difference between anti and late flags is that the former allow a transition from 1 -> 0 (e.g. we can turn off PRIVATE), while the latter let us switch a flag from 0 -> 1 (a concrete method in a trait becomes DEFERRED once we move its body to the implementation class).

retronym · 2015-07-17T02:39:52Z

Restores the annotations

*assertions

retronym · 2015-07-17T02:44:10Z

I would think we should expose the new flag in /Users/jason/code/scala2/src/reflect/scala/reflect/api/FlagSets.scala so that macro authors can use it.

retronym · 2015-07-17T02:53:11Z

Given that we should never pickle this new flag (it is only on external symbols), we don't need to update the pickle format or rawPickledCorrespondence.

But we probably should update JavaClassMirror to populate this flag when creating symbols based on the Java reflection API. Actually, on closer inspection I think we'll get this for free as it shares code with flagTranslation, but it would be good to add a test case.

retronym · 2015-07-17T02:54:22Z

src/reflect/scala/reflect/internal/Flags.scala

@@ -120,7 +120,8 @@ class ModifierFlags {
  final val ARTIFACT      = 1L << 46      // symbol should be ignored when typechecking; will be marked ACC_SYNTHETIC in bytecode
                                          // to see which symbols are marked as ARTIFACT, see scaladocs for FlagValues.ARTIFACT
  final val DEFAULTMETHOD = 1L << 47      // symbol is a java default method
-  final val ENUM          = 1L << 48      // symbol is an enum
+  final val ENUM          = 1L << 48      // symbol is a java enum
+  final val ANNOTATION    = 1L << 49      // symbol is a java annotation


I wonder if we should call this JAVA_ANNOTATION. Otherwise it might be surprising to find it absent from Scala defined annotations.

If I ever find the issue with typechecking Java annotation code correctly, Scala will also be able to emit platform annotations. I think it's not simply a question of Scala-defined vs. Java-defined annotations.

i don't fully get what you are saying. in any case, emitting java annotations also needs work in the bytecode generator.

Yep, it's just the part I'm still blocked on, and I lacked the time to solve this issue.

lrytz · 2015-07-17T07:48:18Z

I would think we should expose the new flag in /Users/jason/code/scala2/src/reflect/scala/reflect/api/FlagSets.scala so that macro authors can use it.

Just looked around a bit, there's a comment in that file:

  // Q: I have a pretty flag. Can I put it here?
  // A: Only if there's a tree that cannot be built without it.
  //    If you want to put a flag here so that it can be tested against,
  //    introduce an `isXXX` method in one of the `api.Symbols` classes instead.

lrytz · 2015-07-21T20:41:22Z

re-review by @retronym - it grew quite a bit...

retronym · 2015-07-22T00:06:10Z

src/compiler/scala/reflect/quasiquotes/Parsers.scala

@@ -93,7 +93,7 @@ trait Parsers { self: Quasiquotes =>
      import treeBuilder.{global => _, unit => _}

      // q"def foo($x)"
-      override def param(owner: Name, implicitmod: Int, caseParam: Boolean): ValDef =
+      override def param(owner: Name, implicitmod: Long, caseParam: Boolean): ValDef =


After this change, the subsequent implicitMod.toLong is redundant.

retronym · 2015-07-22T00:17:22Z

I've removed the redundant toLong calls here: https://github.com/lrytz/scala/compare/lrytz:t9393...retronym:review/4638?expand=1

retronym · 2015-07-22T00:32:41Z

test/junit/scala/tools/nsc/symtab/FlagsTest.scala

+    assertTrue(withFlagMask(AllFlags)(sym.setFlag(AllFlags).flags) == allButNegatable)
+    assertTrue(withFlagMask(AllFlags)(sym.setFlag(allButLateable).flags) == allButNegatable)
+
+    assertTrue(withFlagMask(AllFlags)(sym.setFlag(lateFlags).flags) == (lateFlags | lateable))


Prefer assertEquals to assertTrue for better assertion failure messages.

retronym · 2015-07-22T06:09:52Z

This will result in slightly less concise bytecode for places referring to the formerly integer constants. Longs are always loaded from the constant pool with a ldc_w, whereas low range ints can be embedded into the bytecode itself with a {b,s}ipush, and even when loaded from the constant pool can use the shorter ldc instruction for low slots.

% cat sandbox/test.scala && scalac sandbox/test.scala && javap -classpath . -c Test
class Test {
  final val i1 = 42
  final val i2 = 999
  final val i3 = Int.MaxValue
  final val l1 = 42L

  def test = {
    java.lang.Integer.signum(i1)
    java.lang.Integer.signum(i2)
    java.lang.Integer.signum(i3)
    java.lang.Long.signum(l1)
  }
}
Compiled from "test.scala"
public class Test {
  public final int i1();
    Code:
       0: bipush        42
       2: ireturn

  public final int i2();
    Code:
       0: sipush        999
       3: ireturn

  public final int i3();
    Code:
       0: ldc           #18                 // int 2147483647
       2: ireturn

  public final long l1();
    Code:
       0: ldc2_w        #20                 // long 42l
       3: lreturn

  public int test();
    Code:
       0: bipush        42
       2: invokestatic  #28                 // Method java/lang/Integer.signum:(I)I
       5: pop
       6: sipush        999
       9: invokestatic  #28                 // Method java/lang/Integer.signum:(I)I
      12: pop
      13: ldc           #18                 // int 2147483647
      15: invokestatic  #28                 // Method java/lang/Integer.signum:(I)I
      18: pop
      19: ldc2_w        #20                 // long 42l
      22: invokestatic  #33                 // Method java/lang/Long.signum:(J)I
      25: ireturn

  public Test();
    Code:
       0: aload_0
       1: invokespecial #37                 // Method java/lang/Object."<init>":()V
       4: return
}

Does this matter? Maybe not. But we should sanity check performance before we do this, and also structure things so that we could revert just this change without losing any correctness that it brought.

The Scala classfile and java source parsers make Java annotation classes (which are actually interfaces at the classfile level) look like Scala annotation classes: - the INTERFACE / ABSTRACT flags are not added - scala.annotation.Annotation is added as superclass - scala.annotation.ClassfileAnnotation is added as interface This makes type-checking @annot uniform, whether it is defined in Java or Scala. This is a hack that leads to various bugs (SI-9393, SI-9400). Instead the type-checking should be special-cased for Java annotations. This commit fixes SI-9393 and a part of SI-9400, but it's still easy to generate invalid classfiles. Restores the assertions that were disabled in scala#4621. I'd like to leave these assertions in: they are valuable and helped uncovering the issue being fixed here. A new flag JAVA_ANNOTATION is introduced for Java annotation ClassSymbols, similar to the existing ENUM flag. When building ClassBTypes for Java annotations, the flags, superclass and interfaces are recovered to represent the situation in the classfile. Cleans up and documents the flags space in the area of "late" and "anti" flags. The test for SI-9393 is extended to test both the classfile and the java source parser.

Similar to the new JAVA_ANNOTATION flag, be more explicit about flags for java entities.

Adds query methods to the public reflection API for querying the JAVA_ENUM and JAVA_ANNOTATION flags. Didn't include JAVA_DEFAULTMETHOD because it does not correspond to a real java classfile flag (just a non-abstract method in an interface), and we want to clean the usage of this flag before adding it to a public API. The flags themselfs are not added to the reflection API. A comment in api/FlagSets.scala says: Q: I have a pretty flag. Can I put it here? A: Only if there's a tree that cannot be built without it. If you want to put a flag here so that it can be tested against, introduce an `isXXX` method in one of the api.Symbols classes instead.

lrytz · 2015-07-22T07:25:39Z

Thanks for the review and patches! I included your changes to remove more toLong calls into the last commit.

I'll run some a/b testing to see if I can find any performance difference. In any case, the last commit doesn't fix any bugs, it can be reverted.

lrytz · 2015-07-22T09:43:22Z

Some compiler timings for the flags-as-long change.

Compiling the standard library ten times:

before: 71.4662 in average
after: 71.8921

Compiling List.scala ten times

before: 8.1708 in average
after: 8.3

https://docs.google.com/spreadsheets/d/12-GILZectCueVLeyC0HjOlnlQRZPBcTADGf9sVmGAW0/edit#gid=0

retronym · 2015-07-24T01:42:33Z

I'd say that difference is within measurement error. But maybe we should leave that commit aside and use it to motivate us to get at least a small JMH compiler benchmark suite up and running to be able to be more certain.

retronym · 2015-07-24T01:53:46Z

I also noticed that constant folding doesn't work in cases like final val x = (CONSTANT_INT : Long) << 3. I've noted the possibility to improve this in https://issues.scala-lang.org/browse/SI-9416

lrytz · 2015-07-24T06:33:07Z

OK, removed the last commit.

lrytz · 2015-07-24T06:33:54Z

archived here: https://github.com/lrytz/scala/commits/wip/useLongForAllFlags

retronym · 2015-07-24T06:36:34Z

LGTM

SI-9393 fix modifiers of ClassBTypes for Java annotations

scala-jenkins added this to the 2.12.0-M3 milestone Jul 16, 2015

lrytz force-pushed the t9393 branch 2 times, most recently from 1c8c92a to 00bbca9 Compare July 16, 2015 07:42

lrytz force-pushed the t9393 branch 2 times, most recently from 195cd07 to 573a63e Compare July 16, 2015 10:50

retronym reviewed Jul 17, 2015
View reviewed changes

lrytz force-pushed the t9393 branch 2 times, most recently from 36a81ea to 0566ed6 Compare July 21, 2015 20:28

retronym reviewed Jul 22, 2015
View reviewed changes

lrytz force-pushed the t9393 branch 3 times, most recently from 2cfbda9 to 60fd361 Compare July 22, 2015 07:18

lrytz added 3 commits July 22, 2015 09:21

Rename the ENUM / DEFAULTMETHOD flags to include JAVA_

76c133d

Similar to the new JAVA_ANNOTATION flag, be more explicit about flags for java entities.

lrytz force-pushed the t9393 branch from 60fd361 to 9453b9c Compare July 22, 2015 07:21

lrytz mentioned this pull request Jul 23, 2015

Backend Backports #4624

Closed

lrytz force-pushed the t9393 branch from 9453b9c to 59f1ee5 Compare July 24, 2015 06:32

scala-jenkins added the reviewed label Jul 24, 2015

retronym added a commit that referenced this pull request Jul 24, 2015

Merge pull request #4638 from lrytz/t9393

aae695e

SI-9393 fix modifiers of ClassBTypes for Java annotations

retronym merged commit aae695e into scala:2.12.x Jul 24, 2015

lrytz deleted the t9393 branch July 30, 2015 09:14

lrytz mentioned this pull request Sep 21, 2015

SI-8778 Treat annotations as interfaces #4739

Closed

adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016

This was referenced Apr 7, 2017

compiler crash in GenBCode referring to a class that extends an annotation scala/bug#9393

Closed

subclassing a java annotation generates an invalid classfile scala/bug#9400

Closed

smarter mentioned this pull request Oct 30, 2017

Compiler crash in GenBCode referring to a class that extends an annotation scala/scala3#3403

Open

nicolasstucki mentioned this pull request Jan 17, 2018

Meta-issue: Port Backend improvements scala/scala3#3113

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SI-9393 fix modifiers of ClassBTypes for Java annotations #4638

SI-9393 fix modifiers of ClassBTypes for Java annotations #4638

lrytz commented Jul 16, 2015

lrytz commented Jul 16, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym Jul 17, 2015

soc Jul 18, 2015

lrytz Jul 20, 2015

soc Jul 20, 2015

lrytz commented Jul 17, 2015

lrytz commented Jul 21, 2015

retronym Jul 22, 2015

retronym commented Jul 22, 2015

retronym Jul 22, 2015

retronym commented Jul 22, 2015

lrytz commented Jul 22, 2015

lrytz commented Jul 22, 2015

retronym commented Jul 24, 2015

retronym commented Jul 24, 2015

lrytz commented Jul 24, 2015

lrytz commented Jul 24, 2015

retronym commented Jul 24, 2015

SI-9393 fix modifiers of ClassBTypes for Java annotations #4638

SI-9393 fix modifiers of ClassBTypes for Java annotations #4638

Conversation

lrytz commented Jul 16, 2015

lrytz commented Jul 16, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym commented Jul 17, 2015

retronym Jul 17, 2015

Choose a reason for hiding this comment

soc Jul 18, 2015

Choose a reason for hiding this comment

lrytz Jul 20, 2015

Choose a reason for hiding this comment

soc Jul 20, 2015

Choose a reason for hiding this comment

lrytz commented Jul 17, 2015

lrytz commented Jul 21, 2015

retronym Jul 22, 2015

Choose a reason for hiding this comment

retronym commented Jul 22, 2015

retronym Jul 22, 2015

Choose a reason for hiding this comment

retronym commented Jul 22, 2015

lrytz commented Jul 22, 2015

lrytz commented Jul 22, 2015

retronym commented Jul 24, 2015

retronym commented Jul 24, 2015

lrytz commented Jul 24, 2015

lrytz commented Jul 24, 2015

retronym commented Jul 24, 2015