From 3b4dafe6918d0b7568f224e9136e67966916f684 Mon Sep 17 00:00:00 2001 From: Dmitry Neverov Date: Fri, 12 May 2017 06:02:57 +0200 Subject: [PATCH] Detect recursive property accessors #KT-17221 Fixed --- .../RecursivePropertyAccessor.html | 23 ++++ idea/src/META-INF/plugin.xml | 8 ++ .../KotlinRecursiveCallLineMarkerProvider.kt | 3 + .../RecursivePropertyAccessorInspection.kt | 118 +++++++++++++++++ .../recursiveCall/propertyAccessors.kt | 27 ++++ .../a/JavaInterface.java | 7 ++ .../inspectionData/expected.xml | 119 ++++++++++++++++++ .../inspectionData/inspections.test | 1 + .../recursivePropertyAccessors.kt | 41 ++++++ .../recursiveSyntheticPropertyAccessor.kt | 17 +++ .../codeInsight/InspectionTestGenerated.java | 6 + .../codeInsight/LineMarkersTestGenerated.java | 6 + 12 files changed, 376 insertions(+) create mode 100644 idea/resources/inspectionDescriptions/RecursivePropertyAccessor.html create mode 100644 idea/src/org/jetbrains/kotlin/idea/inspections/RecursivePropertyAccessorInspection.kt create mode 100644 idea/testData/codeInsight/lineMarker/recursiveCall/propertyAccessors.kt create mode 100644 idea/testData/inspections/recursivePropertyAccessor/a/JavaInterface.java create mode 100644 idea/testData/inspections/recursivePropertyAccessor/inspectionData/expected.xml create mode 100644 idea/testData/inspections/recursivePropertyAccessor/inspectionData/inspections.test create mode 100644 idea/testData/inspections/recursivePropertyAccessor/recursivePropertyAccessors.kt create mode 100644 idea/testData/inspections/recursivePropertyAccessor/recursiveSyntheticPropertyAccessor.kt diff --git a/idea/resources/inspectionDescriptions/RecursivePropertyAccessor.html b/idea/resources/inspectionDescriptions/RecursivePropertyAccessor.html new file mode 100644 index 00000000000..a084dc3d550 --- /dev/null +++ b/idea/resources/inspectionDescriptions/RecursivePropertyAccessor.html @@ -0,0 +1,23 @@ + + +Reports recursive property accessor calls which can end up with StackOverflowError +

+ For example: + 

+    class A {
+        var x = 0
+            get() {
+                return x //recursive getter call
+            }
+
+        var y = 0
+            set(value) {
+                if (value > 0) {
+                    y = value //recursive setter call
+                }
+            }
+    }
+
+

