Carry over Scalac semantics for Java annotations

[panacekcz]

Looks like ClassfileParser.addAnnotationConstructor.addConstr is missing Flags.Method, so the synthetic constructor for a java annotation is created as a val instead of a method.

Even with that, instantiating java annotations should better be a compile error.

Test code:

object AnnotInst{
  def main(a: Array[String]) = {
    new java.lang.annotation.Inherited
  }
}

Dotc output:

java.lang.AssertionError: assertion failed: not a method-symbol: val <init>
	at scala.Predef$.assert(Predef.scala:223)
	at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen.asmMethodType(BCodeHelpers.scala:253)
	at scala.tools.nsc.backend.jvm.BCodeHelpers$BCInnerClassGen.asmMethodType$(BCodeHelpers.scala:252)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.asmMethodType(BCodeSkelBuilder.scala:50)
	at scala.tools.nsc.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genCallMethod(BCodeBodyBuilder.scala:1177)
	at scala.tools.nsc.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genApply(BCodeBodyBuilder.scala:753)
	at scala.tools.nsc.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genLoad(BCodeBodyBuilder.scala:344)
	at scala.tools.nsc.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genLoad(BCodeBodyBuilder.scala:414)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.emitNormalMethodBody$1(BCodeSkelBuilder.scala:602)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.genDefDef(BCodeSkelBuilder.scala:635)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.gen(BCodeSkelBuilder.scala:506)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.$anonfun$gen$1(BCodeSkelBuilder.scala:508)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.$anonfun$gen$1$adapted(BCodeSkelBuilder.scala:508)
	at scala.collection.immutable.List.foreach(List.scala:392)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.gen(BCodeSkelBuilder.scala:508)
	at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.genPlainClass(BCodeSkelBuilder.scala:116)
	at dotty.tools.backend.jvm.GenBCodePipeline$Worker1.visit(GenBCode.scala:212)
	at dotty.tools.backend.jvm.GenBCodePipeline$Worker1.run(GenBCode.scala:179)
	at dotty.tools.backend.jvm.GenBCodePipeline.buildAndSendToDisk(GenBCode.scala:417)
	at dotty.tools.backend.jvm.GenBCodePipeline.run(GenBCode.scala:383)
	at dotty.tools.backend.jvm.GenBCode.run(GenBCode.scala:53)
	at dotty.tools.dotc.core.Phases$Phase.$anonfun$runOn$1(Phases.scala:297)
	at scala.collection.immutable.List.map(List.scala:286)
	at dotty.tools.dotc.core.Phases$Phase.runOn(Phases.scala:295)
	at dotty.tools.backend.jvm.GenBCode.runOn(GenBCode.scala:58)
	at dotty.tools.dotc.Run.$anonfun$compileUnits$3(Run.scala:172)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
	at dotty.tools.dotc.util.Stats$.trackTime(Stats.scala:49)
	at dotty.tools.dotc.Run.$anonfun$compileUnits$2(Run.scala:169)
	at dotty.tools.dotc.Run.$anonfun$compileUnits$2$adapted(Run.scala:167)
	at scala.collection.IndexedSeqOptimized.foreach(IndexedSeqOptimized.scala:36)
	at scala.collection.IndexedSeqOptimized.foreach$(IndexedSeqOptimized.scala:33)
	at scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:198)
	at dotty.tools.dotc.Run.runPhases$1(Run.scala:167)
	at dotty.tools.dotc.Run.$anonfun$compileUnits$1(Run.scala:192)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
	at dotty.tools.dotc.util.Stats$.maybeMonitored(Stats.scala:90)
	at dotty.tools.dotc.Run.compileUnits(Run.scala:147)
	at dotty.tools.dotc.Run.compileSources(Run.scala:134)
	at dotty.tools.dotc.Run.compile(Run.scala:118)
	at dotty.tools.dotc.Driver.doCompile(Driver.scala:33)
	at dotty.tools.dotc.Driver.process(Driver.scala:166)
	at dotty.tools.dotc.Driver.process(Driver.scala:135)
	at dotty.tools.dotc.Driver.process(Driver.scala:147)
	at dotty.tools.dotc.Driver.main(Driver.scala:174)
	at dotty.tools.dotc.Main.main(Main.scala)
Error while emitting AnnotInst.scala
assertion failed: not a method-symbol: val <init>
one error found

[Blaisorblade]

Have tried adding the missing flag to fix the crash? Could you send a PR if that works?

[panacekcz]

Yes, but if the correct result is compile error, it would only be a partial fix. So should I do that PR without adding a pos test? Should that PR close this issue?

[Blaisorblade]

Such a PR should reference this issue without closing it. It'd still help.

I'd maybe add a test in pos/ with a comment that this should give an error, and maybe also the desired test in tests/pending/.

==
On the rest of the issue, I am not sure why and when the instantiation should fail; in this case, based on the Java error, the annotation class should probably be marked as abstract (with Flags.Abstract IIUC?). Are Java annotation classes always abstract?

$ javac foo.java
foo.java:3: error: Inherited is abstract; cannot be instantiated
		new java.lang.annotation.Inherited();
		^
1 error

[panacekcz]

By JLS 9.6, annotation types are a special kind of interface, and by JLS 9.1.1.1 every interface is abstract.

[Blaisorblade]

• Thanks for the pointers! Dug into this a bit. The JVM spec, 4.1 mandates that Java annotation are also marked as interfaces and as abstract.*

• But we remove those flags, and have done so since Dotty was born:
  https://github.com/lampepfl/dotty/blob/5547a0b4f2fb8b722589bf9513b922cdb79bfd99/compiler/src/dotty/tools/dotc/core/classfile/ClassfileConstants.scala#L354-L357

• That comes from Scalac (tho code diverged), where this has been fixed. That file's history suggests looking into scala/scala@77b8b6a, probably superseded by Java annotations are not classes. scala#6869.
  It's not clear whether creating annotations by hand is rejected; the problem seems that they can be created almost as easily via @, so the Scalac fix involves a new typechecking mode:
  https://github.com/scala/scala/pull/6869/files#diff-04639e477fcdf844cf3ae0e3e4bf5d7eR1656

*JVM Spec quotes:

If the ACC_ANNOTATION flag is set, the ACC_INTERFACE flag must also be set.
If the ACC_INTERFACE flag is set, the ACC_ABSTRACT flag must also be set

[Blaisorblade]

TLDR: Action plan: carry over Scalac fixes. (See previous commit for what exactly).

[smarter]

Action plan: kidnap @hrhino and get him to port all his fixes to dotty :).