[java-inspections] Non functional style Optional.isPresent - missed opportunity

IDEA-360860 fixed

* split tests into individual methods, to make debugging easier
* add javadoc for VariableAccessUtils#isVariableTypeChangeSafeForReference


(cherry picked from commit f6b3468079959f7adec737e0b8ec3ad1d97e7ad2)

IJ-MR-150176

GitOrigin-RevId: 4b430e49819c7cba1e08ecee47b881f05be570d4
This commit is contained in:
Bartek Pacia
2024-11-28 23:23:40 +00:00
committed by intellij-monorepo-bot
parent 1e5e5b9bff
commit 6682d9efc3
20 changed files with 349 additions and 39 deletions
@@ -598,7 +598,31 @@ public final class VariableAccessUtils {
.allMatch(context -> context == null || PsiTreeUtil.isAncestor(context, block, false));
}
static boolean isVariableTypeChangeSafeForReference(@NotNull PsiType targetType, @NotNull PsiReferenceExpression reference) {
/// Returns true if the type of element that `reference` refers to can be safely changed to `targetType`.
///
/// ### Example
///
/// Consider the following (obviously wrong, but good enough for demo purpose) code:
///
/// ```java
/// void foo(Optional<String> opt) {
/// Object obj = opt.get();
/// if (obj instanceof Integer) {
/// doStuff();
/// }
/// // ...
/// }
/// ```
///
/// Let's say you'd like to know if it's "safe" to change the type of local variable `obj` from `Object` to `String`.
/// "Safe" means that there likely will be no compile-time errors or behavior difference after such a type change is performed.
///
/// In our example, this method returns false because changing the type of `obj`
/// to `String` will cause a compile-time error like `cannot cast 'java.lang.String' to 'java.lang.Integer'`.
///
/// It's possible there are cases not covered by this method.
/// If you discover such a case, consider updating the implementation.
public static boolean isVariableTypeChangeSafeForReference(@NotNull PsiType targetType, @NotNull PsiReferenceExpression reference) {
PsiElement parent = PsiUtil.skipParenthesizedExprUp(reference.getParent());
if (PsiUtil.isAccessedForWriting(reference)) {
PsiAssignmentExpression assignmentExpression = tryCast(parent, PsiAssignmentExpression.class);
@@ -670,4 +694,4 @@ public final class VariableAccessUtils {
return usedVariables;
}
}
}
}
@@ -1,4 +1,4 @@
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInspection;
import com.intellij.codeInsight.Nullability;
@@ -27,7 +27,9 @@ import org.jetbrains.annotations.Nls;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import static com.intellij.codeInsight.PsiEquivalenceUtil.areElementsEquivalent;
@@ -47,12 +49,12 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
WARNING, INFO, NONE;
void registerProblem(@NotNull ProblemsHolder holder, @NotNull PsiExpression condition, OptionalIsPresentCase scenario) {
if(this != NONE) {
if (this != NONE) {
if (this == INFO && !holder.isOnTheFly()) {
return; //don't register fixes in batch mode
}
holder.registerProblem(condition, JavaBundle.message(
"inspection.message.can.be.replaced.with.single.expression.in.functional.style"),
"inspection.message.can.be.replaced.with.single.expression.in.functional.style"),
this == INFO ? ProblemHighlightType.INFORMATION : ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
new OptionalIsPresentFix(scenario));
}
@@ -137,7 +139,8 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
PsiMethodCallExpression call = ObjectUtils.tryCast(condition, PsiMethodCallExpression.class);
if (!OPTIONAL_IS_PRESENT.matches(call) && !OPTIONAL_IS_EMPTY.matches(call)) return null;
PsiReferenceExpression qualifier =
ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()), PsiReferenceExpression.class);
ObjectUtils.tryCast(PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()),
PsiReferenceExpression.class);
if (qualifier == null) return null;
PsiElement element = qualifier.resolve();
if (!(element instanceof PsiVariable) || isRaw((PsiVariable)element)) return null;
@@ -174,6 +177,7 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
areElementsEquivalent(lambdaCandidate, optionalRef) && OptionalUtil.isOptionalEmptyCall(falseExpression)) {
return ProblemType.WARNING;
}
if (!LambdaGenerationUtil.canBeUncheckedLambda(lambdaCandidate, optionalRef::isReferenceTo)) return ProblemType.NONE;
Ref<Boolean> hasOptionalReference = new Ref<>(Boolean.FALSE);
boolean hasNoBadRefs = PsiTreeUtil.processElements(lambdaCandidate, e -> {
@@ -183,10 +187,15 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
hasOptionalReference.set(Boolean.TRUE);
return isOptionalGetCall(e.getParent().getParent(), optionalRef);
});
if (hasOptionalReference.get() && getOnlyLocalVariable(lambdaCandidate, optionalRef) != null) {
return ProblemType.WARNING;
}
if (!hasNoBadRefs) return ProblemType.NONE;
if (!hasOptionalReference.get() || !(lambdaCandidate instanceof PsiExpression expression)) return ProblemType.INFO;
if (falseExpression != null) {
// falseExpression == null is "consumer" case (to be replaced with ifPresent())
// falseExpression == null is a "consumer" case (to be replaced with ifPresent())
if (!ExpressionUtils.isNullLiteral(falseExpression) &&
NullabilityUtil.getExpressionNullability(expression, true) != Nullability.NOT_NULL) {
// if falseExpression is null literal, then semantics is preserved
@@ -197,7 +206,7 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
if (falseType == null || trueType == null) return ProblemType.NONE;
if (falseType instanceof PsiPrimitiveType && trueType instanceof PsiPrimitiveType) {
if (falseType.equals(trueType) || JavaPsiMathUtil.getNumberFromLiteral(falseExpression) != null) {
// like x ? double_expression : integer_expression; support only if integer_expression is simple literal,
// like x ? double_expression : integer_expression; support only if integer_expression is a simple literal,
// so could be converted explicitly to double
return ProblemType.WARNING;
}
@@ -210,6 +219,42 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
return ProblemType.WARNING;
}
/// Returns non-null if `lambdaCandidate` is a special case from IDEA-360860.
///
/// If the returned [PsiLocalVariable] is non-null, then this local variable:
/// * is the only local variable declaration inside `lambdaCandidate`
/// * it's initialized with a call to [Optional#get()] on `optionalRef`
private static @Nullable PsiLocalVariable getOnlyLocalVariable(@NotNull PsiElement lambdaCandidate,
@NotNull PsiReferenceExpression optionalRef) {
if (!(lambdaCandidate instanceof PsiBlockStatement blockStmt)) return null;
PsiStatement[] statements = blockStmt.getCodeBlock().getStatements();
if (statements.length == 2 &&
statements[0] instanceof PsiDeclarationStatement declStmt &&
declStmt.getFirstChild() instanceof PsiLocalVariable localVariable &&
statements[1] instanceof PsiExpressionStatement exprStmt) {
// Check that the Optional variable is referenced indirectly: first assigned to variable with get() call, then the variable is used
PsiExpression localVariableInitializer = localVariable.getInitializer();
if (localVariableInitializer == null) return null;
if (!VariableAccessUtils.variableIsUsed(localVariable, exprStmt)) return null;
if (!isOptionalGetCall(localVariableInitializer, optionalRef)) return null;
List<PsiReferenceExpression> varReferences = VariableAccessUtils.getVariableReferences(localVariable);
PsiType unwrappedOptionalType = PsiUtil.substituteTypeParameter(optionalRef.getType(), CommonClassNames.JAVA_UTIL_OPTIONAL, 0, false);
if (unwrappedOptionalType == null) return null;
for (PsiReferenceExpression varReference : varReferences) {
if (!VariableAccessUtils.isVariableTypeChangeSafeForReference(unwrappedOptionalType, varReference)) {
return null;
}
}
return localVariable;
}
return null;
}
@NotNull
static String generateOptionalLambda(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@@ -217,8 +262,19 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
PsiElement trueValue) {
PsiType type = optionalRef.getType();
String paramName = new VariableNameGenerator(trueValue, VariableKind.PARAMETER)
.byType(OptionalUtil.getOptionalElementType(type)).byName("value").generate(true);
if(trueValue instanceof PsiExpressionStatement) {
.byType(OptionalUtil.getOptionalElementType(type))
.byName("value")
.generate(true);
PsiLocalVariable theOnlyVariable = getOnlyLocalVariable(trueValue, optionalRef);
if (theOnlyVariable != null && trueValue instanceof PsiBlockStatement blockStmt) {
paramName = theOnlyVariable.getName();
PsiStatement varDecl = blockStmt.getCodeBlock().getStatements()[0];
varDecl.delete();
trueValue = blockStmt.getCodeBlock().getStatements()[0];
}
if (trueValue instanceof PsiExpressionStatement) {
trueValue = ((PsiExpressionStatement)trueValue).getExpression();
}
ct.markUnchanged(trueValue);
@@ -227,8 +283,8 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
PsiElement result = getCall.replace(factory.createIdentifier(paramName));
if (copy == getCall) copy = result;
}
if(copy instanceof PsiStatement && !(copy instanceof PsiBlockStatement)) {
return paramName + "->{" + copy.getText()+"}";
if (copy instanceof PsiStatement && !(copy instanceof PsiBlockStatement)) {
return paramName + "->{" + copy.getText() + "}";
}
return paramName + "->" + copy.getText();
}
@@ -314,7 +370,9 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalRef,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
if (!(trueElement instanceof PsiReturnStatement trueReturn) || !(falseElement instanceof PsiReturnStatement falseReturn)) return ProblemType.NONE;
if (!(trueElement instanceof PsiReturnStatement trueReturn) || !(falseElement instanceof PsiReturnStatement falseReturn)) {
return ProblemType.NONE;
}
PsiExpression falseValue = falseReturn.getReturnValue();
PsiExpression trueValue = trueReturn.getReturnValue();
if (!isSimpleOrUnchecked(falseValue)) return ProblemType.NONE;
@@ -332,7 +390,8 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
LOG.assertTrue(trueValue != null);
LOG.assertTrue(falseValue != null);
return "return " +
generateOptionalUnwrap(factory, ct, optionalVariable, trueValue, falseValue, PsiTypesUtil.getMethodReturnType(trueElement)) +
generateOptionalUnwrap(factory, ct, optionalVariable, trueValue, falseValue,
PsiTypesUtil.getMethodReturnType(trueElement)) +
";";
}
},
@@ -381,7 +440,9 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
public ProblemType getProblemType(@NotNull PsiReferenceExpression optionalVariable,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement) {
if(!(trueElement instanceof PsiExpression trueExpression) || !(falseElement instanceof PsiExpression falseExpression)) return ProblemType.NONE;
if (!(trueElement instanceof PsiExpression trueExpression) || !(falseElement instanceof PsiExpression falseExpression)) {
return ProblemType.NONE;
}
PsiType trueType = trueExpression.getType();
PsiType falseType = falseExpression.getType();
if (trueType == null || falseType == null || !trueType.isAssignableFrom(falseType) || !isSimpleOrUnchecked(falseExpression)) {
@@ -413,8 +474,8 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
@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 (trueElement instanceof PsiExpressionStatement expressionStmt) {
PsiExpression expression = expressionStmt.getExpression();
if (isOptionalGetCall(expression, optionalRef)) return ProblemType.NONE;
trueElement = expression;
}
@@ -434,14 +495,14 @@ public final class OptionalIsPresentInspection extends AbstractBaseJavaLocalInsp
@NotNull
abstract ProblemType getProblemType(@NotNull PsiReferenceExpression optionalVariable,
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement);
@Nullable PsiElement trueElement,
@Nullable PsiElement falseElement);
@NotNull
abstract String generateReplacement(@NotNull PsiElementFactory factory,
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalVariable,
PsiElement trueElement,
PsiElement falseElement);
@NotNull CommentTracker ct,
@NotNull PsiReferenceExpression optionalVariable,
PsiElement trueElement,
PsiElement falseElement);
}
}
@@ -1,4 +1,4 @@
// Copyright 2000-2021 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file.
// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.psi.util;
import com.intellij.openapi.util.Comparing;
@@ -0,0 +1,12 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
str.ifPresent(this::use);
}
void use(Object obj) { System.out.println("Object"); }
}
@@ -0,0 +1,12 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
str.ifPresent(obj -> use(obj + ":" + obj));
}
void use(String obj) { System.out.println("String"); }
}
@@ -0,0 +1,18 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
str.ifPresent(this::use);
}
void use(Object obj) {
System.out.println("Object");
}
void use(String obj, int i) {
System.out.println("String");
}
}
@@ -0,0 +1,15 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
if (str.isPrese<caret>nt()) {
String obj = str.get();
use(obj);
}
}
void use(Object obj) { System.out.println("Object"); }
}
@@ -0,0 +1,15 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
if (str.isPrese<caret>nt()) {
String obj = str.get();
use(obj + ":" + obj);
}
}
void use(String obj) { System.out.println("String"); }
}
@@ -0,0 +1,21 @@
// "Replace Optional presence condition with functional style expression" "GENERIC_ERROR_OR_WARNING"
import java.util.*;
public class Main {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
if (str.isPrese<caret>nt()) {
Object obj = str.get();
use(obj);
}
}
void use(Object obj) {
System.out.println("Object");
}
void use(String obj, int i) {
System.out.println("String");
}
}
@@ -1,9 +1,8 @@
// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
import java.lang.invoke.MethodHandle;
import java.util.Optional;
public class OptionalIsPresent {
class OptionalIsPresent {
public void testIsPresent(Optional<String> str) {
String val;
if (<warning descr="Can be replaced with single expression in functional style">str.isPresent()</warning>) {
@@ -13,14 +12,4 @@ public class OptionalIsPresent {
}
System.out.println(val);
}
public void testOptional(Optional<String> str) {
String val;
if (<warning descr="Can be replaced with single expression in functional style">str.isEmpty()</warning>) {
val = "";
} else {
val = str.get();
}
System.out.println(val);
}
}
}
@@ -0,0 +1,10 @@
import java.util.Optional;
class OptionalIsPresent_2 {
public void foo() {
Optional<Object> opt = Optional.ofNullable(Math.random() > 0.5 ? new Object() : null);
if (<warning descr="Can be replaced with single expression in functional style">opt.isPresent()</warning>) {
System.out.println(opt.get());
}
}
}
@@ -0,0 +1,13 @@
import java.util.Optional;
class OptionalIsPresent {
public void foo() {
Optional<Object> opt = Optional.ofNullable(Math.random() > 0.5 ? new Object() : null);
if (<warning descr="Can be replaced with single expression in functional style">opt.isPresent()</warning>) {
Object obj = opt.get();
use(obj);
}
}
void use(Object obj) { System.out.println("Object"); }
}
@@ -0,0 +1,15 @@
import java.util.Optional;
class OptionalIsPresent {
public void testOptional(Optional<String> str) {
if (str == null) str = Optional.empty();
if (str.isPresent()) {
Object obj = str.get();
use(obj, 1); // resolves to use(Object)
}
}
void use(Object obj, int i) { System.out.println("Object"); }
void use(String obj, int i) { System.out.println("String"); }
}
@@ -0,0 +1,14 @@
import java.util.Optional;
class OptionalIsPresent {
public void foo() {
Optional<Object> opt = Optional.ofNullable(Math.random() > 0.5 ? new Object() : null);
if (<warning descr="Can be replaced with single expression in functional style">opt.isPresent()</warning>) {
// who dares wins
Object obj = opt.get();
use(obj);
}
}
void use(Object obj) { System.out.println("Object"); }
}
@@ -0,0 +1,20 @@
import java.util.Optional;
class Super {
private int x;
public void foo(Optional<Sub> opt) {
if (opt == null) opt = Optional.empty();
if (opt.isPresent()) {
// Changing type of `obj` from Super to Sub will cause a compile-time error (field x is private). Hence, don't suggest fix.
Super obj = opt.get();
use(obj.x);
}
}
void use(Object obj) {
System.out.println("Object");
}
}
class Sub extends Super {}
@@ -0,0 +1,12 @@
import java.util.Optional;
class OptionalIsPresent {
public void foo() {
Optional<Object> opt = Optional.ofNullable(Math.random() > 0.5 ? new Object() : null);
if (opt.isPresent()) {
Object obj = opt.get();
Object a = opt.get();
System.out.println(opt.get());
}
}
}
@@ -0,0 +1,13 @@
import java.util.Optional;
class OptionalIsPresent {
public void foo() {
Optional<Object> opt = Optional.ofNullable(Math.random() > 0.5 ? new Object() : null);
if (opt.isPresent()) {
Object obj = opt.get();
Object obj2 = opt.get();
System.out.println(obj);
System.out.println(obj2);
}
}
}
@@ -0,0 +1,14 @@
import java.util.Optional;
class OptionalIsPresent {
public void testOptional(Optional<String> opt) {
if (opt == null) opt = Optional.empty();
if (opt.isPresent()) {
// Changing type of `obj` from Object to String will cause a compile-time error on instanceof. Hence, don't suggest fix.
Object obj = opt.get();
use(obj instanceof Integer ? "foo" : "bar");
}
}
void use(String obj) { System.out.println("String"); }
}
@@ -0,0 +1,13 @@
import java.util.Optional;
class OptionalIsPresent {
public void foo(Optional<String> str) {
String val;
if (<warning descr="Can be replaced with single expression in functional style">str.isEmpty()</warning>) {
val = "";
} else {
val = str.get();
}
System.out.println(val);
}
}
@@ -1,4 +1,4 @@
// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
// Copyright 2000-2024 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.java.codeInspection;
import com.intellij.JavaTestUtil;
@@ -27,5 +27,24 @@ public class OptionalIsPresentInspectionTest extends LightJavaInspectionTestCase
return JAVA_11_ANNOTATED;
}
public void testOptionalIsPresent() { doTest(); }
public void testAssignment() { doTest(); }
public void testConsumer() { doTest(); }
public void testConsumerIndirect() { doTest(); }
public void testConsumerIndirectAmbiguousOverload() { doTest(); }
public void testConsumerIndirectComment() { doTest(); }
public void testConsumerIndirectFieldInaccessible() { doTest(); }
public void testConsumerIndirectTooComplex_1() { doTest(); }
public void testConsumerIndirectTooComplex_2() { doTest(); }
public void testConsumerIndirectTypeMismatch() { doTest(); }
public void testOptional() { doTest(); }
}