MDS/openide
1
0
Fork 0
mirror of https://gitflic.ru/project/openide/openide.git synced 2026-09-27 10:03:11 +07:00
Code Issues Packages Projects Releases Wiki Activity

Java inspection: Don't apply chained assignments in "Move return to computation" (IDEA-121153)

Browse Source
This commit is contained in:
Pavel Dolgov
2016-09-06 19:05:05 +03:00
parent 1fea799671
commit 68276bafcf
27 changed files with 105 additions and 153 deletions
Download Patch File Download Diff File Expand all files Collapse all files
java/java-impl/src/com/intellij/codeInspection/intermediaryVariable/ReturnSeparatedFromComputationInspection.java
+54 -83
View File
@@ -58,7 +58,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
final PsiCodeBlock returnScope = (PsiCodeBlock)returnParent;
final PsiStatement[] statements = returnScope.getStatements();
if (statements.length != 0 && statements[statements.length - 1] == returnStatement) {
PsiStatement refactoredStatement = getPrevNonEmptyStatement(returnStatement, new THashSet<>());
PsiStatement refactoredStatement = getPrevNonEmptyStatement(returnStatement, null);
if (refactoredStatement != null) {
final PsiExpression returnValue = returnStatement.getReturnValue();
if (returnValue instanceof PsiReferenceExpression) {
@@ -82,7 +82,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
if (variable instanceof PsiLocalVariable) {
final PsiElement variableScope = RefactoringUtil.getVariableScope((PsiLocalVariable)variable);
if (variableScope instanceof PsiCodeBlock) {
return ((PsiCodeBlock)variableScope);
return (PsiCodeBlock)variableScope;
}
}
else if (variable instanceof PsiParameter) {
@@ -101,6 +101,44 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
return null;
}
/**
* Detect the case like the following:
* <pre>
* int result = size;
* result = 31 * result + width;
* result = 31 * result + height;
* return result;
* </pre>
*/
private static boolean hasChainedAssignmentsInScope(@NotNull ControlFlow flow, @NotNull PsiVariable variable,
@NotNull PsiReturnStatement returnStatement,
@NotNull PsiCodeBlock variableScope) {
for (PsiStatement statement = getPrevNonEmptyStatement(returnStatement, null);
statement != null;
statement = getPrevNonEmptyStatement(statement, null)) {
if (statement instanceof PsiExpressionStatement) {
PsiExpressionStatement expressionStatement = (PsiExpressionStatement)statement;
PsiExpression expression = expressionStatement.getExpression();
if (expression instanceof PsiAssignmentExpression) {
PsiAssignmentExpression assignmentExpression = (PsiAssignmentExpression)expression;
if (isVariableUsed(flow, assignmentExpression.getLExpression(), variable) &&
isVariableUsed(flow, assignmentExpression.getRExpression(), variable)) {
return true;
}
}
}
}
return false;
}
private static boolean isVariableUsed(@NotNull ControlFlow flow, @Nullable PsiElement element, @NotNull PsiVariable variable) {
if (element == null) return false;
int startOffset = flow.getStartOffset(element);
int endOffset = flow.getEndOffset(element);
if (startOffset < 0 || endOffset < 0) return true;
return ControlFlowUtil.isVariableUsed(flow, startOffset, endOffset, variable);
}
private static boolean isApplicable(@NotNull ReturnContext context) {
final ControlFlow flow = createControlFlow(context);
return flow != null && isApplicable(flow, context);
@@ -126,6 +164,10 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
final int returnEndOffset = flow.getEndOffset(context.returnStatement);
if (returnStartOffset < 0 || returnEndOffset < 0) return false;
if (hasChainedAssignmentsInScope(flow, context.returnedVariable, context.returnStatement, context.variableScope)) {
return false;
}
if (context.returnScope != context.variableScope &&
ControlFlowUtil.hasObservableThrowExitPoints(flow, flowStart, flowEnd,
new PsiElement[]{context.refactoredStatement}, context.variableScope)) {
@@ -174,68 +216,6 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
context.returnStatement.delete();
}
private static void inlineReturnedValue(Mover mover, ReturnContext context) {
List<Instruction> instructions = mover.flow.getInstructions();
PsiAssignmentExpression assignment = null;
for (int i = 0; i < instructions.size(); i++) {
Instruction instruction = instructions.get(i);
if (instruction instanceof WriteVariableInstruction && ((WriteVariableInstruction)instruction).variable == mover.resultVariable) {
PsiElement element = mover.flow.getElement(i);
PsiElement parent = element.getParent();
if (element instanceof PsiAssignmentExpression && parent instanceof PsiExpressionStatement) {
if (!mover.replaceInline.contains(element)) {
if (assignment != null) {
return;
}
assignment = (PsiAssignmentExpression)element;
}
}
else if (!(getNearestEnclosingStatement(element) instanceof PsiDeclarationStatement)) {
return;
}
}
}
PsiExpression localInitializer = context.returnedVariable instanceof PsiLocalVariable ?
context.returnedVariable.getInitializer() : null;
if (assignment != null && localInitializer == null) {
PsiExpression rExpression = assignment.getRExpression();
replaceReturnedValue(rExpression, context.returnStatement, mover.flow);
}
else if (assignment == null && localInitializer != null) {
replaceReturnedValue(localInitializer, context.returnStatement, mover.flow);
}
}
private static void replaceReturnedValue(PsiExpression newReturnValue, PsiReturnStatement returnStatement, ControlFlow flow) {
PsiExpression returnValue = returnStatement.getReturnValue();
if (returnValue != null &&
(ExpressionUtils.computeConstantExpression(newReturnValue) != null || isUnchangedReferenceToLocal(newReturnValue, flow))) {
returnValue.replace(newReturnValue);
}
}
private static boolean isUnchangedReferenceToLocal(PsiExpression expression, ControlFlow flow) {
if (expression instanceof PsiReferenceExpression) {
PsiReferenceExpression referenceExpression = (PsiReferenceExpression)expression;
if (!referenceExpression.isQualified()) {
PsiElement resolved = referenceExpression.resolve();
if (resolved instanceof PsiLocalVariable || resolved instanceof PsiParameter) {
if (((PsiVariable)resolved).hasModifierProperty(PsiModifier.FINAL)) {
return true;
}
for (Instruction instruction : flow.getInstructions()) {
if (instruction instanceof WriteVariableInstruction && ((WriteVariableInstruction)instruction).variable == resolved) {
return false;
}
}
return true;
}
}
}
return false;
}
private static void inlineAssignment(PsiAssignmentExpression assignmentExpression, PsiReturnStatement returnStatement) {
PsiElement assignmentParent = assignmentExpression.getParent();
LOG.assertTrue(assignmentParent instanceof PsiExpressionStatement, "PsiExpressionStatement");
@@ -374,7 +354,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
return false;
}
PsiCodeBlock finallyBlock = targetStatement.getFinallyBlock();
if (finallyBlock != null && usesVariable(finallyBlock)) {
if (finallyBlock != null && isVariableUsed(flow, finallyBlock, resultVariable)) {
return false;
}
boolean allCatchesReturn = true;
@@ -423,15 +403,6 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
}
}
private boolean usesVariable(@NotNull PsiElement element) {
int startOffset = flow.getStartOffset(element);
int endOffset = flow.getEndOffset(element);
if (startOffset < 0 || endOffset < 0) {
return true;
}
return ControlFlowUtil.isVariableUsed(flow, startOffset, endOffset, resultVariable);
}
private static boolean isAlwaysTrue(@Nullable PsiExpression condition, boolean nullIsTrue) {
if(condition == null) return nullIsTrue;
return ExpressionUtils.computeConstantExpression(condition) == Boolean.TRUE;
@@ -467,7 +438,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
}
}
private static PsiStatement getPrevNonEmptyStatement(@NotNull PsiElement psiElement, @NotNull Set<PsiElement> skippedEmptyStatements) {
private static PsiStatement getPrevNonEmptyStatement(@NotNull PsiElement psiElement, @Nullable Set<PsiElement> skippedEmptyStatements) {
if (!(psiElement.getParent() instanceof PsiCodeBlock)) {
return null;
}
@@ -477,14 +448,14 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
skipped.add(prevStatement);
prevStatement = PsiTreeUtil.getPrevSiblingOfType(prevStatement, PsiStatement.class);
}
if (prevStatement != null) {
if (prevStatement != null && skippedEmptyStatements != null) {
skippedEmptyStatements.addAll(skipped);
}
return prevStatement;
}
@Nullable
private static PsiStatement getNearestEnclosingStatement(@NotNull PsiElement element) {
private static PsiStatement getNearestEnclosingStatement(@Nullable PsiElement element) {
return element instanceof PsiStatement ? (PsiStatement)element : PsiTreeUtil.getParentOfType(element, PsiStatement.class);
}
@@ -492,17 +463,17 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
@NotNull PsiReturnStatement returnStatement,
@NotNull PsiVariable variable) {
String name = variable.getName();
holder.registerProblem(returnStatement, InspectionsBundle.message("inspection.return.separated.from.computation.descriptor", name),
new VariableFix(name, variable instanceof PsiParameter));
PsiElement returnElement = returnStatement.getFirstChild();
holder.registerProblem(returnElement instanceof PsiKeyword ? returnElement : returnStatement,
InspectionsBundle.message("inspection.return.separated.from.computation.descriptor", name),
new VariableFix(name));
}
private static class VariableFix implements LocalQuickFix {
private String myName;
private boolean myIsParameter;
public VariableFix(String name, boolean isParameter) {
public VariableFix(String name) {
myName = name;
myIsParameter = isParameter;
}
@Nls
@@ -521,7 +492,7 @@ public class ReturnSeparatedFromComputationInspection extends BaseJavaBatchLocal
@Override
public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
PsiElement element = descriptor.getPsiElement();
PsiElement element = getNearestEnclosingStatement(descriptor.getPsiElement());
if (element instanceof PsiReturnStatement) {
doApply(((PsiReturnStatement)element));
}
java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation/beforeHashCode.java
+13
View File
@@ -0,0 +1,13 @@
// "Move 'return' to computation of the value of 'result'" "false"
class T {
int size;
int width;
int height;
public int hashCode() {
int result = size;
result = 31 * result + width;
result = 31 * result + height;
ret<caret>urn result;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/BreakFromLoopInTryWithResources.java
+1 -1
View File
@@ -11,7 +11,7 @@ class T {
}
}
}
<warning descr="Return separated from computation of value of 's'">return s;</warning>
<warning descr="Return separated from computation of value of 's'">return</warning> s;
}
private static BufferedReader open() throws FileNotFoundException {
return null;
java/java-tests/testData/inspection/returnSeparatedFromComputation/DoWhileTrue.java
+1 -1
View File
@@ -9,7 +9,7 @@ class T {
break;
}
} while (true);
<warning descr="Return separated from computation of value of 'r'">return r;</warning>
<warning descr="Return separated from computation of value of 'r'">return</warning> r;
}
boolean hasNext() {
java/java-tests/testData/inspection/returnSeparatedFromComputation/ForWithoutCondition.java
+1 -1
View File
@@ -7,6 +7,6 @@ class T {
break;
}
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/HashCode.java
+12
View File
@@ -0,0 +1,12 @@
class T {
int size;
int width;
int height;
public int hashCode() {
int result = size;
result = 31 * result + width;
result = 31 * result + height;
return result;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/IfElseWriteInBoth.java
+1 -1
View File
@@ -3,6 +3,6 @@ class T {
int n = 0;
if (b) n = 1;
else n = 2;
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/IfElseWriteInElse.java
+1 -1
View File
@@ -3,6 +3,6 @@ class T {
int n = 0;
if (b) System.out.println("yes");
else n = 2;
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/IfElseWriteInIf.java
+1 -1
View File
@@ -3,6 +3,6 @@ class T {
int n = 0;
if (b) n = 1;
else System.out.println("no");
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/LabeledBlock.java
+1 -1
View File
@@ -7,6 +7,6 @@ class T {
if (b) break myLabel;
n = 2;
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/LabeledFor.java
+1 -1
View File
@@ -8,6 +8,6 @@ class T {
break myLabel;
}
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/LabeledFor2.java
+1 -1
View File
@@ -6,6 +6,6 @@ class T {
n = i;
if (a[0] == 0) break myLabel;
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/LabeledIf.java
+1 -1
View File
@@ -4,6 +4,6 @@ class T {
myLabel:
if (b) n = 1;
else break myLabel;
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/NestedBlock.java
+1 -1
View File
@@ -4,6 +4,6 @@ class T {
{
n = 1;
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/NestedBlockSideEffect.java
+1 -1
View File
@@ -5,6 +5,6 @@ class T {
n = 1;
System.out.println();
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/NestedIf.java
+1 -1
View File
@@ -6,6 +6,6 @@ class T {
n = 1;
}
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/NestedIfInnerElse.java
+1 -1
View File
@@ -5,6 +5,6 @@ class T {
if (b) n = 1;
else n = 2;
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/NestedIfOuterElse.java
+1 -1
View File
@@ -5,6 +5,6 @@ class T {
if (b) n = 1;
}
else n = 2;
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/ReturnOutsideTryWithResources.java
+1 -1
View File
@@ -6,7 +6,7 @@ class T {
try (BufferedReader r = open()) {
s = r.readLine();
}
<warning descr="Return separated from computation of value of 's'">return s;</warning>
<warning descr="Return separated from computation of value of 's'">return</warning> s;
}
private static BufferedReader open() throws FileNotFoundException {
java/java-tests/testData/inspection/returnSeparatedFromComputation/SimpleDoWhile.java
+1 -1
View File
@@ -12,7 +12,7 @@ class T {
i = j + 1;
}
while (i >= 0);
<warning descr="Return separated from computation of value of 'r'">return r;</warning>
<warning descr="Return separated from computation of value of 'r'">return</warning> r;
}
boolean hasNext() {
java/java-tests/testData/inspection/returnSeparatedFromComputation/SimpleFor.java
+1 -1
View File
@@ -7,6 +7,6 @@ class T {
break;
}
}
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/SimpleForeach.java
+1 -1
View File
@@ -7,6 +7,6 @@ class T {
break;
}
}
<warning descr="Return separated from computation of value of 'r'">return r;</warning>
<warning descr="Return separated from computation of value of 'r'">return</warning> r;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/SimpleIf.java
+1 -1
View File
@@ -2,6 +2,6 @@ class T {
int f(boolean b) {
int n = 0;
if (b) n = 1;
<warning descr="Return separated from computation of value of 'n'">return n;</warning>
<warning descr="Return separated from computation of value of 'n'">return</warning> n;
}
}
java/java-tests/testData/inspection/returnSeparatedFromComputation/SimpleWhile.java
+1 -1
View File
@@ -8,7 +8,7 @@ class T {
break;
}
}
<warning descr="Return separated from computation of value of 'r'">return r;</warning>
<warning descr="Return separated from computation of value of 'r'">return</warning> r;
}
boolean hasNext() {
java/java-tests/testData/inspection/returnSeparatedFromComputation/WhileTrue.java
+1 -1
View File
@@ -10,6 +10,6 @@ class T {
break;
}
}
<warning descr="Return separated from computation of value of 'r'">return r;</warning>
<warning descr="Return separated from computation of value of 'r'">return</warning> r;
}
}
java/java-tests/testSrc/com/intellij/codeInsight/daemon/quickFix/ReturnSeparatedFromComputationFix2Test.java
-48
View File
@@ -1,48 +0,0 @@
/*
* Copyright 2000-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 com.intellij.codeInsight.daemon.quickFix;
import com.intellij.codeInspection.LocalInspectionTool;
import com.intellij.codeInspection.intermediaryVariable.ReturnSeparatedFromComputationInspection;
import org.jetbrains.annotations.NotNull;
/**
* @author Pavel.Dolgov
*/
public class ReturnSeparatedFromComputationFix2Test extends LightQuickFixTestCase {
@NotNull
@Override
protected LocalInspectionTool[] configureLocalInspectionTools() {
return new LocalInspectionTool[]{new ReturnSeparatedFromComputationInspection()};
}
public void testAssert() {
doTest();
}
public void testBreakFromLoopInTryWithResources() {
doTest();
}
private void doTest() {
doSingleTest(getTestName(false) +".java");
}
@Override
protected String getBasePath() {
return "/codeInsight/daemonCodeAnalyzer/quickFix/returnSeparatedFromComputation";
}
}
java/java-tests/testSrc/com/intellij/codeInspection/ReturnSeparatedFromComputationTest.java
+4
View File
@@ -128,6 +128,10 @@ public class ReturnSeparatedFromComputationTest extends LightCodeInsightFixtureT
doTest();
}
public void testHashCode() {
doTest();
}
private void doTest() {
myFixture.enableInspections(new ReturnSeparatedFromComputationInspection());