From 28b6e263ec53f6f058327a9373dd4c28be0e53b1 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 26 Oct 2022 20:02:38 +0100 Subject: [PATCH] Kotlin: reintroduce pointless wildcards when a Java declaration explicitly uses them For example, Java code might use `HasOutVariance`, or `HasInVariance`, both of which are needless wildcards and which the Kotlin extractor would previously have refused to reintroduce due to their not specifying a larger type than their bound. However this led to inconsistency with Java extraction, which extracts the type as it appears in source. This seems to particularly happen with generated code, e.g. the output of the Kotlin protobuf compiler. --- .../src/main/kotlin/KotlinUsesExtractor.kt | 15 ++++++++++++--- .../kotlin/needless-java-wildcards/Test.java | 9 +++++++++ .../kotlin/needless-java-wildcards/kConsumer.kt | 1 + .../kotlin/needless-java-wildcards/test.expected | 2 ++ .../kotlin/needless-java-wildcards/test.py | 5 +++++ .../kotlin/needless-java-wildcards/test.ql | 5 +++++ .../kotlin/needless-java-wildcards/user.kt | 4 ++++ 7 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/Test.java create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/kConsumer.kt create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.expected create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.py create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.ql create mode 100644 java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/user.kt diff --git a/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt b/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt index 613c973f3c3..6a6d3e446c3 100644 --- a/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt +++ b/java/kotlin-extractor/src/main/kotlin/KotlinUsesExtractor.kt @@ -960,11 +960,13 @@ open class KotlinUsesExtractor( ((t as? IrSimpleType)?.classOrNull?.owner?.isFinalClass) != true } - private fun wildcardAdditionAllowed(v: Variance, t: IrType, addByDefault: Boolean) = + private fun wildcardAdditionAllowed(v: Variance, t: IrType, addByDefault: Boolean, javaVariance: Variance?) = when { t.hasAnnotation(jvmWildcardAnnotation) -> true !addByDefault -> false t.hasAnnotation(jvmWildcardSuppressionAnnotation) -> false + // If a Java declaration specifies a variance, introduce it even if it's pointless (e.g. ? extends FinalClass, or ? super Object) + javaVariance == v -> true v == Variance.IN_VARIANCE -> !(t.isNullableAny() || t.isAny()) v == Variance.OUT_VARIANCE -> extendsAdditionAllowed(t) else -> false @@ -973,14 +975,21 @@ open class KotlinUsesExtractor( private fun addJavaLoweringArgumentWildcards(p: IrTypeParameter, t: IrTypeArgument, addByDefault: Boolean, javaType: JavaType?): IrTypeArgument = (t as? IrTypeProjection)?.let { val newBase = addJavaLoweringWildcards(it.type, addByDefault, javaType) + // Note javaVariance == null means we don't have a Java type to conform to -- for example if this is a Kotlin source definition. + val javaVariance = javaType?.let { jType -> + when (jType) { + is JavaWildcardType -> if (jType.isExtends) Variance.OUT_VARIANCE else Variance.IN_VARIANCE + else -> Variance.INVARIANT + } + } val newVariance = if (it.variance == Variance.INVARIANT && p.variance != Variance.INVARIANT && // The next line forbids inferring a wildcard type when we have a corresponding Java type with conflicting variance. // For example, Java might declare f(Comparable cs), in which case we shouldn't add a `? super ...` // wildcard. Note if javaType is unknown (e.g. this is a Kotlin source element), we assume wildcards should be added. - (javaType?.let { jt -> jt is JavaWildcardType && jt.isExtends == (p.variance == Variance.OUT_VARIANCE) } != false) && - wildcardAdditionAllowed(p.variance, it.type, addByDefault)) + (javaVariance == null || javaVariance == p.variance) && + wildcardAdditionAllowed(p.variance, it.type, addByDefault, javaVariance)) p.variance else it.variance diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/Test.java b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/Test.java new file mode 100644 index 00000000000..2f2f6b6765a --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/Test.java @@ -0,0 +1,9 @@ +public class Test { + + // This gets mapped to kotlin.Iterable, meaning we must reintroduce the use-site extends variance to get a type consistent with Java. + public static void needlessExtends(Iterable l) { } + + // This type is defined KotlinConsumer, meaning we must reintroduce the use-site extends variance to get a type consistent with Java. + public static void needlessSuper(KotlinConsumer l) { } + +} diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/kConsumer.kt b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/kConsumer.kt new file mode 100644 index 00000000000..8824663b463 --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/kConsumer.kt @@ -0,0 +1 @@ +public class KotlinConsumer { } diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.expected b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.expected new file mode 100644 index 00000000000..99ca3ea3160 --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.expected @@ -0,0 +1,2 @@ +| Test.java:4:22:4:36 | needlessExtends | file:///modules/java.base/java/lang/Iterable.class:0:0:0:0 | Iterable | +| Test.java:7:22:7:34 | needlessSuper | build1/KotlinConsumer.class:0:0:0:0 | KotlinConsumer | diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.py b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.py new file mode 100644 index 00000000000..1b16dd4934a --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.py @@ -0,0 +1,5 @@ +from create_database_utils import * + +os.mkdir('build1') +os.mkdir('build2') +run_codeql_database_create(["kotlinc kConsumer.kt -d build1", "javac Test.java -cp build1 -d build2", "kotlinc user.kt -cp build1:build2"], lang="java") diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.ql b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.ql new file mode 100644 index 00000000000..10d3eb1774c --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/test.ql @@ -0,0 +1,5 @@ +import java + +from Method m +where m.fromSource() +select m, m.getAParamType() diff --git a/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/user.kt b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/user.kt new file mode 100644 index 00000000000..38d4de2db8d --- /dev/null +++ b/java/ql/integration-tests/posix-only/kotlin/needless-java-wildcards/user.kt @@ -0,0 +1,4 @@ +fun f() { + Test.needlessExtends(null) + Test.needlessSuper(null) +}