FIR checker: warn redundant open in interface members properly

compiler/fir/analysis-tests/testData/resolve/openInInterface.kt

@@ -1,12 +1,12 @@
 interface Some {
-    open fun foo()
+    <!REDUNDANT_OPEN_IN_INTERFACE{LT}!><!REDUNDANT_OPEN_IN_INTERFACE{PSI}!>open<!> fun foo()<!>
     open fun bar() {}
 
-    open val x: Int
+    <!REDUNDANT_OPEN_IN_INTERFACE{LT}!><!REDUNDANT_OPEN_IN_INTERFACE{PSI}!>open<!> val x: Int<!>
     open val y = <!PROPERTY_INITIALIZER_IN_INTERFACE!>1<!>
     open val z get() = 1
 
-    open var xx: Int
+    <!REDUNDANT_OPEN_IN_INTERFACE{LT}!><!REDUNDANT_OPEN_IN_INTERFACE{PSI}!>open<!> var xx: Int<!>
     open var yy = <!PROPERTY_INITIALIZER_IN_INTERFACE!>1<!>
     open var zz: Int
         set(value) {

compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirDeclarationCheckerUtils.kt

@@ -0,0 +1,14 @@
+/*
+ * Copyright 2010-2021 JetBrains s.r.o. and Kotlin Programming Language contributors.
+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+package org.jetbrains.kotlin.fir.analysis.checkers.declaration
+
+import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
+import org.jetbrains.kotlin.fir.declarations.FirRegularClass
+import org.jetbrains.kotlin.fir.declarations.isExpect
+
+// Note that the class that contains the currently visiting declaration will *not* be in the context's containing declarations *yet*.
+internal fun isInsideExpectClass(containingDeclaration: FirRegularClass, context: CheckerContext): Boolean =
+    containingDeclaration.isExpect || context.containingDeclarations.asReversed().any { it is FirRegularClass && it.isExpect }

compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberFunctionChecker.kt

@@ -35,7 +35,8 @@ object FirMemberFunctionChecker : FirRegularClassChecker() {
         // If multiple (potentially conflicting) modality modifiers are specified, not all modifiers are recorded at `status`.
         // So, our source of truth should be the full modifier list retrieved from the source.
         val modifierList = with(FirModifierList) { source.getModifierList() }
-        val isAbstract = function.isAbstract || modifierList?.modifiers?.any { it.token == KtTokens.ABSTRACT_KEYWORD } == true
+        val hasAbstractModifier = modifierList?.modifiers?.any { it.token == KtTokens.ABSTRACT_KEYWORD } == true
+        val isAbstract = function.isAbstract || hasAbstractModifier
         if (isAbstract) {
             if (!containingDeclaration.canHaveAbstractDeclaration) {
                 reporter.report(source, FirErrors.ABSTRACT_FUNCTION_IN_NON_ABSTRACT_CLASS)
@@ -45,25 +46,20 @@ object FirMemberFunctionChecker : FirRegularClassChecker() {
             }
         }
         val isInsideExpectClass = isInsideExpectClass(containingDeclaration, context)
-        val isOpen = function.isOpen || modifierList?.modifiers?.any { it.token == KtTokens.OPEN_KEYWORD } == true
+        val hasOpenModifier = modifierList?.modifiers?.any { it.token == KtTokens.OPEN_KEYWORD } == true
         val isExternal = function.isExternal || modifierList?.modifiers?.any { it.token == KtTokens.EXTERNAL_KEYWORD } == true
         if (!function.hasBody) {
             if (containingDeclaration.isInterface) {
                 if (Visibilities.isPrivate(function.visibility)) {
                     reporter.report(source, FirErrors.PRIVATE_FUNCTION_WITH_NO_BODY)
                 }
-                if (!isInsideExpectClass && !isAbstract && isOpen) {
+                if (!isInsideExpectClass && !hasAbstractModifier && hasOpenModifier) {
                     reporter.report(source, FirErrors.REDUNDANT_OPEN_IN_INTERFACE)
                 }
-            } else if (!isInsideExpectClass && !isAbstract && !isExternal) {
+            } else if (!isInsideExpectClass && !hasAbstractModifier && !isExternal) {
-                // TODO: we need to check if modifiers of the function already get some errors, e.g., INCOMPATIBLE_MODIFIERS
                 reporter.report(FirErrors.NON_ABSTRACT_FUNCTION_WITH_NO_BODY.on(source, function.symbol))
             }
         }
     }
 
-    private fun isInsideExpectClass(containingDeclaration: FirRegularClass, context: CheckerContext): Boolean =
-        // Note that the class that contains the currently visiting function is *not* in the context's containing declarations *yet*.
-        containingDeclaration.isExpect || context.containingDeclarations.asReversed().any { it is FirRegularClass && it.isExpect }
-
 }

compiler/fir/checkers/src/org/jetbrains/kotlin/fir/analysis/checkers/declaration/FirMemberPropertyChecker.kt

@@ -23,16 +23,22 @@ object FirMemberPropertyChecker : FirRegularClassChecker() {
     override fun check(declaration: FirRegularClass, context: CheckerContext, reporter: DiagnosticReporter) {
         for (member in declaration.declarations) {
             if (member is FirProperty) {
-                checkProperty(declaration, member, reporter)
+                checkProperty(declaration, member, context, reporter)
             }
         }
     }
 
-    private fun checkProperty(containingDeclaration: FirRegularClass, property: FirProperty, reporter: DiagnosticReporter) {
+    private fun checkProperty(
+        containingDeclaration: FirRegularClass,
+        property: FirProperty,
+        context: CheckerContext,
+        reporter: DiagnosticReporter
+    ) {
         // If multiple (potentially conflicting) modality modifiers are specified, not all modifiers are recorded at `status`.
         // So, our source of truth should be the full modifier list retrieved from the source.
         val modifierList = with(FirModifierList) { property.source.getModifierList() }
-        val isAbstract = property.isAbstract || modifierList?.modifiers?.any { it.token == KtTokens.ABSTRACT_KEYWORD } == true
+        val hasAbstractModifier = modifierList?.modifiers?.any { it.token == KtTokens.ABSTRACT_KEYWORD } == true
+        val isAbstract = property.isAbstract || hasAbstractModifier
         if (containingDeclaration.isInterface &&
             Visibilities.isPrivate(property.visibility) &&
             !isAbstract &&
@@ -75,7 +81,18 @@ object FirMemberPropertyChecker : FirRegularClassChecker() {
 
         checkPropertyInitializer(containingDeclaration, property, isAbstract, reporter)
 
-        val isOpen = property.isOpen || modifierList?.modifiers?.any { it.token == KtTokens.OPEN_KEYWORD } == true
+        val hasOpenModifier = modifierList?.modifiers?.any { it.token == KtTokens.OPEN_KEYWORD } == true
+        if (hasOpenModifier &&
+            containingDeclaration.isInterface &&
+            !hasAbstractModifier &&
+            property.isAbstract &&
+            !isInsideExpectClass(containingDeclaration, context)
+        ) {
+            property.source?.let {
+                reporter.report(it, FirErrors.REDUNDANT_OPEN_IN_INTERFACE)
+            }
+        }
+        val isOpen = property.isOpen || hasOpenModifier
         if (isOpen) {
             checkAccessor(property.setter, property.delegate) { src, symbol ->
                 if (symbol.fir.visibility == Visibilities.Private && property.visibility != Visibilities.Private) {

compiler/testData/diagnostics/tests/delegation/Delegation_MultipleDelegates.fir.kt

@@ -1,10 +1,10 @@
 // !DIAGNOSTICS: -CONFLICTING_JVM_DECLARATIONS
 interface One {
-  public open fun foo() : Int
+  public <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo() : Int
   private fun boo() = 10
 }
 interface Two {
-  public open fun foo() : Int
+  public <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo() : Int
 }
 
 interface OneImpl : One {

compiler/testData/diagnostics/tests/modifiers/openInInterface.fir.kt

@@ -1,10 +0,0 @@
-interface My {
-    open fun foo()
-    open fun bar() {}
-    <!REDUNDANT_MODIFIER!>open<!> abstract fun baz(): Int
-
-    open val x: Int
-    open val y: String
-        get() = ""
-    <!REDUNDANT_MODIFIER!>open<!> abstract val z: Double
-}

compiler/testData/diagnostics/tests/modifiers/openInInterface.kt

@@ -1,3 +1,4 @@
+// FIR_IDENTICAL
 interface My {
     <!REDUNDANT_OPEN_IN_INTERFACE!>open<!> fun foo()
     open fun bar() {}

compiler/testData/diagnostics/tests/override/kt880.fir.kt

@@ -1,16 +0,0 @@
-// KT-880 Overload resolution ambiguity
-
-public interface I {
-    open fun test() : Unit
-}
-
-abstract public class A() {
-    open public fun test() : Unit {
-    }
-}
-
-public open class T() : A(), I {
-    open fun main() : Unit {
-        test() // Test no "Overload resolution ambiguity" is here
-    }
-}

compiler/testData/diagnostics/tests/override/kt880.kt

@@ -1,3 +1,4 @@
+// FIR_IDENTICAL
 // KT-880 Overload resolution ambiguity
 
 public interface I {