Don't introduce @NotNull on Kotlin methods that already have that annotation

smowton · smowton · commit f5dc5155f932 · 2022-12-09T10:33:05.000Z
This usually can't happen, but delegates pointing at Java appear to be synthesised with this normally-hidden annotation
diff --git a/java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt b/java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt
@@ -1366,14 +1366,19 @@ open class KotlinFileExtractor(
         if (t !is IrSimpleType)
             return null
 
+        fun hasExistingAnnotation(name: FqName) =
+            existingAnnotations.any { existing -> existing.type.classFqName == name }
+
         return if (declOrigin == IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB) {
             // Java declaration: restore a NotNull or Nullable annotation if the original Java member had one but the Kotlin compiler removed it.
             javaAnnotations?.mapNotNull { it.classId?.asSingleFqName() }
             ?.singleOrNull { NOT_NULL_ANNOTATIONS.contains(it) || NULLABLE_ANNOTATIONS.contains(it) }
-            ?.takeUnless { existingAnnotations.any { existing -> existing.type.classFqName == it } }
+            ?.takeUnless { hasExistingAnnotation(it) }
         } else {
-            // Kotlin declaration: add a NotNull annotation to a non-nullable non-primitive type.
-            JvmAnnotationNames.JETBRAINS_NOT_NULL_ANNOTATION.takeUnless { t.isNullable() || primitiveTypeMapping.getPrimitiveInfo(t) != null }
+            // Kotlin declaration: add a NotNull annotation to a non-nullable non-primitive type, unless one is already present.
+            // Usually Kotlin declarations can't have a manual `@NotNull`, but this happens at least when delegating members are
+            // synthesised and inherit the annotation from the delegate (which given it has @NotNull, is likely written in Java)
+            JvmAnnotationNames.JETBRAINS_NOT_NULL_ANNOTATION.takeUnless { t.isNullable() || primitiveTypeMapping.getPrimitiveInfo(t) != null || hasExistingAnnotation(it) }
         }
     }
 
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/AnnotatedInterface.java b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/AnnotatedInterface.java
@@ -0,0 +1,10 @@
+import org.jetbrains.annotations.*;
+import zpkg.A;
+
+public interface AnnotatedInterface {
+
+  public @A @NotNull String notNullAnnotated(@A @NotNull String param);
+
+  public @A @Nullable String nullableAnnotated(@A @Nullable String param);
+
+}
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/AnnotatedMethods.java b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/AnnotatedMethods.java
@@ -1,7 +1,7 @@
 import org.jetbrains.annotations.*;
 import zpkg.A;
 
