From 588d978362a9df8a41b34550bde1f732c4718926 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Wed, 31 May 2017 17:48:57 +0700 Subject: [PATCH] OptionalIsPresentInspection: do not warn if map() part can be null If we cannot determine the non-nullity of map expression, then info level is used. Fixes IDEA-172609 "Replace Optional.isPresent() checks with functional-style expressions" is broken. --- .../codeInspection/dataFlow/NullnessUtil.java | 24 +++++++++++++++---- .../OptionalIsPresentInspection.java | 10 ++++++-- .../optionalIsPresent/afterTernary.java | 2 +- .../afterTernaryFunction.java | 13 ++++++++++ .../afterTernaryFunctionNotNull.java | 14 +++++++++++ .../optionalIsPresent/beforeTernary.java | 2 +- .../beforeTernaryFunction.java | 13 ++++++++++ .../beforeTernaryFunctionNotNull.java | 14 +++++++++++ .../OptionalIsPresentInspectionTest.java | 18 ++++++++++++++ 9 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunction.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunctionNotNull.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunction.java create mode 100644 java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunctionNotNull.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullnessUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullnessUtil.java index a1a599673dfa..e918d50bb218 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullnessUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullnessUtil.java @@ -74,7 +74,7 @@ public class NullnessUtil { if (DfaPsiUtil.isFinalField(field)) { PsiExpression initializer = field.getInitializer(); if (initializer != null) { - return getFieldInitializerNullness(initializer); + return getExpressionNullness(initializer); } List initializers = DfaPsiUtil.findAllConstructorInitializers(field); @@ -83,7 +83,7 @@ public class NullnessUtil { } for (PsiExpression expression : initializers) { - if (getFieldInitializerNullness(expression) == Nullness.NULLABLE) { + if (getExpressionNullness(expression) == Nullness.NULLABLE) { return Nullness.NULLABLE; } } @@ -129,13 +129,29 @@ public class NullnessUtil { return result != PsiSearchHelper.SearchCostResult.TOO_MANY_OCCURRENCES; } - private static Nullness getFieldInitializerNullness(@NotNull PsiExpression expression) { + public static Nullness getExpressionNullness(@Nullable PsiExpression expression) { + expression = PsiUtil.skipParenthesizedExprDown(expression); + if (expression == null) return Nullness.UNKNOWN; if (expression.textMatches(PsiKeyword.NULL)) return Nullness.NULLABLE; if (expression instanceof PsiNewExpression || expression instanceof PsiLiteralExpression || - expression instanceof PsiPolyadicExpression) { + expression instanceof PsiPolyadicExpression || + expression instanceof PsiFunctionalExpression || + expression.getType() instanceof PsiPrimitiveType) { return Nullness.NOT_NULL; } + if (expression instanceof PsiConditionalExpression) { + PsiExpression thenExpression = ((PsiConditionalExpression)expression).getThenExpression(); + PsiExpression elseExpression = ((PsiConditionalExpression)expression).getElseExpression(); + if (thenExpression == null || elseExpression == null) return Nullness.UNKNOWN; + Nullness left = getExpressionNullness(thenExpression); + if (left == Nullness.UNKNOWN) return Nullness.UNKNOWN; + Nullness right = getExpressionNullness(elseExpression); + return left == right ? left : Nullness.UNKNOWN; + } + if (expression instanceof PsiTypeCastExpression) { + return getExpressionNullness(((PsiTypeCastExpression)expression).getOperand()); + } if (expression instanceof PsiReferenceExpression) { PsiElement target = ((PsiReferenceExpression)expression).resolve(); return DfaPsiUtil.getElementNullability(expression.getType(), (PsiModifierListOwner)target); diff --git a/java/java-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java b/java/java-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java index 20f2f66f0ef7..645d4dc8b713 100644 --- a/java/java-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/OptionalIsPresentInspection.java @@ -16,6 +16,8 @@ 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; import com.intellij.codeInspection.util.OptionalUtil; import com.intellij.openapi.diagnostic.Logger; @@ -189,8 +191,12 @@ public class OptionalIsPresentInspection extends BaseJavaBatchLocalInspectionToo return isOptionalGetCall(e.getParent().getParent(), optionalVariable); }); if(!hasNoBadRefs) return ProblemType.NONE; - if(hasOptionalReference.get() && lambdaCandidate instanceof PsiExpression) return ProblemType.WARNING; - return ProblemType.INFO; + if (!hasOptionalReference.get() || !(lambdaCandidate instanceof PsiExpression)) return ProblemType.INFO; + PsiExpression expression = (PsiExpression)lambdaCandidate; + if (!PsiType.VOID.equals(expression.getType()) && NullnessUtil.getExpressionNullness(expression) != Nullness.NOT_NULL) { + return ProblemType.INFO; + } + return ProblemType.WARNING; } @NotNull diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernary.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernary.java index 70cbb3f5b4d1..dd0822bc02b9 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernary.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernary.java @@ -1,4 +1,4 @@ -// "Replace Optional.isPresent() condition with functional style expression" "true" +// "Replace Optional.isPresent() condition with functional style expression" "GENERIC_ERROR_OR_WARNING" import java.util.Arrays; import java.util.List; diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunction.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunction.java new file mode 100644 index 000000000000..22c4ffc5d320 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunction.java @@ -0,0 +1,13 @@ +// "Replace Optional.isPresent() condition with functional style expression" "INFORMATION" + +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + +public class Main { + + public void test(Optional opt, Function onPresent, Supplier onEmpty) { + // information level: could be semantic change if onPresent returns null + Object o = opt.map(onPresent::apply).orElseGet(onEmpty::get); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunctionNotNull.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunctionNotNull.java new file mode 100644 index 000000000000..35fd739011b8 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/afterTernaryFunctionNotNull.java @@ -0,0 +1,14 @@ +// "Replace Optional.isPresent() condition with functional style expression" "GENERIC_ERROR_OR_WARNING" + +import org.jetbrains.annotations.NotNull; + +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + +public class Main { + + public void test(Optional opt, Function onPresent, Supplier onEmpty) { + Object o = opt.map(onPresent::apply).orElseGet(onEmpty::get); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernary.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernary.java index be2bead547ca..95b4262fcdaf 100644 --- a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernary.java +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernary.java @@ -1,4 +1,4 @@ -// "Replace Optional.isPresent() condition with functional style expression" "true" +// "Replace Optional.isPresent() condition with functional style expression" "GENERIC_ERROR_OR_WARNING" import java.util.Arrays; import java.util.List; diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunction.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunction.java new file mode 100644 index 000000000000..1a2bd8845f25 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunction.java @@ -0,0 +1,13 @@ +// "Replace Optional.isPresent() condition with functional style expression" "INFORMATION" + +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + +public class Main { + + public void test(Optional opt, Function onPresent, Supplier onEmpty) { + // information level: could be semantic change if onPresent returns null + Object o = opt.isPresent() ? onPresent.apply(opt.get()) : onEmpty.get(); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunctionNotNull.java b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunctionNotNull.java new file mode 100644 index 000000000000..6932fbdec531 --- /dev/null +++ b/java/java-tests/testData/codeInsight/daemonCodeAnalyzer/quickFix/optionalIsPresent/beforeTernaryFunctionNotNull.java @@ -0,0 +1,14 @@ +// "Replace Optional.isPresent() condition with functional style expression" "GENERIC_ERROR_OR_WARNING" + +import org.jetbrains.annotations.NotNull; + +import java.util.Optional; +import java.util.function.Function; +import java.util.function.Supplier; + +public class Main { + + public void test(Optional opt, Function onPresent, Supplier onEmpty) { + Object o = opt.isPresent() ? onPresent.apply(opt.get()) : onEmpty.get(); + } +} \ No newline at end of file diff --git a/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/OptionalIsPresentInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/OptionalIsPresentInspectionTest.java index b51424efabb1..b2e43090711b 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/OptionalIsPresentInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInsight/daemon/quickFix/OptionalIsPresentInspectionTest.java @@ -18,10 +18,28 @@ package com.intellij.java.codeInsight.daemon.quickFix; import com.intellij.codeInsight.daemon.quickFix.LightQuickFixParameterizedTestCase; import com.intellij.codeInspection.LocalInspectionTool; import com.intellij.codeInspection.OptionalIsPresentInspection; +import com.intellij.openapi.projectRoots.Sdk; +import com.intellij.testFramework.IdeaTestUtil; +import com.intellij.testFramework.LightProjectDescriptor; +import com.intellij.testFramework.PsiTestUtil; +import com.intellij.testFramework.fixtures.DefaultLightProjectDescriptor; import org.jetbrains.annotations.NotNull; public class OptionalIsPresentInspectionTest extends LightQuickFixParameterizedTestCase { + private static final DefaultLightProjectDescriptor PROJECT_DESCRIPTOR = new DefaultLightProjectDescriptor() { + @Override + public Sdk getSdk() { + return PsiTestUtil.addJdkAnnotations(IdeaTestUtil.getMockJdk18()); + } + }; + + @NotNull + @Override + protected LightProjectDescriptor getProjectDescriptor() { + return PROJECT_DESCRIPTOR; + } + @NotNull @Override protected LocalInspectionTool[] configureLocalInspectionTools() {