From c0b6b0f9572ea35d52961881da53a92bd57206c7 Mon Sep 17 00:00:00 2001 From: Hamza REMMAL Date: Mon, 5 Aug 2024 14:36:46 +0200 Subject: [PATCH 1/2] Require named arguments for java defined annotations --- .../tools/dotc/reporting/ErrorMessageID.scala | 1 + .../dotty/tools/dotc/reporting/messages.scala | 22 ++++++++++ .../src/dotty/tools/dotc/typer/Checking.scala | 20 +++++++++ .../src/dotty/tools/dotc/typer/Namer.scala | 23 ++++++---- tests/neg/i20554-a.check | 42 +++++++++++++++++++ tests/neg/i20554-a/Annotation.java | 4 ++ tests/neg/i20554-a/Test.scala | 4 ++ tests/neg/i20554-b.check | 21 ++++++++++ tests/neg/i20554-b/SimpleAnnotation.java | 4 ++ tests/neg/i20554-b/Test.scala | 4 ++ tests/pos/i20554-a/Annotation.java | 5 +++ tests/pos/i20554-a/Test.scala | 3 ++ tests/pos/i20554-b/SimpleAnnotation.java | 9 ++++ tests/pos/i20554-b/Test.scala | 3 ++ tests/pos/i20554-c.scala | 5 +++ 15 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 tests/neg/i20554-a.check create mode 100644 tests/neg/i20554-a/Annotation.java create mode 100644 tests/neg/i20554-a/Test.scala create mode 100644 tests/neg/i20554-b.check create mode 100644 tests/neg/i20554-b/SimpleAnnotation.java create mode 100644 tests/neg/i20554-b/Test.scala create mode 100644 tests/pos/i20554-a/Annotation.java create mode 100644 tests/pos/i20554-a/Test.scala create mode 100644 tests/pos/i20554-b/SimpleAnnotation.java create mode 100644 tests/pos/i20554-b/Test.scala create mode 100644 tests/pos/i20554-c.scala diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index e3613e3f783a..cb5e8a7b314c 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala @@ -214,6 +214,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe case UnusedSymbolID // errorNumber: 198 case TailrecNestedCallID //errorNumber: 199 case FinalLocalDefID // errorNumber: 200 + case NonNamedArgumentInJavaAnnotationID // errorNumber: 201 def errorNumber = ordinal - 1 diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 1d906130d4e4..f112b6fb5aa7 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -3288,3 +3288,25 @@ object UnusedSymbol { def privateMembers(using Context): UnusedSymbol = new UnusedSymbol(i"unused private member") def patVars(using Context): UnusedSymbol = new UnusedSymbol(i"unused pattern variable") } + +class NonNamedArgumentInJavaAnnotation(using Context) extends SyntaxMsg(NonNamedArgumentInJavaAnnotationID): + + override protected def msg(using Context): String = + "Named arguments are required for Java defined annotations" + + override protected def explain(using Context): String = + i"""Starting from Scala 3.6.0, named arguments are required for Java defined annotations. + |Java defined annotations don't have an exact constructor representation + |and we previously relied on the order of the fields to create one. + |One possible issue with this representation is the reordering of the fields. + |Lets take the following example: + | + | public @interface Annotation { + | int a() default 41; + | int b() default 42; + | } + | + |Reordering the fields is binary-compatible but it might affect the meaning of @Annotation(1) + """ + +end NonNamedArgumentInJavaAnnotation diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 421f00e61584..aeda38cc7646 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -883,6 +883,26 @@ object Checking { templ.parents.find(_.tpe.derivesFrom(defn.PolyFunctionClass)) match case Some(parent) => report.error(s"`PolyFunction` marker trait is reserved for compiler generated refinements", parent.srcPos) case None => + + /** check that parameters of a java defined annotations are all named arguments if we have more than one parameter */ + def checkNamedArgumentForJavaAnnotation(annot: untpd.Tree, sym: ClassSymbol)(using Context): untpd.Tree = + assert(sym.is(JavaDefined)) + + def annotationHasValueField: Boolean = + sym.info.decls.exists(_.name == nme.value) + + annot match + case untpd.Apply(fun, List(param)) if !param.isInstanceOf[untpd.NamedArg] && annotationHasValueField => + untpd.cpy.Apply(annot)(fun, List(untpd.cpy.NamedArg(param)(nme.value, param))) + case untpd.Apply(_, params) => + for + param <- params + if !param.isInstanceOf[untpd.NamedArg] + do report.error(NonNamedArgumentInJavaAnnotation(), param) + annot + case _ => annot + end checkNamedArgumentForJavaAnnotation + } trait Checking { diff --git a/compiler/src/dotty/tools/dotc/typer/Namer.scala b/compiler/src/dotty/tools/dotc/typer/Namer.scala index 2089e3f14be7..3844380f8952 100644 --- a/compiler/src/dotty/tools/dotc/typer/Namer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Namer.scala @@ -868,15 +868,20 @@ class Namer { typer: Typer => protected def addAnnotations(sym: Symbol): Unit = original match { case original: untpd.MemberDef => lazy val annotCtx = annotContext(original, sym) - for (annotTree <- original.mods.annotations) { - val cls = typedAheadAnnotationClass(annotTree)(using annotCtx) - if (cls eq sym) - report.error(em"An annotation class cannot be annotated with iself", annotTree.srcPos) - else { - val ann = Annotation.deferred(cls)(typedAheadExpr(annotTree)(using annotCtx)) - sym.addAnnotation(ann) - } - } + original.setMods: + original.mods.withAnnotations : + original.mods.annotations.mapConserve: annotTree => + val cls = typedAheadAnnotationClass(annotTree)(using annotCtx) + if (cls eq sym) + report.error(em"An annotation class cannot be annotated with iself", annotTree.srcPos) + annotTree + else + val ann = + if cls.is(JavaDefined) then Checking.checkNamedArgumentForJavaAnnotation(annotTree, cls.asClass) + else annotTree + val ann1 = Annotation.deferred(cls)(typedAheadExpr(ann)(using annotCtx)) + sym.addAnnotation(ann1) + ann case _ => } diff --git a/tests/neg/i20554-a.check b/tests/neg/i20554-a.check new file mode 100644 index 000000000000..5cfa4e3faaad --- /dev/null +++ b/tests/neg/i20554-a.check @@ -0,0 +1,42 @@ +-- [E201] Syntax Error: tests/neg/i20554-a/Test.scala:3:12 ------------------------------------------------------------- +3 |@Annotation(3, 4) // error // error : Java defined annotation should be called with named arguments + | ^ + | Named arguments are required for Java defined annotations + |--------------------------------------------------------------------------------------------------------------------- + | Explanation (enabled by `-explain`) + |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + | Starting from Scala 3.6.0, named arguments are required for Java defined annotations. + | Java defined annotations don't have an exact constructor representation + | and we previously relied on the order of the fields to create one. + | One possible issue with this representation is the reordering of the fields. + | Lets take the following example: + | + | public @interface Annotation { + | int a() default 41; + | int b() default 42; + | } + | + | Reordering the fields is binary-compatible but it might affect the meaning of @Annotation(1) + | + --------------------------------------------------------------------------------------------------------------------- +-- [E201] Syntax Error: tests/neg/i20554-a/Test.scala:3:15 ------------------------------------------------------------- +3 |@Annotation(3, 4) // error // error : Java defined annotation should be called with named arguments + | ^ + | Named arguments are required for Java defined annotations + |--------------------------------------------------------------------------------------------------------------------- + | Explanation (enabled by `-explain`) + |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + | Starting from Scala 3.6.0, named arguments are required for Java defined annotations. + | Java defined annotations don't have an exact constructor representation + | and we previously relied on the order of the fields to create one. + | One possible issue with this representation is the reordering of the fields. + | Lets take the following example: + | + | public @interface Annotation { + | int a() default 41; + | int b() default 42; + | } + | + | Reordering the fields is binary-compatible but it might affect the meaning of @Annotation(1) + | + --------------------------------------------------------------------------------------------------------------------- diff --git a/tests/neg/i20554-a/Annotation.java b/tests/neg/i20554-a/Annotation.java new file mode 100644 index 000000000000..728bbded7a06 --- /dev/null +++ b/tests/neg/i20554-a/Annotation.java @@ -0,0 +1,4 @@ +public @interface Annotation { + int a() default 41; + int b() default 42; +} diff --git a/tests/neg/i20554-a/Test.scala b/tests/neg/i20554-a/Test.scala new file mode 100644 index 000000000000..f0b3ea40b87a --- /dev/null +++ b/tests/neg/i20554-a/Test.scala @@ -0,0 +1,4 @@ +//> using options -explain + +@Annotation(3, 4) // error // error : Java defined annotation should be called with named arguments +class Test \ No newline at end of file diff --git a/tests/neg/i20554-b.check b/tests/neg/i20554-b.check new file mode 100644 index 000000000000..2395554a7485 --- /dev/null +++ b/tests/neg/i20554-b.check @@ -0,0 +1,21 @@ +-- [E201] Syntax Error: tests/neg/i20554-b/Test.scala:3:18 ------------------------------------------------------------- +3 |@SimpleAnnotation(1) // error: the parameters is not named 'value' + | ^ + | Named arguments are required for Java defined annotations + |--------------------------------------------------------------------------------------------------------------------- + | Explanation (enabled by `-explain`) + |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + | Starting from Scala 3.6.0, named arguments are required for Java defined annotations. + | Java defined annotations don't have an exact constructor representation + | and we previously relied on the order of the fields to create one. + | One possible issue with this representation is the reordering of the fields. + | Lets take the following example: + | + | public @interface Annotation { + | int a() default 41; + | int b() default 42; + | } + | + | Reordering the fields is binary-compatible but it might affect the meaning of @Annotation(1) + | + --------------------------------------------------------------------------------------------------------------------- diff --git a/tests/neg/i20554-b/SimpleAnnotation.java b/tests/neg/i20554-b/SimpleAnnotation.java new file mode 100644 index 000000000000..65b37a7508d2 --- /dev/null +++ b/tests/neg/i20554-b/SimpleAnnotation.java @@ -0,0 +1,4 @@ + +public @interface SimpleAnnotation { + int a() default 1; +} diff --git a/tests/neg/i20554-b/Test.scala b/tests/neg/i20554-b/Test.scala new file mode 100644 index 000000000000..c6586409aa62 --- /dev/null +++ b/tests/neg/i20554-b/Test.scala @@ -0,0 +1,4 @@ +//> using options -explain + +@SimpleAnnotation(1) // error: the parameters is not named 'value' +class Test \ No newline at end of file diff --git a/tests/pos/i20554-a/Annotation.java b/tests/pos/i20554-a/Annotation.java new file mode 100644 index 000000000000..3f8389517eae --- /dev/null +++ b/tests/pos/i20554-a/Annotation.java @@ -0,0 +1,5 @@ +public @interface Annotation { + int a() default 41; + int b() default 42; + int c() default 43; +} diff --git a/tests/pos/i20554-a/Test.scala b/tests/pos/i20554-a/Test.scala new file mode 100644 index 000000000000..4747f3a06783 --- /dev/null +++ b/tests/pos/i20554-a/Test.scala @@ -0,0 +1,3 @@ + +@Annotation(a = 1, b = 2) +class Test \ No newline at end of file diff --git a/tests/pos/i20554-b/SimpleAnnotation.java b/tests/pos/i20554-b/SimpleAnnotation.java new file mode 100644 index 000000000000..24fc988b6050 --- /dev/null +++ b/tests/pos/i20554-b/SimpleAnnotation.java @@ -0,0 +1,9 @@ + +public @interface SimpleAnnotation { + + int a() default 0; + + int value() default 1; + + int b() default 0; +} diff --git a/tests/pos/i20554-b/Test.scala b/tests/pos/i20554-b/Test.scala new file mode 100644 index 000000000000..c4a442f75fdb --- /dev/null +++ b/tests/pos/i20554-b/Test.scala @@ -0,0 +1,3 @@ + +@SimpleAnnotation(1) // works because of the presence of a field called value +class Test \ No newline at end of file diff --git a/tests/pos/i20554-c.scala b/tests/pos/i20554-c.scala new file mode 100644 index 000000000000..b8a43584a00a --- /dev/null +++ b/tests/pos/i20554-c.scala @@ -0,0 +1,5 @@ + +class MyAnnotation(a: Int, b: Int) extends annotation.StaticAnnotation + +@MyAnnotation(1, 2) // don't require named arguments as it is Scala Defined +class Test \ No newline at end of file From 6ccabeaf14982c6ad3a12b3f34d058f1cffd2418 Mon Sep 17 00:00:00 2001 From: Hamza REMMAL Date: Tue, 6 Aug 2024 13:30:25 +0200 Subject: [PATCH 2/2] Fix tests to new scheme --- tests/pos-java-interop-separate/i6868/MyScala_2.scala | 2 +- tests/pos/i6151/Test.scala | 2 +- .../ScalaUser_1.scala | 11 +++-------- .../i19951-java-annotations-tasty-compat.check | 1 - .../ScalaUser_2.scala | 10 ++++------ 5 files changed, 9 insertions(+), 17 deletions(-) diff --git a/tests/pos-java-interop-separate/i6868/MyScala_2.scala b/tests/pos-java-interop-separate/i6868/MyScala_2.scala index e0fd84008f39..607eefafa6a3 100644 --- a/tests/pos-java-interop-separate/i6868/MyScala_2.scala +++ b/tests/pos-java-interop-separate/i6868/MyScala_2.scala @@ -1,4 +1,4 @@ -@MyJava_1("MyScala1", typeA = MyJava_1.MyClassTypeA.B) +@MyJava_1(value = "MyScala1", typeA = MyJava_1.MyClassTypeA.B) object MyScala { def a(mj: MyJava_1): Unit = { println("MyJava") diff --git a/tests/pos/i6151/Test.scala b/tests/pos/i6151/Test.scala index 314cf5a0ea8f..118e6a72c354 100644 --- a/tests/pos/i6151/Test.scala +++ b/tests/pos/i6151/Test.scala @@ -1,3 +1,3 @@ import Expect.* -@Outcome(ExpectVal) +@Outcome(enm = ExpectVal) class SimpleTest diff --git a/tests/run-macros/i19951-java-annotations-tasty-compat-2/ScalaUser_1.scala b/tests/run-macros/i19951-java-annotations-tasty-compat-2/ScalaUser_1.scala index a14a69eae21b..7601dcf43ed2 100644 --- a/tests/run-macros/i19951-java-annotations-tasty-compat-2/ScalaUser_1.scala +++ b/tests/run-macros/i19951-java-annotations-tasty-compat-2/ScalaUser_1.scala @@ -1,20 +1,15 @@ class ScalaUser { - @JavaAnnot(5) - def f1(): Int = 1 @JavaAnnot(a = 5) def f2(): Int = 1 - @JavaAnnot(5, "foo") + @JavaAnnot(a = 5, b = "foo") def f3(): Int = 1 - @JavaAnnot(5, "foo", 3) - def f4(): Int = 1 - - @JavaAnnot(5, c = 3) + @JavaAnnot(a = 5, c = 3) def f5(): Int = 1 - @JavaAnnot(5, c = 3, b = "foo") + @JavaAnnot(a = 5, c = 3, b = "foo") def f6(): Int = 1 @JavaAnnot(b = "foo", c = 3, a = 5) diff --git a/tests/run-macros/i19951-java-annotations-tasty-compat.check b/tests/run-macros/i19951-java-annotations-tasty-compat.check index c41fcc64c559..60cf38794296 100644 --- a/tests/run-macros/i19951-java-annotations-tasty-compat.check +++ b/tests/run-macros/i19951-java-annotations-tasty-compat.check @@ -1,6 +1,5 @@ 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 = _) diff --git a/tests/run-macros/i19951-java-annotations-tasty-compat/ScalaUser_2.scala b/tests/run-macros/i19951-java-annotations-tasty-compat/ScalaUser_2.scala index a14a69eae21b..421192636dbc 100644 --- a/tests/run-macros/i19951-java-annotations-tasty-compat/ScalaUser_2.scala +++ b/tests/run-macros/i19951-java-annotations-tasty-compat/ScalaUser_2.scala @@ -1,20 +1,18 @@ class ScalaUser { - @JavaAnnot(5) - def f1(): Int = 1 @JavaAnnot(a = 5) def f2(): Int = 1 - @JavaAnnot(5, "foo") + @JavaAnnot(a = 5, b = "foo") def f3(): Int = 1 - @JavaAnnot(5, "foo", 3) + @JavaAnnot(a = 5, b = "foo", c = 3) def f4(): Int = 1 - @JavaAnnot(5, c = 3) + @JavaAnnot(a = 5, c = 3) def f5(): Int = 1 - @JavaAnnot(5, c = 3, b = "foo") + @JavaAnnot(a = 5, c = 3, b = "foo") def f6(): Int = 1 @JavaAnnot(b = "foo", c = 3, a = 5)