From 4ab86dbbfad3c8a942f2bb626ddb393609cb46b5 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Tue, 30 Jul 2019 21:36:32 +0200 Subject: [PATCH 1/3] Simplify ClassfileParser#addAnnotationConstructor It turns out that Java annotations cannot have type parameters, so part of this method was dead code. --- .../dotc/core/classfile/ClassfileParser.scala | 88 +++++++++---------- 1 file changed, 42 insertions(+), 46 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 7a99fb341b9f..e32c8c01922f 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -166,7 +166,10 @@ class ClassfileParser( for (i <- 0 until in.nextChar) parseMember(method = false) for (i <- 0 until in.nextChar) parseMember(method = true) classInfo = parseAttributes(classRoot.symbol, classInfo) - if (isAnnotation) addAnnotationConstructor(classInfo) + if (isAnnotation) + // classInfo must be a TempClassInfoType and not a TempPolyType, + // because Java annotations cannot have type parameters. + addAnnotationConstructor(classInfo.asInstanceOf[TempClassInfoType]) classRoot.registerCompanion(moduleRoot.symbol) moduleRoot.registerCompanion(classRoot.symbol) @@ -639,7 +642,7 @@ class ClassfileParser( * Note that default getters have type Nothing. That's OK because we need * them only to signal that the corresponding parameter is optional. */ - def addAnnotationConstructor(classInfo: Type, tparams: List[TypeSymbol] = Nil)(implicit ctx: Context): Unit = { + def addAnnotationConstructor(classInfo: TempClassInfoType)(implicit ctx: Context): Unit = { def addDefaultGetter(attr: Symbol, n: Int) = ctx.newSymbol( owner = moduleRoot.symbol, @@ -647,53 +650,46 @@ class ClassfileParser( flags = attr.flags & Flags.AccessFlags, info = defn.NothingType).entered - classInfo match { - case classInfo @ TempPolyType(tparams, restpe) if tparams.isEmpty => - addAnnotationConstructor(restpe, tparams) - case classInfo: TempClassInfoType => - val attrs = classInfo.decls.toList.filter(_.isTerm) - val targs = tparams.map(_.typeRef) - val paramNames = attrs.map(_.name.asTermName) - val paramTypes = attrs.map(_.info.resultType) - - def addConstr(ptypes: List[Type]) = { - val mtype = MethodType(paramNames, ptypes, classRoot.typeRef.appliedTo(targs)) - val constrType = if (tparams.isEmpty) mtype else TempPolyType(tparams, mtype) - val constr = ctx.newSymbol( - owner = classRoot.symbol, - name = nme.CONSTRUCTOR, - flags = Flags.Synthetic | Flags.JavaDefined | Flags.Method, - info = constrType - ).entered - for ((attr, i) <- attrs.zipWithIndex) - if (attr.hasAnnotation(defn.AnnotationDefaultAnnot)) { - constr.setFlag(Flags.DefaultParameterized) - addDefaultGetter(attr, i) - } + val attrs = classInfo.decls.toList.filter(_.isTerm) + val paramNames = attrs.map(_.name.asTermName) + val paramTypes = attrs.map(_.info.resultType) + + def addConstr(ptypes: List[Type]) = { + val mtype = MethodType(paramNames, ptypes, classRoot.typeRef) + val constr = ctx.newSymbol( + owner = classRoot.symbol, + name = nme.CONSTRUCTOR, + flags = Flags.Synthetic | Flags.JavaDefined | Flags.Method, + info = mtype + ).entered + for ((attr, i) <- attrs.zipWithIndex) + if (attr.hasAnnotation(defn.AnnotationDefaultAnnot)) { + constr.setFlag(Flags.DefaultParameterized) + addDefaultGetter(attr, i) } + } - addConstr(paramTypes) - - // The code below added an extra constructor to annotations where the - // last parameter of the constructor is an Array[X] for some X, the - // array was replaced by a vararg argument. Unfortunately this breaks - // inference when doing: - // @Annot(Array()) - // The constructor is overloaded so the expected type of `Array()` is - // WildcardType, and the type parameter of the Array apply method gets - // instantiated to `Nothing` instead of `X`. - // I'm leaving this commented out in case we improve inference to make this work. - // Note that if this is reenabled then JavaParser will also need to be modified - // to add the extra constructor (this was not implemented before). - /* - if (paramTypes.nonEmpty) - paramTypes.last match { - case defn.ArrayOf(elemtp) => - addConstr(paramTypes.init :+ defn.RepeatedParamType.appliedTo(elemtp)) - case _ => - } - */ + addConstr(paramTypes) + + // The code below added an extra constructor to annotations where the + // last parameter of the constructor is an Array[X] for some X, the + // array was replaced by a vararg argument. Unfortunately this breaks + // inference when doing: + // @Annot(Array()) + // The constructor is overloaded so the expected type of `Array()` is + // WildcardType, and the type parameter of the Array apply method gets + // instantiated to `Nothing` instead of `X`. + // I'm leaving this commented out in case we improve inference to make this work. + // Note that if this is reenabled then JavaParser will also need to be modified + // to add the extra constructor (this was not implemented before). + /* + if (paramTypes.nonEmpty) + paramTypes.last match { + case defn.ArrayOf(elemtp) => + addConstr(paramTypes.init :+ defn.RepeatedParamType.appliedTo(elemtp)) + case _ => } + */ } /** Enter own inner classes in the right scope. It needs the scopes to be set up, From 85cd1cf3f2e549d02ca74845672a8e585b3fc727 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 31 Jul 2019 17:58:29 +0200 Subject: [PATCH 2/3] New scheme for handling defaults of Java annotations Previously, we made up default getters that were then recognized by the backend, this has at least two downsides: - Creating the default getters requires forcing the info of all the methods in the annotation class, this might lead to cycles. - This scheme leaks into Tasty, and therefore becomes part of the Tasty specification, this is inelegant at best. In the new scheme, no default getters are created, instead we special-case the typing of annotation constructors: when an argument for the annotation constructor is missing, we replace it with a typed wildcard Ident if the corresponding parameter is known to have a default value. The Tasty spec and the backend need to be aware of this usage of wildcard, but that's easier to specify and to implement than default getters. --- .../backend/jvm/DottyBackendInterface.scala | 17 ++++---- .../dotc/core/classfile/ClassfileParser.scala | 18 +------- .../tools/dotc/parsing/JavaParsers.scala | 16 ++----- .../dotty/tools/dotc/typer/Applications.scala | 42 ++++++++++++++----- 4 files changed, 44 insertions(+), 49 deletions(-) diff --git a/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala b/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala index 3414dea956bc..dea62949ed79 100644 --- a/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala +++ b/compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala @@ -255,16 +255,15 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma } case t: TypeApply if (t.fun.symbol == Predef_classOf) => av.visit(name, t.args.head.tpe.classSymbol.denot.info.toTypeKind(bcodeStore)(innerClasesStore).toASMType) + case Ident(nme.WILDCARD) => + // An underscore argument indicates that we want to use the default value for this parameter, so do not emit anything case t: tpd.RefTree => - if (t.symbol.denot.owner.isAllOf(Flags.JavaEnumTrait)) { - val edesc = innerClasesStore.typeDescriptor(t.tpe.asInstanceOf[bcodeStore.int.Type]) // the class descriptor of the enumeration class. - val evalue = t.symbol.name.mangledString // value the actual enumeration value. - av.visitEnum(name, edesc, evalue) - } else { - // println(i"not an enum: ${t.symbol} / ${t.symbol.denot.owner} / ${t.symbol.denot.owner.isTerm} / ${t.symbol.denot.owner.flagsString}") - assert(toDenot(t.symbol).name.is(DefaultGetterName), - s"${toDenot(t.symbol).name.debugString}") // this should be default getter. do not emit. - } + assert(t.symbol.denot.owner.isAllOf(Flags.JavaEnumTrait), + i"not an enum: $t / ${t.symbol} / ${t.symbol.denot.owner} / ${t.symbol.denot.owner.isTerm} / ${t.symbol.denot.owner.flagsString}") + + val edesc = innerClasesStore.typeDescriptor(t.tpe.asInstanceOf[bcodeStore.int.Type]) // the class descriptor of the enumeration class. + val evalue = t.symbol.name.mangledString // value the actual enumeration value. + av.visitEnum(name, edesc, evalue) case t: SeqLiteral => val arrAnnotV: AnnotationVisitor = av.visitArray(name) for (arg <- t.elems) { emitArgument(arrAnnotV, null, arg, bcodeStore)(innerClasesStore) } diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index e32c8c01922f..690314d761c5 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -635,21 +635,10 @@ class ClassfileParser( cook.apply(newType) } - /** Add synthetic constructor(s) and potentially also default getters which - * reflects the fields of the annotation with given `classInfo`. - * Annotations in Scala are assumed to get all their arguments as constructor + /** 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. - * Note that default getters have type Nothing. That's OK because we need - * them only to signal that the corresponding parameter is optional. */ def addAnnotationConstructor(classInfo: TempClassInfoType)(implicit ctx: Context): Unit = { - def addDefaultGetter(attr: Symbol, n: Int) = - ctx.newSymbol( - owner = moduleRoot.symbol, - name = DefaultGetterName(nme.CONSTRUCTOR, n), - flags = attr.flags & Flags.AccessFlags, - info = defn.NothingType).entered - val attrs = classInfo.decls.toList.filter(_.isTerm) val paramNames = attrs.map(_.name.asTermName) val paramTypes = attrs.map(_.info.resultType) @@ -662,11 +651,6 @@ class ClassfileParser( flags = Flags.Synthetic | Flags.JavaDefined | Flags.Method, info = mtype ).entered - for ((attr, i) <- attrs.zipWithIndex) - if (attr.hasAnnotation(defn.AnnotationDefaultAnnot)) { - constr.setFlag(Flags.DefaultParameterized) - addDefaultGetter(attr, i) - } } addConstr(paramTypes) diff --git a/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala b/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala index 170d236a5e16..71599dffc254 100644 --- a/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala +++ b/compiler/src/dotty/tools/dotc/parsing/JavaParsers.scala @@ -127,8 +127,8 @@ object JavaParsers { def makeSyntheticParam(count: Int, tpt: Tree): ValDef = makeParam(nme.syntheticParamName(count), tpt) - def makeParam(name: TermName, tpt: Tree, defaultValue: Tree = EmptyTree): ValDef = - ValDef(name, tpt, defaultValue).withMods(Modifiers(Flags.JavaDefined | Flags.Param)) + def makeParam(name: TermName, tpt: Tree): ValDef = + ValDef(name, tpt, EmptyTree).withMods(Modifiers(Flags.JavaDefined | Flags.Param)) def makeConstructor(formals: List[Tree], tparams: List[TypeDef], flags: FlagSet = Flags.JavaDefined): DefDef = { val vparams = formals.zipWithIndex.map { case (p, i) => makeSyntheticParam(i + 1, p) } @@ -768,17 +768,7 @@ object JavaParsers { val (statics, body) = typeBody(AT, name, List()) val constructorParams = body.collect { case dd: DefDef => - val hasDefault = - dd.mods.annotations.exists { - case Apply(Select(New(Select(_, tpnme.AnnotationDefaultATTR)), nme.CONSTRUCTOR), Nil) => - true - case _ => - false - } - // If the annotation has a default value we don't need to parse it, providing - // any value at all is enough to typecheck usages of annotations correctly. - val defaultParam = if (hasDefault) unimplementedExpr else EmptyTree - makeParam(dd.name, dd.tpt, defaultParam) + makeParam(dd.name, dd.tpt) } val constr = DefDef(nme.CONSTRUCTOR, List(), List(constructorParams), TypeTree(), EmptyTree).withMods(Modifiers(Flags.JavaDefined)) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index b00dfabf08d4..8f7929328eff 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -543,17 +543,39 @@ trait Applications extends Compatibility { self: Typer with Dynamic => } def tryDefault(n: Int, args1: List[Arg]): Unit = { - val getter = - // `methRef.symbol` doesn't exist for structural calls - if (methRef.symbol.exists) findDefaultGetter(n + numArgs(normalizedFun)) - else EmptyTree - if (getter.isEmpty) missingArg(n) - else { - val substParam = addTyped( - treeToArg(spliceMeth(getter.withSpan(normalizedFun.span), normalizedFun)), - formal) + val sym = methRef.symbol + + val defaultExpr = + if (isJavaAnnotConstr(sym)) { + val cinfo = sym.owner.asClass.classInfo + val pname = methodType.paramNames(n) + val hasDefault = cinfo.member(pname) + .suchThat(d => d.is(Method) && d.hasAnnotation(defn.AnnotationDefaultAnnot)).exists + + // Use `_` as a placeholder for the default value of an + // annotation parameter, this will be recognized by the backend. + if (hasDefault) + tpd.Underscore(formal) + else + EmptyTree + } else { + val getter = + if (sym.exists) // `sym` doesn't exist for structural calls + findDefaultGetter(n + numArgs(normalizedFun)) + else + EmptyTree + + if (!getter.isEmpty) + spliceMeth(getter.withSpan(normalizedFun.span), normalizedFun) + else + EmptyTree + } + + if (!defaultExpr.isEmpty) { + val substParam = addTyped(treeToArg(defaultExpr), formal) matchArgs(args1, formals1.mapconserve(substParam), n + 1) - } + } else + missingArg(n) } if (formal.isRepeatedParam) From 0ea949a8ace5114610e10f51b53db66e9a3d1630 Mon Sep 17 00:00:00 2001 From: Guillaume Martres Date: Wed, 31 Jul 2019 18:11:11 +0200 Subject: [PATCH 3/3] Fix #6868: Avoid cycles involving Java annotations Compute the info of synthesized Java annotation constructors lazily, just like the info of all the other members created by ClassfileParser. Thanks to noti0na1 for the initial investigation and test case! --- .../dotc/core/classfile/ClassfileParser.scala | 49 ++++++------------- .../i6868/MyJava_1.java | 15 ++++++ .../i6868/MyScala_2.scala | 13 +++++ 3 files changed, 42 insertions(+), 35 deletions(-) create mode 100644 tests/pos-java-interop-separate/i6868/MyJava_1.java create mode 100644 tests/pos-java-interop-separate/i6868/MyScala_2.scala diff --git a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala index 690314d761c5..1d87723cd258 100644 --- a/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala +++ b/compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala @@ -638,42 +638,21 @@ class ClassfileParser( /** 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)(implicit ctx: Context): Unit = { - val attrs = classInfo.decls.toList.filter(_.isTerm) - val paramNames = attrs.map(_.name.asTermName) - val paramTypes = attrs.map(_.info.resultType) - - def addConstr(ptypes: List[Type]) = { - val mtype = MethodType(paramNames, ptypes, classRoot.typeRef) - val constr = ctx.newSymbol( - owner = classRoot.symbol, - name = nme.CONSTRUCTOR, - flags = Flags.Synthetic | Flags.JavaDefined | Flags.Method, - info = mtype - ).entered - } - - addConstr(paramTypes) - - // The code below added an extra constructor to annotations where the - // last parameter of the constructor is an Array[X] for some X, the - // array was replaced by a vararg argument. Unfortunately this breaks - // inference when doing: - // @Annot(Array()) - // The constructor is overloaded so the expected type of `Array()` is - // WildcardType, and the type parameter of the Array apply method gets - // instantiated to `Nothing` instead of `X`. - // I'm leaving this commented out in case we improve inference to make this work. - // Note that if this is reenabled then JavaParser will also need to be modified - // to add the extra constructor (this was not implemented before). - /* - if (paramTypes.nonEmpty) - paramTypes.last match { - case defn.ArrayOf(elemtp) => - addConstr(paramTypes.init :+ defn.RepeatedParamType.appliedTo(elemtp)) - case _ => + def addAnnotationConstructor(classInfo: TempClassInfoType)(implicit ctx: Context): Unit = + ctx.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)(implicit ctx: Context): Unit = { + val attrs = classInfo.decls.toList.filter(sym => sym.isTerm && sym != denot.symbol) + val paramNames = attrs.map(_.name.asTermName) + val paramTypes = attrs.map(_.info.resultType) + denot.info = MethodType(paramNames, paramTypes, classRoot.typeRef) } - */ } /** Enter own inner classes in the right scope. It needs the scopes to be set up, diff --git a/tests/pos-java-interop-separate/i6868/MyJava_1.java b/tests/pos-java-interop-separate/i6868/MyJava_1.java new file mode 100644 index 000000000000..916297cbc569 --- /dev/null +++ b/tests/pos-java-interop-separate/i6868/MyJava_1.java @@ -0,0 +1,15 @@ +public @interface MyJava_1 { + + public String value() default "MyJava"; + + public MyClassTypeA typeA(); + + public MyClassTypeB typeB() default @MyClassTypeB; + + public enum MyClassTypeA { + A, B + } + + public @interface MyClassTypeB {} +} + diff --git a/tests/pos-java-interop-separate/i6868/MyScala_2.scala b/tests/pos-java-interop-separate/i6868/MyScala_2.scala new file mode 100644 index 000000000000..e0fd84008f39 --- /dev/null +++ b/tests/pos-java-interop-separate/i6868/MyScala_2.scala @@ -0,0 +1,13 @@ +@MyJava_1("MyScala1", typeA = MyJava_1.MyClassTypeA.B) +object MyScala { + def a(mj: MyJava_1): Unit = { + println("MyJava") + } + + @MyJava_1(typeA = MyJava_1.MyClassTypeA.A) + def b(): Int = 1 + + @MyJava_1(value = "MyScala2", typeA = MyJava_1.MyClassTypeA.B) + def c(): Int = 2 +} +