+ + \ No newline at end of file diff --git a/idea/src/META-INF/plugin.xml b/idea/src/META-INF/plugin.xml index 9760e57ae37..2475ae6d2b9 100644 --- a/idea/src/META-INF/plugin.xml +++ b/idea/src/META-INF/plugin.xml @@ -2172,6 +2172,14 @@ language="kotlin" /> + + diff --git a/idea/src/org/jetbrains/kotlin/idea/highlighter/KotlinRecursiveCallLineMarkerProvider.kt b/idea/src/org/jetbrains/kotlin/idea/highlighter/KotlinRecursiveCallLineMarkerProvider.kt index 0945d78f212..27a55db7eaf 100644 --- a/idea/src/org/jetbrains/kotlin/idea/highlighter/KotlinRecursiveCallLineMarkerProvider.kt +++ b/idea/src/org/jetbrains/kotlin/idea/highlighter/KotlinRecursiveCallLineMarkerProvider.kt @@ -27,6 +27,7 @@ import com.intellij.psi.PsiElement import org.jetbrains.kotlin.descriptors.ClassDescriptor import org.jetbrains.kotlin.descriptors.SimpleFunctionDescriptor import org.jetbrains.kotlin.idea.caches.resolve.analyze +import org.jetbrains.kotlin.idea.inspections.RecursivePropertyAccessorInspection import org.jetbrains.kotlin.idea.util.getThisReceiverOwner import org.jetbrains.kotlin.lexer.KtToken import org.jetbrains.kotlin.name.Name @@ -75,6 +76,8 @@ class KotlinRecursiveCallLineMarkerProvider : LineMarkerProvider { } private fun isRecursiveCall(element: KtElement): Boolean { + if (RecursivePropertyAccessorInspection.isRecursivePropertyAccess(element)) return true + if (RecursivePropertyAccessorInspection.isRecursiveSyntheticPropertyAccess(element)) return true // Fast check for names without resolve val resolveName = getCallNameFromPsi(element) ?: return false val enclosingFunction = getEnclosingFunction(element, false) ?: return false diff --git a/idea/src/org/jetbrains/kotlin/idea/inspections/RecursivePropertyAccessorInspection.kt b/idea/src/org/jetbrains/kotlin/idea/inspections/RecursivePropertyAccessorInspection.kt new file mode 100644 index 00000000000..eaa2d628d2f --- /dev/null +++ b/idea/src/org/jetbrains/kotlin/idea/inspections/RecursivePropertyAccessorInspection.kt @@ -0,0 +1,118 @@ +/* + * Copyright 2010-2017 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.* +import com.intellij.openapi.project.Project +import com.intellij.psi.PsiElementVisitor +import com.intellij.psi.util.PsiTreeUtil +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.getParentOfType +import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType +import org.jetbrains.kotlin.resolve.BindingContext +import org.jetbrains.kotlin.resolve.BindingContext.DECLARATION_TO_DESCRIPTOR +import org.jetbrains.kotlin.resolve.BindingContext.REFERENCE_TARGET +import org.jetbrains.kotlin.synthetic.SyntheticJavaPropertyDescriptor + +class RecursivePropertyAccessorInspection : AbstractKotlinInspection() { + + override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean, session: LocalInspectionToolSession): PsiElementVisitor { + return object : KtVisitorVoid() { + override fun visitSimpleNameExpression(expression: KtSimpleNameExpression) { + super.visitSimpleNameExpression(expression) + if (isRecursivePropertyAccess(expression)) { + holder.registerProblem(expression, + "Recursive property accessor", + ProblemHighlightType.GENERIC_ERROR_OR_WARNING, + ReplaceWithFieldFix()) + } + else if (isRecursiveSyntheticPropertyAccess(expression)) { + holder.registerProblem(expression, + "Recursive synthetic property accessor", + ProblemHighlightType.GENERIC_ERROR_OR_WARNING) + } + } + } + } + + class ReplaceWithFieldFix : LocalQuickFix { + + override fun getName() = "Replace with 'field'" + + override fun getFamilyName() = name + + override fun applyFix(project: Project, descriptor: ProblemDescriptor) { + val expression = descriptor.psiElement as KtExpression + val factory = KtPsiFactory(expression) + expression.replace(factory.createExpression("field")) + } + } + + companion object { + + private fun KtBinaryExpression?.isAssignmentTo(expression: KtSimpleNameExpression): Boolean = + this != null && KtPsiUtil.isAssignment(this) && PsiTreeUtil.isAncestor(left, expression, false) + + private fun isSameAccessor(expression: KtSimpleNameExpression, isGetter: Boolean): Boolean { + val binaryExpr = expression.getStrictParentOfType() + if (isGetter) { + if (binaryExpr.isAssignmentTo(expression)) { + return KtTokens.AUGMENTED_ASSIGNMENTS.contains(binaryExpr?.operationToken) + } + return true + } + else /* isSetter */ { + if (binaryExpr.isAssignmentTo(expression)) { + return true + } + val unaryExpr = expression.getStrictParentOfType() + if (unaryExpr?.operationToken.let { it == KtTokens.PLUSPLUS || it == KtTokens.MINUSMINUS }) { + return true + } + } + return false + } + + fun isRecursivePropertyAccess(element: KtElement): Boolean { + if (element !is KtSimpleNameExpression) return false + val propertyAccessor = element.getParentOfType(true) as? KtPropertyAccessor ?: return false + if (element.text != propertyAccessor.property.name) return false + val bindingContext = element.analyze() + val target = bindingContext[REFERENCE_TARGET, element] + if (target != bindingContext[BindingContext.DECLARATION_TO_DESCRIPTOR, propertyAccessor.property]) return false + return isSameAccessor(element, propertyAccessor.isGetter) + } + + fun isRecursiveSyntheticPropertyAccess(element: KtElement): Boolean { + if (element !is KtSimpleNameExpression) return false + val namedFunction = element.getParentOfType(true) as? KtNamedFunction ?: return false + val name = namedFunction.name ?: return false + val referencedName = element.text.capitalize() + val isGetter = name == "get$referencedName" + val isSetter = name == "set$referencedName" + if (!isGetter && !isSetter) return false + val bindingContext = element.analyze() + val syntheticDescriptor = bindingContext[REFERENCE_TARGET, element] as? SyntheticJavaPropertyDescriptor ?: return false + val namedFunctionDescriptor = bindingContext[DECLARATION_TO_DESCRIPTOR, namedFunction] + if (namedFunctionDescriptor != syntheticDescriptor.getMethod && + namedFunctionDescriptor != syntheticDescriptor.setMethod) return false + return isSameAccessor(element, isGetter) + } + } +} \ No newline at end of file diff --git a/idea/testData/codeInsight/lineMarker/recursiveCall/propertyAccessors.kt b/idea/testData/codeInsight/lineMarker/recursiveCall/propertyAccessors.kt new file mode 100644 index 00000000000..89cb9598dfb --- /dev/null +++ b/idea/testData/codeInsight/lineMarker/recursiveCall/propertyAccessors.kt @@ -0,0 +1,27 @@ +class A { + var x = 0 + set(value) { + x++ + x+=1 + x = value + } + + var y = 0 + get() { + println("$y") + y++ + y += 1 + return y + } + + var z = 0 + get() = z + + var field = 0 + get() { + return if (field != 0) field else -1 + } + set(value) { + if (value >= 0) field = value + } +} \ No newline at end of file diff --git a/idea/testData/inspections/recursivePropertyAccessor/a/JavaInterface.java b/idea/testData/inspections/recursivePropertyAccessor/a/JavaInterface.java new file mode 100644 index 00000000000..a1613d29f1d --- /dev/null +++ b/idea/testData/inspections/recursivePropertyAccessor/a/JavaInterface.java @@ -0,0 +1,7 @@ +package a; + +public interface JavaInterface { + String getSomething(); + + void setSomething(String value); +} diff --git a/idea/testData/inspections/recursivePropertyAccessor/inspectionData/expected.xml b/idea/testData/inspections/recursivePropertyAccessor/inspectionData/expected.xml new file mode 100644 index 00000000000..0dbf3ec6ef5 --- /dev/null +++ b/idea/testData/inspections/recursivePropertyAccessor/inspectionData/expected.xml @@ -0,0 +1,119 @@ + + + recursivePropertyAccessors.kt + 4 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 5 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 6 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 11 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 12 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 13 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 14 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 18 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursiveSyntheticPropertyAccessor.kt + 9 + light_idea_test_case + + Recursive property accessor + Recursive synthetic property accessor + + + + recursiveSyntheticPropertyAccessor.kt + 14 + light_idea_test_case + + Recursive property accessor + Recursive synthetic property accessor + + + + recursivePropertyAccessors.kt + 27 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 31 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + + recursivePropertyAccessors.kt + 38 + light_idea_test_case + + Recursive property accessor + Recursive property accessor + + + diff --git a/idea/testData/inspections/recursivePropertyAccessor/inspectionData/inspections.test b/idea/testData/inspections/recursivePropertyAccessor/inspectionData/inspections.test new file mode 100644 index 00000000000..ea94c506ab1 --- /dev/null +++ b/idea/testData/inspections/recursivePropertyAccessor/inspectionData/inspections.test @@ -0,0 +1 @@ +// INSPECTION_CLASS: org.jetbrains.kotlin.idea.inspections.RecursivePropertyAccessorInspection \ No newline at end of file diff --git a/idea/testData/inspections/recursivePropertyAccessor/recursivePropertyAccessors.kt b/idea/testData/inspections/recursivePropertyAccessor/recursivePropertyAccessors.kt new file mode 100644 index 00000000000..ba6820401e3 --- /dev/null +++ b/idea/testData/inspections/recursivePropertyAccessor/recursivePropertyAccessors.kt @@ -0,0 +1,41 @@ +class A { + var x = 0 + set(value) { + x++ + x += 1 + x = value + } + + var y = 0 + get() { + println("$y") + y++ + y += 1 + return y + } + + var z = 0 + get() = z + + var w + set(value) { + field = w + value + } + + var field = 0 + get() { + this.field + return if (field != 0) field else -1 + } + set(value) { + this.field = value + if (value >= 0) field = value + } + + companion object { + var g = 0 + set(value) { + A.g = 99 + } + } +} \ No newline at end of file diff --git a/idea/testData/inspections/recursivePropertyAccessor/recursiveSyntheticPropertyAccessor.kt b/idea/testData/inspections/recursivePropertyAccessor/recursiveSyntheticPropertyAccessor.kt new file mode 100644 index 00000000000..2fab7848907 --- /dev/null +++ b/idea/testData/inspections/recursivePropertyAccessor/recursiveSyntheticPropertyAccessor.kt @@ -0,0 +1,17 @@ +import a.JavaInterface + +class B { + var something: String = "123" + + class Nested : JavaInterface { + override fun setSomething(value: String) { + val x = something // OK + something = value + } + + override fun getSomething(): String { + something = "456" // OK + return something + } + } +} \ No newline at end of file diff --git a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/InspectionTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/InspectionTestGenerated.java index 09f375314a1..5fd09d9853a 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/InspectionTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/InspectionTestGenerated.java @@ -233,6 +233,12 @@ public class InspectionTestGenerated extends AbstractInspectionTest { doTest(fileName); } + @TestMetadata("recursivePropertyAccessor/inspectionData/inspections.test") + public void testRecursivePropertyAccessor_inspectionData_Inspections_test() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspections/recursivePropertyAccessor/inspectionData/inspections.test"); + doTest(fileName); + } + @TestMetadata("redundantIf/inspectionData/inspections.test") public void testRedundantIf_inspectionData_Inspections_test() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("idea/testData/inspections/redundantIf/inspectionData/inspections.test"); diff --git a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/LineMarkersTestGenerated.java b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/LineMarkersTestGenerated.java index 13a606ddb7f..bad67c21789 100644 --- a/idea/tests/org/jetbrains/kotlin/idea/codeInsight/LineMarkersTestGenerated.java +++ b/idea/tests/org/jetbrains/kotlin/idea/codeInsight/LineMarkersTestGenerated.java @@ -281,6 +281,12 @@ public class LineMarkersTestGenerated extends AbstractLineMarkersTest { doTest(fileName); } + @TestMetadata("propertyAccessors.kt") + public void testPropertyAccessors() throws Exception { + String fileName = KotlinTestUtils.navigationMetadata("idea/testData/codeInsight/lineMarker/recursiveCall/propertyAccessors.kt"); + doTest(fileName); + } + @TestMetadata("severalCallsInOneLine.kt") public void testSeveralCallsInOneLine() throws Exception { String fileName = KotlinTestUtils.navigationMetadata("idea/testData/codeInsight/lineMarker/recursiveCall/severalCallsInOneLine.kt");