KT-7715 Highlight var's that can be replaced by val's

#KT-7715 Fixed
2016-03-04 16:27:01 +03:00
parent e5b5a8db42
commit 28c5dde875
28 changed files with 350 additions and 15 deletions
@@ -53,6 +53,11 @@ public class KtDestructuringDeclaration extends KtDeclarationImpl implements KtV
        return PsiTreeUtil.getNextSiblingOfType(eqNode.getPsi(), KtExpression.class);
    }

+    public boolean isVar() {
+        return getNode().findChildByType(KtTokens.VAR_KEYWORD) != null;
+    }
+
+    @Override
    @Nullable
    public PsiElement getValOrVarKeyword() {
        return findChildByType(TokenSet.create(VAL_KEYWORD, VAR_KEYWORD));
@@ -0,0 +1,5 @@
+<html>
+<body>
+This inspection reports mutable local variables (declared with 'var' keyword) that can be made immutable.
+</body>
+</html>
@@ -1391,6 +1391,13 @@
                     cleanupTool="false"
                     level="WEAK WARNING"/>

+    <localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.CanBeValInspection"
+                     displayName="Local 'var' can be declared as 'val'"
+                     groupName="Kotlin"
+                     enabledByDefault="true"
+                     level="WARNING"
+    />
+
    <referenceImporter implementation="org.jetbrains.kotlin.idea.quickfix.KotlinReferenceImporter"/>

    <fileType.fileViewProviderFactory filetype="KJSM" implementationClass="com.intellij.psi.ClassFileViewProviderFactory"/>
@@ -0,0 +1,129 @@
+/*
+ * Copyright 2010-2016 JetBrains s.r.o.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.jetbrains.kotlin.idea.inspections
+
+import com.intellij.codeInspection.IntentionWrapper
+import com.intellij.codeInspection.ProblemHighlightType
+import com.intellij.codeInspection.ProblemsHolder
+import com.intellij.psi.PsiElementVisitor
+import com.intellij.psi.search.searches.ReferencesSearch
+import org.jetbrains.kotlin.cfg.pseudocode.Pseudocode
+import org.jetbrains.kotlin.cfg.pseudocode.PseudocodeUtil
+import org.jetbrains.kotlin.cfg.pseudocode.containingDeclarationForPseudocode
+import org.jetbrains.kotlin.cfg.pseudocode.instructions.Instruction
+import org.jetbrains.kotlin.cfg.pseudocode.instructions.InstructionWithNext
+import org.jetbrains.kotlin.cfg.pseudocode.instructions.eval.AccessTarget
+import org.jetbrains.kotlin.cfg.pseudocode.instructions.eval.WriteValueInstruction
+import org.jetbrains.kotlin.idea.caches.resolve.analyze
+import org.jetbrains.kotlin.idea.quickfix.ChangeVariableMutabilityFix
+import org.jetbrains.kotlin.idea.references.KtSimpleNameReference
+import org.jetbrains.kotlin.idea.references.readWriteAccess
+import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.lazy.BodyResolveMode
+import java.util.*
+
+class CanBeValInspection : AbstractKotlinInspection() {
+    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor {
+        return object: KtVisitorVoid() {
+            private val pseudocodeCache = HashMap<KtDeclaration, Pseudocode>()
+
+            override fun visitDeclaration(declaration: KtDeclaration) {
+                super.visitDeclaration(declaration)
+
+                when (declaration) {
+                    is KtProperty -> {
+                        if (declaration.isVar && declaration.isLocal && canBeVal(declaration, declaration.hasInitializer(), listOf(declaration))) {
+                            reportCanBeVal(declaration)
+                        }
+                    }
+
+                    is KtDestructuringDeclaration -> {
+                        val entries = declaration.entries
+                        if (declaration.isVar && entries.all { canBeVal(it, true, entries) }) {
+                            reportCanBeVal(declaration)
+                        }
+                    }
+                }
+            }
+
+            private fun canBeVal(declaration: KtVariableDeclaration, hasInitializer: Boolean, allDeclarations: Collection<KtVariableDeclaration>): Boolean {
+                if (allDeclarations.all { ReferencesSearch.search(it, it.useScope).none() }) {
+                    // do not report for unused var's (otherwise we'll get it highlighted immediately after typing the declaration
+                    return false
+                }
+
+                if (hasInitializer) {
+                    val hasWriteUsages = ReferencesSearch.search(declaration, declaration.useScope).any {
+                        (it as? KtSimpleNameReference)?.element?.readWriteAccess(useResolveForReadWrite = true)?.isWrite == true
+                    }
+                    return !hasWriteUsages
+                }
+                else {
+                    val bindingContext = declaration.analyze(BodyResolveMode.FULL)
+                    val pseudocode = pseudocode(declaration, bindingContext) ?: return false
+                    val descriptor = bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, declaration] ?: return false
+
+                    val writeInstructions = pseudocode.instructionsIncludingDeadCode
+                            .filterIsInstance<WriteValueInstruction>()
+                            .filter { (it.target as? AccessTarget.Call)?.resolvedCall?.resultingDescriptor == descriptor }
+                            .toSet()
+                    if (writeInstructions.isEmpty()) return false // incorrect code - do not report
+
+                    return writeInstructions.none { canReach(it, writeInstructions) }
+                }
+            }
+
+            private fun pseudocode(element: KtElement, bindingContext: BindingContext): Pseudocode? {
+                val declaration = element.containingDeclarationForPseudocode ?: return null
+                return pseudocodeCache.getOrPut(declaration) { PseudocodeUtil.generatePseudocode(declaration, bindingContext) }
+            }
+
+            private fun canReach(from: Instruction, targets: Set<Instruction>, visited: HashSet<Instruction> = HashSet<Instruction>()): Boolean {
+                // special algorithm for linear code to avoid too deep recursion
+                var instruction = from
+                while (instruction is InstructionWithNext) {
+                    val next = instruction.next ?: return false
+                    if (next in visited) continue
+                    if (next in targets) return true
+                    visited.add(next)
+                    instruction = next
+                }
+
+                for (next in instruction.nextInstructions) {
+                    if (next in visited) continue
+                    if (next in targets) return true
+                    visited.add(next)
+                    if (canReach(next, targets)) return true
+                }
+                return false
+            }
+
+            private fun reportCanBeVal(declaration: KtValVarKeywordOwner) {
+                val problemDescriptor = holder.manager.createProblemDescriptor(
+                        declaration,
+                        declaration.valOrVarKeyword!!,
+                        "Can be declared as 'val'",
+                        ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                        isOnTheFly,
+                        IntentionWrapper(ChangeVariableMutabilityFix(declaration, false), declaration.containingFile)
+                )
+                holder.registerProblem(problemDescriptor)
+            }
+        }
+    }
+}
@@ -24,32 +24,23 @@ import org.jetbrains.kotlin.diagnostics.Diagnostic
 import org.jetbrains.kotlin.diagnostics.Errors
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.*
-import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
 import org.jetbrains.kotlin.resolve.DescriptorToSourceUtils

-class ChangeVariableMutabilityFix(element: KtNamedDeclaration, private val makeVar: Boolean) : KotlinQuickFixAction<KtNamedDeclaration>(element) {
+class ChangeVariableMutabilityFix(element: KtValVarKeywordOwner, private val makeVar: Boolean) : KotlinQuickFixAction<KtValVarKeywordOwner>(element) {

    override fun getText() = if (makeVar) "Make variable mutable" else "Make variable immutable"

    override fun getFamilyName(): String = text

    override fun isAvailable(project: Project, editor: Editor?, file: PsiFile): Boolean {
-        return when (element) {
-            is KtProperty -> element.isVar != makeVar
-            is KtParameter ->
-                element.getStrictParentOfType<KtPrimaryConstructor>()?.valueParameterList == element.parent
-                && (element.valOrVarKeyword == KtTokens.VAR_KEYWORD) != makeVar
-            else -> false
-        }
+        val valOrVar = element.valOrVarKeyword?.node?.elementType ?: return false
+        return (valOrVar == KtTokens.VAR_KEYWORD) != makeVar
    }

    override fun invoke(project: Project, editor: Editor?, file: KtFile) {
        val factory = KtPsiFactory(project)
        val newKeyword = if (makeVar) factory.createVarKeyword() else factory.createValKeyword()
-        when (element) {
-            is KtProperty -> element.valOrVarKeyword.replace(newKeyword)
-            is KtParameter -> element.valOrVarKeyword!!.replace(newKeyword)
-        }
+        element.valOrVarKeyword!!.replace(newKeyword)
    }

    companion object {
@@ -63,7 +54,7 @@ class ChangeVariableMutabilityFix(element: KtNamedDeclaration, private val makeV
        val VAL_REASSIGNMENT_FACTORY: KotlinSingleIntentionActionFactory = object: KotlinSingleIntentionActionFactory() {
            override fun createAction(diagnostic: Diagnostic): IntentionAction? {
                val propertyDescriptor = Errors.VAL_REASSIGNMENT.cast(diagnostic).a
-                val declaration = DescriptorToSourceUtils.descriptorToDeclaration(propertyDescriptor) as? KtNamedDeclaration ?: return null
+                val declaration = DescriptorToSourceUtils.descriptorToDeclaration(propertyDescriptor) as? KtValVarKeywordOwner ?: return null
                return ChangeVariableMutabilityFix(declaration, true)
            }
        }
@@ -72,7 +63,7 @@ class ChangeVariableMutabilityFix(element: KtNamedDeclaration, private val makeV
            override fun createAction(diagnostic: Diagnostic): IntentionAction? {
                val element = diagnostic.psiElement
                return when (element) {
-                    is KtProperty, is KtParameter -> ChangeVariableMutabilityFix(element as KtNamedDeclaration, true)
+                    is KtProperty, is KtParameter -> ChangeVariableMutabilityFix(element as KtValVarKeywordOwner, true)
                    else -> null
                }
            }
@@ -0,0 +1,6 @@
+fun foo(p: Int) {
+    var v1: Int
+    var v2: Int = 0
+    if (p > 0) v1 = 1 else v1 = 2
+    v2 = 1
+}
@@ -0,0 +1,6 @@
+fun foo(p: Int) {
+    var v: Int
+    for (i in 1..10) {
+        v = i
+    }
+}
@@ -0,0 +1,5 @@
+fun foo(p: Int) {
+    var v: Int
+    v = 0
+    if (p > 0) v = 1
+}
@@ -0,0 +1,6 @@
+fun foo(p: Int) {
+    var (v1, v2) = getPair()
+    print(v1)
+}
+
+fun getPair(): Pair<Int, String> = 1 to ""
@@ -0,0 +1,7 @@
+fun foo(p: Int) {
+    var (v1, v2) = getPair()
+    print(v1)
+    v2 = ""
+}
+
+fun getPair(): Pair<Int, String> = 1 to ""
@@ -0,0 +1,5 @@
+fun foo(p: Int) {
+    var (v1, v2) = getPair()
+}
+
+fun getPair(): Pair<Int, String> = 1 to ""
@@ -0,0 +1,55 @@
+<problems>
+    <problem>
+        <file>withInitializer.kt</file>
+        <line>4</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="withInitializer.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+
+    <problem>
+        <file>alwaysAssigned.kt</file>
+        <line>2</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="alwaysAssigned.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+
+    <problem>
+        <file>notAssignedWhenNotUsed.kt</file>
+        <line>2</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="notAssignedWhenNotUsed.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+
+    <problem>
+        <file>twoVariables.kt</file>
+        <line>2</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="twoVariables.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+
+    <problem>
+        <file>twoVariables.kt</file>
+        <line>3</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="twoVariables.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+
+    <problem>
+        <file>desctructuringDeclaration1.kt</file>
+        <line>2</line>
+        <module>light_idea_test_case</module>
+        <entry_point TYPE="file" FQNAME="desctructuringDeclaration1.kt" />
+        <problem_class severity="WARNING" attribute_key="WARNING_ATTRIBUTES">Local 'var' can be declared as 'val'</problem_class>
+        <description>Can be declared as 'val'</description>
+    </problem>
+</problems>
@@ -0,0 +1 @@
+// INSPECTION_CLASS: org.jetbrains.kotlin.idea.inspections.CanBeValInspection
@@ -0,0 +1,7 @@
+fun foo(p: Int) {
+    var v: Int
+    if (p > 0) {
+        v = 1
+        print(v)
+    }
+}
@@ -0,0 +1,10 @@
+var global = 1
+
+class C {
+    var field = 2
+
+    fun foo() {
+        print(field)
+        print(global)
+    }
+}
@@ -0,0 +1,4 @@
+fun foo() {
+    val v = 1
+    print(v)
+}
@@ -0,0 +1,6 @@
+fun foo(p: Int) {
+    var v1: Int
+    var v2: Int
+    if (p > 0) v1 = 1 else v1 = 2
+    v2 = 1
+}
@@ -0,0 +1,3 @@
+fun foo() {
+    var v: Int
+}
@@ -0,0 +1,3 @@
+fun foo() {
+    var v = 1
+}
@@ -0,0 +1,8 @@
+fun foo() {
+    var v1 = 1
+    var v2 = 2
+    var v3 = 3
+    v1 = 1
+    v2++
+    print(v3)
+}
@@ -0,0 +1 @@
+org.jetbrains.kotlin.idea.inspections.CanBeValInspection
@@ -0,0 +1,9 @@
+// "Make variable immutable" "true"
+fun foo(p: Int) {
+    <caret>var (v1, v2) = getPair()!!
+    v1
+}
+
+fun getPair(): Pair<Int, String>? = null
+
+data class Pair<T1, T2>(val a: T1, val b: T2)
@@ -0,0 +1,9 @@
+// "Make variable immutable" "true"
+fun foo(p: Int) {
+    <caret>val (v1, v2) = getPair()!!
+    v1
+}
+
+fun getPair(): Pair<Int, String>? = null
+
+data class Pair<T1, T2>(val a: T1, val b: T2)
@@ -0,0 +1,5 @@
+// "Make variable immutable" "true"
+fun foo(p: Int) {
+    <caret>var v: Int
+    if (p > 0) v = 1 else v = 2
+}
@@ -0,0 +1,5 @@
+// "Make variable immutable" "true"
+fun foo(p: Int) {
+    <caret>val v: Int
+    if (p > 0) v = 1 else v = 2
+}
@@ -112,6 +112,12 @@ public class InspectionTestGenerated extends AbstractInspectionTest {
            doTest(fileName);
        }

+        @TestMetadata("canBeVal/inspectionData/inspections.test")
+        public void testCanBeVal_inspectionData_Inspections_test() throws Exception {
+            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspections/canBeVal/inspectionData/inspections.test");
+            doTest(fileName);
+        }
+
        @TestMetadata("conflictingExtensionProperty/inspectionData/inspections.test")
        public void testConflictingExtensionProperty_inspectionData_Inspections_test() throws Exception {
            String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspections/conflictingExtensionProperty/inspectionData/inspections.test");
@@ -1581,6 +1581,16 @@ public class QuickFixMultiFileTestGenerated extends AbstractQuickFixMultiFileTes
            KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/quickfix/variables"), Pattern.compile("^(\\w+)\\.((before\\.Main\\.\\w+)|(test))$"), true);
        }

+        @TestMetadata("idea/testData/quickfix/variables/changeMutability")
+        @TestDataPath("$PROJECT_ROOT")
+        @RunWith(JUnit3RunnerWithInners.class)
+        public static class ChangeMutability extends AbstractQuickFixMultiFileTest {
+            public void testAllFilesPresentInChangeMutability() throws Exception {
+                KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/quickfix/variables/changeMutability"), Pattern.compile("^(\\w+)\\.((before\\.Main\\.\\w+)|(test))$"), true);
+            }
+
+        }
+
    }

 }
@@ -7781,6 +7781,27 @@ public class QuickFixTestGenerated extends AbstractQuickFixTest {
                String fileName = KotlinTestUtils.navigationMetadata("idea/testData/quickfix/variables/changeMutability/valWithSetter.kt");
                doTest(fileName);
            }
+
+            @TestMetadata("idea/testData/quickfix/variables/changeMutability/canBeVal")
+            @TestDataPath("$PROJECT_ROOT")
+            @RunWith(JUnit3RunnerWithInners.class)
+            public static class CanBeVal extends AbstractQuickFixTest {
+                public void testAllFilesPresentInCanBeVal() throws Exception {
+                    KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/quickfix/variables/changeMutability/canBeVal"), Pattern.compile("^([\\w\\-_]+)\\.kt$"), true);
+                }
+
+                @TestMetadata("multiVariable.kt")
+                public void testMultiVariable() throws Exception {
+                    String fileName = KotlinTestUtils.navigationMetadata("idea/testData/quickfix/variables/changeMutability/canBeVal/multiVariable.kt");
+                    doTest(fileName);
+                }
+
+                @TestMetadata("singleVariable.kt")
+                public void testSingleVariable() throws Exception {
+                    String fileName = KotlinTestUtils.navigationMetadata("idea/testData/quickfix/variables/changeMutability/canBeVal/singleVariable.kt");
+                    doTest(fileName);
+                }
+            }
        }

        @TestMetadata("idea/testData/quickfix/variables/changeToFunctionInvocation")
				`@@ -0,0 +1 @@`
				`// INSPECTION_CLASS: org.jetbrains.kotlin.idea.inspections.CanBeValInspection`
				`@@ -0,0 +1 @@`
				`org.jetbrains.kotlin.idea.inspections.CanBeValInspection`