[FE] Type-safety refactoring: extract main logic of areCompatibleCallables into two functions

Review: https://jetbrains.team/p/kt/reviews/11039/timeline Extract main logic of `areCompatibleCallables` into two functions: `areStrongIncompatibleCallables` and `areWeakIncompatibleCallables`. The main point is that `areStrongIncompatibleCallables` & `areWeakIncompatibleCallables` have very specific return types. This commit doesn't change any logic. The commit makes the API more type-safe ensuring that bugs like in previous commit (KT-60902) won't happen again
2023-08-02 18:27:54 +02:00
parent eac4b81b11
commit 98ec13b51a
4 changed files with 85 additions and 75 deletions
@@ -274,12 +274,11 @@ object FirExpectActualDeclarationChecker : FirBasicDeclarationChecker() {
    }

    fun Map<out ExpectActualCompatibility<*>, *>.allStrongIncompatibilities(): Boolean {
-        return keys.all { it is Incompatible && it.kind == IncompatibilityKind.STRONG }
+        return keys.all { it is Incompatible.StrongIncompatible }
    }

    private fun ExpectActualCompatibility<FirBasedSymbol<*>>.isCompatibleOrWeakCompatible(): Boolean {
-        return this is Compatible ||
-                this is Incompatible && kind == IncompatibilityKind.WEAK
+        return this is Compatible || this is Incompatible.WeakIncompatible
    }

    // we don't require `actual` modifier on
@@ -505,11 +505,10 @@ class ExpectedActualDeclarationChecker(

    companion object {
        fun Map<out ExpectActualCompatibility<MemberDescriptor>, Collection<MemberDescriptor>>.allStrongIncompatibilities(): Boolean =
-            this.keys.all { it is Incompatible && it.kind == IncompatibilityKind.STRONG }
+            this.keys.all { it is Incompatible.StrongIncompatible }

        internal fun ExpectActualCompatibility<MemberDescriptor>.isCompatibleOrWeakCompatible() =
-            this is Compatible ||
-                    this is Incompatible && kind == IncompatibilityKind.WEAK
+            this is Compatible || this is Incompatible.WeakIncompatible
    }
 }

@@ -298,8 +298,18 @@ object AbstractExpectActualCompatibilityChecker {
            return ExpectActualCompatibility.Compatible
        }

-        // ALL THE FOLLOWING ARE STRONG INCOMPATIBILITIES
+        // We must prioritize to return STRONG incompatible over WEAK incompatible (because STRONG incompatibility allows to search for overloads)
+        return getCallablesStrongIncompatibility(expectDeclaration, actualDeclaration, parentSubstitutor)
+            ?: getCallablesWeakIncompatibility(expectDeclaration, actualDeclaration, expectContainingClass, actualContainingClass)
+            ?: ExpectActualCompatibility.Compatible
+    }

+    context(ExpectActualMatchingContext<*>)
+    private fun getCallablesStrongIncompatibility(
+        expectDeclaration: CallableSymbolMarker,
+        actualDeclaration: CallableSymbolMarker,
+        parentSubstitutor: TypeSubstitutorMarker?,
+    ): Incompatible.StrongIncompatible<*>? {
        if (expectDeclaration is FunctionSymbolMarker != actualDeclaration is FunctionSymbolMarker) {
            return Incompatible.CallableKind
        }
@@ -347,9 +357,22 @@ object AbstractExpectActualCompatibilityChecker {
            }
        }

-        areStrongIncompatibleTypeParameters(expectedTypeParameters, actualTypeParameters, substitutor)?.let { return it }
+        getTypeParametersStrongIncompatibility(expectedTypeParameters, actualTypeParameters, substitutor)?.let { return it }

