From 360d67410d57f344df09ab80e07cb1365429438f Mon Sep 17 00:00:00 2001 From: "Denis.Zharkov" Date: Thu, 28 Oct 2021 11:38:20 +0300 Subject: [PATCH] FIR: Fix overridability rule for Java declarations with different return type kinds See the class at org/jmock/Expectations public T with(Matcher matcher); public boolean with(Matcher matcher); When we extending such class it we start assuming that fake generic override overrides both of the overridden that is wrong from POV of Java and it fails at FIR ultimate build NB: It's hard to write a test because such Expectation-like class is impossible to write in pure Java --- .../kotlin/fir/java/JavaScopeProvider.kt | 4 +- .../scopes/JavaClassStaticUseSiteScope.kt | 2 +- .../scopes/JavaClassUseSiteMemberScope.kt | 2 +- .../fir/java/scopes/JavaOverrideChecker.kt | 66 +++++++++++++++++-- .../firIncorrectJavaSignature/A.java | 6 ++ .../firIncorrectJavaSignature/B.java | 6 ++ .../firIncorrectJavaSignature/output.txt | 1 + .../firIncorrectJavaSignature/source.kt | 4 ++ .../AbstractKotlinCompilerIntegrationTest.kt | 5 +- .../CompileKotlinAgainstCustomBinariesTest.kt | 9 +++ 10 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/A.java create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/B.java create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/output.txt create mode 100644 compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/source.kt diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/JavaScopeProvider.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/JavaScopeProvider.kt index 53415d1054c..223d44f379f 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/JavaScopeProvider.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/JavaScopeProvider.kt @@ -102,7 +102,9 @@ object JavaScopeProvider : FirScopeProvider() { useSiteSession, JavaOverrideChecker( useSiteSession, - regularClass.javaTypeParameterStack + regularClass.javaTypeParameterStack, + baseScope = null, + considerReturnTypeKinds = false, ), superTypeScopes, regularClass.defaultType(), diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassStaticUseSiteScope.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassStaticUseSiteScope.kt index 23380cb5065..ffdd5ed8ca3 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassStaticUseSiteScope.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassStaticUseSiteScope.kt @@ -23,7 +23,7 @@ class JavaClassStaticUseSiteScope internal constructor( ) : FirContainingNamesAwareScope() { private val functions = hashMapOf>() private val properties = hashMapOf>>() - private val overrideChecker = JavaOverrideChecker(session, javaTypeParameterStack) + private val overrideChecker = JavaOverrideChecker(session, javaTypeParameterStack, baseScope = null, considerReturnTypeKinds = false) override fun processFunctionsByName(name: Name, processor: (FirNamedFunctionSymbol) -> Unit) { functions.getOrPut(name) { diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassUseSiteMemberScope.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassUseSiteMemberScope.kt index ac5ceee2ab2..23970279de6 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassUseSiteMemberScope.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaClassUseSiteMemberScope.kt @@ -45,7 +45,7 @@ class JavaClassUseSiteMemberScope( declaredMemberScope: FirContainingNamesAwareScope ) : AbstractFirUseSiteMemberScope( session, - JavaOverrideChecker(session, klass.javaTypeParameterStack), + JavaOverrideChecker(session, klass.javaTypeParameterStack, superTypesScope, considerReturnTypeKinds = true), superTypesScope, declaredMemberScope ) { diff --git a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaOverrideChecker.kt b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaOverrideChecker.kt index 95597725e04..15c73a5ed8e 100644 --- a/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaOverrideChecker.kt +++ b/compiler/fir/java/src/org/jetbrains/kotlin/fir/java/scopes/JavaOverrideChecker.kt @@ -16,15 +16,20 @@ import org.jetbrains.kotlin.fir.java.toConeKotlinTypeProbablyFlexible import org.jetbrains.kotlin.fir.resolve.fullyExpandedType import org.jetbrains.kotlin.fir.resolve.substitution.ConeSubstitutor import org.jetbrains.kotlin.fir.resolve.substitution.substitutorByMap +import org.jetbrains.kotlin.fir.scopes.FirTypeScope +import org.jetbrains.kotlin.fir.scopes.ProcessorAction import org.jetbrains.kotlin.fir.scopes.impl.FirAbstractOverrideChecker import org.jetbrains.kotlin.fir.scopes.jvm.computeJvmDescriptorRepresentation +import org.jetbrains.kotlin.fir.scopes.processOverriddenFunctions import org.jetbrains.kotlin.fir.symbols.ensureResolved import org.jetbrains.kotlin.fir.types.* import org.jetbrains.kotlin.name.StandardClassIds class JavaOverrideChecker internal constructor( private val session: FirSession, - private val javaTypeParameterStack: JavaTypeParameterStack + private val javaTypeParameterStack: JavaTypeParameterStack, + private val baseScope: FirTypeScope?, + private val considerReturnTypeKinds: Boolean, ) : FirAbstractOverrideChecker() { private val context: ConeTypeContext = session.typeContext @@ -74,7 +79,7 @@ class JavaOverrideChecker internal constructor( val candidateType = candidateTypeRef.toConeKotlinTypeProbablyFlexible(session, javaTypeParameterStack) val baseType = baseTypeRef.toConeKotlinTypeProbablyFlexible(session, javaTypeParameterStack) - if (candidateType.isPrimitiveInJava() != baseType.isPrimitiveInJava()) return false + if (candidateType.isPrimitiveInJava(isReturnType = false) != baseType.isPrimitiveInJava(isReturnType = false)) return false return isEqualTypes( candidateType, @@ -83,8 +88,50 @@ class JavaOverrideChecker internal constructor( ) } - private fun ConeKotlinType.isPrimitiveInJava(): Boolean = with(context) { - !isNullableType() && CompilerConeAttributes.EnhancedNullability !in attributes && isPrimitiveOrNullablePrimitive + // In most cases checking erasure of value parameters should be enough, but in some cases there might be semi-valid Java hierarchies + // with same value parameters, but different return type kinds, so it's worth distinguishing them as different non-overridable members + private fun doesReturnTypesHaveSameKind( + candidate: FirSimpleFunction, + base: FirSimpleFunction, + ): Boolean { + val candidateTypeRef = candidate.returnTypeRef + val baseTypeRef = base.returnTypeRef + + val candidateType = candidateTypeRef.toConeKotlinTypeProbablyFlexible(session, javaTypeParameterStack) + val baseType = baseTypeRef.toConeKotlinTypeProbablyFlexible(session, javaTypeParameterStack) + + val candidateHasPrimitiveReturnType = candidate.hasPrimitiveReturnTypeInJvm(candidateType) + if (candidateHasPrimitiveReturnType != base.hasPrimitiveReturnTypeInJvm(baseType)) return false + + // Both candidate and base are not primitive + if (!candidateHasPrimitiveReturnType) return true + + return (candidateType as? ConeClassLikeType)?.lookupTag == (baseType as? ConeClassLikeType)?.lookupTag + } + + private fun ConeKotlinType.isPrimitiveInJava(isReturnType: Boolean): Boolean = with(context) { + if (isNullableType() || CompilerConeAttributes.EnhancedNullability in attributes) return false + + val isVoid = isReturnType && isUnit + return isPrimitiveOrNullablePrimitive || isVoid + } + + private fun FirSimpleFunction.hasPrimitiveReturnTypeInJvm(returnType: ConeKotlinType): Boolean { + if (!returnType.isPrimitiveInJava(isReturnType = true)) return false + + var foundNonPrimitiveOverridden = false + + baseScope?.processOverriddenFunctions(symbol) { + val type = it.fir.returnTypeRef.toConeKotlinTypeProbablyFlexible(session, javaTypeParameterStack) + if (!type.isPrimitiveInJava(isReturnType = true)) { + foundNonPrimitiveOverridden = true + ProcessorAction.STOP + } else { + ProcessorAction.NEXT + } + } + + return !foundNonPrimitiveOverridden } private fun isEqualArrayElementTypeProjections( @@ -176,9 +223,16 @@ class JavaOverrideChecker internal constructor( if (overrideCandidate.valueParameters.size != baseParameterTypes.size) return false val substitutor = buildTypeParametersSubstitutorIfCompatible(overrideCandidate, baseDeclaration) - return overrideCandidate.valueParameters.zip(baseParameterTypes).all { (paramFromJava, baseType) -> - isEqualTypes(paramFromJava.returnTypeRef, baseType, substitutor) + + if (!overrideCandidate.valueParameters.zip(baseParameterTypes).all { (paramFromJava, baseType) -> + isEqualTypes(paramFromJava.returnTypeRef, baseType, substitutor) + }) { + return false } + + if (considerReturnTypeKinds && !doesReturnTypesHaveSameKind(overrideCandidate, baseDeclaration)) return false + + return true } override fun isOverriddenProperty(overrideCandidate: FirCallableDeclaration, baseDeclaration: FirProperty): Boolean { diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/A.java b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/A.java new file mode 100644 index 00000000000..81226999ec0 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/A.java @@ -0,0 +1,6 @@ +package test; +import java.util.List; +public class A { + public T foo(List t) { return null; } + public boolean foo(List t) { return false; } +} diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/B.java b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/B.java new file mode 100644 index 00000000000..d948c9b2687 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/B.java @@ -0,0 +1,6 @@ +package test; +import java.util.List; +public class B extends A { + public T foo(List t) { return null; } + public boolean foo(List t) { return false; } +} diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/output.txt b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/output.txt new file mode 100644 index 00000000000..d86bac9de59 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/output.txt @@ -0,0 +1 @@ +OK diff --git a/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/source.kt b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/source.kt new file mode 100644 index 00000000000..cc63ce63e03 --- /dev/null +++ b/compiler/testData/compileKotlinAgainstCustomBinaries/firIncorrectJavaSignature/source.kt @@ -0,0 +1,4 @@ +package test +fun main() { + object : B() {} +} diff --git a/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/AbstractKotlinCompilerIntegrationTest.kt b/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/AbstractKotlinCompilerIntegrationTest.kt index 7e85c2c0932..3994f33d244 100644 --- a/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/AbstractKotlinCompilerIntegrationTest.kt +++ b/compiler/tests-common/tests/org/jetbrains/kotlin/jvm/compiler/AbstractKotlinCompilerIntegrationTest.kt @@ -150,13 +150,16 @@ abstract class AbstractKotlinCompilerIntegrationTest : TestCaseWithTmpdir() { classpath: List = emptyList(), compiler: CLICompiler<*> = K2JVMCompiler(), additionalOptions: List = emptyList(), - expectedFileName: String? = "output.txt" + expectedFileName: String? = "output.txt", + additionalSources: List = emptyList(), ): Pair { val args = mutableListOf() val sourceFile = File(testDataDirectory, fileName) assert(sourceFile.exists()) { "Source file does not exist: ${sourceFile.absolutePath}" } args.add(sourceFile.path) + additionalSources.mapTo(args) { File(testDataDirectory, it).path } + if (compiler is K2JSCompiler) { if (classpath.isNotEmpty()) { args.add("-libraries") diff --git a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt index b442f3453b6..e432a90c32b 100644 --- a/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt +++ b/compiler/tests/org/jetbrains/kotlin/jvm/compiler/CompileKotlinAgainstCustomBinariesTest.kt @@ -612,6 +612,15 @@ class CompileKotlinAgainstCustomBinariesTest : AbstractKotlinCompilerIntegration compileKotlin("source.kt", tmpdir, listOf(library), additionalOptions = listOf("-Xuse-fir")) } + fun testFirIncorrectJavaSignature() { + compileKotlin( + "source.kt", tmpdir, + listOf(), + additionalOptions = listOf("-Xuse-fir"), + additionalSources = listOf("A.java", "B.java"), + ) + } + fun testOldJvmAgainstJvmIr() { val library = compileLibrary("library", additionalOptions = listOf("-Xuse-ir")) compileKotlin("source.kt", tmpdir, listOf(library))