[FIR] Introduce RedundantVisibilityModifierChecker

2020-07-06 15:06:02 +03:00
parent 510ac66ce1
commit 4643d483a0
8 changed files with 264 additions and 1 deletions
@@ -0,0 +1,52 @@
+<!REDUNDANT_VISIBILITY_MODIFIER!>public<!> class C {
+    <!REDUNDANT_VISIBILITY_MODIFIER!>public<!> val foo: Int = 0
+
+    <!REDUNDANT_VISIBILITY_MODIFIER!>public<!> fun bar() {}
+
+}
+
+open class D {
+    protected open fun willRemainProtected() {
+    }
+
+    protected open fun willBecomePublic() {
+    }
+}
+
+class E : D() {
+    <!REDUNDANT_VISIBILITY_MODIFIER!>protected<!> override fun willRemainProtected() {
+    }
+
+    public override fun willBecomePublic() {
+    }
+}
+
+enum class F <!REDUNDANT_VISIBILITY_MODIFIER!>private<!> constructor(val x: Int) {
+    FIRST(42)
+}
+
+sealed class G constructor(val y: Int) {
+    <!REDUNDANT_VISIBILITY_MODIFIER!>private<!> constructor(): this(42)
+
+    object H : G()
+}
+
+interface I {
+    fun bar()
+}
+
+fun f() {
+    val i = object : I {
+        <!REDUNDANT_VISIBILITY_MODIFIER!>internal<!> var foo = 0
+        override fun bar() {}
+    }
+    i.foo = 1
+
+    class LocalClass {
+        <!REDUNDANT_VISIBILITY_MODIFIER!>internal<!> var foo = 0
+    }
+    LocalClass().foo = 1
+    <!REDUNDANT_VISIBILITY_MODIFIER!>public<!> var baz = 0
+}
+
+<!REDUNDANT_VISIBILITY_MODIFIER!>public<!> var baz = 0
@@ -1414,6 +1414,24 @@ public class FirDiagnosticsTestGenerated extends AbstractFirDiagnosticsTest {
        }
    }

+    @TestMetadata("compiler/fir/analysis-tests/testData/resolve/extendedCheckers")
+    @TestDataPath("$PROJECT_ROOT")
+    @RunWith(JUnit3RunnerWithInners.class)
+    public static class ExtendedCheckers extends AbstractFirDiagnosticsTest {
+        private void runTest(String testDataFilePath) throws Exception {
+            KotlinTestUtils.runTest(this::doTest, this, testDataFilePath);
+        }
+
+        public void testAllFilesPresentInExtendedCheckers() throws Exception {
+            KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/fir/analysis-tests/testData/resolve/extendedCheckers"), Pattern.compile("^([^.]+)\\.kt$"), null, true);
+        }
+
+        @TestMetadata("RedundantVisibilityModifierChecker.kt")
+        public void testRedundantVisibilityModifierChecker() throws Exception {
+            runTest("compiler/fir/analysis-tests/testData/resolve/extendedCheckers/RedundantVisibilityModifierChecker.kt");
+        }
+    }
+
    @TestMetadata("compiler/fir/analysis-tests/testData/resolve/fromBuilder")
    @TestDataPath("$PROJECT_ROOT")
    @RunWith(JUnit3RunnerWithInners.class)
@@ -1414,6 +1414,24 @@ public class FirDiagnosticsWithLightTreeTestGenerated extends AbstractFirDiagnos
        }
    }

+    @TestMetadata("compiler/fir/analysis-tests/testData/resolve/extendedCheckers")
+    @TestDataPath("$PROJECT_ROOT")
+    @RunWith(JUnit3RunnerWithInners.class)
+    public static class ExtendedCheckers extends AbstractFirDiagnosticsWithLightTreeTest {
+        private void runTest(String testDataFilePath) throws Exception {
+            KotlinTestUtils.runTest(this::doTest, this, testDataFilePath);
+        }
+
+        public void testAllFilesPresentInExtendedCheckers() throws Exception {
+            KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("compiler/fir/analysis-tests/testData/resolve/extendedCheckers"), Pattern.compile("^([^.]+)\\.kt$"), null, true);
+        }
+
+        @TestMetadata("RedundantVisibilityModifierChecker.kt")
+        public void testRedundantVisibilityModifierChecker() throws Exception {
+            runTest("compiler/fir/analysis-tests/testData/resolve/extendedCheckers/RedundantVisibilityModifierChecker.kt");
+        }
+    }
+
    @TestMetadata("compiler/fir/analysis-tests/testData/resolve/fromBuilder")
    @TestDataPath("$PROJECT_ROOT")
    @RunWith(JUnit3RunnerWithInners.class)
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.fir.analysis.checkers

 import org.jetbrains.kotlin.descriptors.ClassKind
+import org.jetbrains.kotlin.descriptors.Modality
 import org.jetbrains.kotlin.fir.FirSession
 import org.jetbrains.kotlin.fir.FirSymbolOwner
 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
