[FIR] Report all Visibilities.Unknown in FirOverrideChecker

Check all members for `Visibility.Unknown`, otherwise we miss them when they come from supertypes. This is the reason why the FP intellij build failed with a cryptic stacktrace instead of a human-readable diagnostic. Also, do report the diagnostic at all cases of `Visibilities.Unknown`. Turns out, there are no "simple to reason about" situations here :( Also, an interesting detail: `retrieveDirectOverriddenOf` returns an empty list for intersection overrides. But this doesn't seem to break anything... Replacing `CANNOT_INFER_VISIBILITY`'s type `KtDeclaration` with `PsiNameIdentifierOwner` and the related changes in `PositioningStrategies` were needed to prevent an exception saying that `PsiClassImpl` is not a subtype of `KtDeclaration`.
2024-02-20 14:20:22 +02:00
parent 053eb07692
commit 76ed5453b3
27 changed files with 149 additions and 46 deletions
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.analysis.api.fir.diagnostics

 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.KtPsiSourceElement
 import org.jetbrains.kotlin.diagnostics.KtPsiDiagnostic
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.analysis.api.fir.diagnostics

 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.analysis.api.diagnostics.KtDiagnosticWithPsi
 import org.jetbrains.kotlin.analysis.api.symbols.KtCallableSymbol
@@ -1927,7 +1928,7 @@ sealed interface KtFirDiagnostic<PSI : PsiElement> : KtDiagnosticWithPsi<PSI> {
        val containingClassName: Name
    }

-    interface CannotInferVisibility : KtFirDiagnostic<KtDeclaration> {
+    interface CannotInferVisibility : KtFirDiagnostic<PsiNameIdentifierOwner> {
        override val diagnosticClass get() = CannotInferVisibility::class
        val callable: KtCallableSymbol
    }
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.analysis.api.fir.diagnostics

 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.analysis.api.lifetime.KtLifetimeToken
 import org.jetbrains.kotlin.analysis.api.symbols.KtCallableSymbol
@@ -2319,7 +2320,7 @@ internal class CannotInferVisibilityImpl(
    override val callable: KtCallableSymbol,
    firDiagnostic: KtPsiDiagnostic,
    token: KtLifetimeToken,
-) : KtAbstractFirDiagnostic<KtDeclaration>(firDiagnostic, token), KtFirDiagnostic.CannotInferVisibility
+) : KtAbstractFirDiagnostic<PsiNameIdentifierOwner>(firDiagnostic, token), KtFirDiagnostic.CannotInferVisibility

 internal class MultipleDefaultsInheritedFromSupertypesImpl(
    override val name: Name,
@@ -11,7 +11,7 @@ open class B {
    open fun baz() {}
 }

-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C<!> : A, B() {
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C<!> : A, B() {
    override fun foo() {
        super.foo()

@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.fir.checkers.generator.diagnostics

 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.KtSourceElement
 import org.jetbrains.kotlin.builtins.functions.FunctionTypeKind
@@ -926,7 +927,7 @@ object DIAGNOSTICS_LIST : DiagnosticList("FirErrors") {
            parameter<FirCallableSymbol<*>>("overridden")
            parameter<Name>("containingClassName")
        }
-        val CANNOT_INFER_VISIBILITY by error<KtDeclaration>(PositioningStrategy.DECLARATION_NAME) {
+        val CANNOT_INFER_VISIBILITY by error<PsiNameIdentifierOwner>(PositioningStrategy.DECLARATION_NAME) {
            parameter<FirCallableSymbol<*>>("callable")
        }

@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.fir.analysis.diagnostics

 import com.intellij.psi.PsiElement
+import com.intellij.psi.PsiNameIdentifierOwner
 import com.intellij.psi.impl.source.tree.LeafPsiElement
 import org.jetbrains.kotlin.KtSourceElement
 import org.jetbrains.kotlin.builtins.functions.FunctionTypeKind
@@ -518,7 +519,7 @@ object FirErrors {
    val DATA_CLASS_OVERRIDE_DEFAULT_VALUES: KtDiagnosticFactory2<FirCallableSymbol<*>, FirClassSymbol<*>> by error2<KtElement, FirCallableSymbol<*>, FirClassSymbol<*>>(SourceElementPositioningStrategies.DATA_MODIFIER)
    val CANNOT_WEAKEN_ACCESS_PRIVILEGE: KtDiagnosticFactory3<Visibility, FirCallableSymbol<*>, Name> by error3<KtModifierListOwner, Visibility, FirCallableSymbol<*>, Name>(SourceElementPositioningStrategies.VISIBILITY_MODIFIER)
    val CANNOT_CHANGE_ACCESS_PRIVILEGE: KtDiagnosticFactory3<Visibility, FirCallableSymbol<*>, Name> by error3<KtModifierListOwner, Visibility, FirCallableSymbol<*>, Name>(SourceElementPositioningStrategies.VISIBILITY_MODIFIER)
-    val CANNOT_INFER_VISIBILITY: KtDiagnosticFactory1<FirCallableSymbol<*>> by error1<KtDeclaration, FirCallableSymbol<*>>(SourceElementPositioningStrategies.DECLARATION_NAME)
+    val CANNOT_INFER_VISIBILITY: KtDiagnosticFactory1<FirCallableSymbol<*>> by error1<PsiNameIdentifierOwner, FirCallableSymbol<*>>(SourceElementPositioningStrategies.DECLARATION_NAME)
    val MULTIPLE_DEFAULTS_INHERITED_FROM_SUPERTYPES: KtDiagnosticFactory3<Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>> by error3<KtElement, Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>>(SourceElementPositioningStrategies.DECLARATION_SIGNATURE_OR_DEFAULT)
    val MULTIPLE_DEFAULTS_INHERITED_FROM_SUPERTYPES_WHEN_NO_EXPLICIT_OVERRIDE: KtDiagnosticFactory3<Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>> by error3<KtElement, Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>>(SourceElementPositioningStrategies.DECLARATION_NAME)
    val MULTIPLE_DEFAULTS_INHERITED_FROM_SUPERTYPES_DEPRECATION: KtDiagnosticFactoryForDeprecation3<Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>> by deprecationError3<KtElement, Name, FirValueParameterSymbol, List<FirCallableSymbol<*>>>(ProhibitAllMultipleDefaultsInheritedFromSupertypes, SourceElementPositioningStrategies.DECLARATION_SIGNATURE_OR_DEFAULT)
@@ -7,6 +7,7 @@ package org.jetbrains.kotlin.fir.analysis.checkers.declaration

 import org.jetbrains.kotlin.KtFakeSourceElementKind
 import org.jetbrains.kotlin.KtRealSourceElementKind
+import org.jetbrains.kotlin.KtSourceElement
 import org.jetbrains.kotlin.builtins.StandardNames
 import org.jetbrains.kotlin.descriptors.Modality
 import org.jetbrains.kotlin.descriptors.Visibilities
@@ -141,17 +142,31 @@ sealed class FirOverrideChecker(mppKind: MppCheckerKind) : FirAbstractOverrideCh
        // Other kinds of fake overrides may also be incorrect, but not to that extent, so we can
        // check them more granularly. See the relevant comments.

-        firTypeScope.processAllProperties {
-            if (it.containingClassLookupTag() == declaration.symbol.toLookupTag() && !it.isSubstitutionOverride) {
+        fun checkMember(it: FirCallableSymbol<*>) {
+            val isFromThis = it.containingClassLookupTag() == declaration.symbol.toLookupTag()
+
+            if (isFromThis && !it.isSubstitutionOverride) {
                checkMember(it, declaration, reporter, typeCheckerState, firTypeScope, context)
+            } else {
+                val source = it.source?.takeIf { isFromThis } ?: declaration.source
+                it.ensureKnownVisibility(context, reporter, source)
            }
        }

-        firTypeScope.processAllFunctions {
-            if (it.containingClassLookupTag() == declaration.symbol.toLookupTag() && !it.isSubstitutionOverride) {
-                checkMember(it, declaration, reporter, typeCheckerState, firTypeScope, context)
-            }
-        }
+        firTypeScope.processAllProperties(::checkMember)
+        firTypeScope.processAllFunctions(::checkMember)
+    }
+
+    /**
+     * Returns `false` if [Visibilities.Unknown].
+     */
+    private fun FirCallableSymbol<*>.ensureKnownVisibility(
+        context: CheckerContext,
+        reporter: DiagnosticReporter,
+        source: KtSourceElement? = this.source,
+    ) = when {
+        visibility != Visibilities.Unknown -> true
+        else -> false.also { reporter.reportOn(source, FirErrors.CANNOT_INFER_VISIBILITY, this, context) }
    }

    private fun checkModality(
@@ -178,13 +193,7 @@ sealed class FirOverrideChecker(mppKind: MppCheckerKind) : FirAbstractOverrideCh
        overriddenSymbols: List<FirCallableSymbol<*>>,
        context: CheckerContext
    ) {
-        if (visibility == Visibilities.Unknown) {
-            // MANY_*_NOT_IMPLEMENTED implies CANNOT_INFER_VISIBILITY as per KT-63741
-            val isManyNotImplementedDiagnosticReported = overriddenSymbols.count { !it.isAbstract } != 1
-            val isSimpleToReasonAboutSituation = overriddenSymbols.none { it.isIntersectionOverride }
-            if (!(isManyNotImplementedDiagnosticReported && isSimpleToReasonAboutSituation)) {
-                reporter.reportOn(source, FirErrors.CANNOT_INFER_VISIBILITY, this, context)
-            }
+        if (!ensureKnownVisibility(context, reporter)) {
            return
        }

@@ -22,7 +22,7 @@ import org.jetbrains.kotlin.utils.addToStdlib.safeAs
 import org.jetbrains.kotlin.utils.sure

 object PositioningStrategies {
-    open class DeclarationHeader<T : KtDeclaration> : PositioningStrategy<T>() {
+    open class DeclarationHeader<T : PsiElement> : PositioningStrategy<T>() {
        override fun isValid(element: T): Boolean {
            if (element is KtNamedDeclaration &&
                element !is KtObjectDeclaration &&
@@ -142,8 +142,8 @@ object PositioningStrategies {
    }

    @JvmField
-    val DECLARATION_NAME: PositioningStrategy<KtNamedDeclaration> = object : DeclarationHeader<KtNamedDeclaration>() {
-        override fun mark(element: KtNamedDeclaration): List<TextRange> {
+    val DECLARATION_NAME: PositioningStrategy<PsiNameIdentifierOwner> = object : DeclarationHeader<PsiNameIdentifierOwner>() {
+        override fun mark(element: PsiNameIdentifierOwner): List<TextRange> {
            val nameIdentifier = element.nameIdentifier
            if (nameIdentifier != null) {
                if (element is KtClassOrObject) {
@@ -45,4 +45,4 @@ interface GI : G {
 }

 <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED("Class 'AG1'; a")!>class AG1<!>(val a: A, val g: G) : A by a, G by g
-<!MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED("Class 'AG2'; a")!>class AG2<!>() : AI, GI
+<!CANNOT_INFER_VISIBILITY("a"), MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED("Class 'AG2'; a")!>class AG2<!>() : AI, GI
@@ -0,0 +1,19 @@
+// !DIAGNOSTICS: -CONFLICTING_JVM_DECLARATIONS
+interface One {
+  public <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo() : Int
+  private fun boo() = 10
+}
+interface Two {
+  public <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo() : Int
+}
+
+interface OneImpl : One {
+  public override fun foo() = 1
+}
+interface TwoImpl : Two {
+  public override fun foo() = 2
+}
+
+<!CANNOT_INFER_VISIBILITY, MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class Test1<!>() : TwoImpl, OneImpl {}
+class Test2(a : One) : One by a, Two {}
+<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Test3<!>(a : One, b : Two) : Two by b, One by a {}
@@ -1,4 +1,3 @@
-// FIR_IDENTICAL
 // !DIAGNOSTICS: -CONFLICTING_JVM_DECLARATIONS
 interface One {
  public <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo() : Int
@@ -78,7 +78,7 @@ open class NWEH: NotDeprecated, WarningDeprecated, ErrorDeprecated, HiddenDeprec

 class WE2: WE()

-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class NWE2<!>: WE(), NotDeprecated
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class NWE2<!>: WE(), NotDeprecated

 class NWE3: WE(), NotDeprecated {
    override fun f() {
@@ -78,7 +78,7 @@ open class NWEH: NotDeprecated, WarningDeprecated, ErrorDeprecated, HiddenDeprec

 class WE2: WE()

-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class NWE2<!>: WE(), NotDeprecated
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class NWE2<!>: WE(), NotDeprecated

 class NWE3: WE(), NotDeprecated {
    override fun f() {
@@ -0,0 +1,34 @@
+// TARGET_BACKEND: JVM
+
+// FILE: IntCollection.java
+interface IntCollection<E> {
+    public boolean add(int key);
+}
+
+// FILE: IntSet.java
+interface IntSet extends IntCollection {
+    public default boolean add(Integer it) { return true; }
+
+    // from the supertype
+    // public boolean add(int key);
+}
+
+// FILE: AbstractCollection.java
+abstract class AbstractCollection<E> {
+    public boolean add(E it) { return true; }
+}
+
+// FILE: AbstractIntCollection.java
+abstract class AbstractIntCollection extends AbstractCollection<Integer> {
+    public boolean add(int it) { return true; }
+
+    // from the supertype
+    // public default boolen add(Integer it) { return true; }
+}
+
+// FILE: AbstractIntSet.java
+public abstract class AbstractIntSet extends AbstractIntCollection implements IntSet {}
+
+// FILE: Main.kt
+
+<!CANNOT_INFER_VISIBILITY!>class KotlinClass<!> : AbstractIntSet()
@@ -1,4 +1,3 @@
-// FIR_IDENTICAL
 // TARGET_BACKEND: JVM

 // FILE: IntCollection.java
@@ -21,7 +21,7 @@ open class C : A<Int>()

 interface D : B<Int>

-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class E<!> : C(), D
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class E<!> : C(), D

 fun main() {
    E().foo(42)
@@ -10,8 +10,8 @@
 expect open class C1()
 expect interface I1

-open <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Common1_1<!> : C1(), I1
-open <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Common1_2<!> : I1, C1()
+open <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Common1_1<!> : C1(), I1
+open <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Common1_2<!> : I1, C1()

 expect open class Expect1_1 : C1, I1
 expect open class Expect1_2 : I1, C1
@@ -37,8 +37,8 @@ actual interface I1 {
    fun f() {}
 }

-actual open <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Expect1_1<!> : C1(), I1
-actual open <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Expect1_2<!> : I1, C1()
+actual open <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Expect1_1<!> : C1(), I1
+actual open <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class Expect1_2<!> : I1, C1()


 actual abstract class C2 actual constructor() {
@@ -11,9 +11,9 @@
 expect interface S1
 expect interface S2

-open <!MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class A<!> : S1, S2
+open <!CANNOT_INFER_VISIBILITY, MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class A<!> : S1, S2

-class B : A()
+<!CANNOT_INFER_VISIBILITY!>class B<!> : A()

 // MODULE: jvm()()(common)
 // TARGET_PLATFORM: JVM
@@ -0,0 +1,11 @@
+interface A {
+    fun f(): String = "string"
+}
+
+open class B {
+    open fun f(): CharSequence = "charSequence"
+}
+
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C<!> : B(), A
+
+val obj: A = <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>object<!> : B(), A {}
@@ -1,4 +1,3 @@
-// FIR_IDENTICAL
 interface A {
    fun f(): String = "string"
 }
@@ -0,0 +1,17 @@
+package d
+
+interface A {
+    fun foo() = 1
+}
+
+interface B {
+    fun foo() = 2
+}
+
+open <!CANNOT_INFER_VISIBILITY, MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class C<!> : A, B {}
+
+interface E {
+    fun foo(): Int
+}
+
+<!CANNOT_INFER_VISIBILITY!>class D<!> : C() {}
@@ -1,4 +1,3 @@
-// FIR_IDENTICAL
 package d

 interface A {
@@ -20,9 +20,9 @@ open class CVar {

 <!MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class C1<!> : IVar, IVarDefault
 <!VAR_IMPLEMENTED_BY_INHERITED_VAL_WARNING!>class C2<!> : CVal(), IVar
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C3<!> : CVal(), IVarDefault
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C4<!> : CVal(), IVar, IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C3<!> : CVal(), IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C4<!> : CVal(), IVar, IVarDefault
 class C5 : CVar(), IVar
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C6<!> : CVar(), IVarDefault
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C7<!> : CVar(), IVar, IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C6<!> : CVar(), IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C7<!> : CVar(), IVar, IVarDefault
 <!VAR_OVERRIDDEN_BY_VAL_BY_DELEGATION!>class C8<!>(ival: IVal) : IVar, IVal by ival
@@ -20,9 +20,9 @@ open class CVar {

 <!MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class C1<!> : IVar, IVarDefault
 <!VAR_IMPLEMENTED_BY_INHERITED_VAL_ERROR!>class C2<!> : CVal(), IVar
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C3<!> : CVal(), IVarDefault
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C4<!> : CVal(), IVar, IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C3<!> : CVal(), IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C4<!> : CVal(), IVar, IVarDefault
 class C5 : CVar(), IVar
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C6<!> : CVar(), IVarDefault
-<!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C7<!> : CVar(), IVar, IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C6<!> : CVar(), IVarDefault
+<!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class C7<!> : CVar(), IVar, IVarDefault
 <!VAR_OVERRIDDEN_BY_VAL_BY_DELEGATION!>class C8<!>(ival: IVal) : IVar, IVal by ival
@@ -0,0 +1,13 @@
+// KT-302 Report an error when inheriting many implementations of the same member
+
+package kt302
+
+interface A {
+    open fun foo() {}
+}
+
+interface B {
+    open fun foo() {}
+}
+
+<!CANNOT_INFER_VISIBILITY, MANY_INTERFACES_MEMBER_NOT_IMPLEMENTED!>class C<!> : A, B {} //should be error here
@@ -1,4 +1,3 @@
-// FIR_IDENTICAL
 // KT-302 Report an error when inheriting many implementations of the same member

 package kt302
@@ -14,4 +14,4 @@ interface IS {
    fun replace(key: String, value: String): String? = null
 }

-abstract <!MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class ZS<!> : MyMap<String, String>(), IS
+abstract <!CANNOT_INFER_VISIBILITY, MANY_IMPL_MEMBER_NOT_IMPLEMENTED!>class ZS<!> : MyMap<String, String>(), IS