Adding a method to a Java annotation breaks tasty-compatibility

[Florian3k]

Compiler version

3.4.0, 3.3.3 (also 3.2.0, 3.1.0, 3.0.0)

Minimized code

Run on jvm newer than 8:

File test.scala:

object Foo {
  @Deprecated
  def foo(): Unit = ???
}

scalac test.scala -release:8 -d Foo.jar
scalac -decompile Foo.jar

Output

Fails with

-- Error: test.scala:2:4 -----------------------------------------------------
2 |    @Deprecated
  |    ^^^^^^^^^^^
  |wrong number of arguments at <no phase> for (since: String, forRemoval: Boolean): Deprecated: (Deprecated#<init> : (since: String, forRemoval: Boolean): Deprecated), expected: 2, found: 0
1 error found

Expectation

Something like that:

/** Decompiled from Foo.jar(Foo.tasty) */
@scala.annotation.internal.SourceFile("test.scala") object Foo {
  @java.lang.Deprecated def foo(): scala.Unit = scala.Predef.???
}

[nicolasstucki]

@smarter the issue happens when unpickling the reference to the constructor of Deprecated. Note that in 2 methods with default implementation where added to this interface.

[smarter]

There's a few things going on here:

• When we read a Java annotation from a classfile, we pretend it has a constructor whose arguments are all the methods defined in the annotation:

  scala3/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala

  Lines 1036 to 1054 in e2c456f

  	/** Annotations in Scala are assumed to get all their arguments as constructor
  	* parameters. For Java annotations we need to fake it by making up the constructor.
  	*/
  	def addAnnotationConstructor(classInfo: TempClassInfoType)(using Context): Unit =
  	newSymbol(
  	owner = classRoot.symbol,
  	name = nme.CONSTRUCTOR,
  	flags = Flags.Synthetic | Flags.JavaDefined | Flags.Method,
  	info = new AnnotConstructorCompleter(classInfo)
  	).entered
  	class AnnotConstructorCompleter(classInfo: TempClassInfoType) extends LazyType {
  	def complete(denot: SymDenotation)(using Context): Unit = {
  	val attrs = classInfo.decls.toList.filter(sym => sym.isTerm && sym != denot.symbol && sym.name != nme.CONSTRUCTOR)
  	val paramNames = attrs.map(_.name.asTermName)
  	val paramTypes = attrs.map(_.info.resultType)
  	denot.info = MethodType(paramNames, paramTypes, classRoot.typeRef)
  	}
  	}

• When we write @foo(args) we create an Annotation structure which wraps a tree created by typechecking new foo(args)
• When tasty pickles an annotation, it pickles that tree.

Adding a new method with a default value to an annotation definition is binary-compatible (https://stackoverflow.com/a/31721504), but it breaks tasty since it changes the annotation constructor,. This is a design flaw in tasty and is going to be hard to fix properly.

Note that if we start from something like:

public @interface Annot {
  String a() default "";
}

@Annot("hi") def foo = 1

And we then add an extra parameter:

public @interface Annot {
  String a() default "";
  String b() default "";
}

... then when unpickling the annotation on def foo, we don't have enough information to even know whether the argument "hi" corresponds to the parameter a or b. If the typechecker desugared the user call to:

@Annot(a = "hi") def foo = 1

Then we'd have the necessary information in Tasty since we'll see a NAMEDARG node in that case.

---

So one plan would be to:

• Change the typechecker to use named arguments for annotation constructors
• Special-case unpickling from tasty for Java annotation constructors: fill-up missing arguments with nme.WILDCARD (see 85cd1cf).
  • For old tasty where we don't have named arguments, we might still be able to fill-up missing arguments based on type alone if there is no ambiguity, otherwise emit an error.

Alternatively, we could envision some more drastic changes in tasty, but that would prevent us from backporting the fix to 3.3.

/cc @sjrd since whatever scheme we come up with needs to be compatible with tasty-query .

[smarter]

Another (unlikely) way for the existing code to do the wrong thing would be the case of a java annotation where the author decided to swap the order of the definitions, for example, if I have:

public @interface Annot {
  String a() default "";
  String b() default "";
}

@Annot("hi", "hello") def foo = 1

and one day Annot is changed to:

public @interface Annot {
  String b() default "";
  String a() default "";
}

Then when we unpickle @Annot("hi", "hello"), we will assign them the wrong way around. I think we have to live with that issue for existing tasty files. For new ones we have to rely on unpickling NamedArg trees and having the TypeAssigner perform the swapping and insertion of nme.WILDCARD.

[sjrd]

We discussed this issue offline with @smarter. Here is the conclusion.

We have so far tried to fit Java annotations into the Scala annotation model. This turns out to be a dead end any way we look at it, because of the following core inconsistency:

• Scala annotations are contructor-based: signed, ordered, with explicit placeholders for optional values
• Java annotations are field-based: unordered, with missing elements for optional values

In order to fix this issue, we have to handle Java annotations using the Java model. Here is how we think we should represent it:

• Use an unsigned nme.CONSTRUCTOR in the Select(New(_), _) node -- despite the fact that we will apply to arguments
• Force NamedArgs for all arguments
• Do not store arguments that were not mentioned in source

When reading TASTy (in dotc or tasty-query), we can resolve trees with the shape Apply(Select(New(_), nme.CONSTRUCTOR), args) specially:

• Check that the New refers to a Java annotation (otherwise something's wrong anyway because all constructors must have signed names)
• Reorder the args and fill in missing elements according to the fields found in the annotation class.