From f9bfad829639fdd8e56b6e100c9a725309f1f555 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 15 Jun 2018 11:14:40 +0700 Subject: [PATCH] DFA instruction visitor refactoring wave#3 Always failing calls, constant calls moved to beforeExpressionPush handler JavaMethodContractUtil API tweaks DfaConstValue#isContractFail --- .../dataFlow/CommonDataflow.java | 12 +- .../dataFlow/CustomMethodHandlers.java | 4 +- .../dataFlow/DataFlowInspectionBase.java | 10 +- .../dataFlow/DataFlowInstructionVisitor.java | 119 ++++++++++-------- .../dataFlow/JavaMethodContractUtil.java | 27 ++-- .../dataFlow/StandardInstructionVisitor.java | 8 +- .../dataFlow/value/DfaConstValue.java | 12 ++ ...SuspiciousComparatorCompareInspection.java | 7 +- 8 files changed, 109 insertions(+), 90 deletions(-) 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 806669f78bd5..88334d5bac9b 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 @@ -93,7 +93,7 @@ public class CommonDataflow { private static DataflowResult runDFA(@Nullable PsiElement block) { if (block == null) return null; DataFlowRunner runner = new DataFlowRunner(false, block); - CommonDataflowVisitor visitor = new CommonDataflowVisitor(runner); + CommonDataflowVisitor visitor = new CommonDataflowVisitor(); RunnerResult result = runner.analyzeMethodRecursively(block, visitor); if (result != RunnerResult.OK) return null; if (!(block instanceof PsiClass)) return visitor.myResult; @@ -148,15 +148,9 @@ public class CommonDataflow { } private static class CommonDataflowVisitor extends StandardInstructionVisitor { - private DataflowResult myResult; - private final DfaConstValue myFail; + private DataflowResult myResult = new DataflowResult(); private final List myEndOfInitializerStates = new ArrayList<>(); - public CommonDataflowVisitor(DataFlowRunner runner) { - myFail = runner.getFactory().getConstFactory().getContractFail(); - myResult = new DataflowResult(); - } - @Override public DfaInstructionState[] visitEndOfInitializer(EndOfInitializerInstruction instruction, DataFlowRunner runner, @@ -172,7 +166,7 @@ public class CommonDataflow { @NotNull PsiExpression expression, @Nullable TextRange range, @NotNull DfaMemoryState state) { - if (range == null && value != myFail) { + if (range == null && !DfaConstValue.isContractFail(value)) { // Do not track instructions which cover part of expression myResult.add(expression, (DfaMemoryStateImpl)state, value); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CustomMethodHandlers.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CustomMethodHandlers.java index db11486872b6..d6914948701e 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CustomMethodHandlers.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CustomMethodHandlers.java @@ -15,7 +15,6 @@ */ package com.intellij.codeInspection.dataFlow; -import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; @@ -76,8 +75,7 @@ class CustomMethodHandlers { .register(DfaOptionalSupport.OPTIONAL_OF_NULLABLE, (args, memState, factory) -> ofNullable(args.myArguments[0], memState, factory)); - public static CustomMethodHandler find(MethodCallInstruction instruction) { - PsiMethod method = instruction.getTargetMethod(); + public static CustomMethodHandler find(PsiMethod method) { CustomMethodHandler handler = null; if (isConstantCall(method)) { handler = (args, memState, factory) -> handleConstantCall(args, memState, factory, method); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java index 38ce33cf1fc2..06dd5e9f32af 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java @@ -437,17 +437,15 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool private static void reportAlwaysFailingCalls(ProblemsHolder holder, DataFlowInstructionVisitor visitor, HashSet reportedAnchors) { - visitor.getAlwaysFailingCalls().forEach((call, contracts) -> { - if (TestUtils.isExceptionExpected(call)) return; - PsiMethod method = call.resolveMethod(); - if (method != null && reportedAnchors.add(call)) { - holder.registerProblem(getElementToHighlight(call), getContractMessage(contracts)); + visitor.alwaysFailingCalls().remove(TestUtils::isExceptionExpected).forEach(call -> { + if (reportedAnchors.add(call)) { + holder.registerProblem(getElementToHighlight(call), getContractMessage(JavaMethodContractUtil.getMethodCallContracts(call))); } }); } @NotNull - private static String getContractMessage(List contracts) { + private static String getContractMessage(List contracts) { if (contracts.stream().allMatch(mc -> mc.getConditions().stream().allMatch(ContractValue::isBoundCheckingCondition))) { return InspectionsBundle.message("dataflow.message.contract.fail.index"); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java index 6dde8efe837d..8fbc0f9bbda6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInstructionVisitor.java @@ -21,12 +21,14 @@ import org.jetbrains.annotations.Nullable; import java.util.*; import java.util.stream.Stream; +import static com.intellij.util.ObjectUtils.tryCast; + final class DataFlowInstructionVisitor extends StandardInstructionVisitor { private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.DataFlowInstructionVisitor"); private static final Object ANY_VALUE = ObjectUtils.sentinel("ANY_VALUE"); private final Map, StateInfo> myStateInfos = new LinkedHashMap<>(); private final Set myCCEInstructions = ContainerUtil.newHashSet(); - private final Map myFailingCalls = new HashMap<>(); + private final Map myFailingCalls = new HashMap<>(); private final Map myBooleanCalls = new HashMap<>(); private final Map myOfNullableCalls = new HashMap<>(); private final Map> myArrayStoreProblems = new HashMap<>(); @@ -152,9 +154,8 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return StreamEx.ofKeys(myOutOfBoundsArrayAccesses, ThreeState.YES::equals); } - Map> getAlwaysFailingCalls() { - return StreamEx.ofKeys(myFailingCalls, v -> v) - .mapToEntry(MethodCallInstruction::getCallExpression, MethodCallInstruction::getContracts).toMap(); + StreamEx alwaysFailingCalls() { + return StreamEx.ofKeys(myFailingCalls, v -> v); } boolean isAlwaysReturnsNotNull(Instruction[] instructions) { @@ -167,10 +168,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { @NotNull PsiExpression expression, @Nullable TextRange range, @NotNull DfaMemoryState memState) { - if (expression instanceof PsiMethodCallExpression && - DfaOptionalSupport.OPTIONAL_OF_NULLABLE.test((PsiMethodCallExpression)expression)) { - processOfNullableResult(value, memState, ((PsiMethodCallExpression)expression).getArgumentList().getExpressions()[0]); - } + expression.accept(new ExpressionVisitor(value, memState)); } @Override @@ -180,7 +178,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { if (DfaOptionalSupport.OPTIONAL_OF_NULLABLE.methodReferenceMatches(methodRef)) { processOfNullableResult(value, state, methodRef.getReferenceNameElement()); } - PsiMethod method = ObjectUtils.tryCast(methodRef.resolve(), PsiMethod.class); + PsiMethod method = tryCast(methodRef.resolve(), PsiMethod.class); if (method != null) { List contracts = JavaMethodContractUtil.getMethodContracts(method); if (contracts.isEmpty() || !contracts.get(0).isTrivial()) { @@ -190,47 +188,12 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { } } - private void processOfNullableResult(@NotNull DfaValue value, - @NotNull DfaMemoryState memState, PsiElement anchor) { + private void processOfNullableResult(@NotNull DfaValue value, @NotNull DfaMemoryState memState, PsiElement anchor) { Boolean fact = memState.getValueFact(value, DfaFactType.OPTIONAL_PRESENCE); ThreeState present = fact == null ? ThreeState.UNSURE : ThreeState.fromBoolean(fact); myOfNullableCalls.merge(anchor, present, ThreeState::merge); } - @Override - public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState) { - DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); - if (hasNonTrivialFailingContracts(instruction)) { - DfaConstValue fail = runner.getFactory().getConstFactory().getContractFail(); - boolean allFail = Arrays.stream(states).allMatch(s -> s.getMemoryState().peek() == fail); - myFailingCalls.merge(instruction, allFail, Boolean::logicalAnd); - } - handleBooleanCalls(instruction, states); - return states; - } - - void handleBooleanCalls(MethodCallInstruction instruction, DfaInstructionState[] states) { - if (!hasNonTrivialBooleanContracts(instruction)) return; - PsiMethod method = instruction.getTargetMethod(); - if (method == null || !JavaMethodContractUtil.isPure(method)) return; - PsiMethodCallExpression call = ObjectUtils.tryCast(instruction.getCallExpression(), PsiMethodCallExpression.class); - if (call == null || myBooleanCalls.get(call) == ThreeState.UNSURE) return; - if (ExpressionUtils.isVoidContext(call)) return; - for (DfaInstructionState s : states) { - DfaValue val = s.getMemoryState().peek(); - ThreeState state = ThreeState.UNSURE; - if (val instanceof DfaConstValue) { - Object value = ((DfaConstValue)val).getValue(); - if (value instanceof Boolean) { - state = ThreeState.fromBoolean((Boolean)value); - } - } - myBooleanCalls.merge(call, state, ThreeState::merge); - } - } - @Override protected void processArrayAccess(PsiArrayAccessExpression expression, boolean alwaysOutOfBounds) { myOutOfBoundsArrayAccesses.merge(expression, ThreeState.fromBoolean(alwaysOutOfBounds), ThreeState::merge); @@ -279,17 +242,19 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return result; } - private static boolean hasNonTrivialFailingContracts(MethodCallInstruction instruction) { - List contracts = instruction.getContracts(); - return !contracts.isEmpty() && contracts.stream().anyMatch( - contract -> contract.getReturnValue().isFail() && !contract.isTrivial()); + private static boolean hasNonTrivialFailingContracts(PsiCallExpression call) { + List contracts = JavaMethodContractUtil.getMethodCallContracts(call); + return !contracts.isEmpty() && + contracts.stream().anyMatch(contract -> contract.getReturnValue().isFail() && !contract.isTrivial()); } - private static boolean hasNonTrivialBooleanContracts(MethodCallInstruction instruction) { - if (CustomMethodHandlers.find(instruction) != null) return true; - List contracts = instruction.getContracts(); - return !contracts.isEmpty() && contracts.stream().anyMatch( - contract -> contract.getReturnValue().isBoolean() && !contract.isTrivial()); + private static boolean shouldCollectBooleanCallResult(PsiMethodCallExpression call) { + if (ExpressionUtils.isVoidContext(call)) return false; + PsiMethod method = call.resolveMethod(); + if (method == null || !PsiType.BOOLEAN.equals(method.getReturnType()) || !JavaMethodContractUtil.isPure(method)) return false; + List contracts = JavaMethodContractUtil.getMethodCallContracts(method, call); + return CustomMethodHandlers.find(method) != null || + !contracts.isEmpty() && contracts.stream().anyMatch(contract -> contract.getReturnValue().isBoolean() && !contract.isTrivial()); } @Override @@ -336,4 +301,50 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { boolean normalNpe; boolean normalOk; } + + private class ExpressionVisitor extends JavaElementVisitor { + private final DfaValue myValue; + private final DfaMemoryState myMemState; + + public ExpressionVisitor(DfaValue value, DfaMemoryState memState) { + myValue = value; + myMemState = memState; + } + + @Override + public void visitMethodCallExpression(PsiMethodCallExpression call) { + super.visitMethodCallExpression(call); + if (DfaOptionalSupport.OPTIONAL_OF_NULLABLE.test(call)) { + processOfNullableResult(myValue, myMemState, call.getArgumentList().getExpressions()[0]); + } + handleBooleanCalls(call); + } + + @Override + public void visitCallExpression(PsiCallExpression call) { + super.visitCallExpression(call); + Boolean isFailing = myFailingCalls.get(call); + if (isFailing != null || hasNonTrivialFailingContracts(call)) { + myFailingCalls.put(call, DfaConstValue.isContractFail(myValue) && !Boolean.FALSE.equals(isFailing)); + } + } + + private void handleBooleanCalls(PsiMethodCallExpression call) { + ThreeState curState = myBooleanCalls.get(call); + if (curState == ThreeState.UNSURE) return; + ThreeState nextState = ThreeState.UNSURE; + if (myValue instanceof DfaConstValue) { + Object val = ((DfaConstValue)myValue).getValue(); + if (val instanceof Boolean) { + nextState = ThreeState.fromBoolean((Boolean)val); + if (curState != null && curState != nextState) { + nextState = ThreeState.UNSURE; + } + } + } + if (curState != null || shouldCollectBooleanCallResult(call)) { + myBooleanCalls.put(call, nextState); + } + } + } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/JavaMethodContractUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/JavaMethodContractUtil.java index 72901ec078f9..9ee7914ff677 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/JavaMethodContractUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/JavaMethodContractUtil.java @@ -2,13 +2,11 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; -import com.intellij.psi.PsiAnnotation; -import com.intellij.psi.PsiExpression; -import com.intellij.psi.PsiMethod; -import com.intellij.psi.PsiMethodCallExpression; +import com.intellij.psi.*; import com.intellij.psi.util.CachedValueProvider; import com.intellij.psi.util.CachedValuesManager; import com.intellij.psi.util.PsiModificationTracker; +import com.intellij.util.ObjectUtils; import com.siyeh.ig.psiutils.ExpressionUtils; import com.siyeh.ig.psiutils.MethodCallUtils; import org.jetbrains.annotations.Contract; @@ -30,6 +28,18 @@ public class JavaMethodContractUtil { */ public static final String ORG_JETBRAINS_ANNOTATIONS_CONTRACT = Contract.class.getName(); + /** + * Returns a list of contracts defined for given method call (including hardcoded contracts if any) + * + * @param call a method call site. + * @return list of contracts (empty list if no contracts found) + */ + @NotNull + public static List getMethodCallContracts(@NotNull PsiCallExpression call) { + PsiMethod method = call.resolveMethod(); + return method == null ? Collections.emptyList() : getMethodCallContracts(method, call); + } + /** * Returns a list of contracts defined for given method call (including hardcoded contracts if any) * @@ -40,8 +50,9 @@ public class JavaMethodContractUtil { */ @NotNull public static List getMethodCallContracts(@NotNull final PsiMethod method, - @Nullable PsiMethodCallExpression call) { - List contracts = HardcodedContracts.getHardcodedContracts(method, call); + @Nullable PsiCallExpression call) { + List contracts = + HardcodedContracts.getHardcodedContracts(method, ObjectUtils.tryCast(call, PsiMethodCallExpression.class)); return !contracts.isEmpty() ? contracts : getMethodContracts(method); } @@ -200,9 +211,7 @@ public class JavaMethodContractUtil { @Contract("null -> null") public static PsiExpression findReturnedValue(@Nullable PsiMethodCallExpression call) { if (call == null) return null; - PsiMethod method = call.resolveMethod(); - if (method == null) return null; - List contracts = getMethodCallContracts(method, call); + List contracts = getMethodCallContracts(call); ContractReturnValue returnValue = getNonFailingReturnValue(contracts); if (returnValue == null) return null; if (returnValue.equals(ContractReturnValue.returnThis())) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index 04fb6e1956df..1cc234abd194 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java @@ -331,8 +331,9 @@ public class StandardInstructionVisitor extends InstructionVisitor { @NotNull private List handleKnownMethods(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - if (instruction.getTargetMethod() == null) return Collections.emptyList(); - CustomMethodHandlers.CustomMethodHandler handler = CustomMethodHandlers.find(instruction); + PsiMethod method = instruction.getTargetMethod(); + if (method == null) return Collections.emptyList(); + CustomMethodHandlers.CustomMethodHandler handler = CustomMethodHandlers.find(method); if (handler == null) return Collections.emptyList(); memState = memState.createCopy(); DfaCallArguments callArguments = popCall(instruction, runner, memState, false); @@ -784,8 +785,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { } if (dfaLeft instanceof DfaConstValue && dfaRight instanceof DfaConstValue || - dfaLeft == runner.getFactory().getConstFactory().getContractFail() || - dfaRight == runner.getFactory().getConstFactory().getContractFail()) { + DfaConstValue.isContractFail(dfaLeft) || DfaConstValue.isContractFail(dfaRight)) { boolean negated = (relationType == RelationType.NE) ^ (DfaMemoryStateImpl.isNaN(dfaLeft) || DfaMemoryStateImpl.isNaN(dfaRight)); boolean result = dfaLeft == dfaRight ^ negated; return makeBooleanResultArray(instruction, runner, memState, result); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java index f1454dcd55e5..513914ff8dc1 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaConstValue.java @@ -24,6 +24,7 @@ import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.ObjectUtils; import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.ExpressionUtils; +import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -190,4 +191,15 @@ public class DfaConstValue extends DfaValue { if (this == myFactory.getConstFactory().getFalse()) return myFactory.getConstFactory() .getTrue(); return DfaUnknownValue.getInstance(); } + + /** + * Checks whether given value is a special value representing method failure, according to its contract + * + * @param value value to check + * @return true if specified value represents method failure + */ + @Contract("null -> false") + public static boolean isContractFail(DfaValue value) { + return value instanceof DfaConstValue && ((DfaConstValue)value).getValue() == ourThrowable; + } } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java index f729249f4d74..26bdfd8fa0e7 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java @@ -100,11 +100,8 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } PsiMethodCallExpression soleCall = ObjectUtils.tryCast(LambdaUtil.extractSingleExpressionFromBody(body), PsiMethodCallExpression.class); if (soleCall != null) { - PsiMethod method = soleCall.resolveMethod(); - if (method != null) { - MethodContract contract = ContainerUtil.getOnlyItem(JavaMethodContractUtil.getMethodCallContracts(method, soleCall)); - if (contract != null && contract.isTrivial() && contract.getReturnValue().isFail()) return; - } + MethodContract contract = ContainerUtil.getOnlyItem(JavaMethodContractUtil.getMethodCallContracts(soleCall)); + if (contract != null && contract.isTrivial() && contract.getReturnValue().isFail()) return; } PsiParameter[] parameters = parameterList.getParameters(); checkParameterList(parameters, body);