From 5ba76ee75794e222e9be78d0ed5bd60095e1cfc7 Mon Sep 17 00:00:00 2001 From: pyos Date: Thu, 15 Sep 2022 11:48:33 +0200 Subject: [PATCH] FIR: convert Java type parameter bounds before reading annotations This avoids a crash due to circular class references through annotation arguments. --- .../java/FirTypeEnhancementTestGenerated.java | 5 ++++ .../kotlin/fir/java/FirJavaFacade.kt | 24 ++++++++++++++++--- .../java/enhancement/SignatureEnhancement.kt | 13 ++++------ .../ReferenceCycleThroughAnnotation.fir.txt | 18 ++++++++++++++ .../ReferenceCycleThroughAnnotation.java | 16 +++++++++++++ .../ReferenceCycleThroughAnnotation.txt | 19 +++++++++++++++ .../jvm/compiler/LoadJavaTestGenerated.java | 5 ++++ ...dJavaWithPsiClassReadingTestGenerated.java | 5 ++++ .../compiler/ir/IrLoadJavaTestGenerated.java | 5 ++++ .../LoadJavaUsingJavacTestGenerated.java | 5 ++++ ...mRuntimeDescriptorLoaderTestGenerated.java | 5 ++++ 11 files changed, 108 insertions(+), 12 deletions(-) create mode 100644 compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.fir.txt create mode 100644 compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java create mode 100644 compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.txt diff --git a/compiler/fir/analysis-tests/legacy-fir-tests/tests-gen/org/jetbrains/kotlin/fir/java/FirTypeEnhancementTestGenerated.java b/compiler/fir/analysis-tests/legacy-fir-tests/tests-gen/org/jetbrains/kotlin/fir/java/FirTypeEnhancementTestGenerated.java index 5cac70a7ab9..5725e988417 100644 --- a/compiler/fir/analysis-tests/legacy-fir-tests/tests-gen/org/jetbrains/kotlin/fir/java/FirTypeEnhancementTestGenerated.java +++ b/compiler/fir/analysis-tests/legacy-fir-tests/tests-gen/org/jetbrains/kotlin/fir/java/FirTypeEnhancementTestGenerated.java @@ -224,6 +224,11 @@ public class FirTypeEnhancementTestGenerated extends AbstractFirTypeEnhancementT runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java"); diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaFacade.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaFacade.kt index 5b44bda03ee..b293f98ad3c 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaFacade.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/FirJavaFacade.kt @@ -164,9 +164,27 @@ abstract class FirJavaFacade( parentClassTypeParameterStackCache.remove(classSymbol) parentClassEffectiveVisibilityCache.remove(classSymbol) - // There's a bit of an ordering restriction here: - // 1. annotations should be added after the symbol is bound, as annotations can refer to the class itself; - // 2. type enhancement requires annotations to be already present (and supertypes can refer to type parameters). + // This is where the problems begin. We need to enhance nullability of super types and type parameter bounds, + // for which we need the annotations of this class as they may specify default nullability. + // However, all three - annotations, type parameter bounds, and supertypes - can refer to other classes, + // which will cause the type parameter bounds and supertypes of *those* classes to get enhanced first, + // but they may refer back to this class again - which, thanks to the magic of symbol resolver caches, + // will be observed in a state where we've not done the enhancement yet. For those cases, we must publish + // at least unenhanced resolved types, or else FIR may crash upon encountering a FirJavaTypeRef where FirResolvedTypeRef + // is expected. + // TODO: some (all?) of those loops can be avoided, e.g. we don't actually need to resolve class arguments of annotations + // to determine whether they set default nullability - but without laziness, breaking those loops is somewhat hard, + // as we have a nested ordering here. + for (typeParameter in firJavaClass.typeParameters) { + if (typeParameter is FirTypeParameter) { + typeParameter.replaceBounds(typeParameter.bounds.map { + it.resolveIfJavaType(session, javaTypeParameterStack, FirJavaTypeConversionMode.TYPE_PARAMETER_BOUND) + }) + } + } + // 1. Resolve annotations + // 2. Enhance type parameter bounds - may refer to each other, take default nullability from annotations + // 3. Enhance super types - may refer to type parameter bounds, take default nullability from annotations firJavaClass.annotations.addFromJava(session, javaClass, javaTypeParameterStack) val enhancement = FirSignatureEnhancement(firJavaClass, session) { emptyList() } enhancement.enhanceTypeParameterBounds(firJavaClass.typeParameters) diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/enhancement/SignatureEnhancement.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/enhancement/SignatureEnhancement.kt index db16bee0993..c9faf5f7e4e 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/enhancement/SignatureEnhancement.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/enhancement/SignatureEnhancement.kt @@ -333,16 +333,11 @@ class FirSignatureEnhancement( // (`A : B, B : A` - invalid), the cycles can still appear when type parameters use each other in argument // position (`A : C, B : D` - valid). In this case the precise enhancement of each bound depends on // the others' nullability, for which we need to enhance at least its head type constructor. - // - // While this is straightforward to do within a single class/method (enhance all bounds' head type - // constructors, then enhance fully), it's not so simple when two classes depend on each other (we need - // to enhance *both* classes' type parameters' bounds' heads first). This is why we replace each bound - // with an unenhanced version first: this ensures that the frontend at least doesn't fail. - // - // TODO: find a way to partially enhance type parameters of all classes before fully enhancing anything. - // TODO: should this be done in topological order on head type constructors? - // I.e. for `A : B, B : C` should we process `B` first? typeParameters.replaceBounds { _, bound -> + // Resolve without enhancement so we don't crash the frontend if there is a restricted cycle (`A : B, B : A`) + // or if we visit type parameters in the wrong order (`A : B, B : C` with `A` enhanced before `B`). + // TODO: the second case technically produces incorrect results - the loop below should visit type parameters + // in topological order, then a resolved-but-not-enhanced type will never be observable with valid code. bound.resolveIfJavaType(session, javaTypeParameterStack, FirJavaTypeConversionMode.TYPE_PARAMETER_BOUND) } typeParameters.replaceBounds { typeParameter, bound -> diff --git a/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.fir.txt b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.fir.txt new file mode 100644 index 00000000000..2433a3ad4c5 --- /dev/null +++ b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.fir.txt @@ -0,0 +1,18 @@ +public open class ReferenceCycleThroughAnnotation : R|kotlin/Any| { + public constructor(): R|test/ReferenceCycleThroughAnnotation| + + @R|test/ReferenceCycleThroughAnnotation.C|(value = ((R|ft, test/ReferenceCycleThroughAnnotation.A<*>?>>, test/ReferenceCycleThroughAnnotation.B<*>?>|))) public open inner class A : R|kotlin/Any| { + public open fun foo(): R|kotlin/Unit| + + public test/ReferenceCycleThroughAnnotation.constructor(): R|test/ReferenceCycleThroughAnnotation.A| + + } + public open inner class B>, test/ReferenceCycleThroughAnnotation.A>?>|> : R|kotlin/Any| { + public test/ReferenceCycleThroughAnnotation.constructor>, test/ReferenceCycleThroughAnnotation.A>?>|>(): R|test/ReferenceCycleThroughAnnotation.B| + + } + public final annotation class C : R|kotlin/Annotation| { + public constructor(value: R|kotlin/reflect/KClass<*>|): R|test/ReferenceCycleThroughAnnotation.C| + + } +} diff --git a/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java new file mode 100644 index 00000000000..600cc5a1967 --- /dev/null +++ b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java @@ -0,0 +1,16 @@ +package test; + +public class ReferenceCycleThroughAnnotation { + @C(B.class) + public class A { + public void foo() { + } + } + + public class B> { + } + + public @interface C { + public Class value(); + } +} diff --git a/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.txt b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.txt new file mode 100644 index 00000000000..98a0e5d341f --- /dev/null +++ b/compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.txt @@ -0,0 +1,19 @@ +package test + +public open class ReferenceCycleThroughAnnotation { + public constructor ReferenceCycleThroughAnnotation() + + @test.ReferenceCycleThroughAnnotation.C(value = test.ReferenceCycleThroughAnnotation.B::class) public open inner class A { + public constructor A() + public open fun foo(): kotlin.Unit + } + + public open inner class B!> { + public constructor B!>() + } + + public final annotation class C : kotlin.Annotation { + public constructor C(/*0*/ value: kotlin.reflect.KClass<*>) + public final val value: kotlin.reflect.KClass<*> + } +} diff --git a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaTestGenerated.java index 2cca4980846..b46258218db 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaTestGenerated.java @@ -226,6 +226,11 @@ public class LoadJavaTestGenerated extends AbstractLoadJavaTest { runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaWithPsiClassReadingTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaWithPsiClassReadingTestGenerated.java index a3f5d8fbfbe..d4ad5a2a809 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaWithPsiClassReadingTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/LoadJavaWithPsiClassReadingTestGenerated.java @@ -224,6 +224,11 @@ public class LoadJavaWithPsiClassReadingTestGenerated extends AbstractLoadJavaWi runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/ir/IrLoadJavaTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/ir/IrLoadJavaTestGenerated.java index 3081237d691..a047722d944 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/ir/IrLoadJavaTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/ir/IrLoadJavaTestGenerated.java @@ -227,6 +227,11 @@ public class IrLoadJavaTestGenerated extends AbstractIrLoadJavaTest { runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java"); diff --git a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/javac/LoadJavaUsingJavacTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/javac/LoadJavaUsingJavacTestGenerated.java index 2f94b3319de..2300d88eeb0 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/javac/LoadJavaUsingJavacTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/jvm/compiler/javac/LoadJavaUsingJavacTestGenerated.java @@ -226,6 +226,11 @@ public class LoadJavaUsingJavacTestGenerated extends AbstractLoadJavaUsingJavacT runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java"); diff --git a/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/JvmRuntimeDescriptorLoaderTestGenerated.java b/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/JvmRuntimeDescriptorLoaderTestGenerated.java index 425b5036587..49f41d548e4 100644 --- a/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/JvmRuntimeDescriptorLoaderTestGenerated.java +++ b/core/descriptors.runtime/tests/org/jetbrains/kotlin/jvm/runtime/JvmRuntimeDescriptorLoaderTestGenerated.java @@ -3067,6 +3067,11 @@ public class JvmRuntimeDescriptorLoaderTestGenerated extends AbstractJvmRuntimeD runTest("compiler/testData/loadJava/compiledJava/RecursiveWildcardUpperBound.java"); } + @TestMetadata("ReferenceCycleThroughAnnotation.java") + public void testReferenceCycleThroughAnnotation() throws Exception { + runTest("compiler/testData/loadJava/compiledJava/ReferenceCycleThroughAnnotation.java"); + } + @TestMetadata("RemoveRedundantProjectionKind.java") public void testRemoveRedundantProjectionKind() throws Exception { runTest("compiler/testData/loadJava/compiledJava/RemoveRedundantProjectionKind.java");