Introduce inspection: "Setter backing field should be assigned"

#KT-20273 Fixed
2018-10-09 13:35:18 +03:00
parent 784d9f14f6
commit 3ede93df11
21 changed files with 293 additions and 0 deletions
@@ -0,0 +1,5 @@
+<html>
+<body>
+This inspection reports a setter of a property with a backing field that doesn't update the backing field.
+</body>
+</html>
@@ -3060,6 +3060,15 @@
                     language="kotlin"
    />

+    <localInspection implementationClass="org.jetbrains.kotlin.idea.inspections.SetterBackingFieldAssignmentInspection"
+                     displayName="Existing backing field is not assigned by the setter"
+                     groupPath="Kotlin"
+                     groupName="Probable bugs"
+                     enabledByDefault="true"
+                     level="WARNING"
+                     language="kotlin"
+    />
+
    <referenceImporter implementation="org.jetbrains.kotlin.idea.quickfix.KotlinReferenceImporter"/>

    <fileType.fileViewProviderFactory filetype="KJSM" implementationClass="com.intellij.psi.ClassFileViewProviderFactory"/>
@@ -0,0 +1,78 @@
+/*
+ * Copyright 2010-2018 JetBrains s.r.o. 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.idea.inspections
+
+import com.intellij.codeInspection.*
+import com.intellij.openapi.project.Project
+import com.intellij.psi.PsiElementVisitor
+import org.jetbrains.kotlin.descriptors.PropertyDescriptor
+import org.jetbrains.kotlin.descriptors.ValueParameterDescriptor
+import org.jetbrains.kotlin.idea.caches.resolve.analyze
+import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.psiUtil.anyDescendantOfType
+import org.jetbrains.kotlin.resolve.BindingContext
+import org.jetbrains.kotlin.resolve.calls.callUtil.getResolvedCall
+
+class SetterBackingFieldAssignmentInspection : AbstractKotlinInspection(), CleanupLocalInspectionTool {
+    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession): PsiElementVisitor {
+        return propertyAccessorVisitor(fun(accessor) {
+            if (!accessor.isSetter) return
+            val bodyExpression = accessor.bodyExpression as? KtBlockExpression ?: return
+
+            val property = accessor.property
+            val propertyContext = property.analyze()
+            val propertyDescriptor = propertyContext[BindingContext.DECLARATION_TO_DESCRIPTOR, property] as? PropertyDescriptor ?: return
+            if (propertyContext[BindingContext.BACKING_FIELD_REQUIRED, propertyDescriptor] == false) return
+
+            val accessorContext = accessor.analyze()
+            val parameter = accessor.valueParameters.singleOrNull()
+            val parameterDescriptor = accessorContext[BindingContext.VALUE_PARAMETER, parameter] as? ValueParameterDescriptor
+            if (bodyExpression.anyDescendantOfType<KtExpression> {
+                    when (it) {
+                        is KtBinaryExpression ->
+                            it.left?.text == KtTokens.FIELD_KEYWORD.value && it.operationToken in assignmentOperators
+                        is KtUnaryExpression ->
+                            it.baseExpression?.text == KtTokens.FIELD_KEYWORD.value && it.operationToken in incrementAndDecrementOperators
+                        is KtCallExpression ->
+                            it.valueArguments.any { arg ->
+                                arg.text == parameter?.text
+                                        && arg.getArgumentExpression().getResolvedCall(accessorContext)?.resultingDescriptor == parameterDescriptor
+                            }
+                        else -> false
+                    }
+                }) return
+
+            holder.registerProblem(
+                accessor,
+                "Existing backing field is not assigned by the setter",
+                ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                AssignBackingFieldFix()
+            )
+        })
+    }
+}
+
+private val assignmentOperators = listOf(KtTokens.EQ, KtTokens.PLUSEQ, KtTokens.MINUSEQ, KtTokens.MULTEQ, KtTokens.DIVEQ, KtTokens.PERCEQ)
+
+private val incrementAndDecrementOperators = listOf(KtTokens.PLUSPLUS, KtTokens.MINUSMINUS)
+
+private class AssignBackingFieldFix : LocalQuickFix {
+    override fun getName() = "Assign backing filed"
+
+    override fun getFamilyName() = name
+
+    override fun applyFix(project: Project, descriptor: ProblemDescriptor) {
+        val setter = descriptor.psiElement as? KtPropertyAccessor ?: return
+        val parameter = setter.valueParameters.firstOrNull() ?: return
+        val bodyExpression = setter.bodyExpression as? KtBlockExpression ?: return
+        setter.hasBlockBody()
+        bodyExpression.addBefore(
+            KtPsiFactory(setter).createExpression("field = ${parameter.text}"),
+            bodyExpression.rBrace
+        )
+    }
+}
@@ -0,0 +1 @@
+org.jetbrains.kotlin.idea.inspections.SetterBackingFieldAssignmentInspection
@@ -0,0 +1,10 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 1
+        <caret>set(value) {
+            bar()
+            field = value
+        }
+
+    fun bar() {}
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field--
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            --field
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field /= value
+        }
+}
@@ -0,0 +1,9 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            bar(value)
+        }
+
+    fun bar(value: Int) {}
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field++
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            ++field
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field -= value
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field %= value
+        }
+}
@@ -0,0 +1,5 @@
+class Test {
+    var foo: Int = 1
+        <caret>set(value) {
+        }
+}
@@ -0,0 +1,6 @@
+class Test {
+    var foo: Int = 1
+        set(value) {
+            field = value
+        }
+}
@@ -0,0 +1,8 @@
+class Test {
+    var foo: Int = 1
+        <caret>set(value) {
+            bar(field)
+        }
+
+    fun bar(i: Int) {}
+}
@@ -0,0 +1,9 @@
+class Test {
+    var foo: Int = 1
+        set(value) {
+            bar(field)
+            field = value
+        }
+
+    fun bar(i: Int) {}
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int
+        get() = 1
+        <caret>set(value) {
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field += value
+        }
+}
@@ -0,0 +1,7 @@
+// PROBLEM: none
+class Test {
+    var foo: Int = 10
+        <caret>set(value) {
+            field *= value
+        }
+}
@@ -5847,6 +5847,89 @@ public class LocalInspectionTestGenerated extends AbstractLocalInspectionTest {
        }
    }

+    @TestMetadata("idea/testData/inspectionsLocal/setterBackingFieldAssignment")
+    @TestDataPath("$PROJECT_ROOT")
+    @RunWith(JUnit3RunnerWithInners.class)
+    public static class SetterBackingFieldAssignment extends AbstractLocalInspectionTest {
+        private void runTest(String testDataFilePath) throws Exception {
+            KotlinTestUtils.runTest(this::doTest, TargetBackend.ANY, testDataFilePath);
+        }
+
+        public void testAllFilesPresentInSetterBackingFieldAssignment() throws Exception {
+            KotlinTestUtils.assertAllTestsPresentByMetadata(this.getClass(), new File("idea/testData/inspectionsLocal/setterBackingFieldAssignment"), Pattern.compile("^([\\w\\-_]+)\\.(kt|kts)$"), TargetBackend.ANY, true);
+        }
+
+        @TestMetadata("assignment.kt")
+        public void testAssignment() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/assignment.kt");
+        }
+
+        @TestMetadata("decrement.kt")
+        public void testDecrement() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/decrement.kt");
+        }
+
+        @TestMetadata("decrement2.kt")
+        public void testDecrement2() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/decrement2.kt");
+        }
+
+        @TestMetadata("divAssign.kt")
+        public void testDivAssign() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/divAssign.kt");
+        }
+
+        @TestMetadata("functionCallWithSetterParam.kt")
+        public void testFunctionCallWithSetterParam() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/functionCallWithSetterParam.kt");
+        }
+
+        @TestMetadata("increment.kt")
+        public void testIncrement() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/increment.kt");
+        }
+
+        @TestMetadata("increment2.kt")
+        public void testIncrement2() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/increment2.kt");
+        }
+
+        @TestMetadata("minusAssign.kt")
+        public void testMinusAssign() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/minusAssign.kt");
+        }
+
+        @TestMetadata("modAssign.kt")
+        public void testModAssign() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/modAssign.kt");
+        }
+
+        @TestMetadata("noAssignment.kt")
+        public void testNoAssignment() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/noAssignment.kt");
+        }
+
+        @TestMetadata("noAssignment2.kt")
+        public void testNoAssignment2() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/noAssignment2.kt");
+        }
+
+        @TestMetadata("noBackingField.kt")
+        public void testNoBackingField() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/noBackingField.kt");
+        }
+
+        @TestMetadata("plusAssign.kt")
+        public void testPlusAssign() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/plusAssign.kt");
+        }
+
+        @TestMetadata("timesAssign.kt")
+        public void testTimesAssign() throws Exception {
+            runTest("idea/testData/inspectionsLocal/setterBackingFieldAssignment/timesAssign.kt");
+        }
+    }
+
    @TestMetadata("idea/testData/inspectionsLocal/simplifyAssertNotNull")
    @TestDataPath("$PROJECT_ROOT")
    @RunWith(JUnit3RunnerWithInners.class)
				`@@ -0,0 +1 @@`
				`org.jetbrains.kotlin.idea.inspections.SetterBackingFieldAssignmentInspection`