Skip to content

Commit

Permalink
Fix #19951: Align TASTy with the Java annotation model.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sjrd committed Jun 10, 2024
1 parent bd95888 commit 67ad9a2
Show file tree
Hide file tree
Showing 15 changed files with 253 additions and 7 deletions.
14 changes: 12 additions & 2 deletions compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) =>
Expand Down
94 changes: 89 additions & 5 deletions compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 && qual.symbol.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) =
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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 =>
Expand Down
21 changes: 21 additions & 0 deletions sbt-test/scala3-compat/java-annotations-3.4/app/Main.scala
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions sbt-test/scala3-compat/java-annotations-3.4/build.sbt
Original file line number Diff line number Diff line change
@@ -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)
Original file line number Diff line number Diff line change
@@ -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)))
10 changes: 10 additions & 0 deletions sbt-test/scala3-compat/java-annotations-3.4/lib/JavaAnnot.java
Original file line number Diff line number Diff line change
@@ -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;
}
25 changes: 25 additions & 0 deletions sbt-test/scala3-compat/java-annotations-3.4/lib/ScalaUser.scala
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -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")
)
}
1 change: 1 addition & 0 deletions sbt-test/scala3-compat/java-annotations-3.4/test
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
> app/run
9 changes: 9 additions & 0 deletions tests/run-macros/i19951-java-annotations-tasty-compat.check
Original file line number Diff line number Diff line change
@@ -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")
Original file line number Diff line number Diff line change
@@ -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)
}
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
@@ -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";
}
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
object Test {
def main(args: Array[String]): Unit =
showAnnots("ScalaUser")
}

0 comments on commit 67ad9a2

Please sign in to comment.