IDEA-181743 IntelliJ suggests bad refactoring for compareTo with Optionals

Reference expressions are compared instead of variables now
This commit is contained in:
Tagir Valeev
2017-11-10 17:38:53 +07:00
parent f01391541f
commit 5bd30e691d
3 changed files with 142 additions and 66 deletions
@@ -1,7 +1,6 @@
// Copyright 2000-2017 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;
import com.intellij.codeInsight.PsiEquivalenceUtil;
import com.intellij.codeInspection.dataFlow.Nullness;
import com.intellij.codeInspection.dataFlow.NullnessUtil;
import com.intellij.codeInspection.util.LambdaGenerationUtil;
@@ -29,6 +28,8 @@ import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import static com.intellij.codeInsight.PsiEquivalenceUtil.areElementsEquivalent;
/**
* @author Tagir Valeev
*/
@@ -45,7 +46,7 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
private enum ProblemType {
WARNING, INFO, NONE;
void registerProblem(ProblemsHolder holder, PsiExpression condition, OptionalIsPresentCase scenario) {
void registerProblem(@NotNull ProblemsHolder holder, @NotNull PsiExpression condition, OptionalIsPresentCase scenario) {
if(this != NONE) {
holder.registerProblem(condition, "Can be replaced with single expression in functional style",
this == INFO ? ProblemHighlightType.INFORMATION : ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
@@ -62,7 +63,7 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
return new JavaElementVisitor() {
@Override
public void visitConditionalExpression(PsiConditionalExpression expression) {
public void visitConditionalExpression(@NotNull PsiConditionalExpression expression) {
super.visitConditionalExpression(expression);
PsiExpression condition = PsiUtil.skipParenthesizedExprDown(expression.getCondition());
if (condition == null) return;
@@ -72,15 +73,15 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
strippedCondition = BoolUtils.getNegated(condition);
invert = true;
}
PsiVariable optionalVariable = extractOptionalFromIfPresentCheck(strippedCondition);
if (optionalVariable == null) return;
PsiReferenceExpression optionalRef = extractOptionalFromIfPresentCheck(strippedCondition);
if (optionalRef == null) return;
PsiExpression thenExpression = invert ? expression.getElseExpression() : expression.getThenExpression();
PsiExpression elseExpression = invert ? expression.getThenExpression() : expression.getElseExpression();
check(condition, optionalVariable, thenExpression, elseExpression);
check(condition, optionalRef, thenExpression, elseExpression);
}
@Override
public void visitIfStatement(PsiIfStatement statement) {
public void visitIfStatement(@NotNull PsiIfStatement statement) {
super.visitIfStatement(statement);
PsiExpression condition = PsiUtil.skipParenthesizedExprDown(statement.getCondition());
if (condition == null) return;
@@ -90,33 +91,34 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
strippedCondition = BoolUtils.getNegated(condition);
invert = true;
}
PsiVariable optionalVariable = extractOptionalFromIfPresentCheck(strippedCondition);
if (optionalVariable == null) return;
PsiReferenceExpression optionalRef = extractOptionalFromIfPresentCheck(strippedCondition);
if (optionalRef == null) return;
PsiStatement thenStatement = extractThenStatement(statement, invert);
PsiStatement elseStatement = extractElseStatement(statement, invert);
check(condition, optionalVariable, thenStatement, elseStatement);
check(condition, optionalRef, thenStatement, elseStatement);
}
void check(PsiExpression condition, PsiVariable optionalVariable, PsiElement thenElement, PsiElement elseElement) {
void check(@NotNull PsiExpression condition, PsiReferenceExpression optionalRef, PsiElement thenElement, PsiElement elseElement) {
for (OptionalIsPresentCase scenario : CASES) {
scenario.getProblemType(optionalVariable, thenElement, elseElement).registerProblem(holder, condition, scenario);
scenario.getProblemType(optionalRef, thenElement, elseElement).registerProblem(holder, condition, scenario);
}
}
};
}
private static boolean isRaw(PsiVariable variable) {
private static boolean isRaw(@NotNull PsiVariable variable) {
PsiType type = variable.getType();
return type instanceof PsiClassType && ((PsiClassType)type).isRaw();
}
@Nullable
private static PsiStatement extractThenStatement(PsiIfStatement ifStatement, boolean invert) {
private static PsiStatement extractThenStatement(@NotNull PsiIfStatement ifStatement, boolean invert) {
if (invert) return extractElseStatement(ifStatement, false);
return ControlFlowUtils.stripBraces(ifStatement.getThenBranch());
}
private static PsiStatement extractElseStatement(PsiIfStatement ifStatement, boolean invert) {
@Nullable
private static PsiStatement extractElseStatement(@NotNull PsiIfStatement ifStatement, boolean invert) {
if (invert) return extractThenStatement(ifStatement, false);
PsiStatement statement = ControlFlowUtils.stripBraces(ifStatement.getElseBranch());
if (statement == null) {
@@ -131,8 +133,9 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
return statement;
}
@Nullable
@Contract("null -> null")
static PsiVariable extractOptionalFromIfPresentCheck(PsiExpression expression) {
static PsiReferenceExpression extractOptionalFromIfPresentCheck(PsiExpression expression) {
if (!(expression instanceof PsiMethodCallExpression)) return null;
PsiMethodCallExpression call = (PsiMethodCallExpression)expression;
if (call.getArgumentList().getExpressions().length != 0) return null;
@@ -141,41 +144,43 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
if (method == null) return null;
PsiClass containingClass = method.getContainingClass();
if (containingClass == null || !CommonClassNames.JAVA_UTIL_OPTIONAL.equals(containingClass.getQualifiedName())) return null;
PsiExpression qualifier = call.getMethodExpression().getQualifierExpression();
if (!(qualifier instanceof PsiReferenceExpression)) return null;
PsiElement element = ((PsiReferenceExpression)qualifier).resolve();
PsiReferenceExpression qualifier =
ObjectUtils.tryCast(call.getMethodExpression().getQualifierExpression(), PsiReferenceExpression.class);
if (qualifier == null) return null;
PsiElement element = qualifier.resolve();
if (!(element instanceof PsiVariable) || isRaw((PsiVariable)element)) return null;
return (PsiVariable)element;
return qualifier;
}
@Contract("null, _ -> false")
static boolean isOptionalGetCall(PsiElement element, PsiVariable variable) {
static boolean isOptionalGetCall(PsiElement element, @NotNull PsiReferenceExpression optionalRef) {
if (!(element instanceof PsiMethodCallExpression)) return false;
PsiMethodCallExpression call = (PsiMethodCallExpression)element;
if (call.getArgumentList().getExpressions().length != 0) return false;
PsiReferenceExpression methodExpression = call.getMethodExpression();
return "get".equals(methodExpression.getReferenceName()) &&
ExpressionUtils.isReferenceTo(methodExpression.getQualifierExpression(), variable);
areElementsEquivalent(ExpressionUtils.getQualifierOrThis(methodExpression), optionalRef);
}
@NotNull
static ProblemType getTypeByLambdaCandidate(PsiVariable optionalVariable, PsiElement lambdaCandidate, PsiExpression falseExpression) {
static ProblemType getTypeByLambdaCandidate(@NotNull PsiReferenceExpression optionalRef,
@Nullable PsiElement lambdaCandidate,
@Nullable PsiExpression falseExpression) {
if (lambdaCandidate == null) return ProblemType.NONE;
if (lambdaCandidate instanceof PsiReferenceExpression &&
((PsiReferenceExpression)lambdaCandidate).isReferenceTo(optionalVariable) && OptionalUtil.isOptionalEmptyCall(falseExpression)) {
areElementsEquivalent(lambdaCandidate, optionalRef) && OptionalUtil.isOptionalEmptyCall(falseExpression)) {
return ProblemType.WARNING;
}
if (!LambdaGenerationUtil.canBeUncheckedLambda(lambdaCandidate, optionalVariable::equals)) return ProblemType.NONE;
if (!LambdaGenerationUtil.canBeUncheckedLambda(lambdaCandidate, optionalRef::isReferenceTo)) return ProblemType.NONE;
Ref<Boolean> hasOptionalReference = new Ref<>(Boolean.FALSE);
boolean hasNoBadRefs = PsiTreeUtil.processElements(lambdaCandidate, e -> {
if (!(e instanceof PsiReferenceExpression)) return true;
PsiElement element = ((PsiReferenceExpression)e).resolve();
if (element != optionalVariable) return true;
if (!areElementsEquivalent(e, optionalRef)) return true;
// Check that Optional variable is referenced only in context of get() call
hasOptionalReference.set(Boolean.TRUE);
return isOptionalGetCall(e.getParent().getParent(), optionalVariable);
return isOptionalGetCall(e.getParent().getParent(), optionalRef);
});
if(!hasNoBadRefs) return ProblemType.NONE;
if (!hasNoBadRefs) return ProblemType.NONE;
if (!hasOptionalReference.get() || !(lambdaCandidate instanceof PsiExpression)) return ProblemType.INFO;
PsiExpression expression = (PsiExpression)lambdaCandidate;
if (falseExpression != null && NullnessUtil.getExpressionNullness(expression) != Nullness.NOT_NULL) {
@@ -187,8 +192,11 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
@NotNull
static String generateOptionalLambda(PsiElementFactory factory, CommentTracker ct, PsiVariable optionalVariable, PsiElement trueValue) {
PsiType type = optionalVariable.getType();
static String generateOptionalLambda(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
PsiReferenceExpression optionalRef,
PsiElement trueValue) {
PsiType type = optionalRef.getType();
JavaCodeStyleManager javaCodeStyleManager = JavaCodeStyleManager.getInstance(trueValue.getProject());
SuggestedNameInfo info = javaCodeStyleManager.suggestVariableName(VariableKind.PARAMETER, null, null, type);
String baseName = ObjectUtils.coalesce(ArrayUtil.getFirstElement(info.names), "value");
@@ -198,7 +206,7 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
ct.markUnchanged(trueValue);
PsiElement copy = trueValue.copy();
for (PsiElement getCall : PsiTreeUtil.collectElements(copy, e -> isOptionalGetCall(e, optionalVariable))) {
for (PsiElement getCall : PsiTreeUtil.collectElements(copy, e -> isOptionalGetCall(e, optionalRef))) {
PsiElement result = getCall.replace(factory.createIdentifier(paramName));
if (copy == getCall) copy = result;
}
@@ -208,21 +216,22 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
return paramName + "->" + copy.getText();
}
static String generateOptionalUnwrap(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
PsiExpression trueValue,
PsiExpression falseValue,
static String generateOptionalUnwrap(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalRef,
@NotNull PsiExpression trueValue,
@NotNull PsiExpression falseValue,
PsiType targetType) {
if (ExpressionUtils.isReferenceTo(trueValue, optionalVariable) && OptionalUtil.isOptionalEmptyCall(falseValue)) {
if (areElementsEquivalent(trueValue, optionalRef) && OptionalUtil.isOptionalEmptyCall(falseValue)) {
trueValue =
factory.createExpressionFromText(CommonClassNames.JAVA_UTIL_OPTIONAL + ".of(" + optionalVariable.getName() + ".get())", trueValue);
factory.createExpressionFromText(CommonClassNames.JAVA_UTIL_OPTIONAL + ".of(" + optionalRef.getText() + ".get())", trueValue);
}
if (ExpressionUtils.isReferenceTo(falseValue, optionalVariable)) {
if (areElementsEquivalent(falseValue, optionalRef)) {
falseValue = factory.createExpressionFromText(CommonClassNames.JAVA_UTIL_OPTIONAL + ".empty()", falseValue);
}
String lambdaText = generateOptionalLambda(factory, ct, optionalVariable, trueValue);
String lambdaText = generateOptionalLambda(factory, ct, optionalRef, trueValue);
PsiLambdaExpression lambda = (PsiLambdaExpression)factory.createExpressionFromText(lambdaText, trueValue);
return OptionalUtil.generateOptionalUnwrap(optionalVariable.getName(), lambda.getParameterList().getParameters()[0],
return OptionalUtil.generateOptionalUnwrap(optionalRef.getText(), lambda.getParameterList().getParameters()[0],
(PsiExpression)lambda.getBody(), ct.markUnchanged(falseValue), targetType, true);
}
@@ -254,8 +263,8 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
condition = BoolUtils.getNegated(condition);
invert = true;
}
PsiVariable optionalVariable = extractOptionalFromIfPresentCheck(condition);
if (optionalVariable == null) return;
PsiReferenceExpression optionalRef = extractOptionalFromIfPresentCheck(condition);
if (optionalRef == null) return;
PsiElement cond = PsiTreeUtil.getParentOfType(element, PsiIfStatement.class, PsiConditionalExpression.class);
PsiElement thenElement;
PsiElement elseElement;
@@ -266,10 +275,10 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
thenElement = invert ? ((PsiConditionalExpression)cond).getElseExpression() : ((PsiConditionalExpression)cond).getThenExpression();
elseElement = invert ? ((PsiConditionalExpression)cond).getThenExpression() : ((PsiConditionalExpression)cond).getElseExpression();
} else return;
if (myScenario.getProblemType(optionalVariable, thenElement, elseElement) == ProblemType.NONE) return;
if (myScenario.getProblemType(optionalRef, thenElement, elseElement) == ProblemType.NONE) return;
PsiElementFactory factory = JavaPsiFacade.getElementFactory(project);
CommentTracker ct = new CommentTracker();
String replacementText = myScenario.generateReplacement(factory, ct, optionalVariable, thenElement, elseElement);
String replacementText = myScenario.generateReplacement(factory, ct, optionalRef, thenElement, elseElement);
if (thenElement != null && !PsiTreeUtil.isAncestor(cond, thenElement, true)) ct.delete(thenElement);
if (elseElement != null && !PsiTreeUtil.isAncestor(cond, elseElement, true)) ct.delete(elseElement);
PsiElement result = ct.replaceAndRestoreComments(cond, replacementText);
@@ -280,27 +289,36 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
interface OptionalIsPresentCase {
ProblemType getProblemType(PsiVariable optionalVariable, PsiElement trueElement, PsiElement falseElement);
@NotNull
ProblemType getProblemType(@NotNull PsiReferenceExpression optionalVariable,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement);
String generateReplacement(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
@NotNull
String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalVariable,
PsiElement trueElement,
PsiElement falseElement);
}
static class ReturnCase implements OptionalIsPresentCase {
@NotNull
@Override
public ProblemType getProblemType(PsiVariable optionalVariable, PsiElement trueElement, PsiElement falseElement) {
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalRef,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
if (!(trueElement instanceof PsiReturnStatement) || !(falseElement instanceof PsiReturnStatement)) return ProblemType.NONE;
PsiExpression falseValue = ((PsiReturnStatement)falseElement).getReturnValue();
PsiExpression trueValue = ((PsiReturnStatement)trueElement).getReturnValue();
if (!isSimpleOrUnchecked(falseValue)) return ProblemType.NONE;
return getTypeByLambdaCandidate(optionalVariable, trueValue, falseValue);
return getTypeByLambdaCandidate(optionalRef, trueValue, falseValue);
}
@NotNull
@Override
public String generateReplacement(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
public String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct, @NotNull PsiReferenceExpression optionalVariable,
PsiElement trueElement,
PsiElement falseElement) {
PsiExpression trueValue = ((PsiReturnStatement)trueElement).getReturnValue();
@@ -314,23 +332,28 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
static class AssignmentCase implements OptionalIsPresentCase {
@NotNull
@Override
public ProblemType getProblemType(PsiVariable optionalVariable, PsiElement trueElement, PsiElement falseElement) {
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalVariable,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
PsiAssignmentExpression trueAssignment = ExpressionUtils.getAssignment(trueElement);
PsiAssignmentExpression falseAssignment = ExpressionUtils.getAssignment(falseElement);
if (trueAssignment == null || falseAssignment == null) return ProblemType.NONE;
PsiExpression falseVal = falseAssignment.getRExpression();
PsiExpression trueVal = trueAssignment.getRExpression();
if (PsiEquivalenceUtil.areElementsEquivalent(trueAssignment.getLExpression(), falseAssignment.getLExpression()) &&
if (areElementsEquivalent(trueAssignment.getLExpression(), falseAssignment.getLExpression()) &&
isSimpleOrUnchecked(falseVal)) {
return getTypeByLambdaCandidate(optionalVariable, trueVal, falseVal);
}
return ProblemType.NONE;
}
@NotNull
@Override
public String generateReplacement(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
public String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalRef,
PsiElement trueElement,
PsiElement falseElement) {
PsiAssignmentExpression trueAssignment = ExpressionUtils.getAssignment(trueElement);
@@ -340,14 +363,18 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
PsiExpression lValue = trueAssignment.getLExpression();
PsiExpression trueValue = trueAssignment.getRExpression();
PsiExpression falseValue = falseAssignment.getRExpression();
LOG.assertTrue(trueValue != null);
LOG.assertTrue(falseValue != null);
return lValue.getText() + " = " + generateOptionalUnwrap(factory, ct, optionalVariable, trueValue, falseValue, lValue.getType()) + ";";
return lValue.getText() + " = " + generateOptionalUnwrap(factory, ct, optionalRef, trueValue, falseValue, lValue.getType()) + ";";
}
}
static class TernaryCase implements OptionalIsPresentCase {
@NotNull
@Override
public ProblemType getProblemType(PsiVariable optionalVariable, PsiElement trueElement, PsiElement falseElement) {
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalVariable,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
if(!(trueElement instanceof PsiExpression) || !(falseElement instanceof PsiExpression)) return ProblemType.NONE;
PsiExpression trueExpression = (PsiExpression)trueElement;
PsiExpression falseExpression = (PsiExpression)falseElement;
@@ -359,9 +386,11 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
return getTypeByLambdaCandidate(optionalVariable, trueExpression, falseExpression);
}
@NotNull
@Override
public String generateReplacement(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
public String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalVariable,
PsiElement trueElement,
PsiElement falseElement) {
PsiExpression ternary = PsiTreeUtil.getParentOfType(trueElement, PsiConditionalExpression.class);
@@ -373,24 +402,29 @@ public class OptionalIsPresentInspection extends AbstractBaseJavaLocalInspection
}
static class ConsumerCase implements OptionalIsPresentCase {
@NotNull
@Override
public ProblemType getProblemType(PsiVariable optionalVariable, PsiElement trueElement, PsiElement falseElement) {
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalRef,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
if (falseElement != null && !(falseElement instanceof PsiEmptyStatement)) return ProblemType.NONE;
if (!(trueElement instanceof PsiStatement)) return ProblemType.NONE;
if (trueElement instanceof PsiExpressionStatement) {
PsiExpression expression = ((PsiExpressionStatement)trueElement).getExpression();
if(isOptionalGetCall(expression, optionalVariable)) return ProblemType.NONE;
if (isOptionalGetCall(expression, optionalRef)) return ProblemType.NONE;
trueElement = expression;
}
return getTypeByLambdaCandidate(optionalVariable, trueElement, null);
return getTypeByLambdaCandidate(optionalRef, trueElement, null);
}
@NotNull
@Override
public String generateReplacement(PsiElementFactory factory,
CommentTracker ct, PsiVariable optionalVariable,
public String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalRef,
PsiElement trueElement,
PsiElement falseElement) {
return optionalVariable.getName() + ".ifPresent(" + generateOptionalLambda(factory, ct, optionalVariable, trueElement) + ");";
return optionalRef.getText() + ".ifPresent(" + generateOptionalLambda(factory, ct, optionalRef, trueElement) + ");";
}
}
}
@@ -0,0 +1,21 @@
// "Replace Optional.isPresent() condition with functional style expression" "true"
import java.util.Optional;
class Trip implements Comparable<Trip> {
Optional<Integer> originId;
public Trip(Optional<Integer> originId) {
this.originId = originId;
}
public Optional<Integer> getOriginId() {
return originId;
}
@Override
public int compareTo(Trip o) {
return this.originId.isPresent() ?
(o.originId.map(integer -> Integer.compare(this.originId.get(), integer)).orElse(-1)) :
(o.originId.isPresent() ? 1 : 0);
}
}
@@ -0,0 +1,21 @@
// "Replace Optional.isPresent() condition with functional style expression" "true"
import java.util.Optional;
class Trip implements Comparable<Trip> {
Optional<Integer> originId;
public Trip(Optional<Integer> originId) {
this.originId = originId;
}
public Optional<Integer> getOriginId() {
return originId;
}
@Override
public int compareTo(Trip o) {
return this.originId.isPresent() ?
(o.originId.is<caret>Present() ? Integer.compare(this.originId.get(), o.originId.get()) : -1) :
(o.originId.isPresent() ? 1 : 0);
}
}