From 67b393048e961fce3ddc092e1740e86426f554c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Doeraene?= Date: Mon, 10 Jun 2024 10:19:02 +0200 Subject: [PATCH] Fix #19951: Align TASTy with the Java annotation model. 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. --- .../tools/dotc/core/tasty/TreePickler.scala | 14 ++- .../tools/dotc/core/tasty/TreeUnpickler.scala | 93 ++++++++++++++++++- 2 files changed, 100 insertions(+), 7 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala index 8d1eca8fb5f0..02e0e27ab602 100644 --- a/compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala +++ b/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.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) => diff --git a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala index 91a5899146cc..124f03aef9c0 100644 --- a/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala +++ b/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala @@ -1281,7 +1281,13 @@ 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 + qualType.findMember(name, qualType) + else + accessibleDenot(qualType, name, sig, target) makeSelect(qual, name, denot) def readQualId(): (untpd.Ident, TypeRef) = @@ -1335,7 +1341,16 @@ 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, @@ -1343,7 +1358,9 @@ class TreeUnpickler(reader: TastyReader, * 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 +1382,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 +1570,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.decl(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 =>