From 8e22643771e6feabbdc95c2ed1a0f4d9bb22ced3 Mon Sep 17 00:00:00 2001 From: Pavel Dolgov Date: Wed, 11 Oct 2017 15:22:49 +0300 Subject: [PATCH] Java: Fixed safe-deleting variable used in 'for' loop's update or init clause (IDEA-180217) --- .../impl/quickfix/RemoveUnusedVariableUtil.java | 3 ++- .../safeDelete/JavaSafeDeleteProcessor.java | 10 ++++++++-- .../SafeDeleteReferenceJavaDeleteUsageInfo.java | 4 +++- .../refactoring/safeDelete/ForInitExpr.java | 11 +++++++++++ .../safeDelete/ForInitExpr_after.java | 10 ++++++++++ .../refactoring/safeDelete/ForInitList.java | 11 +++++++++++ .../safeDelete/ForInitList_after.java | 10 ++++++++++ .../refactoring/safeDelete/ForUpdateExpr.java | 10 ++++++++++ .../safeDelete/ForUpdateExpr_after.java | 9 +++++++++ .../refactoring/safeDelete/ForUpdateList.java | 10 ++++++++++ .../safeDelete/ForUpdateList_after.java | 9 +++++++++ .../java/refactoring/SafeDeleteTest.java | 16 ++++++++++++++++ 12 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForInitExpr.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForInitExpr_after.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForInitList.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForInitList_after.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr_after.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForUpdateList.java create mode 100644 java/java-tests/testData/refactoring/safeDelete/ForUpdateList_after.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java index 101c6391967c..f1d98b238659 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/quickfix/RemoveUnusedVariableUtil.java @@ -208,7 +208,8 @@ public class RemoveUnusedVariableUtil { return true; } - private static boolean isForLoopUpdate(PsiElement element) { + public static boolean isForLoopUpdate(@Nullable PsiElement element) { + if(element == null) return false; PsiElement parent = element.getParent(); return parent instanceof PsiForStatement && ((PsiForStatement)parent).getUpdate() == element; diff --git a/java/java-impl/src/com/intellij/refactoring/safeDelete/JavaSafeDeleteProcessor.java b/java/java-impl/src/com/intellij/refactoring/safeDelete/JavaSafeDeleteProcessor.java index 964ee645f568..d6e012f69849 100644 --- a/java/java-impl/src/com/intellij/refactoring/safeDelete/JavaSafeDeleteProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/safeDelete/JavaSafeDeleteProcessor.java @@ -88,7 +88,13 @@ public class JavaSafeDeleteProcessor extends SafeDeleteProcessorDelegateBase { else if (element instanceof PsiLocalVariable) { for (PsiReference reference : ReferencesSearch.search(element)) { PsiReferenceExpression referencedElement = (PsiReferenceExpression)reference.getElement(); - final PsiStatement statement = PsiTreeUtil.getParentOfType(referencedElement, PsiStatement.class); + PsiElement statementOrExprInList = PsiTreeUtil.getParentOfType(referencedElement, PsiStatement.class); + if (statementOrExprInList instanceof PsiExpressionListStatement) { + PsiExpressionList expressionList = ((PsiExpressionListStatement)statementOrExprInList).getExpressionList(); + if (expressionList != null) { + statementOrExprInList = PsiTreeUtil.findPrevParent(expressionList, referencedElement); + } + } boolean isSafeToDelete = PsiUtil.isAccessedForWriting(referencedElement); boolean hasSideEffects = false; @@ -98,7 +104,7 @@ public class JavaSafeDeleteProcessor extends SafeDeleteProcessorDelegateBase { .checkSideEffects(((PsiAssignmentExpression)referencedElement.getParent()).getRExpression(), ((PsiLocalVariable)element), new ArrayList<>()); } - usages.add(new SafeDeleteReferenceJavaDeleteUsageInfo(statement, element, isSafeToDelete && !hasSideEffects)); + usages.add(new SafeDeleteReferenceJavaDeleteUsageInfo(statementOrExprInList, element, isSafeToDelete && !hasSideEffects)); } } return new NonCodeUsageSearchInfo(insideDeletedCondition, element); diff --git a/java/java-impl/src/com/intellij/refactoring/safeDelete/usageInfo/SafeDeleteReferenceJavaDeleteUsageInfo.java b/java/java-impl/src/com/intellij/refactoring/safeDelete/usageInfo/SafeDeleteReferenceJavaDeleteUsageInfo.java index 011ca080ae5f..7ee52f79629e 100644 --- a/java/java-impl/src/com/intellij/refactoring/safeDelete/usageInfo/SafeDeleteReferenceJavaDeleteUsageInfo.java +++ b/java/java-impl/src/com/intellij/refactoring/safeDelete/usageInfo/SafeDeleteReferenceJavaDeleteUsageInfo.java @@ -62,7 +62,9 @@ public class SafeDeleteReferenceJavaDeleteUsageInfo extends SafeDeleteReferenceS } } else { - if (element instanceof PsiExpressionStatement && RefactoringUtil.isLoopOrIf(element.getParent())) { + if (element instanceof PsiExpressionStatement && + RefactoringUtil.isLoopOrIf(element.getParent()) && + !RemoveUnusedVariableUtil.isForLoopUpdate(element)) { final PsiStatement emptyTest = JavaPsiFacade.getInstance(getProject()).getElementFactory().createStatementFromText(";", null); element.replace(emptyTest); } else { diff --git a/java/java-tests/testData/refactoring/safeDelete/ForInitExpr.java b/java/java-tests/testData/refactoring/safeDelete/ForInitExpr.java new file mode 100644 index 000000000000..376ec1e0068a --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForInitExpr.java @@ -0,0 +1,11 @@ +class C { + Object foo = null; + + void case01() { + Object problematic; + int i = 10; + for(problematic = foo; (--i) > 0; ) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForInitExpr_after.java b/java/java-tests/testData/refactoring/safeDelete/ForInitExpr_after.java new file mode 100644 index 000000000000..b80730dc1932 --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForInitExpr_after.java @@ -0,0 +1,10 @@ +class C { + Object foo = null; + + void case01() { + int i = 10; + for(; (--i) > 0; ) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForInitList.java b/java/java-tests/testData/refactoring/safeDelete/ForInitList.java new file mode 100644 index 000000000000..a74d8cdd970e --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForInitList.java @@ -0,0 +1,11 @@ +class C { + Object foo = null; + + void case01() { + Object problematic; + int i; + for(i = 10, problematic = foo; (--i) > 0; ) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForInitList_after.java b/java/java-tests/testData/refactoring/safeDelete/ForInitList_after.java new file mode 100644 index 000000000000..f33ac88456e3 --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForInitList_after.java @@ -0,0 +1,10 @@ +class C { + Object foo = null; + + void case01() { + int i; + for(i = 10; (--i) > 0; ) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr.java b/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr.java new file mode 100644 index 000000000000..9e4aa74aed55 --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr.java @@ -0,0 +1,10 @@ +class C { + Object foo = null; + + void case01() { + Object problematic; + for(int i = 10; (--i) > 0; problematic = foo) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr_after.java b/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr_after.java new file mode 100644 index 000000000000..075cf41f09ca --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForUpdateExpr_after.java @@ -0,0 +1,9 @@ +class C { + Object foo = null; + + void case01() { + for(int i = 10; (--i) > 0; ) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForUpdateList.java b/java/java-tests/testData/refactoring/safeDelete/ForUpdateList.java new file mode 100644 index 000000000000..e72b58aab0f0 --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForUpdateList.java @@ -0,0 +1,10 @@ +class C { + Object foo = null; + + void case02() { + Object problematic; + for(int i = 10; i > 0; i--, problematic = foo) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/refactoring/safeDelete/ForUpdateList_after.java b/java/java-tests/testData/refactoring/safeDelete/ForUpdateList_after.java new file mode 100644 index 000000000000..eee6221bb709 --- /dev/null +++ b/java/java-tests/testData/refactoring/safeDelete/ForUpdateList_after.java @@ -0,0 +1,9 @@ +class C { + Object foo = null; + + void case02() { + for(int i = 10; i > 0; i--) { + System.out.println("index = " + i); + } + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/refactoring/SafeDeleteTest.java b/java/java-tests/testSrc/com/intellij/java/refactoring/SafeDeleteTest.java index 37ccaf5ce57f..bd31168da085 100644 --- a/java/java-tests/testSrc/com/intellij/java/refactoring/SafeDeleteTest.java +++ b/java/java-tests/testSrc/com/intellij/java/refactoring/SafeDeleteTest.java @@ -382,6 +382,22 @@ public class SafeDeleteTest extends MultiFileTestCase { doSingleFileTest(); } + public void testForInitExpr() throws Exception { + doSingleFileTest(); + } + + public void testForInitList() throws Exception { + doSingleFileTest(); + } + + public void testForUpdateExpr() throws Exception { + doSingleFileTest(); + } + + public void testForUpdateList() throws Exception { + doSingleFileTest(); + } + private void doTest(@NonNls final String qClassName) { doTest((rootDir, rootAfter) -> this.performAction(qClassName)); }