From 75d6d70bfe90c8ba074c34c9c77aa322fb04a1a7 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 6 Oct 2020 16:01:14 +0700 Subject: [PATCH] [java-dfa] Introduce ephemeral values (possible only if separate compilation happens) Now we visit switch default branch, even if it's reachable only on added enum constant This fixes IDEA-227734 Data flow inspection doesn't report return null inside a switch for @NotNull annotated method Also if-chains and switch are processed more uniformly: if we replace exhaustive switch statement with the equivalent if, we won't get possible NPE anymore GitOrigin-RevId: c090b9e76a31a4626b518461b1b4ffce8cd3a7aa --- .../dataFlow/ControlFlowAnalyzer.java | 39 +++----- .../dataFlow/DfaMemoryState.java | 16 +++- .../dataFlow/DfaMemoryStateImpl.java | 3 + .../types/DfEphemeralReferenceType.java | 92 +++++++++++++++++++ .../dataFlow/types/DfGenericObjectType.java | 35 ++++++- .../types/DfReferenceConstantType.java | 3 +- .../fixture/EphemeralDefaultCaseVisited.java | 38 ++++++++ .../dataFlow/fixture/EphemeralInIfChain.java | 37 ++++++++ .../dataFlow/fixture/SwitchExpressions.java | 20 +++- .../DataFlowInspectionTest.java | 2 + 10 files changed, 250 insertions(+), 35 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfEphemeralReferenceType.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EphemeralDefaultCaseVisited.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EphemeralInIfChain.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java index 031b4f46b339..3e3fe9e7ada3 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ControlFlowAnalyzer.java @@ -886,7 +886,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { private void processSwitch(@NotNull PsiSwitchBlock switchBlock) { PsiExpression selector = PsiUtil.skipParenthesizedExprDown(switchBlock.getExpression()); - Set enumValues = null; DfaVariableValue expressionValue = null; boolean syntheticVar = true; if (selector != null) { @@ -907,17 +906,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } selector.accept(this); generateBoxingUnboxingInstructionFor(selector, targetType); - final PsiClass psiClass = PsiUtil.resolveClassInClassTypeOnly(targetType); - if (psiClass != null) { - if (psiClass.isEnum()) { - enumValues = new HashSet<>(); - for (PsiField f : psiClass.getFields()) { - if (f instanceof PsiEnumConstant) { - enumValues.add((PsiEnumConstant)f); - } - } - } - } if (syntheticVar) { addInstruction(new AssignInstruction(null, null)); } @@ -928,7 +916,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (body != null) { PsiStatement[] statements = body.getStatements(); - ControlFlowOffset offset = null; + ControlFlowOffset offset; PsiSwitchLabelStatementBase defaultLabel = null; for (PsiStatement statement : statements) { if (statement instanceof PsiSwitchLabelStatementBase) { @@ -943,14 +931,8 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (values != null) { for (PsiExpression caseValue : values.getExpressions()) { - boolean enumConstant = false; - if (enumValues != null && caseValue instanceof PsiReferenceExpression) { - PsiEnumConstant target = ObjectUtils.tryCast(((PsiReferenceExpression)caseValue).resolve(), PsiEnumConstant.class); - if (target != null) { - enumValues.remove(target); - enumConstant = true; - } - } + boolean enumConstant = caseValue instanceof PsiReferenceExpression && + ((PsiReferenceExpression)caseValue).resolve() instanceof PsiEnumConstant; if (caseValue != null && expressionValue != null && (enumConstant || PsiUtil.isConstantExpression(caseValue))) { addInstruction(new PushInstruction(expressionValue, null)); @@ -974,10 +956,15 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } } - if (offset == null || enumValues == null || !enumValues.isEmpty()) { - offset = defaultLabel != null ? getStartOffset(defaultLabel) : getEndOffset(body); - } // else default label goes to the last switch label making it always true - addInstruction(new GotoInstruction(offset)); + if (defaultLabel != null) { + addInstruction(new GotoInstruction(getStartOffset(defaultLabel))); + } + else if (switchBlock instanceof PsiSwitchExpression) { + throwException(myExceptionCache.get("java.lang.IncompatibleClassChangeError"), null); + } + else { + addInstruction(new GotoInstruction(getEndOffset(body))); + } body.accept(this); } @@ -2107,7 +2094,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { * @return true if some inliner may add constraints on the precise type of given expression */ public static boolean inlinerMayInferPreciseType(PsiExpression expression) { - return Arrays.stream(INLINERS).anyMatch(inliner -> inliner.mayInferPreciseType(expression)); + return ContainerUtil.exists(INLINERS, inliner -> inliner.mayInferPreciseType(expression)); } private static final class Synthetic implements VariableDescriptor { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java index abaef996ac76..69abcadb0804 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java @@ -141,12 +141,20 @@ public interface DfaMemoryState { boolean isNotNull(DfaValue dfaVar); /** - * Ephemeral means a state that was created when considering a method contract and checking if one of its arguments is null. - * With explicit null check, that would result in any non-annotated variable being treated as nullable and producing possible NPE warnings later. - * With contracts, we don't want this. So the state where this variable is null is marked ephemeral and no NPE warnings are issued for such states. + * Mark this state as ephemeral. See {@link #isEphemeral()} for details. */ void markEphemeral(); - + + /** + * Ephemeral means a state that could be unreachable under normal program execution. Examples of ephemeral states include: + * + * The "unsound" warnings (e.g. possible NPE) are not reported if they happen only in ephemeral states, and there's a non-ephemeral state + * where the same problem doesn't happen. + */ boolean isEphemeral(); boolean isEmptyStack(); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index 89f026200bd1..b03f40afeac1 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -1183,6 +1183,9 @@ public class DfaMemoryStateImpl implements DfaMemoryState { } else { myVariableTypes.put(dfaVar, type); } + if (type instanceof DfEphemeralReferenceType) { + markEphemeral(); + } myCachedHash = null; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfEphemeralReferenceType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfEphemeralReferenceType.java new file mode 100644 index 000000000000..b074ab47d057 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfEphemeralReferenceType.java @@ -0,0 +1,92 @@ +// 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. +package com.intellij.codeInspection.dataFlow.types; + +import com.intellij.codeInspection.dataFlow.*; +import org.jetbrains.annotations.NotNull; + +import java.util.Set; + +/** + * A reference value that is impossible with currently available source code but + * could appear if separate compilation takes place. A common example of ephemeral value + * is a constant of enum type that doesn't equal to any existing constant. + *

+ * When ephemeral value is stored in the memory state variable we assume that the whole + * memory state is ephemeral. + * + * @see DfaMemoryState#isEphemeral() + */ +public class DfEphemeralReferenceType implements DfReferenceType { + private final TypeConstraint myTypeConstraint; + + DfEphemeralReferenceType(TypeConstraint constraint) { + myTypeConstraint = constraint; + } + + @Override + public @NotNull DfaNullability getNullability() { + return DfaNullability.NOT_NULL; + } + + @Override + public @NotNull TypeConstraint getConstraint() { + return myTypeConstraint; + } + + @Override + public @NotNull DfReferenceType dropNullability() { + return this; + } + + @Override + public @NotNull DfReferenceType dropTypeConstraint() { + return DfTypes.NOT_NULL_OBJECT; + } + + @Override + public boolean isSuperType(@NotNull DfType other) { + if (other == DfTypes.BOTTOM) return true; + if (other instanceof DfEphemeralReferenceType) { + return myTypeConstraint.isSuperConstraintOf(((DfEphemeralReferenceType)other).myTypeConstraint); + } + return false; + } + + @Override + public @NotNull DfType join(@NotNull DfType other) { + if (other == DfTypes.BOTTOM) return this; + if (other == DfTypes.TOP || !(other instanceof DfReferenceType)) return DfTypes.TOP; + TypeConstraint otherConstraint = ((DfReferenceType)other).getConstraint(); + TypeConstraint constraint = myTypeConstraint.join(otherConstraint); + if (other instanceof DfEphemeralReferenceType) { + return constraint == myTypeConstraint ? this : + constraint == otherConstraint ? other : + constraint == TypeConstraints.TOP ? DfTypes.NOT_NULL_OBJECT : + new DfEphemeralReferenceType(constraint); + } + Set notValues = other instanceof DfGenericObjectType ? ((DfGenericObjectType)other).getNotValues() : Set.of(); + return new DfGenericObjectType(notValues, constraint, ((DfReferenceType)other).getNullability(), + Mutability.UNKNOWN, null, DfTypes.BOTTOM, false); + } + + @Override + public @NotNull DfType meet(@NotNull DfType other) { + if (other == DfTypes.TOP) return this; + if (other == DfTypes.BOTTOM) return other; + if (other instanceof DfEphemeralReferenceType || + other instanceof DfGenericObjectType) { + TypeConstraint otherConstraint = ((DfReferenceType)other).getConstraint(); + TypeConstraint constraint = myTypeConstraint.meet(otherConstraint); + return constraint == myTypeConstraint ? this : + constraint == otherConstraint ? other : + constraint == TypeConstraints.BOTTOM ? DfTypes.BOTTOM : + new DfEphemeralReferenceType(constraint); + } + return DfTypes.BOTTOM; + } + + @Override + public String toString() { + return "ephemeral " + getConstraint().toString(); + } +} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java index 11ca9d0f0c58..534347174e8e 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfGenericObjectType.java @@ -2,7 +2,11 @@ package com.intellij.codeInspection.dataFlow.types; import com.intellij.codeInspection.dataFlow.*; -import com.intellij.java.JavaBundle;import gnu.trove.THashSet; +import com.intellij.java.JavaBundle; +import com.intellij.psi.JavaPsiFacade; +import com.intellij.psi.PsiClass; +import com.intellij.psi.PsiEnumConstant; +import gnu.trove.THashSet; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -182,7 +186,7 @@ class DfGenericObjectType extends DfAntiConstantType implements DfRefere if (isSuperType(other)) return this; if (other.isSuperType(this)) return other; if (!(other instanceof DfReferenceType)) return TOP; - if (other instanceof DfNullConstantType) { + if (other instanceof DfNullConstantType || other instanceof DfEphemeralReferenceType) { return other.join(this); } DfReferenceType type = (DfReferenceType)other; @@ -207,7 +211,7 @@ class DfGenericObjectType extends DfAntiConstantType implements DfRefere @NotNull @Override public DfType meet(@NotNull DfType other) { - if (other instanceof DfConstantType) { + if (other instanceof DfConstantType || other instanceof DfEphemeralReferenceType) { return other.meet(this); } if (isSuperType(other)) return other; @@ -245,11 +249,36 @@ class DfGenericObjectType extends DfAntiConstantType implements DfRefere } else if (!myNotValues.containsAll(otherNotValues)) { notValues = new THashSet<>(myNotValues); notValues.addAll(otherNotValues); + DfEphemeralReferenceType ephemeralValue = checkEphemeral(constraint, notValues); + if (ephemeralValue != null) { + return ephemeralValue; + } } } return new DfGenericObjectType(notValues, constraint, nullability, mutability, sf, sfType, locality); } + private static DfEphemeralReferenceType checkEphemeral(TypeConstraint constraint, Set notValues) { + if (notValues.isEmpty()) return null; + Object value = notValues.iterator().next(); + if (!(value instanceof PsiEnumConstant)) return null; + PsiClass enumClass = ((PsiEnumConstant)value).getContainingClass(); + if (enumClass == null) return null; + TypeConstraint enumType = TypeConstraints.instanceOf( + JavaPsiFacade.getElementFactory(enumClass.getProject()).createType(enumClass)); + if (!enumType.equals(constraint)) return null; + Set allEnumConstants = StreamEx.of(enumClass.getFields()).select(PsiEnumConstant.class).toSet(); + if (notValues.size() != allEnumConstants.size()) return null; + for (Object notValue : notValues) { + if (!(notValue instanceof PsiEnumConstant)) return null; + if (!allEnumConstants.remove(notValue)) return null; + } + if (allEnumConstants.isEmpty()) { + return new DfEphemeralReferenceType(constraint); + } + return null; + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java index 8ea9fc661e13..73b441c0a2b6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/types/DfReferenceConstantType.java @@ -33,6 +33,7 @@ public class DfReferenceConstantType extends DfConstantType implements D @Override public DfType meet(@NotNull DfType other) { if (other.isSuperType(this)) return this; + if (other instanceof DfEphemeralReferenceType) return BOTTOM; if (other instanceof DfGenericObjectType) { DfReferenceType type = ((DfReferenceType)other).dropMutability(); if (type.isSuperType(this)) return this; @@ -96,7 +97,7 @@ public class DfReferenceConstantType extends DfConstantType implements D @NotNull @Override public DfType join(@NotNull DfType other) { - if (other instanceof DfGenericObjectType) { + if (other instanceof DfGenericObjectType || other instanceof DfEphemeralReferenceType) { return other.join(this); } if (isSuperType(other)) return this; diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralDefaultCaseVisited.java b/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralDefaultCaseVisited.java new file mode 100644 index 000000000000..1d54756df091 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralDefaultCaseVisited.java @@ -0,0 +1,38 @@ +import org.jetbrains.annotations.*; + +// IDEA-227734 +class Test { + enum MyEnum{ A, B, C } + + @NotNull + String doesNotShowWarning(@NotNull MyEnum myEnum){ + switch(myEnum){ + case A: + case B: + case C: + return "YES"; + default: return null; + } + } + + @NotNull + String doesNotShowWarning2(@NotNull final MyEnum myEnum){ + switch(myEnum){ + case A: + case B: + case C: + return "YES"; + } + return null; + } + + @NotNull + String showsWarning(@NotNull MyEnum myEnum){ + switch(myEnum){ + case A: + case B: + return "YES"; + default: return null; + } + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralInIfChain.java b/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralInIfChain.java new file mode 100644 index 000000000000..38e606f9dcb8 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/EphemeralInIfChain.java @@ -0,0 +1,37 @@ +import org.jetbrains.annotations.*; + +class Test { + enum MyEnum {A, B, C} + + void test(MyEnum x) { + String s = null; + if (x == MyEnum.A) { + s = "A"; + } + else if (x == MyEnum.B) { + s = "B"; + } + else if (x == MyEnum.C) { + s = "C"; + } + System.out.println(s.trim()); + } + + void test2(MyEnum x) { + String s = null; + if (x == MyEnum.A) { + s = "A"; + } + else if (x == MyEnum.B) { + s = "B"; + } + else if (x == MyEnum.C) { + s = "C"; + } + if (s == null) { + System.out.println("Incompatible class change!"); + return; + } + System.out.println(s.trim()); + } +} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SwitchExpressions.java b/java/java-tests/testData/inspection/dataFlow/fixture/SwitchExpressions.java index 544f46a29be8..07978f9fe2fd 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/SwitchExpressions.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SwitchExpressions.java @@ -24,7 +24,8 @@ public class SwitchExpressionsJava12 { case B -> 2; }; - if (i == 0) {} // default and case C is missing: we assume that any other result is also possible + // default and case C is missing: we assume that IncompatibleClassChangeError is still thrown + if (i == 0) {} int i1 = switch(x) { case A -> 1; @@ -34,6 +35,23 @@ public class SwitchExpressionsJava12 { if (i1 == 0) {} // exhaustive } + + static void testEnumAndCatch(X x) { + int i1 = 0; + try { + i1 = switch(x) { + case A -> 1; + case B -> 2; + case C -> 3; + }; + } + catch (IncompatibleClassChangeError ex) { + if (i1 == 0) {} + if (x == X.A) {} + if (x == X.B) {} + if (x == X.C) {} + } + } static void testBoxed(int i) { Integer j = switch(i) { diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java index a242dc79d596..bf82ebc212a3 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -86,6 +86,8 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testNotEqualsDoesntImplyNotNullity() { doTest(); } public void testEqualsEnumConstant() { doTest(); } public void testSwitchEnumConstant() { doTest(); } + public void testEphemeralDefaultCaseVisited() { doTest(); } + public void testEphemeralInIfChain() { doTest(); } public void testIncompleteSwitchEnum() { doTest(); } public void testEnumConstantNotNull() { doTest(); } public void testCheckEnumConstantConstructor() { doTest(); }