@@ -15,12 +16,19 @@ import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.resolve.firSymbolProvider
 import org.jetbrains.kotlin.fir.resolve.toSymbol
 import org.jetbrains.kotlin.fir.resolve.transformers.firClassLike
+import org.jetbrains.kotlin.fir.scopes.FirTypeScope
+import org.jetbrains.kotlin.fir.scopes.ProcessorAction
+import org.jetbrains.kotlin.fir.scopes.unsubstitutedScope
 import org.jetbrains.kotlin.fir.symbols.impl.FirCallableSymbol
+import org.jetbrains.kotlin.fir.symbols.impl.FirFunctionSymbol
 import org.jetbrains.kotlin.fir.symbols.impl.FirRegularClassSymbol
 import org.jetbrains.kotlin.fir.types.ConeClassLikeType
 import org.jetbrains.kotlin.fir.types.ConeKotlinType
 import org.jetbrains.kotlin.fir.types.FirResolvedTypeRef
 import org.jetbrains.kotlin.fir.types.FirTypeRef
+import org.jetbrains.kotlin.psi.KtModifierList
+import org.jetbrains.kotlin.psi.psiUtil.toVisibility
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierType
 import org.jetbrains.kotlin.utils.addToStdlib.safeAs

 /**
@@ -185,3 +193,40 @@ fun CheckerContext.findClosestClassOrObject(): FirClass<*>? {

    return null
 }
+
+/**
+ * Returns the list of functions that overridden by given
+ */
+fun FirSimpleFunction.overriddenFunctions(
+    containingClass: FirClass<*>,
+    context: CheckerContext
+): List<FirFunctionSymbol<*>> {
+    val firTypeScope = containingClass.unsubstitutedScope(
+        context.sessionHolder.session,
+        context.sessionHolder.scopeSession
+    ) as FirTypeScope
+
+    val overriddenFunctions = mutableListOf<FirFunctionSymbol<*>>()
+    firTypeScope.processFunctionsByName(symbol.fir.name) { }
+    firTypeScope.processOverriddenFunctions(symbol) {
+        overriddenFunctions.add(it)
+        ProcessorAction.NEXT
+    }
+
+    return overriddenFunctions
+}
+
+/**
+ * Returns the visibility by given KtModifierList
+ */
+fun KtModifierList?.getVisibility() = this?.visibilityModifierType()?.toVisibility()
+
+/**
+ * Returns the modality of the class
+ */
+fun FirClass<*>.modality(): Modality? {
+    return when (this) {
+        is FirRegularClass -> modality
+        else -> Modality.FINAL
+    }
+}
@@ -5,10 +5,13 @@

 package org.jetbrains.kotlin.fir.analysis.checkers.declaration

