Skip to content

Commit 06c6a71

Browse files
sjrdsjrd
authored
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.
2 parents fa58f93 + 6730f19 commit 06c6a71

File tree

23 files changed

+335
-18
lines changed

23 files changed

+335
-18
lines changed

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,10 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
466466
}
467467
case _ =>
468468
if passesConditionForErroringBestEffortCode(tree.hasType) then
469-
val sig = tree.tpe.signature
469+
// #19951 The signature of a constructor of a Java annotation is irrelevant
470+
val sig =
471+
if name == nme.CONSTRUCTOR && tree.symbol.exists && tree.symbol.owner.is(JavaAnnotation) then Signature.NotAMethod
472+
else tree.tpe.signature
470473
var ename = tree.symbol.targetName
471474
val selectFromQualifier =
472475
name.isTypeName
@@ -507,7 +510,14 @@ class TreePickler(pickler: TastyPickler, attributes: Attributes) {
507510
writeByte(APPLY)
508511
withLength {
509512
pickleTree(fun)
510-
args.foreach(pickleTree)
513+
// #19951 Do not pickle default arguments to Java annotation constructors
514+
if fun.symbol.isClassConstructor && fun.symbol.owner.is(JavaAnnotation) then
515+
for arg <- args do
516+
arg match
517+
case NamedArg(_, Ident(nme.WILDCARD)) => ()
518+
case _ => pickleTree(arg)
519+
else
520+
args.foreach(pickleTree)
511521
}
512522
}
513523
case TypeApply(fun, args) =>

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

