From a36af49b467509f490f8dbb019655d192ba52318 Mon Sep 17 00:00:00 2001 From: Hamza REMMAL Date: Mon, 5 Aug 2024 14:36:46 +0200 Subject: [PATCH] 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 | 28 +++++++++++++ .../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, 169 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..26dd66debb11 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/compiler/src/dotty/tools/dotc/typer/Checking.scala @@ -883,6 +883,34 @@ 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) + + def hasOnePositionalParameter(params: List[untpd.Tree]): Boolean = + params.exists(!_.isInstanceOf[untpd.NamedArg]) + + def mapPositionalToNamed(param: untpd.Tree): untpd.Tree = param match + case tree: untpd.NamedArg => tree + case tree => untpd.cpy.NamedArg(param)(nme.value, param) + end mapPositionalToNamed + + annot match + case untpd.Apply(fun, params) if hasOnePositionalParameter(params) && annotationHasValueField => + untpd.cpy.Apply(annot)(fun, params.mapConserve(mapPositionalToNamed)) + case untpd.Apply(_, params) if hasOnePositionalParameter(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