+import org.jetbrains.kotlin.fir.analysis.checkers.extended.RedundantVisibilityModifierChecker
+
 object CommonDeclarationCheckers : DeclarationCheckers() {
    override val declarationCheckers: List<FirBasicDeclarationChecker> = listOf(
        FirAnnotationClassDeclarationChecker,
-        FirModifierChecker
+        FirModifierChecker,
+        RedundantVisibilityModifierChecker
    )

    override val memberDeclarationCheckers: List<FirMemberDeclarationChecker> = listOf(
@@ -0,0 +1,107 @@
+/*
+ * Copyright 2010-2020 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.extended
+
+import org.jetbrains.kotlin.descriptors.ClassKind
+import org.jetbrains.kotlin.descriptors.Modality
+import org.jetbrains.kotlin.descriptors.Visibilities
+import org.jetbrains.kotlin.descriptors.Visibility
+import org.jetbrains.kotlin.fir.FirFakeSourceElement
+import org.jetbrains.kotlin.fir.analysis.checkers.*
+import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
+import org.jetbrains.kotlin.fir.analysis.checkers.declaration.FirBasicDeclarationChecker
+import org.jetbrains.kotlin.fir.analysis.diagnostics.DiagnosticReporter
+import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
+import org.jetbrains.kotlin.fir.declarations.*
+import org.jetbrains.kotlin.fir.psi
+import org.jetbrains.kotlin.fir.toFirPsiSourceElement
+import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.psi.KtModifierList
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifier
+import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierType
+
+object RedundantVisibilityModifierChecker : FirBasicDeclarationChecker() {
+    override fun check(declaration: FirDeclaration, context: CheckerContext, reporter: DiagnosticReporter) {
+        if (declaration is FirPropertyAccessor && declaration.isGetter) return
+        if (declaration !is FirMemberDeclaration) return
+        if (declaration is FirConstructor && declaration.source is FirFakeSourceElement<*>) return
+
+        val modifierList = (declaration.source.getModifierList() as? FirPsiModifierList)?.modifierList ?: return
+        val visibilityModifier = modifierList.getVisibility()
+        val implicitVisibility = declaration.implicitVisibility(context)
+        val containingMemberDeclaration = context.containingDeclarations.getOrNull(1) as? FirMemberDeclaration
+
+        val redundantVisibility = when {
+            visibilityModifier == implicitVisibility -> implicitVisibility
+            visibilityModifier == Visibilities.INTERNAL
+                    && containingMemberDeclaration?.visibility == Visibilities.PRIVATE
+                    || declaration.visibility == Visibilities.INTERNAL -> Visibilities.INTERNAL
+            else -> null
+        } ?: return
+
+        if (redundantVisibility == Visibilities.PUBLIC
+            && declaration is FirProperty
+            && declaration.isVar
+            && declaration.setter?.visibility.let { it != null && it != Visibilities.PUBLIC }
+        ) return
+
+        val source = modifierList.visibilityModifier()?.toFirPsiSourceElement()
+        reporter.report(source, FirErrors.REDUNDANT_VISIBILITY_MODIFIER)
+    }
+
+    private fun FirDeclaration.implicitVisibility(context: CheckerContext): Visibility {
+        return when {
+            this is FirPropertyAccessor && isSetter && status.isOverride -> {
+                this.visibility
+            }
+
+            this is FirConstructor -> {
+                val clazz = this.getContainingClass(context)
+                if (
+                    clazz is FirClass
+                    && (clazz.classKind == ClassKind.ENUM_CLASS || clazz.modality() == Modality.SEALED)
+                ) {
+                    return Visibilities.PRIVATE
+                } else {
+                    Visibilities.DEFAULT_VISIBILITY
+                }
+            }
+
+            this is FirSimpleFunction
+                    && context.containingDeclarations.getOrNull(1) is FirClass<*>
+                    && this.isOverride -> findFunctionVisibility(this, context)
+
+            else -> Visibilities.DEFAULT_VISIBILITY
+        }
+    }
+
+    private fun findFunctionVisibility(function: FirSimpleFunction, context: CheckerContext): Visibility {
+        val currentClass = context.findClosestClassOrObject() ?: return Visibilities.UNKNOWN
+        val overriddenFunctions = function.overriddenFunctions(currentClass, context)
+        var visibility: Visibility = Visibilities.PRIVATE
+        for (func in overriddenFunctions) {
+            val currentVisibility = func.fir.visibility()
+            if (currentVisibility != null) {
+                val compareResult = Visibilities.compare(currentVisibility, visibility)
+                if (compareResult != null && compareResult > 0) {
+                    visibility = currentVisibility
+                }
+            }
+        }
+
+        return visibility
+    }
+
+    private fun FirFunction<*>.visibility(): Visibility? {
+        (symbol.fir as? FirMemberDeclaration)?.visibility?.let {
+            return it
+        }
+        (symbol.fir as? FirPropertyAccessor)?.visibility?.let {
+            return it
+        }
+        return null
+    }
+}
@@ -0,0 +1,18 @@
+/*
+ * Copyright 2010-2020 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.extended
+
+import com.intellij.psi.PsiElement
+import org.jetbrains.kotlin.fir.FirSourceElement
+import org.jetbrains.kotlin.fir.analysis.diagnostics.DiagnosticReporter
+import org.jetbrains.kotlin.fir.analysis.diagnostics.FirDiagnosticFactory0
+
+inline fun <reified T : FirSourceElement, P : PsiElement> DiagnosticReporter.report(
+    source: T?,
+    factory: FirDiagnosticFactory0<T, P>
+) {
+    source?.let { report(factory.on(it)) }
+}
@@ -13,6 +13,7 @@ import org.jetbrains.kotlin.diagnostics.Errors
 import org.jetbrains.kotlin.fir.DeclarationWithRelation
 import org.jetbrains.kotlin.fir.FirEffectiveVisibility
 import org.jetbrains.kotlin.fir.FirSourceElement
+import org.jetbrains.kotlin.fir.declarations.FirDeclaration
 import org.jetbrains.kotlin.fir.symbols.AbstractFirBasedSymbol
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
 import org.jetbrains.kotlin.fir.types.ConeKotlinType
@@ -48,6 +49,7 @@ object FirErrors {
    val SUPERCLASS_NOT_ACCESSIBLE_FROM_INTERFACE by error0<FirSourceElement, PsiElement>()
    val ABSTRACT_SUPER_CALL by error0<FirSourceElement, PsiElement>()
    val QUALIFIED_SUPERTYPE_EXTENDED_BY_OTHER_SUPERTYPE by error0<FirSourceElement, PsiElement>()
+    val REDUNDANT_VISIBILITY_MODIFIER by error0<FirSourceElement, PsiElement>()

    val INAPPLICABLE_INFIX_MODIFIER by existing<FirSourceElement, PsiElement, String>(Errors.INAPPLICABLE_INFIX_MODIFIER)
    val CONSTRUCTOR_IN_OBJECT by existing<FirSourceElement, KtDeclaration>(Errors.CONSTRUCTOR_IN_OBJECT)