From 4d9cffccf2354bf292bff0fa7b61cde064f21733 Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Mon, 8 Feb 2021 11:41:15 +0100 Subject: [PATCH] Support structural equals/hashCode for type constructors of type parameters Use the same logic as for type constructors of classes, based on the fully-qualified name of the classifier, with special cases for error types and local declarations, with an additional check that the type constructors' declaration descriptors are structurally equal via `DescriptorEquivalenceForOverrides`. The latter is required because type parameters of overloaded functions must be different, even though their full FQ name is the same. This (hopefully) has no effect for the compiler, but is useful for kotlin-reflect where `KType.equals` runs the type checker on the underlying `KotlinType` instances, which eventually ends up comparing type constructors. Descriptors and types in kotlin-reflect are cached on soft references, so they may be suddenly garbage-collected and recomputed, and we want copies of the same type parameter to be equal to each other. This fixes flaky codegen tests which started to fail after migration to the new test infrastructure, where tests are now run in parallel in the same process, thus with higher memory pressure and more soft references being GC'd: * `codegen/box/reflection/types/createType/typeParameter.kt` * `codegen/box/reflection/supertypes/genericSubstitution.kt` Also, add a new test to check that we do the instanceof check in overrides of `AbstractTypeConstructor.isSameClassifier`. #KT-44850 Fixed --- .../FirBlackBoxCodegenTestGenerated.java | 6 ++ .../ir/descriptors/IrBasedDescriptors.kt | 2 + ...sForClassAndTypeParameterWithSameFqName.kt | 27 +++++++ .../codegen/BlackBoxCodegenTestGenerated.java | 6 ++ .../IrBlackBoxCodegenTestGenerated.java | 6 ++ .../LightAnalysisModeTestGenerated.java | 5 ++ .../impl/AbstractTypeParameterDescriptor.java | 11 +++ .../DescriptorEquivalenceForOverrides.kt | 3 +- .../types/AbstractClassTypeConstructor.java | 80 ++----------------- .../kotlin/types/AbstractTypeConstructor.kt | 68 +++++++++++++++- 10 files changed, 136 insertions(+), 78 deletions(-) create mode 100644 compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt diff --git a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java index 6e398a6e9af..d11440a7e7e 100644 --- a/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java +++ b/compiler/fir/fir2ir/tests-gen/org/jetbrains/kotlin/test/runners/codegen/FirBlackBoxCodegenTestGenerated.java @@ -35387,6 +35387,12 @@ public class FirBlackBoxCodegenTestGenerated extends AbstractFirBlackBoxCodegenT runTest("compiler/testData/codegen/box/reflection/types/classifiersOfBuiltInTypes.kt"); } + @Test + @TestMetadata("equalsForClassAndTypeParameterWithSameFqName.kt") + public void testEqualsForClassAndTypeParameterWithSameFqName() throws Exception { + runTest("compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt"); + } + @Test @TestMetadata("innerGenericArguments.kt") public void testInnerGenericArguments() throws Exception { diff --git a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/descriptors/IrBasedDescriptors.kt b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/descriptors/IrBasedDescriptors.kt index fa875cbd9d6..30ed8469043 100644 --- a/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/descriptors/IrBasedDescriptors.kt +++ b/compiler/ir/ir.tree/src/org/jetbrains/kotlin/ir/descriptors/IrBasedDescriptors.kt @@ -266,6 +266,8 @@ open class IrBasedTypeParameterDescriptor(owner: IrTypeParameter) : TypeParamete override fun getDeclarationDescriptor() = this@IrBasedTypeParameterDescriptor override fun getBuiltIns() = module.builtIns + + override fun isSameClassifier(classifier: ClassifierDescriptor): Boolean = declarationDescriptor === classifier } } diff --git a/compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt b/compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt new file mode 100644 index 00000000000..deeb33142ed --- /dev/null +++ b/compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt @@ -0,0 +1,27 @@ +// TARGET_BACKEND: JVM +// WITH_REFLECT + +// FIR incorrectly resolves typeParameterType's return type to the nested class `A.T`. +// IGNORE_BACKEND_FIR: JVM_IR + +package test + +class A { + class T + + fun typeParameterType(): T? = null + fun nestedClassType(): A.T? = null +} + +fun box(): String { + val typeParameterType = A<*>::typeParameterType.returnType + val classType = A<*>::nestedClassType.returnType + + if (typeParameterType == classType) + return "Fail 1: type parameter's type constructor shouldn't be equal to the class with the same FQ name" + + if (classType == typeParameterType) + return "Fail 2: class' type constructor shouldn't be equal to the type parameter with the same FQ name" + + return "OK" +} diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java index f3680f868a8..960be73cd2a 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/BlackBoxCodegenTestGenerated.java @@ -35387,6 +35387,12 @@ public class BlackBoxCodegenTestGenerated extends AbstractBlackBoxCodegenTest { runTest("compiler/testData/codegen/box/reflection/types/classifiersOfBuiltInTypes.kt"); } + @Test + @TestMetadata("equalsForClassAndTypeParameterWithSameFqName.kt") + public void testEqualsForClassAndTypeParameterWithSameFqName() throws Exception { + runTest("compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt"); + } + @Test @TestMetadata("innerGenericArguments.kt") public void testInnerGenericArguments() throws Exception { diff --git a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java index c64b6b797c2..096f5e8e728 100644 --- a/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java +++ b/compiler/tests-common-new/tests-gen/org/jetbrains/kotlin/test/runners/codegen/IrBlackBoxCodegenTestGenerated.java @@ -35387,6 +35387,12 @@ public class IrBlackBoxCodegenTestGenerated extends AbstractIrBlackBoxCodegenTes runTest("compiler/testData/codegen/box/reflection/types/classifiersOfBuiltInTypes.kt"); } + @Test + @TestMetadata("equalsForClassAndTypeParameterWithSameFqName.kt") + public void testEqualsForClassAndTypeParameterWithSameFqName() throws Exception { + runTest("compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt"); + } + @Test @TestMetadata("innerGenericArguments.kt") public void testInnerGenericArguments() throws Exception { diff --git a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java index 1d9ecf0f2f0..035e1085ce9 100644 --- a/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java +++ b/compiler/tests-gen/org/jetbrains/kotlin/codegen/LightAnalysisModeTestGenerated.java @@ -28135,6 +28135,11 @@ public class LightAnalysisModeTestGenerated extends AbstractLightAnalysisModeTes runTest("compiler/testData/codegen/box/reflection/types/classifiersOfBuiltInTypes.kt"); } + @TestMetadata("equalsForClassAndTypeParameterWithSameFqName.kt") + public void testEqualsForClassAndTypeParameterWithSameFqName() throws Exception { + runTest("compiler/testData/codegen/box/reflection/types/equalsForClassAndTypeParameterWithSameFqName.kt"); + } + @TestMetadata("innerGenericArguments.kt") public void testInnerGenericArguments() throws Exception { runTest("compiler/testData/codegen/box/reflection/types/innerGenericArguments.kt"); diff --git a/core/descriptors/src/org/jetbrains/kotlin/descriptors/impl/AbstractTypeParameterDescriptor.java b/core/descriptors/src/org/jetbrains/kotlin/descriptors/impl/AbstractTypeParameterDescriptor.java index 112f4ad0754..32fc433d24d 100644 --- a/core/descriptors/src/org/jetbrains/kotlin/descriptors/impl/AbstractTypeParameterDescriptor.java +++ b/core/descriptors/src/org/jetbrains/kotlin/descriptors/impl/AbstractTypeParameterDescriptor.java @@ -23,6 +23,7 @@ import org.jetbrains.kotlin.builtins.KotlinBuiltIns; import org.jetbrains.kotlin.descriptors.*; import org.jetbrains.kotlin.descriptors.annotations.Annotations; import org.jetbrains.kotlin.name.Name; +import org.jetbrains.kotlin.resolve.DescriptorEquivalenceForOverrides; import org.jetbrains.kotlin.resolve.descriptorUtil.DescriptorUtilsKt; import org.jetbrains.kotlin.resolve.scopes.LazyScopeAdapter; import org.jetbrains.kotlin.resolve.scopes.MemberScope; @@ -222,5 +223,15 @@ public abstract class AbstractTypeParameterDescriptor extends DeclarationDescrip protected KotlinType defaultSupertypeIfEmpty() { return ErrorUtils.createErrorType("Cyclic upper bounds"); } + + @Override + protected boolean isSameClassifier(@NotNull ClassifierDescriptor classifier) { + return classifier instanceof TypeParameterDescriptor && + DescriptorEquivalenceForOverrides.INSTANCE.areTypeParametersEquivalent( + AbstractTypeParameterDescriptor.this, + (TypeParameterDescriptor) classifier, + true + ); + } } } diff --git a/core/descriptors/src/org/jetbrains/kotlin/resolve/DescriptorEquivalenceForOverrides.kt b/core/descriptors/src/org/jetbrains/kotlin/resolve/DescriptorEquivalenceForOverrides.kt index dd7504e2946..b215106c057 100644 --- a/core/descriptors/src/org/jetbrains/kotlin/resolve/DescriptorEquivalenceForOverrides.kt +++ b/core/descriptors/src/org/jetbrains/kotlin/resolve/DescriptorEquivalenceForOverrides.kt @@ -56,7 +56,8 @@ object DescriptorEquivalenceForOverrides { return a.typeConstructor == b.typeConstructor } - private fun areTypeParametersEquivalent( + @JvmOverloads + fun areTypeParametersEquivalent( a: TypeParameterDescriptor, b: TypeParameterDescriptor, allowCopiesFromTheSameDeclaration: Boolean, diff --git a/core/descriptors/src/org/jetbrains/kotlin/types/AbstractClassTypeConstructor.java b/core/descriptors/src/org/jetbrains/kotlin/types/AbstractClassTypeConstructor.java index 44065e42bf2..b05e7d20bda 100644 --- a/core/descriptors/src/org/jetbrains/kotlin/types/AbstractClassTypeConstructor.java +++ b/core/descriptors/src/org/jetbrains/kotlin/types/AbstractClassTypeConstructor.java @@ -19,8 +19,10 @@ package org.jetbrains.kotlin.types; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.kotlin.builtins.KotlinBuiltIns; -import org.jetbrains.kotlin.descriptors.*; -import org.jetbrains.kotlin.resolve.DescriptorUtils; +import org.jetbrains.kotlin.descriptors.ClassDescriptor; +import org.jetbrains.kotlin.descriptors.ClassifierDescriptor; +import org.jetbrains.kotlin.descriptors.DeclarationDescriptor; +import org.jetbrains.kotlin.descriptors.ModalityUtilsKt; import org.jetbrains.kotlin.resolve.descriptorUtil.DescriptorUtilsKt; import org.jetbrains.kotlin.storage.StorageManager; import org.jetbrains.kotlin.utils.SmartList; @@ -29,28 +31,10 @@ import java.util.Collection; import java.util.Collections; public abstract class AbstractClassTypeConstructor extends AbstractTypeConstructor implements TypeConstructor { - private int hashCode = 0; - public AbstractClassTypeConstructor(@NotNull StorageManager storageManager) { super(storageManager); } - @Override - public final int hashCode() { - int currentHashCode = hashCode; - if (currentHashCode != 0) return currentHashCode; - - ClassifierDescriptor descriptor = getDeclarationDescriptor(); - if (hasMeaningfulFqName(descriptor)) { - currentHashCode = DescriptorUtils.getFqName(descriptor).hashCode(); - } - else { - currentHashCode = System.identityHashCode(this); - } - hashCode = currentHashCode; - return currentHashCode; - } - @NotNull @Override public abstract ClassDescriptor getDeclarationDescriptor(); @@ -68,60 +52,8 @@ public abstract class AbstractClassTypeConstructor extends AbstractTypeConstruct } @Override - public final boolean equals(Object other) { - if (this == other) return true; - if (!(other instanceof TypeConstructor)) return false; - - // performance optimization: getFqName is slow method - if (other.hashCode() != hashCode()) return false; - - // Sometimes we can get two classes from different modules with different counts of type parameters. - // To avoid problems in type checker we suppose that it is different type constructors. - if (((TypeConstructor) other).getParameters().size() != getParameters().size()) return false; - - ClassifierDescriptor myDescriptor = getDeclarationDescriptor(); - ClassifierDescriptor otherDescriptor = ((TypeConstructor) other).getDeclarationDescriptor(); - - if (!hasMeaningfulFqName(myDescriptor) || - otherDescriptor != null && !hasMeaningfulFqName(otherDescriptor)) { - // All error types and local classes have the same descriptor, - // but we've already checked identity equality in the beginning of the method - return false; - } - - if (otherDescriptor instanceof ClassDescriptor) { - return areFqNamesEqual(((ClassDescriptor) myDescriptor), ((ClassDescriptor) otherDescriptor)); - } - - return false; - } - - private static boolean areFqNamesEqual(ClassDescriptor first, ClassDescriptor second) { - if (!first.getName().equals(second.getName())) return false; - - DeclarationDescriptor a = first.getContainingDeclaration(); - DeclarationDescriptor b = second.getContainingDeclaration(); - while (a != null && b != null) { - if (a instanceof ModuleDescriptor) return b instanceof ModuleDescriptor; - if (b instanceof ModuleDescriptor) return false; - - if (a instanceof PackageFragmentDescriptor) { - return b instanceof PackageFragmentDescriptor && - ((PackageFragmentDescriptor) a).getFqName().equals(((PackageFragmentDescriptor) b).getFqName()); - } - if (b instanceof PackageFragmentDescriptor) return false; - - if (!a.getName().equals(b.getName())) return false; - - a = a.getContainingDeclaration(); - b = b.getContainingDeclaration(); - } - return true; - } - - private static boolean hasMeaningfulFqName(@NotNull ClassifierDescriptor descriptor) { - return !ErrorUtils.isError(descriptor) && - !DescriptorUtils.isLocal(descriptor); + protected boolean isSameClassifier(@NotNull ClassifierDescriptor classifier) { + return classifier instanceof ClassDescriptor && areFqNamesEqual(getDeclarationDescriptor(), classifier); } @NotNull diff --git a/core/descriptors/src/org/jetbrains/kotlin/types/AbstractTypeConstructor.kt b/core/descriptors/src/org/jetbrains/kotlin/types/AbstractTypeConstructor.kt index eaa3c5b2e25..d3a4e02c62f 100644 --- a/core/descriptors/src/org/jetbrains/kotlin/types/AbstractTypeConstructor.kt +++ b/core/descriptors/src/org/jetbrains/kotlin/types/AbstractTypeConstructor.kt @@ -17,15 +17,16 @@ package org.jetbrains.kotlin.types import org.jetbrains.kotlin.builtins.KotlinBuiltIns -import org.jetbrains.kotlin.descriptors.ClassifierDescriptor -import org.jetbrains.kotlin.descriptors.SupertypeLoopChecker -import org.jetbrains.kotlin.descriptors.TypeParameterDescriptor +import org.jetbrains.kotlin.descriptors.* +import org.jetbrains.kotlin.resolve.DescriptorUtils import org.jetbrains.kotlin.storage.StorageManager import org.jetbrains.kotlin.types.checker.KotlinTypeRefiner import org.jetbrains.kotlin.types.checker.refineTypes import org.jetbrains.kotlin.types.refinement.TypeRefinement abstract class AbstractTypeConstructor(storageManager: StorageManager) : TypeConstructor { + private var hashCode = 0 + override fun getSupertypes() = supertypes().supertypesWithoutCycles abstract override fun getDeclarationDescriptor(): ClassifierDescriptor @@ -132,4 +133,65 @@ abstract class AbstractTypeConstructor(storageManager: StorageManager) : TypeCon // Only for debugging fun renderAdditionalDebugInformation(): String = "supertypes=${supertypes.renderDebugInformation()}" + + override fun hashCode(): Int { + val cachedHashCode = hashCode + if (cachedHashCode != 0) return cachedHashCode + + val descriptor = declarationDescriptor + val computedHashCode = if (hasMeaningfulFqName(descriptor)) { + DescriptorUtils.getFqName(descriptor).hashCode() + } else { + System.identityHashCode(this) + } + + return computedHashCode.also { hashCode = it } + } + + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is TypeConstructor) return false + + // performance optimization: getFqName is slow method + if (other.hashCode() != hashCode()) return false + + // Sometimes we can get two classes from different modules with different counts of type parameters. + // To avoid problems in type checker we suppose that it is different type constructors. + if (other.parameters.size != parameters.size) return false + + val myDescriptor = declarationDescriptor + val otherDescriptor = other.declarationDescriptor ?: return false + if (!hasMeaningfulFqName(myDescriptor) || !hasMeaningfulFqName(otherDescriptor)) { + // All error types and local classes have the same descriptor, + // but we've already checked identity equality in the beginning of the method + return false + } + + return isSameClassifier(otherDescriptor) + } + + protected abstract fun isSameClassifier(classifier: ClassifierDescriptor): Boolean + + protected fun areFqNamesEqual(first: ClassifierDescriptor, second: ClassifierDescriptor): Boolean { + if (first.name != second.name) return false + var a: DeclarationDescriptor? = first.containingDeclaration + var b: DeclarationDescriptor? = second.containingDeclaration + while (a != null && b != null) { + when { + a is ModuleDescriptor -> return b is ModuleDescriptor + b is ModuleDescriptor -> return false + a is PackageFragmentDescriptor -> return b is PackageFragmentDescriptor && a.fqName == b.fqName + b is PackageFragmentDescriptor -> return false + a.name != b.name -> return false + else -> { + a = a.containingDeclaration + b = b.containingDeclaration + } + } + } + return true + } + + private fun hasMeaningfulFqName(descriptor: ClassifierDescriptor): Boolean = + !ErrorUtils.isError(descriptor) && !DescriptorUtils.isLocal(descriptor) }