-        // ALL THE FOLLOWING ARE WEAK INCOMPATIBILITIES
+        return null
+    }
+
+    context(ExpectActualMatchingContext<*>)
+    private fun getCallablesWeakIncompatibility(
+        expectDeclaration: CallableSymbolMarker,
+        actualDeclaration: CallableSymbolMarker,
+        expectContainingClass: RegularClassSymbolMarker?,
+        actualContainingClass: RegularClassSymbolMarker?,
+    ): Incompatible.WeakIncompatible<*>? {
+        val expectedTypeParameters = expectDeclaration.typeParameters
+        val actualTypeParameters = actualDeclaration.typeParameters
+        val expectedValueParameters = expectDeclaration.valueParameters
+        val actualValueParameters = actualDeclaration.valueParameters

        if (actualDeclaration.hasStableParameterNames && !equalsBy(expectedValueParameters, actualValueParameters) { it.name }) {
            return Incompatible.ParameterNames
@@ -376,7 +399,7 @@ object AbstractExpectActualCompatibilityChecker {
            return Incompatible.Visibility
        }

-        areWeakIncompatibleTypeParameters(expectedTypeParameters, actualTypeParameters)?.let { return it }
+        getTypeParametersWeakIncompatibility(expectedTypeParameters, actualTypeParameters)?.let { return it }

        if (shouldCheckAbsenceOfDefaultParamsInActual) {
            // "Default parameters in actual" check is required only for functions, because only functions can have parameters
@@ -409,15 +432,11 @@ object AbstractExpectActualCompatibilityChecker {
        }

        when {
-            expectDeclaration is FunctionSymbolMarker && actualDeclaration is FunctionSymbolMarker -> areCompatibleFunctions(
-                expectDeclaration,
-                actualDeclaration
-            ).let { if (it != ExpectActualCompatibility.Compatible) return it }
+            expectDeclaration is FunctionSymbolMarker && actualDeclaration is FunctionSymbolMarker ->
+                getFunctionsIncompatibility(expectDeclaration, actualDeclaration)?.let { return it }

-            expectDeclaration is PropertySymbolMarker && actualDeclaration is PropertySymbolMarker -> areCompatibleProperties(
-                expectDeclaration,
-                actualDeclaration
-            ).let { if (it != ExpectActualCompatibility.Compatible) return it }
+            expectDeclaration is PropertySymbolMarker && actualDeclaration is PropertySymbolMarker ->
+                getPropertiesIncompatibility(expectDeclaration, actualDeclaration)?.let { return it }

            expectDeclaration is EnumEntrySymbolMarker && actualDeclaration is EnumEntrySymbolMarker -> {
                // do nothing, entries are matched only by name
@@ -426,7 +445,7 @@ object AbstractExpectActualCompatibilityChecker {
            else -> error("Unsupported declarations: $expectDeclaration, $actualDeclaration")
        }

-        return ExpectActualCompatibility.Compatible
+        return null
    }

    context(ExpectActualMatchingContext<*>)
@@ -539,16 +558,16 @@ object AbstractExpectActualCompatibilityChecker {
        substitutor: TypeSubstitutorMarker,
    ): ExpectActualCompatibility<*> =
        // We must prioritize to return STRONG incompatible over WEAK incompatible (because STRONG incompatibility allows to search for overloads)
-        areStrongIncompatibleTypeParameters(expectTypeParameterSymbols, actualTypeParameterSymbols, substitutor)
-            ?: areWeakIncompatibleTypeParameters(expectTypeParameterSymbols, actualTypeParameterSymbols)
+        getTypeParametersStrongIncompatibility(expectTypeParameterSymbols, actualTypeParameterSymbols, substitutor)
+            ?: getTypeParametersWeakIncompatibility(expectTypeParameterSymbols, actualTypeParameterSymbols)
            ?: ExpectActualCompatibility.Compatible

    context(ExpectActualMatchingContext<*>)
-    private fun areStrongIncompatibleTypeParameters(
+    private fun getTypeParametersStrongIncompatibility(
        expectTypeParameterSymbols: List<TypeParameterSymbolMarker>,
        actualTypeParameterSymbols: List<TypeParameterSymbolMarker>,
        substitutor: TypeSubstitutorMarker,
-    ): Incompatible<*>? {
+    ): Incompatible.StrongIncompatible<*>? {
        for (i in expectTypeParameterSymbols.indices) {
            val expectBounds = expectTypeParameterSymbols[i].bounds
            val actualBounds = actualTypeParameterSymbols[i].bounds
@@ -564,10 +583,10 @@ object AbstractExpectActualCompatibilityChecker {
    }

    context(ExpectActualMatchingContext<*>)
-    private fun areWeakIncompatibleTypeParameters(
+    private fun getTypeParametersWeakIncompatibility(
        expectTypeParameterSymbols: List<TypeParameterSymbolMarker>,
        actualTypeParameterSymbols: List<TypeParameterSymbolMarker>,
-    ): Incompatible<*>? {
+    ): Incompatible.WeakIncompatible<*>? {
        if (!equalsBy(expectTypeParameterSymbols, actualTypeParameterSymbols) { it.variance }) {
            return Incompatible.TypeParameterVariance
        }
@@ -585,10 +604,10 @@ object AbstractExpectActualCompatibilityChecker {
    }

    context(ExpectActualMatchingContext<*>)
-    private fun areCompatibleFunctions(
+    private fun getFunctionsIncompatibility(
        expectFunction: CallableSymbolMarker,
        actualFunction: CallableSymbolMarker,
-    ): ExpectActualCompatibility<*> {
+    ): Incompatible.WeakIncompatible<*>? {
        if (!equalBy(expectFunction, actualFunction) { f -> f.isSuspend }) {
            return Incompatible.FunctionModifiersDifferent
        }
@@ -601,20 +620,20 @@ object AbstractExpectActualCompatibilityChecker {
            return Incompatible.FunctionModifiersNotSubset
        }

-        return ExpectActualCompatibility.Compatible
+        return null
    }

    context(ExpectActualMatchingContext<*>)
-    private fun areCompatibleProperties(
+    private fun getPropertiesIncompatibility(
        expected: PropertySymbolMarker,
        actual: PropertySymbolMarker,
-    ): ExpectActualCompatibility<*> {
+    ): Incompatible.WeakIncompatible<*>? {
        return when {
            !equalBy(expected, actual) { p -> p.isVar } -> Incompatible.PropertyKind
            !equalBy(expected, actual) { p -> p.isLateinit } -> Incompatible.PropertyLateinitModifier
            expected.isConst && !actual.isConst -> Incompatible.PropertyConstModifier
            !arePropertySettersWithCompatibleVisibilities(expected, actual) -> Incompatible.PropertySetterVisibility
-            else -> ExpectActualCompatibility.Compatible
+            else -> null
        }
    }

@@ -9,90 +9,83 @@ import kotlin.contracts.ExperimentalContracts
 import kotlin.contracts.contract

 sealed class ExpectActualCompatibility<out D> {
-    // For IncompatibilityKind.STRONG `actual` declaration is considered as overload and error reports on expected declaration
-    enum class IncompatibilityKind {
-        WEAK, STRONG
-    }

    // Note that the reason is used in the diagnostic output, see PlatformIncompatibilityDiagnosticRenderer
-    sealed class Incompatible<out D>(
-        val reason: String?,
-        val kind: IncompatibilityKind = IncompatibilityKind.WEAK
-    ) : ExpectActualCompatibility<D>() {
+    sealed class Incompatible<out D>(val reason: String?) : ExpectActualCompatibility<D>() {
+
+        sealed class WeakIncompatible<out D>(reason: String?) : Incompatible<D>(reason)
+
+        // For StrongIncompatible `actual` declaration is considered as overload and error reports on expected declaration
+        sealed class StrongIncompatible<out D>(reason: String?) : Incompatible<D>(reason)
+
        // Callables

-        object CallableKind : Incompatible<Nothing>(
-            "callable kinds are different (function vs property)",
-            IncompatibilityKind.STRONG
-        )
+        object CallableKind : StrongIncompatible<Nothing>("callable kinds are different (function vs property)")

-        object ParameterShape : Incompatible<Nothing>(
-            "parameter shapes are different (extension vs non-extension)",
-            IncompatibilityKind.STRONG
-        )
+        object ParameterShape : StrongIncompatible<Nothing>("parameter shapes are different (extension vs non-extension)")

-        object ParameterCount : Incompatible<Nothing>("number of value parameters is different", IncompatibilityKind.STRONG)
-        object TypeParameterCount : Incompatible<Nothing>("number of type parameters is different", IncompatibilityKind.STRONG)
+        object ParameterCount : StrongIncompatible<Nothing>("number of value parameters is different")
+        object TypeParameterCount : StrongIncompatible<Nothing>("number of type parameters is different")

-        object ParameterTypes : Incompatible<Nothing>("parameter types are different", IncompatibilityKind.STRONG)
-        object ReturnType : Incompatible<Nothing>("return type is different", IncompatibilityKind.STRONG)
+        object ParameterTypes : StrongIncompatible<Nothing>("parameter types are different")
+        object ReturnType : StrongIncompatible<Nothing>("return type is different")

-        object ParameterNames : Incompatible<Nothing>("parameter names are different")
-        object TypeParameterNames : Incompatible<Nothing>("names of type parameters are different")
+        object ParameterNames : WeakIncompatible<Nothing>("parameter names are different")
+        object TypeParameterNames : WeakIncompatible<Nothing>("names of type parameters are different")

-        object ValueParameterVararg : Incompatible<Nothing>("some value parameter is vararg in one declaration and non-vararg in the other")
-        object ValueParameterNoinline : Incompatible<Nothing>(
+        object ValueParameterVararg : WeakIncompatible<Nothing>("some value parameter is vararg in one declaration and non-vararg in the other")
+        object ValueParameterNoinline : WeakIncompatible<Nothing>(
            "some value parameter is noinline in one declaration and not noinline in the other"
        )

-        object ValueParameterCrossinline : Incompatible<Nothing>(
+        object ValueParameterCrossinline : WeakIncompatible<Nothing>(
            "some value parameter is crossinline in one declaration and not crossinline in the other"
        )

        // Functions

-        object FunctionModifiersDifferent : Incompatible<Nothing>("modifiers are different (suspend)")
-        object FunctionModifiersNotSubset : Incompatible<Nothing>(
+        object FunctionModifiersDifferent : WeakIncompatible<Nothing>("modifiers are different (suspend)")
+        object FunctionModifiersNotSubset : WeakIncompatible<Nothing>(
            "some modifiers on expected declaration are missing on the actual one (infix, inline, operator)"
        )
        object ActualFunctionWithDefaultParameters :
-            Incompatible<Nothing>("actual function cannot have default argument values, they should be declared in the expected function")
+            WeakIncompatible<Nothing>("actual function cannot have default argument values, they should be declared in the expected function")

        // Properties

-        object PropertyKind : Incompatible<Nothing>("property kinds are different (val vs var)")
-        object PropertyLateinitModifier : Incompatible<Nothing>("modifiers are different (lateinit)")
-        object PropertyConstModifier : Incompatible<Nothing>("modifiers are different (const)")
-        object PropertySetterVisibility : Incompatible<Nothing>("setter visibility is different")
+        object PropertyKind : WeakIncompatible<Nothing>("property kinds are different (val vs var)")
+        object PropertyLateinitModifier : WeakIncompatible<Nothing>("modifiers are different (lateinit)")
+        object PropertyConstModifier : WeakIncompatible<Nothing>("modifiers are different (const)")
+        object PropertySetterVisibility : WeakIncompatible<Nothing>("setter visibility is different")

        // Classifiers

-        object ClassKind : Incompatible<Nothing>("class kinds are different (class, interface, object, enum, annotation)")
+        object ClassKind : WeakIncompatible<Nothing>("class kinds are different (class, interface, object, enum, annotation)")

-        object ClassModifiers : Incompatible<Nothing>("modifiers are different (companion, inner, inline)")
+        object ClassModifiers : WeakIncompatible<Nothing>("modifiers are different (companion, inner, inline)")

-        object FunInterfaceModifier : Incompatible<Nothing>("actual declaration for fun expect interface is not a functional interface")
+        object FunInterfaceModifier : WeakIncompatible<Nothing>("actual declaration for fun expect interface is not a functional interface")

-        object Supertypes : Incompatible<Nothing>("some supertypes are missing in the actual declaration")
+        object Supertypes : WeakIncompatible<Nothing>("some supertypes are missing in the actual declaration")

        class ClassScopes<D>(
            val unfulfilled: List<Pair<D, Map<Incompatible<D>, Collection<D>>>>
-        ) : Incompatible<D>("some expected members have no actual ones")
+        ) : WeakIncompatible<D>("some expected members have no actual ones")

-        object EnumEntries : Incompatible<Nothing>("some entries from expected enum are missing in the actual enum")
+        object EnumEntries : WeakIncompatible<Nothing>("some entries from expected enum are missing in the actual enum")

        // Common

-        object Modality : Incompatible<Nothing>("modality is different")
-        object Visibility : Incompatible<Nothing>("visibility is different")
+        object Modality : WeakIncompatible<Nothing>("modality is different")
+        object Visibility : WeakIncompatible<Nothing>("visibility is different")

-        object TypeParameterUpperBounds : Incompatible<Nothing>("upper bounds of type parameters are different", IncompatibilityKind.STRONG)
-        object TypeParameterVariance : Incompatible<Nothing>("declaration-site variances of type parameters are different")
-        object TypeParameterReified : Incompatible<Nothing>(
+        object TypeParameterUpperBounds : StrongIncompatible<Nothing>("upper bounds of type parameters are different")
+        object TypeParameterVariance : WeakIncompatible<Nothing>("declaration-site variances of type parameters are different")
+        object TypeParameterReified : WeakIncompatible<Nothing>(
            "some type parameter is reified in one declaration and non-reified in the other"
        )

-        object Unknown : Incompatible<Nothing>(null)
+        object Unknown : WeakIncompatible<Nothing>(null)
    }

    object Compatible : ExpectActualCompatibility<Nothing>()