DeprecatedSymbolUsageFix: dealing with dropping arguments with potential side effects + stdlib's "let" has no receiver type bound

2015-05-19 23:44:58 +03:00
parent 08cfca56f8
commit 5c445ca003
21 changed files with 369 additions and 35 deletions
@@ -137,7 +137,6 @@ public class DeprecatedSymbolUsageFix(

        val introduceValuesForParameters = ArrayList<Pair<ValueParameterDescriptor, JetExpression>>()

-        //TODO: check for dropping expressions with potential side effects
        for (parameter in descriptor.getValueParameters()) {
            val argument = argumentForParameter(parameter) ?: continue
            argument.putCopyableUserData(FROM_PARAMETER_KEY, parameter)
@@ -146,7 +145,7 @@ public class DeprecatedSymbolUsageFix(
            val usages = parameterUsages[parameter.getOriginal()]!!
            usages.forEach { it.replace(argument) }

-            if (usages.size() > 1 && argument.shouldIntroduceVariableIfUsedTwice()) {
+            if (argument.shouldKeepValue(usages.size())) {
                introduceValuesForParameters.add(parameter to argument)
            }
        }
@@ -158,16 +157,15 @@ public class DeprecatedSymbolUsageFix(
            expression = expression.keepInfixFormIfPossible()
        }

-        if (receiver != null && receiver.shouldIntroduceVariableIfUsedTwice()) {
+        if (receiver != null) {
            val thisReplaced = expression.collectElementsOfType<JetExpression> { it.getCopyableUserData(FROM_THIS_KEY) != null }
-            if (thisReplaced.size() > 1) {
+            if (receiver.shouldKeepValue(thisReplaced.size())) {
                expression = expression.introduceValue(receiver, expressionToReplace, bindingContext, thisReplaced)
            }
        }

        for ((parameter, value) in introduceValuesForParameters) {
            val usagesReplaced = expression.collectElementsOfType<JetExpression> { it.getCopyableUserData(FROM_PARAMETER_KEY) == parameter }
-            assert(usagesReplaced.size() > 1)
            expression = expression.introduceValue(value, expressionToReplace, bindingContext, usagesReplaced, nameSuggestion = parameter.getName().asString())
        }

@@ -282,33 +280,39 @@ public class DeprecatedSymbolUsageFix(
            if (block != null) {
                val resolutionScope = bindingContext[BindingContext.RESOLUTION_SCOPE, insertDeclarationsBefore]

-                val valueType = bindingContext.getType(value)
-                var explicitType: JetType? = null
-                if (valueType != null && !ErrorUtils.containsErrorType(valueType)) {
-                    val valueTypeWithoutExpectedType = value.analyzeInContext(
-                            resolutionScope,
-                            dataFlowInfo = bindingContext.getDataFlowInfo(insertDeclarationsBefore)
-                    ).getType(value)
-                    if (valueTypeWithoutExpectedType == null || ErrorUtils.containsErrorType(valueTypeWithoutExpectedType)) {
-                        explicitType = valueType
+                if (usages.isNotEmpty()) {
+                    val valueType = bindingContext.getType(value)
+                    var explicitType: JetType? = null
+                    if (valueType != null && !ErrorUtils.containsErrorType(valueType)) {
+                        val valueTypeWithoutExpectedType = value.analyzeInContext(
+                                resolutionScope,
+                                dataFlowInfo = bindingContext.getDataFlowInfo(insertDeclarationsBefore)
+                        ).getType(value)
+                        if (valueTypeWithoutExpectedType == null || ErrorUtils.containsErrorType(valueTypeWithoutExpectedType)) {
+                            explicitType = valueType
+                        }
                    }
-                }

-                val name = suggestName(object : JetNameValidator() {
-                    override fun validateInner(name: String): Boolean {
-                        return resolutionScope.getLocalVariable(Name.identifier(name)) == null && !isNameUsed(name)
+                    val name = suggestName(object : JetNameValidator() {
+                        override fun validateInner(name: String): Boolean {
+                            return resolutionScope.getLocalVariable(Name.identifier(name)) == null && !isNameUsed(name)
+                        }
+                    })
+
+                    var declaration = psiFactory.createDeclaration<JetVariableDeclaration>("val ${nameInCode(name)} = " + value.getText())
+                    declaration = block.addBefore(declaration, insertDeclarationsBefore) as JetVariableDeclaration
+                    block.addBefore(psiFactory.createNewLine(), insertDeclarationsBefore)
+
+                    if (explicitType != null) {
+                        declaration.setType(explicitType)
                    }
-                })

-                var declaration = psiFactory.createDeclaration<JetVariableDeclaration>("val ${nameInCode(name)} = " + value.getText())
-                declaration = block.addBefore(declaration, insertDeclarationsBefore) as JetVariableDeclaration
-                block.addBefore(psiFactory.createNewLine(), insertDeclarationsBefore)
-
-                if (explicitType != null) {
-                    declaration.setType(explicitType)
+                    replaceUsages(name)
+                }
+                else {
+                    block.addBefore(value, insertDeclarationsBefore)
+                    block.addBefore(psiFactory.createNewLine(), insertDeclarationsBefore)
                }
-
-                replaceUsages(name)
                return this
            }
        }
@@ -331,15 +335,23 @@ public class DeprecatedSymbolUsageFix(
    private fun collectNameUsages(scope: JetExpression, name: String)
            = scope.collectElementsOfType<JetSimpleNameExpression> { it.getReceiverExpression() == null && it.getReferencedName() == name }

-    private fun JetExpression?.shouldIntroduceVariableIfUsedTwice(): Boolean {
+    private fun JetExpression?.shouldKeepValue(usageCount: Int): Boolean {
+        if (usageCount == 1) return false
+        val sideEffectOnly = usageCount == 0
+
        return when (this) {
            is JetSimpleNameExpression -> false
-            is JetQualifiedExpression -> getReceiverExpression().shouldIntroduceVariableIfUsedTwice() || getSelectorExpression().shouldIntroduceVariableIfUsedTwice()
-            is JetUnaryExpression -> getOperationToken() in setOf(JetTokens.PLUSPLUS, JetTokens.MINUSMINUS) || getBaseExpression().shouldIntroduceVariableIfUsedTwice()
-            is JetStringTemplateExpression -> getEntries().any { it is JetStringTemplateEntryWithExpression }
-            is JetThisExpression, is JetSuperExpression -> false
-            is JetParenthesizedExpression -> getExpression().shouldIntroduceVariableIfUsedTwice()
-            is JetBinaryExpression, is JetIfExpression -> true // TODO: discuss it
+            is JetQualifiedExpression -> getReceiverExpression().shouldKeepValue(usageCount) || getSelectorExpression().shouldKeepValue(usageCount)
+            is JetUnaryExpression -> getOperationToken() in setOf(JetTokens.PLUSPLUS, JetTokens.MINUSMINUS) || getBaseExpression().shouldKeepValue(usageCount)
+            is JetStringTemplateExpression -> getEntries().any { if (sideEffectOnly) it.getExpression().shouldKeepValue(usageCount) else it is JetStringTemplateEntryWithExpression }
+            is JetThisExpression, is JetSuperExpression, is JetConstantExpression -> false
+            is JetParenthesizedExpression -> getExpression().shouldKeepValue(usageCount)
+
+            // TODO: discuss it
+            is JetBinaryExpression -> if (sideEffectOnly) getLeft().shouldKeepValue(usageCount) || getRight().shouldKeepValue(usageCount) else true
+            is JetIfExpression -> if (sideEffectOnly) getCondition().shouldKeepValue(usageCount) || getThen().shouldKeepValue(usageCount) || getElse().shouldKeepValue(usageCount) else true
+            is JetBinaryExpressionWithTypeRHS -> true
+
            else -> true // what else it can be?
        }
    }
@@ -0,0 +1,14 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int) {
+    newFun()
+}
+
+fun newFun(){}
+
+fun foo() {
+    <caret>oldFun(bar())
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,15 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int) {
+    newFun()
+}
+
+fun newFun(){}
+
+fun foo() {
+    bar()
+    <caret>newFun()
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun() {
+        newFun()
+    }
+}
+
+fun newFun(){}
+
+fun foo() {
+    getC().<caret>oldFun()
+}
+
+fun getC(): C = C()
@@ -0,0 +1,17 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun() {
+        newFun()
+    }
+}
+
+fun newFun(){}
+
+fun foo() {
+    getC()
+    <caret>newFun()
+}
+
+fun getC(): C = C()
@@ -0,0 +1,14 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int): Int {
+    return newFun()
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int {
+    return <caret>oldFun(bar())
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,14 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int): Int {
+    return newFun()
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int {
+    return bar().let { newFun() }
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,12 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int?): Int {
+    return newFun()
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int = <caret>oldFun(bar())
+
+fun bar(): Int? = 0
@@ -0,0 +1,12 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int?): Int {
+    return newFun()
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int = bar().<caret>let { newFun() }
+
+fun bar(): Int? = 0
@@ -0,0 +1,14 @@
+// "Replace with 'newFun(p2)'" "true"
+
+@deprecated("", ReplaceWith("newFun(p2)"))
+fun oldFun(p1: Int, p2: Int): Boolean {
+    return newFun(p2)
+}
+
+fun newFun(p: Int) = false
+
+fun foo(list: List<Int>) {
+    list.filter { !<caret>oldFun(bar(), it) }
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,14 @@
+// "Replace with 'newFun(p2)'" "true"
+
+@deprecated("", ReplaceWith("newFun(p2)"))
+fun oldFun(p1: Int, p2: Int): Boolean {
+    return newFun(p2)
+}
+
+fun newFun(p: Int) = false
+
+fun foo(list: List<Int>) {
+    list.filter { !bar().let { p1 -> newFun(it) } }
+}
+
+fun bar(): Int = 0
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun() {
+        newFun()
+    }
+}
+
+fun newFun(){}
+
+fun foo() {
+    getC()?.<caret>oldFun()
+}
+
+fun getC(): C? = null
@@ -0,0 +1,18 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun() {
+        newFun()
+    }
+}
+
+fun newFun(){}
+
+fun foo() {
+    <caret>if (getC() != null) {
+        newFun()
+    }
+}
+
+fun getC(): C? = null
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun(): Int {
+        return newFun()
+    }
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int? {
+    return getC()?.<caret>oldFun()
+}
+
+fun getC(): C? = null
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+class C {
+    @deprecated("", ReplaceWith("newFun()"))
+    fun oldFun(): Int {
+        return newFun()
+    }
+}
+
+fun newFun(): Int = 0
+
+fun foo(): Int? {
+    return getC()?.<caret>let { newFun() }
+}
+
+fun getC(): C? = null
@@ -0,0 +1,14 @@
+// "Replace with 'newFun(p, p)'" "true"
+
+@deprecated("", ReplaceWith("newFun(p, p)"))
+fun oldFun(p: Int?): Int {
+    return newFun(p, p)
+}
+
+fun newFun(p1: Int?, p2: Int?): Int = 0
+
+fun foo(): Int {
+    return <caret>oldFun(bar())
+}
+
+fun bar(): Int? = null
@@ -0,0 +1,14 @@
+// "Replace with 'newFun(p, p)'" "true"
+
+@deprecated("", ReplaceWith("newFun(p, p)"))
+fun oldFun(p: Int?): Int {
+    return newFun(p, p)
+}
+
+fun newFun(p1: Int?, p2: Int?): Int = 0
+
+fun foo(): Int {
+    return bar().let { newFun(it, it) }
+}
+
+fun bar(): Int? = null
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int) {
+    newFun()
+}
+
+fun newFun(){}
+
+fun foo() {
+    <caret>oldFun(O.x + 1)
+}
+
+object O {
+    var x = 0
+}
@@ -0,0 +1,16 @@
+// "Replace with 'newFun()'" "true"
+
+@deprecated("", ReplaceWith("newFun()"))
+fun oldFun(p: Int) {
+    newFun()
+}
+
+fun newFun(){}
+
+fun foo() {
+    <caret>newFun()
+}
+
+object O {
+    var x = 0
+}
@@ -2932,6 +2932,48 @@ public class QuickFixTestGenerated extends AbstractQuickFixTest {
            doTest(fileName);
        }

+        @TestMetadata("complexExpressionNotUsed1.kt")
+        public void testComplexExpressionNotUsed1() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsed1.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsed2.kt")
+        public void testComplexExpressionNotUsed2() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsed2.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsed3Runtime.kt")
+        public void testComplexExpressionNotUsed3Runtime() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsed3Runtime.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsed4Runtime.kt")
+        public void testComplexExpressionNotUsed4Runtime() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsed4Runtime.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsed5Runtime.kt")
+        public void testComplexExpressionNotUsed5Runtime() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsed5Runtime.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsedSafeCall.kt")
+        public void testComplexExpressionNotUsedSafeCall() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsedSafeCall.kt");
+            doTest(fileName);
+        }
+
+        @TestMetadata("complexExpressionNotUsedSafeCall2Runtime.kt")
+        public void testComplexExpressionNotUsedSafeCall2Runtime() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionNotUsedSafeCall2Runtime.kt");
+            doTest(fileName);
+        }
+
        @TestMetadata("complexExpressionUsedTwice.kt")
        public void testComplexExpressionUsedTwice() throws Exception {
            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionUsedTwice.kt");
@@ -2962,6 +3004,12 @@ public class QuickFixTestGenerated extends AbstractQuickFixTest {
            doTest(fileName);
        }

+        @TestMetadata("complexExpressionUsedTwice6Runtime.kt")
+        public void testComplexExpressionUsedTwice6Runtime() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/complexExpressionUsedTwice6Runtime.kt");
+            doTest(fileName);
+        }
+
        @TestMetadata("doNotShortenUserReferences.kt")
        public void testDoNotShortenUserReferences() throws Exception {
            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/doNotShortenUserReferences.kt");
@@ -3094,6 +3142,12 @@ public class QuickFixTestGenerated extends AbstractQuickFixTest {
            doTest(fileName);
        }

+        @TestMetadata("simpleExpressionNotUsed.kt")
+        public void testSimpleExpressionNotUsed() throws Exception {
+            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/simpleExpressionNotUsed.kt");
+            doTest(fileName);
+        }
+
        @TestMetadata("simpleExpressionUsedTwice.kt")
        public void testSimpleExpressionUsedTwice() throws Exception {
            String fileName = JetTestUtils.navigationMetadata("idea/testData/quickfix/deprecatedSymbolUsage/simpleExpressionUsedTwice.kt");
@@ -21,4 +21,4 @@ public inline fun <T, R> with(receiver: T, f: T.() -> R): R = receiver.f()
 /**
 * Converts receiver to body parameter
 */
-public inline fun <T : Any, R> T.let(f: (T) -> R): R = f(this)
+public inline fun <T, R> T.let(f: (T) -> R): R = f(this)