Lines changed: 89 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1281,7 +1281,14 @@ class TreeUnpickler(reader: TastyReader,
12811281
if unpicklingJava && name == tpnme.Object && qual.symbol == defn.JavaLangPackageVal then
12821282
defn.FromJavaObjectSymbol.denot
12831283
else
1284-
accessibleDenot(qual.tpe.widenIfUnstable, name, sig, target)
1284+
val qualType = qual.tpe.widenIfUnstable
1285+
if name == nme.CONSTRUCTOR && qualType.classSymbol.is(JavaAnnotation) then
1286+
// #19951 Disregard the signature (or the absence thereof) for constructors of Java annotations
1287+
// Note that Java annotations always have a single public constructor
1288+
// They may have a PrivateLocal constructor if compiled from source in mixed compilation
1289+
qualType.findMember(name, qualType, excluded = Private)
1290+
else
1291+
accessibleDenot(qualType, name, sig, target)
12851292
makeSelect(qual, name, denot)
12861293

12871294
def readQualId(): (untpd.Ident, TypeRef) =
@@ -1335,15 +1342,26 @@ class TreeUnpickler(reader: TastyReader,
13351342
readPathTree()
13361343
}
13371344

1338-
/** Adapt constructor calls where class has only using clauses from old to new scheme.
1345+
/** Adapt constructor calls for Java annot constructors and for the new scheme of `using` clauses.
1346+
*
1347+
* #19951 If the `fn` is the constructor of a Java annotation, reorder and refill
1348+
* arguments against the constructor signature. Only reorder if all the arguments
1349+
* are `NamedArg`s, which is always the case if the TASTy was produced by 3.5+.
1350+
* If some arguments are positional, only *add* missing arguments to the right
1351+
* and hope for the best; this will at least fix #19951 after the fact if the new
1352+
* annotation fields are added after all the existing ones.
1353+
*
1354+
* Otherwise, adapt calls where class has only using clauses from old to new scheme.
13391355
* or class has mixed using clauses and other clauses.
13401356
* Old: leading (), new: nothing, or trailing () if all clauses are using clauses.
13411357
* This is neccessary so that we can read pre-3.2 Tasty correctly. There,
13421358
* constructor calls use the old scheme, but constructor definitions already
13431359
* use the new scheme, since they are reconstituted with normalizeIfConstructor.
13441360
*/
13451361
def constructorApply(fn: Tree, args: List[Tree]): Tree =
1346-
if fn.tpe.widen.isContextualMethod && args.isEmpty then
1362+
if fn.symbol.owner.is(JavaAnnotation) then
1363+
tpd.Apply(fn, fixArgsToJavaAnnotConstructor(fn.tpe.widen, args))
1364+
else if fn.tpe.widen.isContextualMethod && args.isEmpty then
13471365
fn.withAttachment(SuppressedApplyToNone, ())
13481366
else
13491367
val fn1 = fn match
@@ -1365,6 +1383,68 @@ class TreeUnpickler(reader: TastyReader,
13651383
res.withAttachment(SuppressedApplyToNone, ())
13661384
else res
13671385

1386+
def fixArgsToJavaAnnotConstructor(methType: Type, args: List[Tree]): List[Tree] =
1387+
methType match
1388+
case methType: MethodType =>
1389+
val formalNames = methType.paramNames
1390+
val sizeCmp = args.sizeCompare(formalNames)
1391+
1392+
def makeDefault(name: TermName, tpe: Type): NamedArg =
1393+
NamedArg(name, Underscore(tpe))
1394+
1395+
def extendOnly(args: List[NamedArg]): List[NamedArg] =
1396+
if sizeCmp < 0 then
1397+
val argsSize = args.size
1398+
val additionalArgs: List[NamedArg] =
1399+
formalNames.drop(argsSize).lazyZip(methType.paramInfos.drop(argsSize)).map(makeDefault(_, _))
1400+
args ::: additionalArgs
1401+
else
1402+
args // fast path
1403+
1404+
if formalNames.isEmpty then
1405+
// fast path
1406+
args
1407+
else if sizeCmp > 0 then
1408+
// Something's wrong anyway; don't touch anything
1409+
args
1410+
else if args.exists(!_.isInstanceOf[NamedArg]) then
1411+
// Pre 3.5 TASTy -- do our best, assuming that args match as a prefix of the formals
1412+
val prefixMatch = args.lazyZip(formalNames).forall {
1413+
case (NamedArg(actualName, _), formalName) => actualName == formalName
1414+
case _ => true
1415+
}
1416+
// If the prefix does not match, something's wrong; don't touch anything
1417+
if !prefixMatch then
1418+
args
1419+
else
1420+
// Turn non-named args to named and extend with defaults
1421+
extendOnly(args.lazyZip(formalNames).map {
1422+
case (arg: NamedArg, _) => arg
1423+
case (arg, formalName) => NamedArg(formalName, arg)
1424+
})
1425+
else
1426+
// Good TASTy where all the arguments are named; reorder and extend if needed
1427+
val namedArgs = args.asInstanceOf[List[NamedArg]]
1428+
val prefixMatch = namedArgs.lazyZip(formalNames).forall((arg, formalName) => arg.name == formalName)
1429+
if prefixMatch then
1430+
// fast path, extend only
1431+
extendOnly(namedArgs)
1432+
else
1433+
// needs reordering, and possibly fill in holes for default arguments
1434+
val argsByName = mutable.AnyRefMap.from(namedArgs.map(arg => arg.name -> arg))
1435+
val reconstructedArgs = formalNames.lazyZip(methType.paramInfos).map { (name, tpe) =>
1436+
argsByName.remove(name).getOrElse(makeDefault(name, tpe))
1437+
}
1438+
if argsByName.nonEmpty then
1439+
// something's wrong; don't touch anything
1440+
args
1441+
else
1442+
reconstructedArgs
1443+
1444+
case _ =>
1445+
args
1446+
end fixArgsToJavaAnnotConstructor
1447+
13681448
def quotedExpr(fn: Tree, args: List[Tree]): Tree =
13691449
val TypeApply(_, targs) = fn: @unchecked
13701450
untpd.Quote(args.head, Nil).withBodyType(targs.head.tpe)
@@ -1491,8 +1571,12 @@ class TreeUnpickler(reader: TastyReader,
14911571
NoDenotation
14921572

14931573
val denot =
1494-
val d = ownerTpe.decl(name).atSignature(sig, target)
1495-
(if !d.exists then lookupInSuper else d).asSeenFrom(prefix)
1574+
if owner.is(JavaAnnotation) && name == nme.CONSTRUCTOR then
1575+
// #19951 Fix up to read TASTy produced before 3.5.0 -- ignore the signature
1576+
ownerTpe.nonPrivateDecl(name).asSeenFrom(prefix)
1577+
else
1578+
val d = ownerTpe.decl(name).atSignature(sig, target)
1579+
(if !d.exists then lookupInSuper else d).asSeenFrom(prefix)
14961580

14971581
makeSelect(qual, name, denot)
14981582
case REPEATED =>

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -945,12 +945,17 @@ trait Applications extends Compatibility {
945945
val app1 =
946946
if (!success || typedArgs.exists(_.tpe.isError)) app0.withType(UnspecifiedErrorType)
947947
else {
948-
if !sameSeq(args, orderedArgs)
949-
&& !isJavaAnnotConstr(methRef.symbol)
950-
&& !typedArgs.forall(isSafeArg)
951-
then
948+
if isJavaAnnotConstr(methRef.symbol) then
949+
// #19951 Make sure all arguments are NamedArgs for Java annotations
950+
if typedArgs.exists(!_.isInstanceOf[NamedArg]) then
951+
typedArgs = typedArgs.lazyZip(methType.asInstanceOf[MethodType].paramNames).map {
952+
case (arg: NamedArg, _) => arg
953+
case (arg, name) => NamedArg(name, arg)
954+
}
955+
else if !sameSeq(args, orderedArgs) && !typedArgs.forall(isSafeArg) then
952956
// need to lift arguments to maintain evaluation order in the
953957
// presence of argument reorderings.
958+
// (never do this for Java annotation constructors, hence the 'else if')
954959

955960
liftFun()
956961

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
object Test:
2+
def main(args: Array[String]): Unit =
3+
val actual = listAnnots("ScalaUser")
4+
val expected = List(
5+
"new JavaAnnot(a = 5, b = _, c = _)",
6+
"new JavaAnnot(a = 5, b = _, c = _)",
7+
"new JavaAnnot(a = 5, b = \"foo\", c = _)",
8+
"new JavaAnnot(a = 5, b = \"foo\", c = 3)",
9+
"new JavaAnnot(a = 5, b = _, c = 3)",
10+
"new JavaAnnot(a = 5, b = \"foo\", c = 3)",
11+
"new JavaAnnot(a = 5, b = \"foo\", c = 3)",
12+
"new JavaAnnot(a = 5, b = \"foo\", c = _)",
13+
)
14+
if actual != expected then
15+
println("Expected:")
16+
expected.foreach(println(_))
17+
println("Actual:")
18+
actual.foreach(println(_))
19+
throw new AssertionError("test failed")
20+
end main
21+
end Test

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
lazy val lib = project.in(file("lib"))
2+
.settings(
3+
scalaVersion := "3.4.2"
4+
)
5+
6+
lazy val app = project.in(file("app"))
7+
.dependsOn(lib)

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
import scala.quoted.*
2+
3+
inline def listAnnots(inline c: String): List[String] = ${ listAnnotsImpl('c) }
4+
5+
def listAnnotsImpl(c: Expr[String])(using Quotes): Expr[List[String]] =
6+
import quotes.reflect.*
7+
Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
2+
import java.lang.annotation.*;
3+
4+
@Retention(RetentionPolicy.RUNTIME)
5+
@Target(ElementType.METHOD)
6+
@interface JavaAnnot {
7+
int a();
8+
String b() default "empty";
9+
int c() default 5;
10+
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
class ScalaUser {
2+
@JavaAnnot(5)
3+
def f1(): Int = 1
4+
5+
@JavaAnnot(a = 5)
6+
def f2(): Int = 1
7+
8+
@JavaAnnot(5, "foo")
9+
def f3(): Int = 1
10+
11+
@JavaAnnot(5, "foo", 3)
12+
def f4(): Int = 1
13+
14+
@JavaAnnot(5, c = 3)
15+
def f5(): Int = 1
16+
17+
@JavaAnnot(5, c = 3, b = "foo")
18+
def f6(): Int = 1
19+
20+
@JavaAnnot(b = "foo", c = 3, a = 5)
21+
def f7(): Int = 1
22+
23+
@JavaAnnot(b = "foo", a = 5)
24+
def f8(): Int = 1
25+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import sbt._
2+
import Keys._
3+
4+
object DottyInjectedPlugin extends AutoPlugin {
5+
override def requires = plugins.JvmPlugin
6+
override def trigger = allRequirements
7+
8+
override val projectSettings = Seq(
9+
scalaVersion := sys.props("plugin.scalaVersion")
10+
)
11+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
> app/run

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@ new java.lang.SuppressWarnings(value = "a")
33
new java.lang.SuppressWarnings(value = "b")
44
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])))
55
JOtherTypes:
6-
new Annot(value = 1, _, _)
7-
new Annot(value = -2, _, _)
8-
new Annot(_, m = false, _)
9-
new Annot(_, m = true, _)
10-
new Annot(_, _, n = 1.1)
11-
new Annot(_, _, n = -2.1)
6+
new Annot(value = 1, m = _, n = _)
7+
new Annot(value = -2, m = _, n = _)
8+
new Annot(value = _, m = false, n = _)
9+
new Annot(value = _, m = true, n = _)
10+
new Annot(value = _, m = _, n = 1.1)
11+
new Annot(value = _, m = _, n = -2.1)

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,17 @@ def checkSuppressWarningsImpl[T: Type](using Quotes): Expr[Unit] =
88
val sym = TypeRepr.of[T].typeSymbol
99
// Imitate what wartremover does, so we can avoid unintentionally breaking it:
1010
// https://github.com/wartremover/wartremover/blob/fb18e6eafe9a47823e04960aaf4ec7a9293719ef/core/src/main/scala-3/org/wartremover/WartUniverse.scala#L63-L77
11+
// We're intentionally breaking it in 3.5.x, though, with the addition of `NamedArg("value", ...)`
12+
// The previous implementation would be broken for cases where the user explicitly write `value = ...` anyway.
1113
val actualArgs = sym
1214
.getAnnotation(SuppressWarningsSymbol)
1315
.collect {
1416
case Apply(
1517
Select(_, "<init>"),
16-
Apply(Apply(_, Typed(Repeated(values, _), _) :: Nil), Apply(_, _ :: Nil) :: Nil) :: Nil
18+
NamedArg(
19+
"value",
20+
Apply(Apply(_, Typed(Repeated(values, _), _) :: Nil), Apply(_, _ :: Nil) :: Nil)
21+
) :: Nil
1722
) =>
1823
// "-Yexplicit-nulls"
1924
// https://github.com/wartremover/wartremover/issues/660

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import scala.quoted.*
2+
3+
inline def showAnnots(inline c: String): Unit = ${ showAnnotsImpl('c) }
4+
5+
def showAnnotsImpl(c: Expr[String])(using Quotes): Expr[Unit] =
6+
import quotes.reflect.*
7+
val al = Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))
8+
'{
9+
println($c + ":")
10+
$al.foreach(println)
11+
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
2+
import java.lang.annotation.*;
3+
4+
@Retention(RetentionPolicy.RUNTIME)
5+
@Target(ElementType.METHOD)
6+
@interface JavaAnnot {
7+
int a();
8+
String b() default "empty";
9+
int c() default 5;
10+
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
2+
import java.lang.annotation.*;
3+
4+
@Retention(RetentionPolicy.RUNTIME)
5+
@Target(ElementType.METHOD)
6+
@interface JavaAnnot {
7+
int c() default 5;
8+
int a();
9+
int d() default 42;
10+
String b() default "empty";
11+
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
class ScalaUser {
2+
@JavaAnnot(5)
3+
def f1(): Int = 1
4+
5+
@JavaAnnot(a = 5)
6+
def f2(): Int = 1
7+
8+
@JavaAnnot(5, "foo")
9+
def f3(): Int = 1
10+
11+
@JavaAnnot(5, "foo", 3)
12+
def f4(): Int = 1
13+
14+
@JavaAnnot(5, c = 3)
15+
def f5(): Int = 1
16+
17+
@JavaAnnot(5, c = 3, b = "foo")
18+
def f6(): Int = 1
19+
20+
@JavaAnnot(b = "foo", c = 3, a = 5)
21+
def f7(): Int = 1
22+
23+
@JavaAnnot(b = "foo", a = 5)
24+
def f8(): Int = 1
25+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
object Test {
2+
def main(args: Array[String]): Unit =
3+
showAnnots("ScalaUser")
4+
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
ScalaUser:
2+
new JavaAnnot(c = _, a = 5, d = _, b = _)
3+
new JavaAnnot(c = _, a = 5, d = _, b = _)
4+
new JavaAnnot(c = _, a = 5, d = _, b = "foo")
5+
new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
6+
new JavaAnnot(c = 3, a = 5, d = _, b = _)
7+
new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
8+
new JavaAnnot(c = 3, a = 5, d = _, b = "foo")
9+
new JavaAnnot(c = _, a = 5, d = _, b = "foo")

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import scala.quoted.*
2+
3+
inline def showAnnots(inline c: String): Unit = ${ showAnnotsImpl('c) }
4+
5+
def showAnnotsImpl(c: Expr[String])(using Quotes): Expr[Unit] =
6+
import quotes.reflect.*
7+
val al = Expr(Symbol.requiredClass(c.valueOrError).declaredMethods.flatMap(_.annotations.map(_.show)))
8+
'{
9+
println($c + ":")
10+
$al.foreach(println)
11+
}

0 commit comments

Comments
 (0)