Fix #19951: Align TASTy with the Java annotation model. (#20539)

Scala annotations are classes, with a real constructor, which has a real
signature where order is relevant but names are irrelevant.

On the contrary, Java annotations are interfaces, without any real
constructors. The names of "fields" are relevant, whereas their order is
irrelevant.

As illustrated by #19951, trying
to shoehorn Java annotations into the Scala annotation model is not
sustainable, and breaks in real ways. Therefore, in this commit we align
how Java annotations are stored in TASTy with the Java annotation model.

During pickling:

* Selection of the constructor is pickled without a signature.
* Default arguments are dropped.
* (Due to the parent commit, all arguments are `NamedArg`s at this point.)

During unpickling:

* Selection of the constructor resolves to the unique constructor
  (instead of complaining because a signature-less `SELECT` should not
  resolve to a member with a signature).
* Arguments to the constructor are reordered and extended with defaults
  to match the target constructor; we can do this because all the
  arguments are `NamedArg`s.

For backward compatibility, during unpickling:

* If we read a `SELECTin` for a Java annotation constructor, we
  disregard its signature and pretend it was a `SELECT`.
* We adapt arguments in best-effort way if not all of them are
  `NamedArg`s.

compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala

@@ -466,7 +466,10 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
               }
             case _ =>
               if passesConditionForErroringBestEffortCode(tree.hasType) then
-                val sig = tree.tpe.signature
+                // #19951 The signature of a constructor of a Java annotation is irrelevant
+                val sig =
+                  if name == nme.CONSTRUCTOR && tree.symbol.exists && tree.symbol.owner.is(JavaAnnotation) then Signature.NotAMethod
+                  else tree.tpe.signature
                 var ename = tree.symbol.targetName
                 val selectFromQualifier =
                   name.isTypeName
@@ -507,7 +510,14 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
             writeByte(APPLY)
             withLength {
               pickleTree(fun)
-              args.foreach(pickleTree)
+              // #19951 Do not pickle default arguments to Java annotation constructors
+              if fun.symbol.isClassConstructor && fun.symbol.owner.is(JavaAnnotation) then
+                for arg <- args do
+                  arg match
+                    case NamedArg(_, Ident(nme.WILDCARD)) => ()
+                    case _                                => pickleTree(arg)
+              else
+                args.foreach(pickleTree)
             }
           }
         case TypeApply(fun, args) =>

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

