[java-inspections] Extract OptionalOfNullableMisuseInspection from DataFlowInspection (IDEA-319639)

GitOrigin-RevId: 202ecd80a114e93ee0ce7d4536d18f4fb43d6414
This commit is contained in:
Tagir Valeev
2023-05-16 10:53:48 +00:00
committed by intellij-monorepo-bot
parent 2f8aa0254d
commit ac1bd48b50
16 changed files with 207 additions and 122 deletions
@@ -246,8 +246,6 @@ public abstract class DataFlowInspectionBase extends AbstractBaseJavaLocalInspec
reportNullabilityProblems(reporter, problems);
reportNullableReturns(reporter, problems, scope);
reportOptionalOfNullableImprovements(reporter, visitor.getOfNullableCalls());
reportRedundantInstanceOf(visitor, reporter);
reportArrayAccessProblems(holder, visitor);
@@ -640,20 +638,6 @@ public abstract class DataFlowInspectionBase extends AbstractBaseJavaLocalInspec
return call;
}
private static void reportOptionalOfNullableImprovements(ProblemReporter reporter, Map<PsiElement, ThreeState> nullArgs) {
nullArgs.forEach((anchor, alwaysPresent) -> {
if (alwaysPresent == ThreeState.UNSURE) return;
if (alwaysPresent.toBoolean()) {
reporter.registerProblem(anchor, JavaAnalysisBundle.message("dataflow.message.passing.non.null.argument.to.optional"),
LocalQuickFix.notNullElements(DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(anchor)));
}
else {
reporter.registerProblem(anchor, JavaAnalysisBundle.message("dataflow.message.passing.null.argument.to.optional"),
LocalQuickFix.notNullElements(DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(anchor)));
}
});
}
private void reportNullableArgumentPassedToNonAnnotatedMethodRef(@NotNull ProblemReporter reporter,
@NotNull NullabilityProblem<?> problem,
@NotNull PsiMethodReferenceExpression methodRef) {
@@ -1,4 +1,4 @@
// Copyright 2000-2019 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.
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.java.JavaDfaListener;
@@ -7,7 +7,6 @@ import com.intellij.codeInspection.dataFlow.java.anchor.JavaExpressionAnchor;
import com.intellij.codeInspection.dataFlow.java.anchor.JavaMethodReferenceReturnAnchor;
import com.intellij.codeInspection.dataFlow.java.anchor.JavaSwitchLabelTakenAnchor;
import com.intellij.codeInspection.dataFlow.java.inst.InstanceofInstruction;
import com.intellij.codeInspection.dataFlow.jvm.SpecialField;
import com.intellij.codeInspection.dataFlow.jvm.descriptors.ThisDescriptor;
import com.intellij.codeInspection.dataFlow.jvm.problems.*;
import com.intellij.codeInspection.dataFlow.lang.DfaAnchor;
@@ -19,9 +18,7 @@ import com.intellij.codeInspection.dataFlow.types.DfType;
import com.intellij.codeInspection.dataFlow.types.DfTypes;
import com.intellij.codeInspection.dataFlow.value.DfaTypeValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.codeInspection.dataFlow.value.DfaValueFactory;
import com.intellij.codeInspection.dataFlow.value.DfaVariableValue;
import com.intellij.codeInspection.util.OptionalUtil;
import com.intellij.openapi.application.Application;
import com.intellij.openapi.application.ApplicationManager;
import com.intellij.openapi.diagnostic.Logger;
@@ -51,7 +48,6 @@ final class DataFlowInstructionVisitor implements JavaDfaListener {
private final Map<PsiTypeCastExpression, StateInfo> myClassCastProblems = new HashMap<>();
private final Map<PsiTypeCastExpression, TypeConstraint> myRealOperandTypes = new HashMap<>();
private final Map<ContractFailureProblem, Boolean> myFailingCalls = new HashMap<>();
private final Map<PsiElement, ThreeState> myOfNullableCalls = new HashMap<>();
private final Map<PsiAssignmentExpression, Pair<PsiType, PsiType>> myArrayStoreProblems = new HashMap<>();
private final Map<PsiArrayAccessExpression, ThreeState> myOutOfBoundsArrayAccesses = new HashMap<>();
private final Map<PsiExpression, ThreeState> myNegativeArraySizes = new HashMap<>();
@@ -169,10 +165,6 @@ final class DataFlowInstructionVisitor implements JavaDfaListener {
return myArrayStoreProblems;
}
Map<PsiElement, ThreeState> getOfNullableCalls() {
return myOfNullableCalls;
}
Map<PsiCaseLabelElement, ThreeState> getSwitchLabelsReachability() {
return mySwitchLabelsReachability;
}
@@ -295,9 +287,6 @@ final class DataFlowInstructionVisitor implements JavaDfaListener {
throw new IllegalStateException("Non-physical expression is passed");
}
}
if (expression instanceof PsiMethodCallExpression call && OptionalUtil.OPTIONAL_OF_NULLABLE.test(call)) {
processOfNullableResult(value, memState, call.getArgumentList().getExpressions()[0]);
}
PsiElement parent = PsiUtil.skipParenthesizedExprUp(expression.getParent());
if (parent instanceof PsiTypeCastExpression cast) {
TypeConstraint fact = TypeConstraint.fromDfType(memState.getDfType(value));
@@ -313,25 +302,6 @@ final class DataFlowInstructionVisitor implements JavaDfaListener {
if (context instanceof PsiMethod || context instanceof PsiLambdaExpression) {
myAlwaysReturnsNotNull = myAlwaysReturnsNotNull && !state.getDfType(value).isSuperType(DfTypes.NULL);
}
else if (context instanceof PsiMethodReferenceExpression methodRef &&
OptionalUtil.OPTIONAL_OF_NULLABLE.methodReferenceMatches(methodRef)) {
processOfNullableResult(value, state, methodRef.getReferenceNameElement());
}
}
private void processOfNullableResult(@NotNull DfaValue value, @NotNull DfaMemoryState memState, PsiElement anchor) {
DfaValueFactory factory = value.getFactory();
DfaValue optionalValue = SpecialField.OPTIONAL_VALUE.createValue(factory, value);
DfaNullability nullability = DfaNullability.fromDfType(memState.getDfType(optionalValue));
ThreeState present;
if (nullability == DfaNullability.NULL) {
present = ThreeState.NO;
}
else if (nullability == DfaNullability.NOT_NULL) {
present = ThreeState.YES;
}
else present = ThreeState.UNSURE;
myOfNullableCalls.merge(anchor, present, ThreeState::merge);
}
@Override
@@ -1,4 +1,4 @@
// 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.
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.CommonQuickFixBundle;
@@ -52,7 +52,7 @@ public final class DfaOptionalSupport {
}
@Nullable
static LocalQuickFix createReplaceOptionalOfNullableWithEmptyFix(@NotNull PsiElement anchor) {
public static LocalQuickFix createReplaceOptionalOfNullableWithEmptyFix(@NotNull PsiElement anchor) {
final PsiMethodCallExpression parent = findCallExpression(anchor);
if (parent == null) return null;
boolean jdkOptional = OptionalUtil.JDK_OPTIONAL_OF_NULLABLE.test(parent);
@@ -60,7 +60,7 @@ public final class DfaOptionalSupport {
}
@Nullable
static LocalQuickFix createReplaceOptionalOfNullableWithOfFix(@NotNull PsiElement anchor) {
public static LocalQuickFix createReplaceOptionalOfNullableWithOfFix(@NotNull PsiElement anchor) {
final PsiMethodCallExpression parent = findCallExpression(anchor);
if (parent == null) return null;
return new ReplaceOptionalCallFix("of", false);
@@ -0,0 +1,55 @@
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.AbstractBaseJavaLocalInspectionTool;
import com.intellij.codeInspection.LocalQuickFix;
import com.intellij.codeInspection.ProblemsHolder;
import com.intellij.codeInspection.dataFlow.java.anchor.JavaMethodReferenceArgumentAnchor;
import com.intellij.codeInspection.dataFlow.types.DfReferenceType;
import com.intellij.codeInspection.dataFlow.types.DfType;
import com.intellij.codeInspection.util.OptionalUtil;
import com.intellij.java.analysis.JavaAnalysisBundle;
import com.intellij.psi.*;
import com.intellij.psi.util.PsiUtil;
import com.intellij.util.ArrayUtil;
import org.jetbrains.annotations.NotNull;
public class OptionalOfNullableMisuseInspection extends AbstractBaseJavaLocalInspectionTool {
@Override
public @NotNull PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) {
return new JavaElementVisitor() {
@Override
public void visitMethodCallExpression(@NotNull PsiMethodCallExpression call) {
if (!OptionalUtil.OPTIONAL_OF_NULLABLE.matches(call)) return;
PsiExpression arg = PsiUtil.skipParenthesizedExprDown(ArrayUtil.getFirstElement(call.getArgumentList().getExpressions()));
if (arg == null) return;
DfType type = CommonDataflow.getDfType(arg);
processArgument(type, arg);
}
private void processArgument(@NotNull DfType type, @NotNull PsiElement anchor) {
DfaNullability nullability = DfaNullability.fromDfType(type);
if (nullability == DfaNullability.NOT_NULL) {
holder.registerProblem(anchor,
JavaAnalysisBundle.message("dataflow.message.passing.non.null.argument.to.optional"),
LocalQuickFix.notNullElements(DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(anchor)));
}
else if (nullability == DfaNullability.NULL) {
holder.registerProblem(anchor, JavaAnalysisBundle.message("dataflow.message.passing.null.argument.to.optional"),
LocalQuickFix.notNullElements(DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(anchor)));
}
}
@Override
public void visitMethodReferenceExpression(@NotNull PsiMethodReferenceExpression methodRef) {
if (!OptionalUtil.OPTIONAL_OF_NULLABLE.methodReferenceMatches(methodRef)) return;
CommonDataflow.DataflowResult dfr = CommonDataflow.getDataflowResult(methodRef);
if (dfr == null) return;
PsiElement anchor = methodRef.getReferenceNameElement();
if (anchor == null) return;
DfType type = dfr.getDfType(new JavaMethodReferenceArgumentAnchor(methodRef));
processArgument(type, anchor);
}
};
}
}
@@ -0,0 +1,18 @@
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.ex.InspectionElementsMergerBase;
import org.jetbrains.annotations.NonNls;
import org.jetbrains.annotations.NotNull;
public class OptionalOfNullableMisuseInspectionMerger extends InspectionElementsMergerBase {
@Override
public @NotNull String getMergedToolName() {
return "OptionalOfNullableMisuse";
}
@Override
public @NonNls String @NotNull [] getSourceToolNames() {
return new String[] {"DataFlowIssue", "ConstantConditions"};
}
}
@@ -1824,9 +1824,13 @@
bundle="messages.JavaBundle" key="inspection.data.flow.display.name"
groupKey="group.names.probable.bugs" groupBundle="messages.InspectionsBundle" enabledByDefault="true" level="WARNING"
implementationClass="com.intellij.codeInspection.dataFlow.DataFlowInspection"/>
<localInspection groupPath="Java" language="JAVA" shortName="OptionalOfNullableMisuse" bundle="messages.JavaBundle" key="inspection.data.flow.optional.of.nullable.misuse.display.name"
groupKey="group.names.probable.bugs" groupBundle="messages.InspectionsBundle" enabledByDefault="true" level="WARNING"
implementationClass="com.intellij.codeInspection.dataFlow.OptionalOfNullableMisuseInspection"/>
<localInspection groupPath="Java" language="JAVA" shortName="ConstantValue" bundle="messages.JavaBundle" key="inspection.data.flow.constant.values.display.name"
groupKey="group.names.probable.bugs" groupBundle="messages.InspectionsBundle" enabledByDefault="true" level="WARNING"
implementationClass="com.intellij.codeInspection.dataFlow.ConstantValueInspection"/>
<inspectionElementsMerger implementation="com.intellij.codeInspection.dataFlow.OptionalOfNullableMisuseInspectionMerger" />
<inspectionElementsMerger implementation="com.intellij.codeInspection.dataFlow.ConstantValueInspectionMerger" />
<inspectionElementsMerger implementation="com.intellij.codeInspection.dataFlow.DataFlowInspectionMerger" />
<localInspection groupPath="Java" language="JAVA" shortName="Java9UndeclaredServiceUsage"
@@ -0,0 +1,17 @@
<html>
<body>
Reports uses of <code>Optional.ofNullable</code> where always null or always not-null argument is passed.
There's no point in using <code>Optional.ofNullable</code> in this case: either <code>Optional.empty</code>
or <code>Optional.of</code> should be used to explicitly state the intent of creating an always-empty
or always non-empty optional respectively. It's also possible that there's a mistake in
<code>Optional.ofNullable</code> argument, so it should be examined.
<p>
Example:
</p>
<pre><code>
Optional&lt;String&gt; empty = Optional.ofNullable(null); // should be Optional.empty();
Optional&lt;String&gt; present = Optional.ofNullable("value"); // should be Optional.of("value");
</code></pre>
<!-- tooltip end -->
</body>
</html>
@@ -3,11 +3,11 @@ import java.util.*;
public class OptionalInlining {
void testOrElse() {
String s = Optional.ofNullable(<warning descr="Passing a non-null argument to 'Optional'">"foo"</warning>).orElse("bar");
String s = Optional.ofNullable("foo").orElse("bar");
if (<warning descr="Condition 's.equals(\"bar\")' is always 'false'">s.equals("bar")</warning>) {
System.out.println("Never");
}
String s2 = Optional.<String>ofNullable(<warning descr="Passing 'null' argument to 'Optional'">null</warning>).orElse("bar");
String s2 = Optional.<String>ofNullable(null).orElse("bar");
if (<warning descr="Condition 's2.equals(\"bar\")' is always 'true'">s2.equals("bar")</warning>) {
System.out.println("Always");
}
@@ -38,7 +38,7 @@ public class OptionalInlining {
}
void testGuavaOrNull() {
String s = com.google.common.base.Optional.fromNullable(<warning descr="Passing a non-null argument to 'Optional'">"foo"</warning>).orNull();
String s = com.google.common.base.Optional.fromNullable("foo").orNull();
if (<warning descr="Condition 's == null' is always 'false'">s == null</warning>) {
System.out.println("Never");
}
@@ -2,30 +2,6 @@ import java.util.List;
import java.util.Optional;
class Test {
Optional<String> getName(List<String> numbers) {
return Optional.ofNullable(
numbers.isEmpty() ?
null :
numbers.get(0));
}
Optional<String> getName2(List<String> numbers) {
return Optional.ofNullable(
numbers.isEmpty() ?
"2" :
numbers.get(0));
}
Optional<String> getName3() {
return Optional.ofNullable(<warning descr="Passing 'null' argument to 'Optional'">null</warning>);
}
long field;
Optional<Long> getName4() {
return Optional.ofNullable(<warning descr="Passing a non-null argument to 'Optional'">field</warning>);
}
// IDEA-188536
public void test(Long value1, Long value2, Long value3) {
// value1 is assumed to be nullable as passed to ofNullable
@@ -163,10 +163,6 @@ public class StreamInlining {
}
}
Optional<String> testOptionalOfNullable(List<String> list) {
return list.stream().filter(Objects::isNull).map(Optional::<warning descr="Passing 'null' argument to 'Optional'">ofNullable</warning>).findFirst().orElse(Optional.empty());
}
void testGenerate() {
List<String> list1 = Stream.generate(() -> Math.random() > 0.5 ? "foo" : "baz")
.limit(10).filter((xyz -> <warning descr="Result of '\"bar\".equals(xyz)' is always 'false'">"bar".equals(xyz)</warning>)).collect(Collectors.toList());
@@ -0,0 +1,40 @@
import java.util.*;
class Test {
Optional<String> getName(List<String> numbers) {
return Optional.ofNullable(
numbers.isEmpty() ?
null :
numbers.get(0));
}
Optional<String> getName2(List<String> numbers) {
return Optional.ofNullable(
numbers.isEmpty() ?
"2" :
numbers.get(0));
}
Optional<String> getName3() {
return Optional.ofNullable(<warning descr="Passing 'null' argument to 'Optional'">null</warning>);
}
long field;
Optional<Long> getName4() {
return Optional.ofNullable(<warning descr="Passing a non-null argument to 'Optional'">field</warning>);
}
Optional<String> getName5() {
String s = "";
return Optional.ofNullable(<warning descr="Passing a non-null argument to 'Optional'">s</warning>);
}
Optional<String> testMethodRef(List<String> list) {
return list.stream().filter(Objects::isNull).map(Optional::<warning descr="Passing 'null' argument to 'Optional'">ofNullable</warning>).findFirst().orElse(Optional.empty());
}
Optional<String> testMethodRef2(List<String> list) {
return list.stream().filter(Objects::nonNull).map(Optional::<warning descr="Passing a non-null argument to 'Optional'">ofNullable</warning>).findFirst().orElse(Optional.empty());
}
}
@@ -1,40 +0,0 @@
// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInsight.daemon.impl.quickfix
import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase5
import com.intellij.codeInspection.dataFlow.DataFlowInspection
import com.intellij.testFramework.fixtures.JavaCodeInsightTestFixture
import groovy.transform.CompileStatic
import org.junit.jupiter.api.BeforeEach
@CompileStatic
class ReplaceFromOfNullableFixTest extends LightQuickFixParameterizedTestCase5 {
@BeforeEach
void setupInspections() {
getFixture().enableInspections(DataFlowInspection.class);
if (getTestNameRule().getDisplayName().contains("Guava")) {
addGuavaOptional(getFixture());
}
}
@Override
protected String getBasePath() {
return "/codeInsight/daemonCodeAnalyzer/quickFix/replaceFromOfNullable"
}
static void addGuavaOptional(JavaCodeInsightTestFixture fixture) {
fixture.addClass("""
package com.google.common.base;
public abstract class Optional<T> {
public static <T> Optional<T> absent() { }
public static <T> Optional<T> of(@org.jetbrains.annotations.NotNull T reference) { }
public static <T> Optional<T> fromNullable(T nullableReference) { }
}
""")
}
}
@@ -0,0 +1,36 @@
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.codeInsight.daemon.impl.quickfix;
import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase5;
import com.intellij.codeInspection.dataFlow.OptionalOfNullableMisuseInspection;
import com.intellij.testFramework.fixtures.JavaCodeInsightTestFixture;
import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.BeforeEach;
public class ReplaceFromOfNullableFixTest extends LightQuickFixParameterizedTestCase5 {
@BeforeEach
public void setupInspections() {
getFixture().enableInspections(OptionalOfNullableMisuseInspection.class);
if (getTestNameRule().getDisplayName().contains("Guava")) {
addGuavaOptional(getFixture());
}
}
@Override
protected @NotNull String getBasePath() {
return "/codeInsight/daemonCodeAnalyzer/quickFix/replaceFromOfNullable";
}
public static void addGuavaOptional(JavaCodeInsightTestFixture fixture) {
fixture.addClass("""
package com.google.common.base;
public abstract class Optional<T> {
public static <T> Optional<T> absent() { }
public static <T> Optional<T> of(@org.jetbrains.annotations.NotNull T reference) { }
public static <T> Optional<T> fromNullable(T nullableReference) { }
}
""");
}
}
@@ -4,6 +4,7 @@ package com.intellij.java.codeInspection;
import com.intellij.codeInsight.daemon.impl.quickfix.*;
import com.intellij.java.codeInsight.completion.NormalCompletionDfaTest;
import com.intellij.java.codeInsight.completion.SmartTypeCompletionDfaTest;
import com.intellij.java.codeInspection.dataFlow.OptionalOfNullableMisuseInspectionTest;
import com.intellij.java.slicer.SliceBackwardTest;
import com.intellij.java.slicer.SliceTreeTest;
import com.siyeh.ig.redundancy.RedundantOperationOnEmptyContainerInspectionTest;
@@ -61,7 +62,8 @@ import org.junit.platform.suite.api.Suite;
DeleteSwitchLabelFixTest.class,
DeleteRedundantUpdateFixTest.class,
ReplaceTypeInCastFixTest.class,
ReplaceMinMaxWithArgumentFixTest.class
ReplaceMinMaxWithArgumentFixTest.class,
OptionalOfNullableMisuseInspectionTest.class
})
public class DataFlowInspectionTestSuite {
}
@@ -0,0 +1,26 @@
// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license.
package com.intellij.java.codeInspection.dataFlow;
import com.intellij.JavaTestUtil;
import com.intellij.codeInspection.dataFlow.OptionalOfNullableMisuseInspection;
import com.intellij.testFramework.LightProjectDescriptor;
import com.intellij.testFramework.fixtures.LightJavaCodeInsightFixtureTestCase;
import org.jetbrains.annotations.NotNull;
public class OptionalOfNullableMisuseInspectionTest extends LightJavaCodeInsightFixtureTestCase {
@Override
protected String getTestDataPath() {
return JavaTestUtil.getJavaTestDataPath() + "/inspection/dataFlow/optionalOfNullable/";
}
@Override
protected @NotNull LightProjectDescriptor getProjectDescriptor() {
return JAVA_LATEST_WITH_LATEST_JDK;
}
public void testOptionalOfNullableMisuse() {
myFixture.configureByFile(getTestName(false) + ".java");
myFixture.enableInspections(new OptionalOfNullableMisuseInspection());
myFixture.checkHighlighting();
}
}
@@ -374,6 +374,7 @@ inspection.conditional.break.in.infinite.loop.allow.condition.fusion=Allow mergi
inspection.conditional.break.in.infinite.loop.suggest.conversion.when.if.is.single.stmt.in.loop=Suggest conversion when 'if' is a single statement in loop
inspection.convert.to.local.quickfix=Convert to local
inspection.data.flow.display.name=Nullability and data flow problems
inspection.data.flow.optional.of.nullable.misuse.display.name=Use of Optional.ofNullable with null or not-null argument
inspection.data.flow.constant.values.display.name=Constant values
inspection.data.flow.filter.notnull.quickfix=Insert 'filter(Objects::nonNull)' step
inspection.data.flow.nullable.quickfix.option=Suggest @Nullable annotation for methods/fields/parameters where nullable values are used