-public class AnnotatedMethods {
+public class AnnotatedMethods implements AnnotatedInterface {
 
   public @A @NotNull String notNullAnnotated(@A @NotNull String param) { return param; }
 
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/JavaUser.java b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/JavaUser.java
@@ -1,7 +1,8 @@
 public class JavaUser {
 
-  public static void test(KotlinAnnotatedMethods km) {
+  public static void test(KotlinAnnotatedMethods km, KotlinDelegate kd) {
     km.f(null);
+    kd.notNullAnnotated("Hello world");
   }
 
 }
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/ktUser.kt b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/ktUser.kt
@@ -5,3 +5,5 @@ class KotlinAnnotatedMethods {
   @A fun f(@A m: AnnotatedMethods): String = m.notNullAnnotated("hello") + m.nullableAnnotated("world")!!
 
 }
+
+class KotlinDelegate(c: AnnotatedMethods) : AnnotatedInterface by c { }
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/test.expected b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/test.expected
@@ -1,3 +1,11 @@
+| AnnotatedInterface.java:6:29:6:44 | notNullAnnotated | parameter | AnnotatedInterface.java:6:46:6:47 | A |
+| AnnotatedInterface.java:6:29:6:44 | notNullAnnotated | parameter | AnnotatedInterface.java:6:49:6:56 | NotNull |
+| AnnotatedInterface.java:6:29:6:44 | notNullAnnotated | return value | AnnotatedInterface.java:6:10:6:11 | A |
+| AnnotatedInterface.java:6:29:6:44 | notNullAnnotated | return value | AnnotatedInterface.java:6:13:6:20 | NotNull |
+| AnnotatedInterface.java:8:30:8:46 | nullableAnnotated | parameter | AnnotatedInterface.java:8:48:8:49 | A |
+| AnnotatedInterface.java:8:30:8:46 | nullableAnnotated | parameter | AnnotatedInterface.java:8:51:8:59 | Nullable |
+| AnnotatedInterface.java:8:30:8:46 | nullableAnnotated | return value | AnnotatedInterface.java:8:10:8:11 | A |
+| AnnotatedInterface.java:8:30:8:46 | nullableAnnotated | return value | AnnotatedInterface.java:8:13:8:21 | Nullable |
 | AnnotatedMethods.java:6:29:6:44 | notNullAnnotated | parameter | AnnotatedMethods.java:6:46:6:47 | A |
 | AnnotatedMethods.java:6:29:6:44 | notNullAnnotated | parameter | AnnotatedMethods.java:6:49:6:56 | NotNull |
 | AnnotatedMethods.java:6:29:6:44 | notNullAnnotated | return value | AnnotatedMethods.java:6:10:6:11 | A |
@@ -6,6 +14,14 @@
 | AnnotatedMethods.java:8:30:8:46 | nullableAnnotated | parameter | AnnotatedMethods.java:8:51:8:59 | Nullable |
 | AnnotatedMethods.java:8:30:8:46 | nullableAnnotated | return value | AnnotatedMethods.java:8:10:8:11 | A |
 | AnnotatedMethods.java:8:30:8:46 | nullableAnnotated | return value | AnnotatedMethods.java:8:13:8:21 | Nullable |
+| ktUser.kt:0:0:0:0 | notNullAnnotated | parameter | ktUser.kt:0:0:0:0 | A |
+| ktUser.kt:0:0:0:0 | notNullAnnotated | parameter | ktUser.kt:0:0:0:0 | NotNull |
+| ktUser.kt:0:0:0:0 | notNullAnnotated | return value | ktUser.kt:0:0:0:0 | A |
+| ktUser.kt:0:0:0:0 | notNullAnnotated | return value | ktUser.kt:0:0:0:0 | NotNull |
+| ktUser.kt:0:0:0:0 | nullableAnnotated | parameter | ktUser.kt:0:0:0:0 | A |
+| ktUser.kt:0:0:0:0 | nullableAnnotated | parameter | ktUser.kt:0:0:0:0 | Nullable |
+| ktUser.kt:0:0:0:0 | nullableAnnotated | return value | ktUser.kt:0:0:0:0 | A |
+| ktUser.kt:0:0:0:0 | nullableAnnotated | return value | ktUser.kt:0:0:0:0 | Nullable |
 | ktUser.kt:5:6:5:105 | f | parameter | ktUser.kt:0:0:0:0 | NotNull |
 | ktUser.kt:5:6:5:105 | f | parameter | ktUser.kt:5:12:5:13 | A |
 | ktUser.kt:5:6:5:105 | f | return value | ktUser.kt:0:0:0:0 | NotNull |
diff --git a/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/test.py b/java/ql/integration-tests/all-platforms/kotlin/nullability-annotations/test.py
@@ -3,4 +3,4 @@
 os.mkdir('out')
 os.mkdir('out2')
 os.mkdir('out3')
-run_codeql_database_create(["javac AnnotatedMethods.java zpkg/A.java org/jetbrains/annotations/NotNull.java org/jetbrains/annotations/Nullable.java -d out", "kotlinc ktUser.kt -cp out -d out2", "javac JavaUser.java -cp out:out2 -d out3"], lang="java")
+run_codeql_database_create(["javac AnnotatedInterface.java AnnotatedMethods.java zpkg/A.java org/jetbrains/annotations/NotNull.java org/jetbrains/annotations/Nullable.java -d out", "kotlinc ktUser.kt -cp out -d out2", "javac JavaUser.java -cp out:out2 -d out3"], lang="java")

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,8 @@`
`1`	`1`	`public class JavaUser {`
`2`	`2`
`3`		`- public static void test(KotlinAnnotatedMethods km) {`
	`3`	`+ public static void test(KotlinAnnotatedMethods km, KotlinDelegate kd) {`
`4`	`4`	`km.f(null);`
	`5`	`+ kd.notNullAnnotated("Hello world");`
`5`	`6`	`}`
`6`	`7`
`7`	`8`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,3 +5,5 @@ class KotlinAnnotatedMethods {`
`5`	`5`	`@A fun f(@A m: AnnotatedMethods): String = m.notNullAnnotated("hello") + m.nullableAnnotated("world")!!`
`6`	`6`
`7`	`7`	`}`
	`8`	`+`
	`9`	`+class KotlinDelegate(c: AnnotatedMethods) : AnnotatedInterface by c { }`