@@ -1281,7 +1281,14 @@ class TreeUnpickler(reader: TastyReader,
           if unpicklingJava && name == tpnme.Object && qual.symbol == defn.JavaLangPackageVal then
             defn.FromJavaObjectSymbol.denot
           else
-            accessibleDenot(qual.tpe.widenIfUnstable, name, sig, target)
+            val qualType = qual.tpe.widenIfUnstable
+            if name == nme.CONSTRUCTOR && qualType.classSymbol.is(JavaAnnotation) then
+              // #19951 Disregard the signature (or the absence thereof) for constructors of Java annotations
+              // Note that Java annotations always have a single public constructor
+              // They may have a PrivateLocal constructor if compiled from source in mixed compilation
+              qualType.findMember(name, qualType, excluded = Private)
+            else
+              accessibleDenot(qualType, name, sig, target)
         makeSelect(qual, name, denot)
 
       def readQualId(): (untpd.Ident, TypeRef) =
@@ -1335,15 +1342,26 @@ class TreeUnpickler(reader: TastyReader,
           readPathTree()
       }
 
-      /** Adapt constructor calls where class has only using clauses from old to new scheme.
+      /** Adapt constructor calls for Java annot constructors and for the new scheme of `using` clauses.
+       *
+       *  #19951 If the `fn` is the constructor of a Java annotation, reorder and refill
+       *  arguments against the constructor signature. Only reorder if all the arguments
+       *  are `NamedArg`s, which is always the case if the TASTy was produced by 3.5+.
+       *  If some arguments are positional, only *add* missing arguments to the right
+       *  and hope for the best; this will at least fix #19951 after the fact if the new
+       *  annotation fields are added after all the existing ones.
+       *
+       *  Otherwise, adapt calls where class has only using clauses from old to new scheme.
        *  or class has mixed using clauses and other clauses.
        *  Old: leading (), new: nothing, or trailing () if all clauses are using clauses.
        *  This is neccessary so that we can read pre-3.2 Tasty correctly. There,
        *  constructor calls use the old scheme, but constructor definitions already
        *  use the new scheme, since they are reconstituted with normalizeIfConstructor.
        */
       def constructorApply(fn: Tree, args: List[Tree]): Tree =
-        if fn.tpe.widen.isContextualMethod && args.isEmpty then
+        if fn.symbol.owner.is(JavaAnnotation) then
+          tpd.Apply(fn, fixArgsToJavaAnnotConstructor(fn.tpe.widen, args))
+        else if fn.tpe.widen.isContextualMethod && args.isEmpty then
           fn.withAttachment(SuppressedApplyToNone, ())
         else
           val fn1 = fn match
@@ -1365,6 +1383,68 @@ class TreeUnpickler(reader: TastyReader,
                 res.withAttachment(SuppressedApplyToNone, ())
               else res
 
+      def fixArgsToJavaAnnotConstructor(methType: Type, args: List[Tree]): List[Tree] =
+        methType match
+          case methType: MethodType =>
+            val formalNames = methType.paramNames
+            val sizeCmp = args.sizeCompare(formalNames)
+
+            def makeDefault(name: TermName, tpe: Type): NamedArg =
+              NamedArg(name, Underscore(tpe))
+
+            def extendOnly(args: List[NamedArg]): List[NamedArg] =
+              if sizeCmp < 0 then
+                val argsSize = args.size
+                val additionalArgs: List[NamedArg] =
+                  formalNames.drop(argsSize).lazyZip(methType.paramInfos.drop(argsSize)).map(makeDefault(_, _))
+                args ::: additionalArgs
+              else
+                args // fast path
+
+            if formalNames.isEmpty then
+              // fast path
+              args
+            else if sizeCmp > 0 then
+              // Something's wrong anyway; don't touch anything
+              args
+            else if args.exists(!_.isInstanceOf[NamedArg]) then
+              // Pre 3.5 TASTy -- do our best, assuming that args match as a prefix of the formals
+              val prefixMatch = args.lazyZip(formalNames).forall {
+                case (NamedArg(actualName, _), formalName) => actualName == formalName
+                case _                                     => true
+              }
+              // If the prefix does not match, something's wrong; don't touch anything
+              if !prefixMatch then
+                args
+              else
+                // Turn non-named args to named and extend with defaults
+                extendOnly(args.lazyZip(formalNames).map {
+                  case (arg: NamedArg, _) => arg
+                  case (arg, formalName)  => NamedArg(formalName, arg)
+                })
+            else
+              // Good TASTy where all the arguments are named; reorder and extend if needed
+              val namedArgs = args.asInstanceOf[List[NamedArg]]
+              val prefixMatch = namedArgs.lazyZip(formalNames).forall((arg, formalName) => arg.name == formalName)
+              if prefixMatch then
+                // fast path, extend only
+                extendOnly(namedArgs)
+              else
+                // needs reordering, and possibly fill in holes for default arguments
+                val argsByName = mutable.AnyRefMap.from(namedArgs.map(arg => arg.name -> arg))
+                val reconstructedArgs = formalNames.lazyZip(methType.paramInfos).map { (name, tpe) =>
+                  argsByName.remove(name).getOrElse(makeDefault(name, tpe))
+                }
+                if argsByName.nonEmpty then
+                  // something's wrong; don't touch anything
+                  args
+                else
+                  reconstructedArgs
+
+          case _ =>
+            args
+      end fixArgsToJavaAnnotConstructor
+
       def quotedExpr(fn: Tree, args: List[Tree]): Tree =
         val TypeApply(_, targs) = fn: @unchecked
         untpd.Quote(args.head, Nil).withBodyType(targs.head.tpe)
@@ -1491,8 +1571,12 @@ class TreeUnpickler(reader: TastyReader,
                   NoDenotation
 
               val denot =
-                val d = ownerTpe.decl(name).atSignature(sig, target)
-                (if !d.exists then lookupInSuper else d).asSeenFrom(prefix)
+                if owner.is(JavaAnnotation) && name == nme.CONSTRUCTOR then
+                  // #19951 Fix up to read TASTy produced before 3.5.0 -- ignore the signature
+                  ownerTpe.nonPrivateDecl(name).asSeenFrom(prefix)
+                else
+                  val d = ownerTpe.decl(name).atSignature(sig, target)
+                  (if !d.exists then lookupInSuper else d).asSeenFrom(prefix)
 
               makeSelect(qual, name, denot)
             case REPEATED =>

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -945,12 +945,17 @@ trait Applications extends Compatibility {
       val app1 =
         if (!success || typedArgs.exists(_.tpe.isError)) app0.withType(UnspecifiedErrorType)
         else {
-          if !sameSeq(args, orderedArgs)
-             && !isJavaAnnotConstr(methRef.symbol)
-             && !typedArgs.forall(isSafeArg)
-          then
+          if isJavaAnnotConstr(methRef.symbol) then
+            // #19951 Make sure all arguments are NamedArgs for Java annotations
+            if typedArgs.exists(!_.isInstanceOf[NamedArg]) then
+              typedArgs = typedArgs.lazyZip(methType.asInstanceOf[MethodType].paramNames).map {
+                case (arg: NamedArg, _) => arg
+                case (arg, name)        => NamedArg(name, arg)
+              }
+          else if !sameSeq(args, orderedArgs) && !typedArgs.forall(isSafeArg) then
             // need to lift arguments to maintain evaluation order in the
             // presence of argument reorderings.
+            // (never do this for Java annotation constructors, hence the 'else if')
 
             liftFun()
 

sbt-test/scala3-compat/java-annotations-3.4/app/Main.scala

@@ -0,0 +1,21 @@
+object Test:
+  def main(args: Array[String]): Unit =
+    val actual = listAnnots("ScalaUser")
+    val expected = List(
+      "new JavaAnnot(a = 5, b = _, c = _)",
+      "new JavaAnnot(a = 5, b = _, c = _)",
+      "new JavaAnnot(a = 5, b = \"foo\", c = _)",
+      "new JavaAnnot(a = 5, b = \"foo\", c = 3)",
+      "new JavaAnnot(a = 5, b = _, c = 3)",
+      "new JavaAnnot(a = 5, b = \"foo\", c = 3)",
+      "new JavaAnnot(a = 5, b = \"foo\", c = 3)",
+      "new JavaAnnot(a = 5, b = \"foo\", c = _)",
+    )
+    if actual != expected then
+      println("Expected:")
+      expected.foreach(println(_))
+      println("Actual:")
+      actual.foreach(println(_))
+      throw new AssertionError("test failed")
+  end main
+end Test

sbt-test/scala3-compat/java-annotations-3.4/build.sbt

@@ -0,0 +1,7 @@
+lazy val lib = project.in(file("lib"))
+  .settings(
+    scalaVersion := "3.4.2"
+  )
+
+lazy val app = project.in(file("app"))
+  .dependsOn(lib)

sbt-test/scala3-compat/java-annotations-3.4/lib/AnnotMacro.scala

@@ -0,0 +1,7 @@
+import scala.quoted.*
+
+inline def listAnnots(inline c: String): List[String] = ${ listAnnotsImpl('c) }
+
+def listAnnotsImpl(c: Expr[String])(using Quotes): Expr[List[String]] =
+  import quotes.reflect.*
+  Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))

sbt-test/scala3-compat/java-annotations-3.4/lib/JavaAnnot.java

@@ -0,0 +1,10 @@
+
+import java.lang.annotation.*;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.METHOD)
+@interface JavaAnnot {
+  int a();
+  String b() default "empty";
+  int c() default 5;
+}

sbt-test/scala3-compat/java-annotations-3.4/lib/ScalaUser.scala

@@ -0,0 +1,25 @@
+class ScalaUser {
+  @JavaAnnot(5)
+  def f1(): Int = 1
+
+  @JavaAnnot(a = 5)
+  def f2(): Int = 1
+
+  @JavaAnnot(5, "foo")
+  def f3(): Int = 1
+
+  @JavaAnnot(5, "foo", 3)
+  def f4(): Int = 1
+
+  @JavaAnnot(5, c = 3)
+  def f5(): Int = 1
+
+  @JavaAnnot(5, c = 3, b = "foo")
+  def f6(): Int = 1
+
+  @JavaAnnot(b = "foo", c = 3, a = 5)
+  def f7(): Int = 1
+
+  @JavaAnnot(b = "foo", a = 5)
+  def f8(): Int = 1
+}

sbt-test/scala3-compat/java-annotations-3.4/project/DottyInjectedPlugin.scala

@@ -0,0 +1,11 @@
+import sbt._
+import Keys._
+
+object DottyInjectedPlugin extends AutoPlugin {
+  override def requires = plugins.JvmPlugin
+  override def trigger = allRequirements
+
+  override val projectSettings = Seq(
+    scalaVersion := sys.props("plugin.scalaVersion")
+  )
+}

sbt-test/scala3-compat/java-annotations-3.4/test

@@ -0,0 +1 @@
+> app/run

tests/run-macros/annot-arg-value-in-java.check

@@ -3,9 +3,9 @@ new java.lang.SuppressWarnings(value = "a")
 new java.lang.SuppressWarnings(value = "b")
 new java.lang.SuppressWarnings(value = _root_.scala.Array.apply[java.lang.String]("c", "d")(scala.reflect.ClassTag.apply[java.lang.String](classOf[java.lang.String])))
 JOtherTypes:
-new Annot(value = 1, _, _)
-new Annot(value = -2, _, _)
-new Annot(_, m = false, _)
-new Annot(_, m = true, _)
-new Annot(_, _, n = 1.1)
-new Annot(_, _, n = -2.1)
+new Annot(value = 1, m = _, n = _)
+new Annot(value = -2, m = _, n = _)
+new Annot(value = _, m = false, n = _)
+new Annot(value = _, m = true, n = _)
+new Annot(value = _, m = _, n = 1.1)
+new Annot(value = _, m = _, n = -2.1)

tests/run-macros/annot-java-tree/AnnoMacro.scala

@@ -8,12 +8,17 @@ def checkSuppressWarningsImpl[T: Type](using Quotes): Expr[Unit] =
   val sym = TypeRepr.of[T].typeSymbol
   // Imitate what wartremover does, so we can avoid unintentionally breaking it:
   // https://github.com/wartremover/wartremover/blob/fb18e6eafe9a47823e04960aaf4ec7a9293719ef/core/src/main/scala-3/org/wartremover/WartUniverse.scala#L63-L77
+  // We're intentionally breaking it in 3.5.x, though, with the addition of `NamedArg("value", ...)`
+  // The previous implementation would be broken for cases where the user explicitly write `value = ...` anyway.
   val actualArgs = sym
     .getAnnotation(SuppressWarningsSymbol)
     .collect {
       case Apply(
             Select(_, "<init>"),
-            Apply(Apply(_, Typed(Repeated(values, _), _) :: Nil), Apply(_, _ :: Nil) :: Nil) :: Nil
+            NamedArg(
+              "value",
+              Apply(Apply(_, Typed(Repeated(values, _), _) :: Nil), Apply(_, _ :: Nil) :: Nil)
+            ) :: Nil
           ) =>
         // "-Yexplicit-nulls"
         // https://github.com/wartremover/wartremover/issues/660

tests/run-macros/i19951-java-annotations-tasty-compat-2/AnnotMacro_1.scala

@@ -0,0 +1,11 @@
+import scala.quoted.*
+
+inline def showAnnots(inline c: String): Unit = ${ showAnnotsImpl('c) }
+
+def showAnnotsImpl(c: Expr[String])(using Quotes): Expr[Unit] =
+  import quotes.reflect.*
+  val al = Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))
+  '{
+    println($c + ":")
+    $al.foreach(println)
+  }

tests/run-macros/i19951-java-annotations-tasty-compat-2/JavaAnnot_1.java

@@ -0,0 +1,10 @@
+
+import java.lang.annotation.*;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.METHOD)
+@interface JavaAnnot {
+  int a();
+  String b() default "empty";
+  int c() default 5;
+}

tests/run-macros/i19951-java-annotations-tasty-compat-2/JavaAnnot_2.java

@@ -0,0 +1,11 @@
+
+import java.lang.annotation.*;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.METHOD)
+@interface JavaAnnot {
+  int c() default 5;
+  int a();
+  int d() default 42;
+  String b() default "empty";
+}

tests/run-macros/i19951-java-annotations-tasty-compat-2/ScalaUser_1.scala

@@ -0,0 +1,25 @@
+class ScalaUser {
+  @JavaAnnot(5)
+  def f1(): Int = 1
+
+  @JavaAnnot(a = 5)
+  def f2(): Int = 1
+
+  @JavaAnnot(5, "foo")
+  def f3(): Int = 1
+
+  @JavaAnnot(5, "foo", 3)
+  def f4(): Int = 1
+
+  @JavaAnnot(5, c = 3)
+  def f5(): Int = 1
+
+  @JavaAnnot(5, c = 3, b = "foo")
+  def f6(): Int = 1
+
+  @JavaAnnot(b = "foo", c = 3, a = 5)
+  def f7(): Int = 1
+
+  @JavaAnnot(b = "foo", a = 5)
+  def f8(): Int = 1
+}

tests/run-macros/i19951-java-annotations-tasty-compat-2/Test_2.scala

@@ -0,0 +1,4 @@
+object Test {
+  def main(args: Array[String]): Unit =
+    showAnnots("ScalaUser")
+}

tests/run-macros/i19951-java-annotations-tasty-compat.check

@@ -0,0 +1,9 @@
+ScalaUser:
+new JavaAnnot(c = _, a = 5, d = _, b = _)
+new JavaAnnot(c = _, a = 5, d = _, b = _)
+new JavaAnnot(c = _, a = 5, d = _, b = "foo")
+new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
+new JavaAnnot(c = 3, a = 5, d = _, b = _)
+new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
+new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
+new JavaAnnot(c = _, a = 5, d = _, b = "foo")

tests/run-macros/i19951-java-annotations-tasty-compat/AnnotMacro_2.scala

@@ -0,0 +1,11 @@
+import scala.quoted.*
+
+inline def showAnnots(inline c: String): Unit = ${ showAnnotsImpl('c) }
+
+def showAnnotsImpl(c: Expr[String])(using Quotes): Expr[Unit] =
+  import quotes.reflect.*
+  val al = Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))
+  '{
+    println($c + ":")
+    $al.foreach(println)
+  }