From 90fa05f67b5d6d0845db3b7229afcc503051c56f Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 8 Sep 2017 10:39:55 +0700 Subject: [PATCH] IDEA-176397 Warn about pointless rewrapping of Optional value Also fixed CommonDataflow: facts must be united, not intersected --- .../dataFlow/CommonDataflow.java | 9 ++--- .../codeInspection/dataFlow/DfaFactMap.java | 35 ++++++++++++------- .../codeInspection/dataFlow/DfaFactType.java | 18 ++++++++++ .../SimplifyOptionalCallChainsInspection.java | 25 ++++++++++--- .../optionalChains/afterRewrapGet.java | 10 ++++++ .../optionalChains/afterRewrapGetSecond.java | 9 +++++ .../optionalChains/beforeRewrapGet.java | 10 ++++++ .../optionalChains/beforeRewrapGetFirst.java | 9 +++++ .../optionalChains/beforeRewrapGetSecond.java | 9 +++++ .../optionalChains/beforeRewrapGetUnsure.java | 10 ++++++ 10 files changed, 123 insertions(+), 21 deletions(-) create mode 100644 java/java-tests/testData/inspection/optionalChains/afterRewrapGet.java create mode 100644 java/java-tests/testData/inspection/optionalChains/afterRewrapGetSecond.java create mode 100644 java/java-tests/testData/inspection/optionalChains/beforeRewrapGet.java create mode 100644 java/java-tests/testData/inspection/optionalChains/beforeRewrapGetFirst.java create mode 100644 java/java-tests/testData/inspection/optionalChains/beforeRewrapGetSecond.java create mode 100644 java/java-tests/testData/inspection/optionalChains/beforeRewrapGetUnsure.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java index b1d5755a941d..ccee47604b6c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CommonDataflow.java @@ -37,10 +37,11 @@ public class CommonDataflow { void add(PsiExpression expression, DfaMemoryStateImpl memState) { DfaFactMap existing = myFacts.get(expression); - if(existing == null && myFacts.containsKey(expression)) return; // bottom - DfaValue value = memState.peek(); - DfaFactMap newMap = memState.getFactMap(value); - myFacts.put(expression, DfaFactMap.intersect(existing == null ? DfaFactMap.EMPTY : existing, newMap)); + if(existing != DfaFactMap.EMPTY) { + DfaValue value = memState.peek(); + DfaFactMap newMap = memState.getFactMap(value); + myFacts.put(expression, existing == null ? newMap : existing.union(newMap)); + } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactMap.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactMap.java index a7b5734586fe..c255b676f2d6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactMap.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactMap.java @@ -22,7 +22,6 @@ import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.List; import java.util.Objects; /** @@ -112,19 +111,31 @@ public final class DfaFactMap { return newFact == null ? null : with(type, newFact); } - private DfaFactMap intersect(DfaFactMap otherMap, @NotNull DfaFactType type) { - return intersect(type, otherMap.get(type)); + /** + * Returns a fact map which additionally allows having supplied value for the supplied fact + * + * @param type a type of a new fact + * @param value an additional fact value which should be allowed. Passing null means that fact may have any value + * @param a fact value type + * @return a new fact map. May return itself if it's known that new fact does not actually change this map. + */ + @NotNull + public DfaFactMap union(@NotNull DfaFactType type, @Nullable T value) { + if (value == null) return with(type, null); + T curFact = get(type); + if (curFact == null) return this; + T newFact = type.unionFacts(curFact, value); + return with(type, newFact); } - @Nullable - public static DfaFactMap intersect(DfaFactMap map1, DfaFactMap map2) { - if(map1 == null || map2 == null) return null; - List> types = DfaFactType.getTypes(); - for (DfaFactType type : types) { - map1 = map1.intersect(map2, type); - if (map1 == null) return null; - } - return map1; + @NotNull + private DfaFactMap union(DfaFactMap otherMap, @NotNull DfaFactType type) { + return union(type, otherMap.get(type)); + } + + @NotNull + public DfaFactMap union(@NotNull DfaFactMap other) { + return StreamEx.of(DfaFactType.getTypes()).foldLeft(this, (map, type) -> map.union(other, type)); } @Override diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java index 9110206d3814..d95ad07ef446 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaFactType.java @@ -114,6 +114,12 @@ public abstract class DfaFactType extends Key { return LongRangeSet.fromType(var.getVariableType()); } + @Nullable + @Override + LongRangeSet unionFacts(@NotNull LongRangeSet left, @NotNull LongRangeSet right) { + return left.union(right); + } + @Nullable @Override LongRangeSet intersectFacts(@NotNull LongRangeSet left, @NotNull LongRangeSet right) { @@ -160,6 +166,18 @@ public abstract class DfaFactType extends Key { return left.equals(right) ? left : null; } + /** + * Unites two facts of this type. + * + * @param left left fact + * @param right right fact + * @return union fact (null means that the fact can have any value) + */ + @Nullable + T unionFacts(@NotNull T left, @NotNull T right) { + return left.equals(right) ? left : null; + } + String toString(T fact) { return fact.toString(); } diff --git a/java/java-impl/src/com/intellij/codeInspection/SimplifyOptionalCallChainsInspection.java b/java/java-impl/src/com/intellij/codeInspection/SimplifyOptionalCallChainsInspection.java index 92854c5d393c..7f9f2ec14419 100644 --- a/java/java-impl/src/com/intellij/codeInspection/SimplifyOptionalCallChainsInspection.java +++ b/java/java-impl/src/com/intellij/codeInspection/SimplifyOptionalCallChainsInspection.java @@ -15,6 +15,8 @@ */ package com.intellij.codeInspection; +import com.intellij.codeInspection.dataFlow.CommonDataflow; +import com.intellij.codeInspection.dataFlow.DfaFactType; import com.intellij.codeInspection.util.LambdaGenerationUtil; import com.intellij.codeInspection.util.OptionalUtil; import com.intellij.openapi.project.Project; @@ -32,7 +34,7 @@ import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.*; +import java.util.Objects; import java.util.regex.Pattern; import static com.intellij.util.ObjectUtils.tryCast; @@ -44,12 +46,16 @@ import static com.intellij.util.ObjectUtils.tryCast; public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInspectionTool { private static final CallMatcher OPTIONAL_OR_ELSE = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "orElse").parameterCount(1); + private static final CallMatcher OPTIONAL_GET = + CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "get").parameterCount(0); private static final CallMatcher OPTIONAL_OR_ELSE_GET = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "orElseGet").parameterCount(1); private static final CallMatcher OPTIONAL_MAP = CallMatcher.instanceCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "map").parameterCount(1); private static final CallMatcher OPTIONAL_OF_NULLABLE = CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "ofNullable").parameterCount(1); + private static final CallMatcher OPTIONAL_OF_OF_NULLABLE = + CallMatcher.staticCall(CommonClassNames.JAVA_UTIL_OPTIONAL, "ofNullable", "of").parameterCount(1); @NotNull @@ -70,6 +76,10 @@ public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInsp private static abstract class OptionalChainVisitor extends JavaElementVisitor { @Override public void visitMethodCallExpression(PsiMethodCallExpression call) { + if (OPTIONAL_GET.test(call)) { + handleRewrapping(call, OPTIONAL_OF_OF_NULLABLE); + return; + } PsiExpression falseArg = null; boolean useOrElseGet = false; if (OPTIONAL_OR_ELSE.test(call)) { @@ -83,22 +93,27 @@ public class SimplifyOptionalCallChainsInspection extends BaseJavaBatchLocalInsp } if (falseArg == null) return; handleMapOrElse(call, useOrElseGet, falseArg); - handleOfNullableOrElse(call, falseArg); + if (ExpressionUtils.isNullLiteral(falseArg)) { + handleRewrapping(call, OPTIONAL_OF_NULLABLE); + } handleOrElseNullConditionalReturn(call, falseArg); handleOrElseNullConditionalAction(call, falseArg); } - private void handleOfNullableOrElse(PsiMethodCallExpression call, PsiExpression falseArg) { - if (!ExpressionUtils.isNullLiteral(falseArg)) return; + private void handleRewrapping(PsiMethodCallExpression call, CallMatcher wrapper) { PsiElement parent = PsiUtil.skipParenthesizedExprUp(call.getParent()); if (!(parent instanceof PsiExpressionList)) return; PsiMethodCallExpression parentCall = tryCast(parent.getParent(), PsiMethodCallExpression.class); - if (!OPTIONAL_OF_NULLABLE.test(parentCall)) return; + if (!wrapper.test(parentCall)) return; PsiExpression qualifier = call.getMethodExpression().getQualifierExpression(); if (qualifier == null || !EquivalenceChecker.getCanonicalPsiEquivalence().typesAreEquivalent(qualifier.getType(), parentCall.getType())) { return; } + if ("get".equals(call.getMethodExpression().getReferenceName()) && + !Boolean.TRUE.equals(CommonDataflow.getExpressionFact(qualifier, DfaFactType.OPTIONAL_PRESENCE))) { + return; + } SimplifyOptionalChainFix fix = new SimplifyOptionalChainFix(qualifier.getText(), "Unwrap", "Unnecessary Optional rewrapping"); handleSimplification(Objects.requireNonNull(parentCall.getMethodExpression().getReferenceNameElement()), fix); } diff --git a/java/java-tests/testData/inspection/optionalChains/afterRewrapGet.java b/java/java-tests/testData/inspection/optionalChains/afterRewrapGet.java new file mode 100644 index 000000000000..904fb924b804 --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/afterRewrapGet.java @@ -0,0 +1,10 @@ +// "Unwrap" "true" +import java.util.*; + +public class Tests { + void test2(Optional optional) { + if (optional.isPresent()) { + System.out.println(optional); + } + } +} diff --git a/java/java-tests/testData/inspection/optionalChains/afterRewrapGetSecond.java b/java/java-tests/testData/inspection/optionalChains/afterRewrapGetSecond.java new file mode 100644 index 000000000000..26391bff4db0 --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/afterRewrapGetSecond.java @@ -0,0 +1,9 @@ +// "Unwrap" "true" +import java.util.*; + +public class Tests { + void test3(Optional optional) { + System.out.println(Optional.of(optional.get())); + System.out.println(optional); + } +} diff --git a/java/java-tests/testData/inspection/optionalChains/beforeRewrapGet.java b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGet.java new file mode 100644 index 000000000000..6621814c46b2 --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGet.java @@ -0,0 +1,10 @@ +// "Unwrap" "true" +import java.util.*; + +public class Tests { + void test2(Optional optional) { + if (optional.isPresent()) { + System.out.println(Optional.of(optional.get())); + } + } +} diff --git a/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetFirst.java b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetFirst.java new file mode 100644 index 000000000000..8595676b2913 --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetFirst.java @@ -0,0 +1,9 @@ +// "Unwrap" "false" +import java.util.*; + +public class Tests { + void test3(Optional optional) { + System.out.println(Optional.of(optional.get())); + System.out.println(Optional.of(optional.get())); + } +} diff --git a/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetSecond.java b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetSecond.java new file mode 100644 index 000000000000..56181170122c --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetSecond.java @@ -0,0 +1,9 @@ +// "Unwrap" "true" +import java.util.*; + +public class Tests { + void test3(Optional optional) { + System.out.println(Optional.of(optional.get())); + System.out.println(Optional.of(optional.get())); + } +} diff --git a/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetUnsure.java b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetUnsure.java new file mode 100644 index 000000000000..80bf6434c231 --- /dev/null +++ b/java/java-tests/testData/inspection/optionalChains/beforeRewrapGetUnsure.java @@ -0,0 +1,10 @@ +// "Unwrap" "false" +import java.util.*; + +public class Tests { + void test(Optional optional, boolean b) { + if (b || optional.isPresent()) { + System.out.println(Optional.of(optional.get())); + } + } +}