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

IDEA-240285 ConstantConditions: improve detection of pointless 'Math.min' and 'Math.max' operations

Browse Source 
+Quick-fix

GitOrigin-RevId: 9f01fac2ecd4faf7ff76101a956cc30582ad421b
This commit is contained in:
Tagir Valeev
2020-05-12 12:36:52 +07:00
committed by intellij-monorepo-bot
parent 852f9f04d7
commit 7b5c5c2875
15 changed files with 225 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
java/java-analysis-api/resources/messages/JavaAnalysisBundle.properties
View File

@@ -63,6 +63,7 @@ dataflow.message.passing.nullable.argument.nonannotated=Argument <code>#ref</cod
dataflow.message.passing.nullable.argument=Argument <code>#ref</code> #loc might be null
dataflow.message.pointless.assignment.expression=Condition <code>#ref</code> #loc at the left side of assignment expression is always <code>{0}</code>. Can be simplified
dataflow.message.pointless.same.arguments=Arguments of '#ref' are the same. Calling this method with the same arguments is meaningless.
dataflow.message.pointless.same.argument.and.result=Result of '#ref' is the same as the {0,choice,1#first|2#second} argument making the call meaningless.
dataflow.message.redundant.assignment=Variable is already assigned to this value
dataflow.message.redundant.instanceof=Condition <code>#ref</code> #loc is redundant and can be replaced with a null check
dataflow.message.redundant.update=Variable update does nothing

24
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java
View File

@@ -491,10 +491,30 @@ public abstract class DataFlowInspectionBase extends AbstractBaseJavaLocalInspec
}
private static void reportPointlessSameArguments(ProblemReporter reporter, DataFlowInstructionVisitor visitor) {
visitor.pointlessSameArguments().forEach(expr -> {
visitor.pointlessSameArguments().forKeyValue((expr, eq) -> {
PsiElement name = expr.getReferenceNameElement();
if (name != null) {
reporter.registerProblem(name, JavaAnalysisBundle.message("dataflow.message.pointless.same.arguments"));
PsiExpression[] expressions = PsiExpression.EMPTY_ARRAY;
if (expr.getParent() instanceof PsiMethodCallExpression) {
expressions = ((PsiMethodCallExpression)expr.getParent()).getArgumentList().getExpressions();
if (expressions.length == 2 && PsiUtil.isConstantExpression(expressions[0]) && PsiUtil.isConstantExpression(expressions[1]) &&
!EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(expressions[0], expressions[1])) {
return;
}
}
if (eq.firstArgEqualToResult) {
String message = eq.argsEqual ? JavaAnalysisBundle.message("dataflow.message.pointless.same.arguments") :
JavaAnalysisBundle.message("dataflow.message.pointless.same.argument.and.result", 1);
LocalQuickFix fix = expressions.length == 2 ? new ReplaceWithArgumentFix(expressions[0], 0) : null;
reporter.registerProblem(name, message, fix);
}
else if (eq.argsEqual) {
reporter.registerProblem(name, JavaAnalysisBundle.message("dataflow.message.pointless.same.arguments"));
}
else if (eq.secondArgEqualToResult) {
LocalQuickFix fix = expressions.length == 2 ? new ReplaceWithArgumentFix(expressions[1], 1) : null;
reporter.registerProblem(name, JavaAnalysisBundle.message("dataflow.message.pointless.same.argument.and.result", 2), fix);
}
}
});
}

40
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java
View File

@@ -46,7 +46,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor {
private final Set<PsiElement> myReceiverMutabilityViolation = new HashSet<>();
private final Set<PsiElement> myArgumentMutabilityViolation = new HashSet<>();
private final Map<PsiExpression, Boolean> mySameValueAssigned = new HashMap<>();
private final Map<PsiReferenceExpression, Boolean> mySameArguments = new HashMap<>();
private final Map<PsiReferenceExpression, ArgResultEquality> mySameArguments = new HashMap<>();
private final Map<PsiExpression, ThreeState> mySwitchLabelsReachability = new HashMap<>();
private boolean myAlwaysReturnsNotNull = true;
private final List<DfaMemoryState> myEndOfInitializerStates = new ArrayList<>();
@@ -138,8 +138,8 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor {
return StreamEx.ofKeys(mySameValueAssigned, Boolean::booleanValue);
}
StreamEx<PsiReferenceExpression> pointlessSameArguments() {
return StreamEx.ofKeys(mySameArguments, Boolean::booleanValue);
EntryStream<PsiReferenceExpression, ArgResultEquality> pointlessSameArguments() {
return EntryStream.of(mySameArguments).filterValues(ArgResultEquality::hasEquality);
}
@Override
@@ -231,12 +231,17 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor {
}
@Override
protected void beforeMethodCall(@NotNull PsiExpression expression,
@NotNull DfaCallArguments arguments,
@NotNull DfaMemoryState memState) {
protected void onMethodCall(@NotNull DfaValue result,
@NotNull PsiExpression expression,
@NotNull DfaCallArguments arguments,
@NotNull DfaMemoryState memState) {
PsiReferenceExpression reference = USELESS_SAME_ARGUMENTS.getReferenceIfMatched(expression);
if (reference != null) {
mySameArguments.merge(reference, memState.areEqual(arguments.myArguments[0], arguments.myArguments[1]), Boolean::logicalAnd);
ArgResultEquality equality = new ArgResultEquality(
memState.areEqual(arguments.myArguments[0], arguments.myArguments[1]),
memState.areEqual(result, arguments.myArguments[0]),
memState.areEqual(result, arguments.myArguments[1]));
mySameArguments.merge(reference, equality, ArgResultEquality::merge);
}
}
@@ -415,4 +420,25 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor {
return myRange == null ? text : text.substring(myRange.getStartOffset(), myRange.getEndOffset());
}
}
static class ArgResultEquality {
boolean argsEqual;
boolean firstArgEqualToResult;
boolean secondArgEqualToResult;
ArgResultEquality(boolean argsEqual, boolean firstArgEqualToResult, boolean secondArgEqualToResult) {
this.argsEqual = argsEqual;
this.firstArgEqualToResult = firstArgEqualToResult;
this.secondArgEqualToResult = secondArgEqualToResult;
}
ArgResultEquality merge(ArgResultEquality other) {
return new ArgResultEquality(argsEqual && other.argsEqual, firstArgEqualToResult && other.firstArgEqualToResult,
secondArgEqualToResult && other.secondArgEqualToResult);
}
boolean hasEquality() {
return argsEqual || firstArgEqualToResult || secondArgEqualToResult;
}
}
}

6
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java
View File

@@ -854,6 +854,12 @@ public class DfaMemoryStateImpl implements DfaMemoryState {
}
private boolean applyEquivalenceRelation(RelationType type, DfaValue dfaLeft, DfaValue dfaRight) {
RelationType currentRelation = getRelation(dfaLeft, dfaRight);
if (currentRelation != null) {
// Eq: NE & GE => GT
type = type.meet(currentRelation);
if (type == null) return false;
}
boolean isNegated = type == RelationType.NE || type == RelationType.GT || type == RelationType.LT;
if (!isNegated && type != RelationType.EQ) {
return true;

17
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java
View File

@@ -337,9 +337,10 @@ public class StandardInstructionVisitor extends InstructionVisitor {
protected void onTypeCast(PsiTypeCastExpression castExpression, DfaMemoryState state, boolean castPossible) {}
protected void beforeMethodCall(@NotNull PsiExpression expression,
@NotNull DfaCallArguments arguments,
@NotNull DfaMemoryState memState) {
protected void onMethodCall(@NotNull DfaValue result,
@NotNull PsiExpression expression,
@NotNull DfaCallArguments arguments,
@NotNull DfaMemoryState memState) {
}
@@ -348,17 +349,14 @@ public class StandardInstructionVisitor extends InstructionVisitor {
DfaValueFactory factory = runner.getFactory();
DfaCallArguments callArguments = popCall(instruction, factory, memState);
if (callArguments.myArguments != null && instruction.getExpression() != null) {
beforeMethodCall(instruction.getExpression(), callArguments, memState);
}
Set<DfaMemoryState> finalStates = new LinkedHashSet<>();
Set<DfaCallState> currentStates = Collections.singleton(new DfaCallState(memState, callArguments));
DfaValue defaultResult = getMethodResultValue(instruction, callArguments, memState, factory);
PsiExpression expression = instruction.getExpression();
if (callArguments.myArguments != null && !(defaultResult.getDfType() instanceof DfConstantType)) {
for (MethodContract contract : instruction.getContracts()) {
currentStates = addContractResults(contract, currentStates, factory, finalStates, defaultResult, instruction.getExpression());
currentStates = addContractResults(contract, currentStates, factory, finalStates, defaultResult, expression);
if (currentStates.size() + finalStates.size() > DataFlowRunner.MAX_STATES_PER_BRANCH) {
if (LOG.isDebugEnabled()) {
LOG.debug("Too complex contract on " + instruction.getContext() + ", skipping contract processing");
@@ -377,6 +375,9 @@ public class StandardInstructionVisitor extends InstructionVisitor {
DfaInstructionState[] result = new DfaInstructionState[finalStates.size()];
int i = 0;
for (DfaMemoryState state : finalStates) {
if (expression != null) {
onMethodCall(state.peek(), expression, callArguments, state);
}
callArguments.flush(state);
pushExpressionResult(state.pop(), instruction, state);
result[i++] = new DfaInstructionState(runner.getInstruction(instruction.getIndex() + 1), state);

45
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/fix/ReplaceWithArgumentFix.java Normal file
View File

@@ -0,0 +1,45 @@
// Copyright 2000-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
package com.intellij.codeInspection.dataFlow.fix;
import com.intellij.codeInspection.CommonQuickFixBundle;
import com.intellij.codeInspection.LocalQuickFix;
import com.intellij.codeInspection.ProblemDescriptor;
import com.intellij.codeInspection.util.IntentionFamilyName;
import com.intellij.codeInspection.util.IntentionName;
import com.intellij.openapi.project.Project;
import com.intellij.psi.PsiExpression;
import com.intellij.psi.PsiMethodCallExpression;
import com.intellij.psi.util.PsiExpressionTrimRenderer;
import com.intellij.psi.util.PsiTreeUtil;
import com.siyeh.InspectionGadgetsBundle;
import com.siyeh.ig.psiutils.CommentTracker;
import org.jetbrains.annotations.NotNull;
public class ReplaceWithArgumentFix implements LocalQuickFix {
private final String myText;
private final int myArgNum;
public ReplaceWithArgumentFix(PsiExpression argument, int argNum) {
myText = PsiExpressionTrimRenderer.render(argument);
myArgNum = argNum;
}
@Override
public @IntentionName @NotNull String getName() {
return CommonQuickFixBundle.message("fix.replace.with.x", myText);
}
@Override
public @IntentionFamilyName @NotNull String getFamilyName() {
return InspectionGadgetsBundle.message("inspection.redundant.string.replace.with.arg.fix.name");
}
@Override
public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
PsiMethodCallExpression call = PsiTreeUtil.getParentOfType(descriptor.getStartElement(), PsiMethodCallExpression.class);
if (call == null) return;
PsiExpression[] args = call.getArgumentList().getExpressions();
if (args.length <= myArgNum) return;
new CommentTracker().replaceAndRestoreComments(call, args[myArgNum]);
}
}

14
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/RelationType.java
View File

@@ -26,6 +26,20 @@ public enum RelationType {
myName = name;
}
/**
* @param other other relation to meet
* @return result of meet operation: the relation that is a sub-relation of both this and other;
* null if result is bottom
*/
public @Nullable RelationType meet(@NotNull RelationType other) {
if (isSubRelation(other)) return other;
if (other.isSubRelation(this)) return this;
if (this == NE && other == LE || this == LE && other == NE) return LT;
if (this == NE && other == GE || this == GE && other == NE) return GT;
if (this == LE && other == GE || this == GE && other == LE) return EQ;
return null;
}
public boolean isSubRelation(RelationType other) {
if (other == this) return true;
switch (this) {

7
java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/replaceMinMaxWithArgument/afterEqual.java Normal file
View File

@@ -0,0 +1,7 @@
// "Replace with 'x'" "true"
class X {
void test(int x, int y) {
if (x != y) return;
System.out.println(x);
}
}

7
java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/replaceMinMaxWithArgument/afterGreater.java Normal file
View File

@@ -0,0 +1,7 @@
// "Replace with 'y'" "true"
class X {
void test(int x, int y) {
if (x > y) return;
System.out.println(y);
}
}

7
java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/replaceMinMaxWithArgument/beforeEqual.java Normal file
View File

@@ -0,0 +1,7 @@
// "Replace with 'x'" "true"
class X {
void test(int x, int y) {
if (x != y) return;
System.out.println(Math.<caret>max(x, y));
}
}

7
java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/replaceMinMaxWithArgument/beforeGreater.java Normal file
View File

@@ -0,0 +1,7 @@
// "Replace with 'y'" "true"
class X {
void test(int x, int y) {
if (x > y) return;
System.out.println(Math.<caret>max(x, y));
}
}

2
java/java-tests/testData/inspection/dataFlow/fixture/LongRangeKnownMethods.java
View File

@@ -115,7 +115,7 @@ public class LongRangeKnownMethods {
void testMin(long x, long y) {
if (x < 10 && y > 10) {
y = Long.min(x, y);
y = Long.<warning descr="Result of min is the same as the first argument making the call meaningless.">min</warning>(x, y);
if (<warning descr="Condition 'y > 20' is always 'false'">y > 20</warning>) {
System.out.println("Impossible");
}

30
java/java-tests/testData/inspection/dataFlow/fixture/SameArguments.java
View File

@@ -1,4 +1,6 @@
class X {
static final char MY_CHAR = 'x';
void test(int x, int y) {
System.out.println(Math.<warning descr="Arguments of 'max' are the same. Calling this method with the same arguments is meaningless.">max</warning>(x, x));
int z = x;
@@ -7,10 +9,38 @@ class X {
String t = "foo";
System.out.println("foobar".<warning descr="Arguments of 'replace' are the same. Calling this method with the same arguments is meaningless.">replace</warning>(s, t));
System.out.println("foobar".<warning descr="Arguments of 'replace' are the same. Calling this method with the same arguments is meaningless.">replace</warning>('x', 'x'));
System.out.println("foobar".replace(MY_CHAR, 'x'));
}
int testCondition(int a) {
if(a == 100) return Math.<warning descr="Arguments of 'max' are the same. Calling this method with the same arguments is meaningless.">max</warning>(a, 100);
return 0;
}
public static int foo(int x) {
return Math.<warning descr="Result of min is the same as the first argument making the call meaningless.">min</warning>(x, Integer.MAX_VALUE) +
Math.<warning descr="Result of max is the same as the second argument making the call meaningless.">max</warning>(x, Integer.MAX_VALUE);
}
int clamp(int x) {
return Math.<warning descr="Result of min is the same as the first argument making the call meaningless.">min</warning>(1, Math.max(x, 100));
}
void maxGreater(int x, int y) {
if (x < y) return;
System.out.println(Math.<warning descr="Result of max is the same as the first argument making the call meaningless.">max</warning>(x, y));
System.out.println(Math.<warning descr="Result of min is the same as the second argument making the call meaningless.">min</warning>(x, y));
}
int constants() {
final int SIZE1 = 10;
final int SIZE2 = 5;
return Math.max(1, SIZE1/SIZE2);
}
void notEquals(int x, int y) {
if (x == y) return;
System.out.println(Math.max(x, y));
System.out.println(Math.min(x, y));
}
}

34
java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/ReplaceMinMaxWithArgumentFixTest.java Normal file
View File

@@ -0,0 +1,34 @@
/*
* Copyright 2000-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 com.intellij.java.codeInsight.daemon.quickFix;
import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase;
import com.intellij.codeInspection.LocalInspectionTool;
import com.intellij.codeInspection.dataFlow.DataFlowInspection;
import org.jetbrains.annotations.NotNull;
public class ReplaceMinMaxWithArgumentFixTest extends LightQuickFixParameterizedTestCase {
@Override
protected LocalInspectionTool @NotNull [] configureLocalInspectionTools() {
return new LocalInspectionTool[]{new DataFlowInspection()};
}
@Override
protected String getBasePath() {
return "/codeInsight/daemonCodeAnalyzer/quickFix/replaceMinMaxWithArgument";
}
}

3
java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTestSuite.java
View File

@@ -71,7 +71,8 @@ import org.junit.runners.Suite;
ReplaceComputeWithComputeIfPresentFixTest.class,
DeleteSwitchLabelFixTest.class,
DeleteRedundantUpdateFixTest.class,
ReplaceTypeInCastFixTest.class
ReplaceTypeInCastFixTest.class,
ReplaceMinMaxWithArgumentFixTest.class
})
public class DataFlowInspectionTestSuite {
}