From c59f936afcf0835cb90bc4439517313c379508eb Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 26 Jun 2018 10:25:18 +0700 Subject: [PATCH] Revert DFA refactoring in master for a while This reverts commits 3e4cd65, a8798fc, f9bfad8, 7d78237, b3a0e44, b4d1655, fd66719, a4a8455 --- .../guess/impl/GuessManagerImpl.java | 4 +- .../codeInspection/dataFlow/CFGBuilder.java | 25 -- .../dataFlow/CommonDataflow.java | 102 ++++++-- .../dataFlow/ContractChecker.java | 203 ++++++++-------- .../dataFlow/ControlFlowAnalyzer.java | 76 ++++-- .../dataFlow/CustomMethodHandlers.java | 55 +++-- .../dataFlow/DataFlowInspectionBase.java | 127 +++++----- .../dataFlow/DataFlowInstructionVisitor.java | 221 ++++++++---------- .../dataFlow/DfaMemoryStateImpl.java | 44 +--- .../codeInspection/dataFlow/DfaUtil.java | 61 ++--- .../dataFlow/HardcodedContracts.java | 61 ++--- .../dataFlow/InstructionVisitor.java | 96 ++------ .../dataFlow/JavaMethodContractUtil.java | 27 +-- .../dataFlow/StandardInstructionVisitor.java | 157 +++++++------ .../inliner/OptionalChainInliner.java | 36 ++- .../dataFlow/inliner/StreamChainInliner.java | 10 +- .../instructions/ArrayAccessInstruction.java | 2 +- .../instructions/AssignInstruction.java | 10 +- .../instructions/BinopInstruction.java | 15 +- .../CheckReturnValueInstruction.java | 48 ++++ .../ExpressionPushingInstruction.java | 25 -- .../instructions/InstanceofInstruction.java | 7 +- .../instructions/MethodCallInstruction.java | 16 +- .../dataFlow/instructions/NotInstruction.java | 19 +- .../instructions/PushInstruction.java | 5 +- .../instructions/ResultOfInstruction.java | 34 --- .../instructions/TypeCastInstruction.java | 13 +- .../dataFlow/value/DfaConstValue.java | 22 -- .../extractMethod/ExtractMethodProcessor.java | 39 ++-- .../dataFlow/boxingBoolean/expected.xml | 45 ++++ .../dataFlow/boxingBoolean/src/Test.java | 65 ++++++ .../dataFlow/fixture/AndAndWithOr.java | 9 - .../dataFlow/fixture/AndEquals.java | 2 +- .../dataFlow/fixture/BoxingBoolean.java | 65 ------ .../fixture/BuildRegexpNotComplex.java | 2 +- .../dataFlow/fixture/DoubleNaN.java | 2 +- .../fixture/EqualsInLoopNotTooComplex.java | 29 --- .../dataFlow/fixture/EqualsWithItself.java | 18 -- .../dataFlow/fixture/OptionalIsPresent.java | 4 +- .../dataFlow/fixture/OrWithAssignment.java | 10 - .../fixture/ReturningConstantExpression.java | 2 +- .../dataFlow/fixture/SkipAssertions.java | 2 +- .../inspection/dataFlow/fixture/Xor.java | 2 +- .../DataFlowInspectionAncientTest.java | 1 + .../DataFlowInspectionTest.java | 5 - .../ig/bugs/EqualsWithItselfInspection.java | 48 ++-- ...SuspiciousComparatorCompareInspection.java | 62 +++-- .../ComparatorIsNotReflexive.java | 10 +- 48 files changed, 889 insertions(+), 1054 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java delete mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ExpressionPushingInstruction.java delete mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java create mode 100644 java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml create mode 100644 java/java-tests/testData/inspection/dataFlow/boxingBoolean/src/Test.java delete mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/AndAndWithOr.java delete mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/BoxingBoolean.java delete mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java delete mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java delete mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/OrWithAssignment.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java b/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java index 3b58de19b10c..1472cd8b3b0c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInsight/guess/impl/GuessManagerImpl.java @@ -555,11 +555,11 @@ public class GuessManagerImpl extends GuessManager { @Override public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - if (myForPlace == instruction.getExpression()) { + if (myForPlace == instruction.getPlace()) { addToResult(((ExpressionTypeMemoryState)memState).getStates()); } DfaInstructionState[] states = super.visitPush(instruction, runner, memState); - if (myForPlace == instruction.getExpression()) { + if (myForPlace == instruction.getPlace()) { addConstraints(states); } return states; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java index e9c854bd7213..d2b872f612b0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/CFGBuilder.java @@ -125,21 +125,6 @@ public class CFGBuilder { return add(new PushInstruction(value, null)); } - /** - * Generate instructions to push given DfaValue on stack and bind it to given expression. - *

- * Stack before: ... - *

- * Stack after: ... value - * - * @param value value to push - * @param expression expression which result is being pushed - * @return this builder - */ - public CFGBuilder push(DfaValue value, PsiExpression expression) { - return add(new PushInstruction(value, expression)); - } - /** * Generate instructions to pop single DfaValue from stack *

@@ -207,16 +192,6 @@ public class CFGBuilder { return add(new ObjectOfInstruction()); } - /** - * Generate instructions to bind top-of-stack value to the given expression. Stack remains unchanged. - * - * @param expression expression to bind top-of-stack value to - * @return this builder - */ - public CFGBuilder resultOf(PsiExpression expression) { - return add(new ResultOfInstruction(expression)); - } - /** * Generate instructions to perform an Class.isInstance operation *

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 88334d5bac9b..8d4f91402088 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 @@ -1,13 +1,17 @@ // Copyright 2000-2018 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; -import com.intellij.codeInspection.dataFlow.instructions.EndOfInitializerInstruction; +import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; -import com.intellij.openapi.util.TextRange; import com.intellij.psi.*; -import com.intellij.psi.util.*; +import com.intellij.psi.util.CachedValueProvider; +import com.intellij.psi.util.CachedValuesManager; +import com.intellij.psi.util.PsiModificationTracker; +import com.intellij.psi.util.PsiUtil; import com.intellij.util.JavaPsiConstructorUtil; +import com.intellij.util.ObjectUtils; +import com.siyeh.ig.psiutils.ExpressionUtils; import one.util.streamex.StreamEx; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.NotNull; @@ -38,12 +42,6 @@ public class CommonDataflow { newMap = newMap.with(DfaFactType.CAN_BE_NULL, false); } myFacts.put(expression, existing == null ? newMap : existing.union(newMap)); - - PsiElement parent = PsiUtil.skipParenthesizedExprUp(expression.getParent()); - if (parent instanceof PsiConditionalExpression && - !PsiTreeUtil.isAncestor(((PsiConditionalExpression)parent).getCondition(), expression, false)) { - add((PsiExpression)parent, memState, value); - } } } @@ -93,7 +91,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(); + CommonDataflowVisitor visitor = new CommonDataflowVisitor(runner); RunnerResult result = runner.analyzeMethodRecursively(block, visitor); if (result != RunnerResult.OK) return null; if (!(block instanceof PsiClass)) return visitor.myResult; @@ -148,9 +146,15 @@ public class CommonDataflow { } private static class CommonDataflowVisitor extends StandardInstructionVisitor { - private DataflowResult myResult = new DataflowResult(); + private DataflowResult myResult; + private final DfaConstValue myFail; 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, @@ -162,14 +166,76 @@ public class CommonDataflow { } @Override - protected void beforeExpressionPush(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @Nullable TextRange range, - @NotNull DfaMemoryState state) { - if (range == null && !DfaConstValue.isContractFail(value)) { - // Do not track instructions which cover part of expression - myResult.add(expression, (DfaMemoryStateImpl)state, value); + public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + DfaInstructionState[] states = super.visitPush(instruction, runner, memState); + PsiExpression place = instruction.getPlace(); + if (place != null && !instruction.isReferenceWrite()) { + for (DfaInstructionState state : states) { + DfaMemoryState afterState = state.getMemoryState(); + myResult.add(place, (DfaMemoryStateImpl)afterState, instruction.getValue()); + } } + return states; + } + + @Override + public DfaInstructionState[] visitArrayAccess(ArrayAccessInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + DfaInstructionState[] states = super.visitArrayAccess(instruction, runner, memState); + PsiArrayAccessExpression anchor = instruction.getExpression(); + for (DfaInstructionState state : states) { + DfaMemoryState afterState = state.getMemoryState(); + myResult.add(anchor, (DfaMemoryStateImpl)afterState, afterState.peek()); + } + return states; + } + + @Override + public DfaInstructionState[] visitBinop(BinopInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + DfaInstructionState[] states = super.visitBinop(instruction, runner, memState); + PsiElement anchor = instruction.getPsiAnchor(); + if (anchor instanceof PsiExpression) { + for (DfaInstructionState state : states) { + DfaMemoryState afterState = state.getMemoryState(); + myResult.add((PsiExpression)anchor, (DfaMemoryStateImpl)afterState, afterState.peek()); + } + } + return states; + } + + @NotNull + @Override + protected DfaCallArguments popCall(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + boolean contractOnly) { + DfaCallArguments arguments = super.popCall(instruction, runner, memState, contractOnly); + PsiElement context = instruction.getContext(); + if (instruction.getMethodType() == MethodCallInstruction.MethodType.REGULAR_METHOD_CALL && + context instanceof PsiMethodCallExpression) { + PsiExpression qualifier = + PsiUtil.skipParenthesizedExprDown(((PsiMethodCallExpression)context).getMethodExpression().getQualifierExpression()); + if (qualifier != null) { + myResult.add(qualifier, (DfaMemoryStateImpl)memState, arguments.myQualifier); + } + } + return arguments; + } + + @Override + public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); + PsiExpression context = ObjectUtils.tryCast(instruction.getContext(), PsiExpression.class); + if (context != null && ExpressionUtils.getCallForQualifier(context) == null) { + for (DfaInstructionState state : states) { + DfaValue value = state.getMemoryState().peek(); + if (value != myFail) { + myResult.add(context, (DfaMemoryStateImpl)state.getMemoryState(), value); + } + } + } + return states; } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java index 327eb928f44b..0add795dda55 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java @@ -3,7 +3,8 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.NullableNotNullManager; import com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint; -import com.intellij.codeInspection.dataFlow.instructions.ControlTransferInstruction; +import com.intellij.codeInspection.dataFlow.instructions.CheckReturnValueInstruction; +import com.intellij.codeInspection.dataFlow.instructions.Instruction; import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; import com.intellij.codeInspection.dataFlow.instructions.ReturnInstruction; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; @@ -25,111 +26,32 @@ import java.util.Set; /** * @author peter */ -class ContractChecker { - private static class ContractCheckerVisitor extends StandardInstructionVisitor { - private final PsiMethod myMethod; - private final StandardMethodContract myContract; - private final boolean myOwnContract; - private final Set myViolations = ContainerUtil.newHashSet(); - private final Set myNonViolations = ContainerUtil.newHashSet(); - private final Set myFailures = ContainerUtil.newHashSet(); - private boolean myMayReturnNormally = false; +class ContractChecker extends DataFlowRunner { + private final PsiMethod myMethod; + private final StandardMethodContract myContract; + private final boolean myOwnContract; + private final Set myViolations = ContainerUtil.newHashSet(); + private final Set myNonViolations = ContainerUtil.newHashSet(); + private final Set myFailures = ContainerUtil.newHashSet(); + private boolean myMayReturnNormally = false; - ContractCheckerVisitor(PsiMethod method, StandardMethodContract contract, boolean ownContract) { - myMethod = method; - myContract = contract; - myOwnContract = ownContract; - } - - @Override - protected void checkReturnValue(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @NotNull PsiParameterListOwner context, - @NotNull DfaMemoryState state) { - if (context != myMethod || state.isEphemeral()) return; - if (!myContract.getReturnValue().isValueCompatible(state, value)) { - myViolations.add(expression); - } else { - myNonViolations.add(expression); - } - } - - @Override - public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState) { - if (!memState.isEphemeral() && instruction.getMethodType() == MethodCallInstruction.MethodType.REGULAR_METHOD_CALL) { - if (myContract.getReturnValue().isFail()) { - ContainerUtil.addIfNotNull(myFailures, instruction.getCallExpression()); - return DfaInstructionState.EMPTY_ARRAY; - } - if (weCannotInferAnythingAboutMethodReturnValue(instruction)) { - DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); - for (DfaInstructionState state: states) { - state.getMemoryState().markEphemeral(); - } - return states; - } - } - return super.visitMethodCall(instruction, runner, memState); - } - - @NotNull - @Override - public DfaInstructionState[] visitControlTransfer(@NotNull ControlTransferInstruction instruction, - @NotNull DataFlowRunner runner, - @NotNull DfaMemoryState state) { - if (!state.isEphemeral()) { - if (instruction instanceof ReturnInstruction && ((ReturnInstruction)instruction).isViaException()) { - ContainerUtil.addIfNotNull(myFailures, ((ReturnInstruction)instruction).getAnchor()); - } - else { - myMayReturnNormally = true; - } - } - return super.visitControlTransfer(instruction, runner, state); - } - - private Map getErrors() { - HashMap errors = ContainerUtil.newHashMap(); - for (PsiElement element : myViolations) { - if (!myNonViolations.contains(element)) { - errors.put(element, "Contract clause '" + myContract + "' is violated"); - } - } - - if (!myContract.getReturnValue().isFail()) { - if (myOwnContract && !myMayReturnNormally && - !(PsiUtil.canBeOverridden(myMethod) && ControlFlowUtils.methodAlwaysThrowsException(myMethod))) { - for (PsiElement element : myFailures) { - errors.put(element, "Return value of clause '" + myContract + "' could be replaced with 'fail' as method always fails"+ - (myContract.isTrivial() ? "" : " in this case")); - } - } - } else if (myFailures.isEmpty() && errors.isEmpty()) { - PsiIdentifier nameIdentifier = myMethod.getNameIdentifier(); - errors.put(nameIdentifier != null ? nameIdentifier : myMethod, - "Contract clause '" + myContract + "' is violated: no exception is thrown"); - } - - return errors; - } - - private static boolean weCannotInferAnythingAboutMethodReturnValue(MethodCallInstruction instruction) { - PsiMethod target = instruction.getTargetMethod(); - return instruction.getContracts().isEmpty() && target != null && !target.isConstructor() && !NullableNotNullManager.isNotNull(target); - } + private ContractChecker(PsiMethod method, StandardMethodContract contract, boolean ownContract) { + super(false, null); + myMethod = method; + myContract = contract; + myOwnContract = ownContract; } static Map checkContractClause(PsiMethod method, StandardMethodContract contract, boolean ownContract) { + PsiCodeBlock body = method.getBody(); if (body == null) return Collections.emptyMap(); - DataFlowRunner runner = new StandardDataFlowRunner(false, null); + ContractChecker checker = new ContractChecker(method, contract, ownContract); PsiParameter[] parameters = method.getParameterList().getParameters(); - final DfaMemoryState initialState = runner.createMemoryState(); - final DfaValueFactory factory = runner.getFactory(); + final DfaMemoryState initialState = checker.createMemoryState(); + final DfaValueFactory factory = checker.getFactory(); for (int i = 0; i < contract.getParameterCount(); i++) { ValueConstraint constraint = contract.getParameterConstraint(i); DfaConstValue comparisonValue = constraint.getComparisonValue(factory); @@ -140,8 +62,89 @@ class ContractChecker { } } - ContractCheckerVisitor visitor = new ContractCheckerVisitor(method, contract, ownContract); - runner.analyzeMethod(body, visitor, false, Collections.singletonList(initialState)); - return visitor.getErrors(); + checker.analyzeMethod(body, new StandardInstructionVisitor(), false, Collections.singletonList(initialState)); + return checker.getErrors(); + } + + @NotNull + @Override + protected DfaInstructionState[] acceptInstruction(@NotNull InstructionVisitor visitor, @NotNull DfaInstructionState instructionState) { + DfaMemoryState memState = instructionState.getMemoryState(); + if (memState.isEphemeral()) { + return super.acceptInstruction(visitor, instructionState); + } + Instruction instruction = instructionState.getInstruction(); + if (instruction instanceof CheckReturnValueInstruction) { + PsiElement anchor = ((CheckReturnValueInstruction)instruction).getReturn(); + DfaValue retValue = memState.pop(); + if (!myContract.getReturnValue().isValueCompatible(memState, retValue)) { + myViolations.add(anchor); + } else { + myNonViolations.add(anchor); + } + return InstructionVisitor.nextInstruction(instruction, this, memState); + + } + + if (instruction instanceof ReturnInstruction) { + if (((ReturnInstruction)instruction).isViaException()) { + ContainerUtil.addIfNotNull(myFailures, ((ReturnInstruction)instruction).getAnchor()); + } else { + myMayReturnNormally = true; + } + } + + if (instruction instanceof MethodCallInstruction && + ((MethodCallInstruction)instruction).getMethodType() == MethodCallInstruction.MethodType.REGULAR_METHOD_CALL) { + if (myContract.getReturnValue().isFail()) { + ContainerUtil.addIfNotNull(myFailures, ((MethodCallInstruction)instruction).getCallExpression()); + return DfaInstructionState.EMPTY_ARRAY; + } + if (weCannotInferAnythingAboutMethodReturnValue((MethodCallInstruction)instruction)) { + return markEverythingEphemeral(visitor, instructionState); + } + } + + return super.acceptInstruction(visitor, instructionState); + } + + private static boolean weCannotInferAnythingAboutMethodReturnValue(MethodCallInstruction instruction) { + PsiMethod target = instruction.getTargetMethod(); + return instruction.getContracts().isEmpty() && target != null && !target.isConstructor() && !NullableNotNullManager.isNotNull(target); + } + + @NotNull + private DfaInstructionState[] markEverythingEphemeral(@NotNull InstructionVisitor visitor, + @NotNull DfaInstructionState instructionState) { + DfaInstructionState[] result = super.acceptInstruction(visitor, instructionState); + for (DfaInstructionState state : result) { + state.getMemoryState().markEphemeral(); + } + return result; + } + + private Map getErrors() { + HashMap errors = ContainerUtil.newHashMap(); + for (PsiElement element : myViolations) { + if (!myNonViolations.contains(element)) { + errors.put(element, "Contract clause '" + myContract + "' is violated"); + } + } + + if (!myContract.getReturnValue().isFail()) { + if (myOwnContract && !myMayReturnNormally && + !(PsiUtil.canBeOverridden(myMethod) && ControlFlowUtils.methodAlwaysThrowsException(myMethod))) { + for (PsiElement element : myFailures) { + errors.put(element, "Return value of clause '" + myContract + "' could be replaced with 'fail' as method always fails"+ + (myContract.isTrivial() ? "" : " in this case")); + } + } + } else if (myFailures.isEmpty() && errors.isEmpty()) { + PsiIdentifier nameIdentifier = myMethod.getNameIdentifier(); + errors.put(nameIdentifier != null ? nameIdentifier : myMethod, + "Contract clause '" + myContract + "' is violated: no exception is thrown"); + } + + return errors; } } 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 b648dde79d63..31eb41637794 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 @@ -140,8 +140,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (parent instanceof PsiLambdaExpression && myCodeFragment instanceof PsiExpression) { generateBoxingUnboxingInstructionFor((PsiExpression)myCodeFragment, LambdaUtil.getFunctionalInterfaceReturnType((PsiLambdaExpression)parent)); - addInstruction(new CheckNotNullInstruction(NullabilityProblemKind.nullableReturn.problem((PsiExpression)myCodeFragment))); - addInstruction(new PopInstruction()); + addInstruction(new CheckReturnValueInstruction((PsiExpression)myCodeFragment)); } addInstruction(new ReturnInstruction(myFactory.controlTransfer(ReturnTransfer.INSTANCE, FList.emptyList()), null)); @@ -832,8 +831,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { generateBoxingUnboxingInstructionFor(returnValue, LambdaUtil.getFunctionalInterfaceReturnType(lambdaExpression)); } } - addInstruction(new CheckNotNullInstruction(NullabilityProblemKind.nullableReturn.problem(returnValue))); - addInstruction(new PopInstruction()); + addInstruction(new CheckReturnValueInstruction(returnValue)); } addInstruction(new ReturnInstruction(myFactory.controlTransfer(ReturnTransfer.INSTANCE, myTrapStack), statement)); @@ -1294,19 +1292,19 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } PsiType type = expression.getType(); if (op == JavaTokenType.ANDAND) { - generateAndOrExpression(expression, operands, type, true, true); + generateAndExpression(operands, type, true); } else if (op == JavaTokenType.OROR) { - generateAndOrExpression(expression, operands, type, false, true); + generateOrExpression(operands, type, true); } else if (op == JavaTokenType.XOR && PsiType.BOOLEAN.equals(type)) { generateXorExpression(expression, operands, type, false); } else if (op == JavaTokenType.AND && PsiType.BOOLEAN.equals(type)) { - generateAndOrExpression(expression, operands, type, true, false); + generateAndExpression(operands, type, false); } else if (op == JavaTokenType.OR && PsiType.BOOLEAN.equals(type)) { - generateAndOrExpression(expression, operands, type, false, false); + generateOrExpression(operands, type, false); } else if (isBinaryDivision(op) && operands.length == 2 && type != null && PsiType.LONG.isAssignableFrom(type)) { @@ -1456,8 +1454,29 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { operand = operands[i]; operand.accept(this); generateBoxingUnboxingInstructionFor(operand, exprType); - PsiExpression psiAnchor = expression.isPhysical() ? expression : null; - addInstruction(new BinopInstruction(JavaTokenType.NE, psiAnchor, exprType, i)); + PsiElement psiAnchor = i == operands.length - 1 && expression.isPhysical() ? expression : null; + addInstruction(new BinopInstruction(JavaTokenType.NE, psiAnchor, exprType)); + } + } + + private void generateOrExpression(PsiExpression[] operands, final PsiType exprType, boolean shortCircuit) { + for (int i = 0; i < operands.length; i++) { + PsiExpression operand = operands[i]; + operand.accept(this); + generateBoxingUnboxingInstructionFor(operand, exprType); + if (!shortCircuit) { + if (i > 0) { + combineStackBooleans(false, operand); + } + continue; + } + + PsiExpression nextOperand = i == operands.length - 1 ? null : operands[i + 1]; + if (nextOperand != null) { + addInstruction(new ConditionalGotoInstruction(getStartOffset(nextOperand), true, operand)); + addInstruction(new PushInstruction(myFactory.getConstFactory().getTrue(), null)); + addInstruction(new GotoInstruction(getEndOffset(operands[operands.length - 1]))); + } } } @@ -1489,32 +1508,39 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { overPushSuccess.setOffset(pushSuccess.getIndex() + 1); } - private void generateAndOrExpression(PsiExpression expression, - PsiExpression[] operands, - final PsiType exprType, - boolean and, - boolean shortCircuit) { + private void generateAndExpression(PsiExpression[] operands, final PsiType exprType, boolean shortCircuit) { + List branchToFail = new ArrayList<>(); for (int i = 0; i < operands.length; i++) { PsiExpression operand = operands[i]; operand.accept(this); generateBoxingUnboxingInstructionFor(operand, exprType); + if (!shortCircuit) { if (i > 0) { - combineStackBooleans(and, operand); + combineStackBooleans(true, operand); } continue; } - PsiExpression nextOperand = i == operands.length - 1 ? null : operands[i + 1]; - if (nextOperand != null) { - addInstruction(new ConditionalGotoInstruction(getStartOffset(nextOperand), !and, operand)); - addInstruction(new PushInstruction(myFactory.getBoolean(!and), expression)); - addInstruction(new GotoInstruction(getEndOffset(operands[operands.length - 1]))); - } + ConditionalGotoInstruction onFail = new ConditionalGotoInstruction(null, true, operand); + branchToFail.add(onFail); + addInstruction(onFail); } - if (shortCircuit) { - addInstruction(new ResultOfInstruction(expression)); + + if (!shortCircuit) { + return; } + + addInstruction(new PushInstruction(myFactory.getConstFactory().getTrue(), null)); + GotoInstruction toSuccess = new GotoInstruction(null); + addInstruction(toSuccess); + PushInstruction pushFalse = new PushInstruction(myFactory.getConstFactory().getFalse(), null); + addInstruction(pushFalse); + for (ConditionalGotoInstruction toFail : branchToFail) { + toFail.setOffset(pushFalse.getIndex()); + } + toSuccess.setOffset(pushFalse.getIndex()+1); + } @Override public void visitClassObjectAccessExpression(PsiClassObjectAccessExpression expression) { @@ -1906,7 +1932,7 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { addInstruction(new AssignInstruction(operand, null, myFactory.createValue(operand))); } else if (expression.getOperationTokenType() == JavaTokenType.EXCL) { - addInstruction(new NotInstruction(expression)); + addInstruction(new NotInstruction()); } else if (expression.getOperationTokenType() == JavaTokenType.MINUS && (PsiType.INT.equals(type) || PsiType.LONG.equals(type))) { addInstruction(new PushInstruction(myFactory.getConstFactory().createDefault(type), null)); 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 d6914948701e..a8f212296a4e 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,6 +15,7 @@ */ 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; @@ -35,6 +36,7 @@ import org.jetbrains.annotations.Nullable; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import static com.intellij.psi.CommonClassNames.*; @@ -51,14 +53,12 @@ class CustomMethodHandlers { interface CustomMethodHandler { - @Nullable - DfaValue getMethodResult(DfaCallArguments callArguments, DfaMemoryState memState, DfaValueFactory factory); - + List handle(DfaCallArguments callArguments, DfaMemoryState memState, DfaValueFactory factory); default CustomMethodHandler compose(CustomMethodHandler other) { if (other == null) return this; return (args, memState, factory) -> { - DfaValue result = this.getMethodResult(args, memState, factory); - return result == null ? other.getMethodResult(args, memState, factory) : result; + List result = this.handle(args, memState, factory); + return result.isEmpty() ? other.handle(args, memState, factory) : result; }; } @@ -71,14 +71,16 @@ class CustomMethodHandlers { .register(staticCall(JAVA_LANG_MATH, "abs").parameterTypes("int"), (args, memState, factory) -> mathAbs(args.myArguments, memState, factory, false)) .register(staticCall(JAVA_LANG_MATH, "abs").parameterTypes("long"), - (args, memState, factory) -> mathAbs(args.myArguments, memState, factory, true)) - .register(DfaOptionalSupport.OPTIONAL_OF_NULLABLE, - (args, memState, factory) -> ofNullable(args.myArguments[0], memState, factory)); + (args, memState, factory) -> mathAbs(args.myArguments, memState, factory, true)); - public static CustomMethodHandler find(PsiMethod method) { + public static CustomMethodHandler find(MethodCallInstruction instruction) { + PsiMethod method = instruction.getTargetMethod(); CustomMethodHandler handler = null; if (isConstantCall(method)) { - handler = (args, memState, factory) -> handleConstantCall(args, memState, factory, method); + handler = (args, memState, factory) -> { + DfaValue value = handleConstantCall(args, memState, factory, method); + return value == null ? Collections.emptyList() : singleResult(memState, value); + }; } CustomMethodHandler handler2 = CUSTOM_METHOD_HANDLERS.mapFirst(method); return handler == null ? handler2 : handler.compose(handler2); @@ -188,32 +190,27 @@ class CustomMethodHandlers { }); } - private static DfaValue indexOf(DfaValue qualifier, - DfaMemoryState memState, - DfaValueFactory factory, - SpecialField specialField) { + private static List indexOf(DfaValue qualifier, + DfaMemoryState memState, + DfaValueFactory factory, + SpecialField specialField) { DfaValue length = specialField.createValue(factory, qualifier); LongRangeSet range = memState.getValueFact(length, DfaFactType.RANGE); long maxLen = range == null || range.isEmpty() ? Integer.MAX_VALUE : range.max(); - return factory.getFactValue(DfaFactType.RANGE, LongRangeSet.range(-1, maxLen - 1)); + return singleResult(memState, factory.getFactValue(DfaFactType.RANGE, LongRangeSet.range(-1, maxLen - 1))); } - private static DfaValue ofNullable(DfaValue argument, DfaMemoryState state, DfaValueFactory factory) { - if (state.isNull(argument)) { - return factory.getFactValue(DfaFactType.OPTIONAL_PRESENCE, false); - } - if (state.isNotNull(argument)) { - return factory.getFactValue(DfaFactType.OPTIONAL_PRESENCE, true); - } - return null; - } - - private static DfaValue mathAbs(DfaValue[] args, DfaMemoryState memState, DfaValueFactory factory, boolean isLong) { + private static List mathAbs(DfaValue[] args, DfaMemoryState memState, DfaValueFactory factory, boolean isLong) { DfaValue arg = ArrayUtil.getFirstElement(args); - if (arg == null) return null; + if(arg == null) return Collections.emptyList(); LongRangeSet range = memState.getValueFact(arg, DfaFactType.RANGE); - if (range == null) return null; - return factory.getFactValue(DfaFactType.RANGE, range.abs(isLong)); + if (range == null) return Collections.emptyList(); + return singleResult(memState, factory.getFactValue(DfaFactType.RANGE, range.abs(isLong))); + } + + private static List singleResult(DfaMemoryState state, DfaValue value) { + state.push(value); + return Collections.singletonList(state); } private static Object getConstantValue(DfaMemoryState memoryState, DfaValue value) { 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 d23cae024866..f6cdeca3d005 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 @@ -14,6 +14,7 @@ import com.intellij.codeInspection.dataFlow.fix.ReplaceWithObjectsEqualsFix; import com.intellij.codeInspection.dataFlow.fix.SimplifyToAssignmentFix; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; +import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.nullable.NullableStuffInspectionBase; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.project.Project; @@ -29,7 +30,6 @@ import com.intellij.psi.util.PsiUtil; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.*; import com.intellij.util.containers.ContainerUtil; -import com.siyeh.ig.bugs.EqualsWithItselfInspection; import com.siyeh.ig.fixes.EqualsToEqualityFix; import com.siyeh.ig.psiutils.*; import one.util.streamex.IntStreamEx; @@ -253,7 +253,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool for (Instruction instruction : allProblems) { if (instruction instanceof TypeCastInstruction && - reportedAnchors.add(((TypeCastInstruction)instruction).getExpression().getCastType())) { + reportedAnchors.add(((TypeCastInstruction)instruction).getCastExpression().getCastType())) { reportCastMayFail(holder, (TypeCastInstruction)instruction); } else if (instruction instanceof BranchingInstruction) { @@ -263,6 +263,8 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reportAlwaysFailingCalls(holder, visitor, reportedAnchors); + reportConstantPushes(runner, holder, reportedAnchors); + reportNullabilityProblems(holder, visitor, reportedAnchors); reportNullableReturns(visitor, holder, reportedAnchors, scope); if (SUGGEST_NULLABLE_ANNOTATIONS) { @@ -272,9 +274,9 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reportOptionalOfNullableImprovements(holder, reportedAnchors, visitor.getOfNullableCalls()); - visitor.getBooleanExpressions().forEach((expression, state) -> { - if (state != ThreeState.UNSURE && reportedAnchors.add(expression)) { - reportConstantBoolean(holder, expression, state.toBoolean()); + visitor.getBooleanCalls().forEach((call, state) -> { + if (state != ThreeState.UNSURE && reportedAnchors.add(call)) { + reportConstantCondition(holder, call, state.toBoolean()); } }); @@ -435,15 +437,17 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool private static void reportAlwaysFailingCalls(ProblemsHolder holder, DataFlowInstructionVisitor visitor, HashSet reportedAnchors) { - visitor.alwaysFailingCalls().remove(TestUtils::isExceptionExpected).forEach(call -> { - if (reportedAnchors.add(call)) { - holder.registerProblem(getElementToHighlight(call), getContractMessage(JavaMethodContractUtil.getMethodCallContracts(call))); + 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)); } }); } @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"); } @@ -468,37 +472,61 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return call; } + private void reportConstantPushes(StandardDataFlowRunner runner, + ProblemsHolder holder, + Set reportedAnchors) { + for (Instruction instruction : runner.getInstructions()) { + if (instruction instanceof PushInstruction) { + PsiExpression place = ((PushInstruction)instruction).getPlace(); + DfaValue value = ((PushInstruction)instruction).getValue(); + Object constant = value instanceof DfaConstValue ? ((DfaConstValue)value).getValue() : null; + if (place instanceof PsiPolyadicExpression && constant instanceof Boolean && !isFlagCheck(place) && reportedAnchors.add(place)) { + reportConstantCondition(holder, place, (Boolean)constant); + } + } + } + } + private static void reportOptionalOfNullableImprovements(ProblemsHolder holder, Set reportedAnchors, - Map nullArgs) { - nullArgs.forEach((anchor, alwaysPresent) -> { - if (alwaysPresent == ThreeState.UNSURE) return; - if (reportedAnchors.add(anchor)) { - if (alwaysPresent.toBoolean()) { - holder.registerProblem(anchor, "Passing a non-null argument to Optional", - DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(anchor)); - } else { - holder.registerProblem(anchor, "Passing null argument to Optional", - DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(anchor)); + Map nullArgs) { + nullArgs.forEach((call, nullArg) -> { + PsiElement arg = call.getArgumentAnchor(0); + if (reportedAnchors.add(arg)) { + switch (nullArg) { + case YES: + holder.registerProblem(arg, "Passing null argument to Optional", + DfaOptionalSupport.createReplaceOptionalOfNullableWithEmptyFix(arg)); + break; + case NO: + holder.registerProblem(arg, "Passing a non-null argument to Optional", + DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix(arg)); + break; + default: } } }); } private void reportConstantReferenceValues(ProblemsHolder holder, DataFlowInstructionVisitor visitor, Set reportedAnchors) { - visitor.getConstantReferenceValues().forEach((ref, dfaConst) -> { - if (ref.getParent() instanceof PsiReferenceExpression || DfaConstValue.isSentinel(dfaConst)) return; - if (!reportedAnchors.add(ref)) return; + for (Pair pair : visitor.getConstantReferenceValues()) { + PsiReferenceExpression ref = pair.first; + if (ref.getParent() instanceof PsiReferenceExpression || !reportedAnchors.add(ref)) { + continue; + } - final Object value = dfaConst.getValue(); - PsiVariable constant = dfaConst.getConstant(); + final Object value = pair.second.getValue(); + PsiVariable constant = pair.second.getConstant(); + final String presentableName = constant != null ? constant.getName() : String.valueOf(value); final String exprText = String.valueOf(value); - final String presentableName = constant != null ? constant.getName() : exprText; + if (presentableName == null || exprText == null) { + continue; + } List fixes = new SmartList<>(); fixes.add(new ReplaceWithConstantValueFix(presentableName, exprText)); boolean isAssertion = value instanceof Boolean && isAssertionEffectively(ref, (Boolean)value); - if (isAssertion && DONT_REPORT_TRUE_ASSERT_STATEMENTS) return; + if (isAssertion && DONT_REPORT_TRUE_ASSERT_STATEMENTS) continue; if (holder.isOnTheFly()) { fixes.add(new SetInspectionOptionFix(this, "REPORT_CONSTANT_REFERENCE_VALUES", InspectionsBundle.message("inspection.data.flow.turn.off.constant.references.quickfix"), @@ -510,8 +538,9 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } holder.registerProblem(ref, "Value #ref #loc is always '" + presentableName + "'", - ProblemHighlightType.WEAK_WARNING, fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); - }); + ProblemHighlightType.WEAK_WARNING, + fixes.toArray(LocalQuickFix.EMPTY_ARRAY)); + } } private void reportNullableArgumentsPassedToNonAnnotated(DataFlowInstructionVisitor visitor, ProblemsHolder holder, Set reportedAnchors) { @@ -593,7 +622,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } private static void reportCastMayFail(ProblemsHolder holder, TypeCastInstruction instruction) { - PsiTypeCastExpression typeCast = instruction.getExpression(); + PsiTypeCastExpression typeCast = instruction.getCastExpression(); PsiExpression operand = typeCast.getOperand(); PsiTypeElement castType = typeCast.getCastType(); assert castType != null; @@ -624,12 +653,10 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool InspectionsBundle.message("dataflow.message.unreachable.switch.label")); } } - else if (psiAnchor != null && !isFlagCheck(psiAnchor)) { + else if (psiAnchor != null && !reportedAnchors.contains(psiAnchor) && !isFlagCheck(psiAnchor)) { boolean evaluatesToTrue = trueSet.contains(instruction); final PsiElement parent = psiAnchor.getParent(); - if (parent instanceof PsiAssignmentExpression && - ((PsiAssignmentExpression)parent).getLExpression() == psiAnchor && - reportedAnchors.add(psiAnchor)) { + if (parent instanceof PsiAssignmentExpression && ((PsiAssignmentExpression)parent).getLExpression() == psiAnchor) { holder.registerProblem( psiAnchor, InspectionsBundle.message("dataflow.message.pointless.assignment.expression", Boolean.toString(evaluatesToTrue)), @@ -637,17 +664,19 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool ); } else { - TextRange range = - instruction instanceof ExpressionPushingInstruction ? ((ExpressionPushingInstruction)instruction).getExpressionRange() : null; - if (range != null) { - // report rare cases like a == b == c where "a == b" part is constant - String message = InspectionsBundle.message("dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); - holder.registerProblem(psiAnchor, range, message); - // do not add to reported anchors if only part of expression was reported - } else if (reportedAnchors.add(psiAnchor)) { - reportConstantCondition(holder, psiAnchor, evaluatesToTrue); + if (instruction instanceof BinopInstruction) { + TextRange range = ((BinopInstruction)instruction).getAnchorRange(); + if (range != null) { + // report rare cases like a == b == c where "a == b" part is constant + String message = InspectionsBundle.message("dataflow.message.constant.condition", Boolean.toString(evaluatesToTrue)); + holder.registerProblem(psiAnchor, range, message); + // do not add to reported anchors if only part of expression was reported + return; + } } + reportConstantCondition(holder, psiAnchor, evaluatesToTrue); } + reportedAnchors.add(psiAnchor); } } @@ -673,7 +702,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } private void reportConstantBoolean(ProblemsHolder holder, PsiElement psiAnchor, boolean evaluatesToTrue) { - if (shouldBeSuppressed(psiAnchor)) return; boolean isAssertion = isAssertionEffectively(psiAnchor, evaluatesToTrue); if (!DONT_REPORT_TRUE_ASSERT_STATEMENTS || !isAssertion) { List fixes = new ArrayList<>(); @@ -690,17 +718,6 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } } - private static boolean shouldBeSuppressed(PsiElement anchor) { - if (!(anchor instanceof PsiExpression)) return false; - PsiExpression expression = (PsiExpression)anchor; - while (expression != null && BoolUtils.isNegation(expression)) { - expression = BoolUtils.getNegated(expression); - } - PsiMethodCallExpression call = ObjectUtils.tryCast(expression, PsiMethodCallExpression.class); - // Reported by "Equals with itself" inspection; avoid double reporting - return call != null && EqualsWithItselfInspection.isEqualsWithItself(call); - } - private static LocalQuickFix createReplaceWithNullCheckFix(PsiElement psiAnchor, boolean evaluatesToTrue) { if (evaluatesToTrue) return null; if (!(psiAnchor instanceof PsiMethodCallExpression) || !MethodCallUtils.isEqualsCall((PsiMethodCallExpression)psiAnchor)) return null; @@ -864,7 +881,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return false; } - static boolean isFlagCheck(PsiElement element) { + private static boolean isFlagCheck(PsiElement element) { PsiElement scope = PsiTreeUtil.getParentOfType(element, PsiStatement.class, PsiVariable.class); PsiExpression topExpression = scope instanceof PsiIfStatement ? ((PsiIfStatement)scope).getCondition() : scope instanceof PsiVariable ? ((PsiVariable)scope).getInitializer() : 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 55a929705418..494e69a90d7f 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 @@ -5,13 +5,13 @@ import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.*; import com.intellij.openapi.diagnostic.Logger; import com.intellij.openapi.util.Pair; -import com.intellij.openapi.util.TextRange; import com.intellij.psi.*; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiTypesUtil; -import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ObjectUtils; import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; +import com.intellij.util.containers.MultiMap; import com.siyeh.ig.psiutils.ExpressionUtils; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; @@ -20,19 +20,18 @@ 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 myBooleanExpressions = new HashMap<>(); - private final Map myOfNullableCalls = 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<>(); private final Map myMethodReferenceResults = new HashMap<>(); private final Map myOutOfBoundsArrayAccesses = new HashMap<>(); - private final Map myValues = new HashMap<>(); + private final MultiMap myPossibleVariableValues = MultiMap.createSet(); private final Set myReceiverMutabilityViolation = new HashSet<>(); private final Set myArgumentMutabilityViolation = new HashSet<>(); private final Map mySameValueAssigned = new HashMap<>(); @@ -124,12 +123,12 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return myArrayStoreProblems; } - Map getOfNullableCalls() { + Map getOfNullableCalls() { return myOfNullableCalls; } - Map getBooleanExpressions() { - return myBooleanExpressions; + Map getBooleanCalls() { + return myBooleanCalls; } Map getMethodReferenceResults() { @@ -152,8 +151,9 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return StreamEx.ofKeys(myOutOfBoundsArrayAccesses, ThreeState.YES::equals); } - StreamEx alwaysFailingCalls() { - return StreamEx.ofKeys(myFailingCalls, v -> v); + Map> getAlwaysFailingCalls() { + return StreamEx.ofKeys(myFailingCalls, v -> v) + .mapToEntry(MethodCallInstruction::getCallExpression, MethodCallInstruction::getContracts).toMap(); } boolean isAlwaysReturnsNotNull(Instruction[] instructions) { @@ -162,37 +162,46 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { } @Override - protected void beforeExpressionPush(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @Nullable TextRange range, - @NotNull DfaMemoryState memState) { - expression.accept(new ExpressionVisitor(value, memState)); - handleBooleanResults(value, memState, expression); + public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + if (instruction.matches(DfaOptionalSupport.OPTIONAL_OF_NULLABLE)) { + DfaValue arg = memState.peek(); + ThreeState nullArg = memState.isNull(arg) ? ThreeState.YES : memState.isNotNull(arg) ? ThreeState.NO : ThreeState.UNSURE; + // Passing variable with unknown nullity to ofNullable assumes that it can be null + memState.applyFact(arg, DfaFactType.CAN_BE_NULL, true); + myOfNullableCalls.merge(instruction, nullArg, ThreeState::merge); + } + 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; } - @Override - protected void beforeMethodReferenceResultPush(@NotNull DfaValue value, - @NotNull PsiMethodReferenceExpression methodRef, - @NotNull DfaMemoryState state) { - if (DfaOptionalSupport.OPTIONAL_OF_NULLABLE.methodReferenceMatches(methodRef)) { - processOfNullableResult(value, state, methodRef.getReferenceNameElement()); - } - PsiMethod method = tryCast(methodRef.resolve(), PsiMethod.class); - if (method != null) { - List contracts = JavaMethodContractUtil.getMethodContracts(method); - if (contracts.isEmpty() || !contracts.get(0).isTrivial()) { - // Do not track if method reference may have different results - myMethodReferenceResults.merge(methodRef, value, (a, b) -> a == b ? a : DfaUnknownValue.getInstance()); + 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); } } - 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 protected void processArrayAccess(PsiArrayAccessExpression expression, boolean alwaysOutOfBounds) { myOutOfBoundsArrayAccesses.merge(expression, ThreeState.fromBoolean(alwaysOutOfBounds), ThreeState::merge); @@ -205,6 +214,30 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { } } + @Override + protected void processMethodReferenceResult(PsiMethodReferenceExpression methodRef, + List contracts, + DfaValue res) { + if(contracts.isEmpty() || !contracts.get(0).isTrivial()) { + // Do not track if method reference may have different results + myMethodReferenceResults.merge(methodRef, res, (a, b) -> a == b ? a : DfaUnknownValue.getInstance()); + } + } + + @Override + public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + PsiExpression place = instruction.getPlace(); + if (!instruction.isReferenceWrite() && place instanceof PsiReferenceExpression) { + DfaValue dfaValue = instruction.getValue(); + if (dfaValue instanceof DfaVariableValue) { + DfaConstValue constValue = memState.getConstantValue((DfaVariableValue)dfaValue); + boolean report = constValue != null && shouldReportConstValue(constValue.getValue()); + myPossibleVariableValues.putValue(instruction, report ? constValue : ANY_VALUE); + } + } + return super.visitPush(instruction, runner, memState); + } + @Override public DfaInstructionState[] visitEndOfInitializer(EndOfInitializerInstruction instruction, DataFlowRunner runner, DfaMemoryState state) { if (!instruction.isStatic()) { @@ -213,58 +246,31 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return super.visitEndOfInitializer(instruction, runner, state); } - public Map getConstantReferenceValues() { - return myValues; - } - - private static boolean hasNonTrivialFailingContracts(PsiCallExpression call) { - List contracts = JavaMethodContractUtil.getMethodCallContracts(call); - return !contracts.isEmpty() && - contracts.stream().anyMatch(contract -> contract.getReturnValue().isFail() && !contract.isTrivial()); - } - - private void handleBooleanResults(DfaValue value, DfaMemoryState memState, PsiExpression expression) { - ThreeState curState = myBooleanExpressions.get(expression); - if (curState == ThreeState.UNSURE) return; - ThreeState nextState = ThreeState.UNSURE; - value = value instanceof DfaVariableValue ? memState.getConstantValue((DfaVariableValue)value) : value; - if (value instanceof DfaConstValue) { - Object val = ((DfaConstValue)value).getValue(); - if (val instanceof Boolean) { - nextState = ThreeState.fromBoolean((Boolean)val); - if (curState != null && curState != nextState) { - nextState = ThreeState.UNSURE; + public List> getConstantReferenceValues() { + List> result = ContainerUtil.newArrayList(); + for (PushInstruction instruction : myPossibleVariableValues.keySet()) { + Collection values = myPossibleVariableValues.get(instruction); + if (values.size() == 1) { + Object singleValue = values.iterator().next(); + if (singleValue != ANY_VALUE) { + result.add(Pair.create((PsiReferenceExpression)instruction.getPlace(), (DfaConstValue)singleValue)); } } } - if (curState != null || shouldCollectBooleanResult(expression)) { - myBooleanExpressions.put(expression, nextState); - } + return result; } - private static boolean shouldCollectBooleanResult(PsiExpression expression) { - if (expression instanceof PsiLiteralExpression) return false; - PsiType type = expression.getType(); - if (type == null || !PsiType.BOOLEAN.isAssignableFrom(type)) return false; - if (expression instanceof PsiPrefixExpression || expression instanceof PsiPolyadicExpression) { - return !DataFlowInspectionBase.isFlagCheck(expression); - } - PsiPolyadicExpression polyadic = tryCast(PsiUtil.skipParenthesizedExprUp(expression.getParent()), PsiPolyadicExpression.class); - if (polyadic != null) { - if ((polyadic.getOperationTokenType().equals(JavaTokenType.ANDAND) || polyadic.getOperationTokenType().equals(JavaTokenType.OROR)) && - !DataFlowInspectionBase.isFlagCheck(expression)) return true; - } - if (expression instanceof PsiMethodCallExpression) { - PsiMethodCallExpression call = (PsiMethodCallExpression)expression; - if (ExpressionUtils.isVoidContext(call)) return false; - PsiMethod method = call.resolveMethod(); - if (method == null || !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()); - } - return false; + 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 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()); } @Override @@ -311,49 +317,4 @@ 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]); - } - } - - @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)); - } - } - - @Override - public void visitReferenceExpression(PsiReferenceExpression expression) { - super.visitReferenceExpression(expression); - DfaConstValue oldValue = myValues.get(expression); - if (DfaConstValue.isSentinel(oldValue)) return; - if (myValue instanceof DfaVariableValue) { - DfaConstValue constValue = myMemState.getConstantValue((DfaVariableValue)myValue); - boolean report = constValue != null && shouldReportConstValue(constValue.getValue()); - if (!report) { - constValue = null; - } - DfaConstValue newValue = constValue != null && (oldValue == null || oldValue == constValue) - ? constValue - : myValue.getFactory().getConstFactory().getSentinel(); - myValues.put(expression, newValue); - } - } - } } 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 882c2703e1ac..0bc7e0634ddb 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 @@ -30,7 +30,6 @@ import com.intellij.psi.util.PropertyUtilBase; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; -import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.Stack; import gnu.trove.TIntObjectHashMap; @@ -998,9 +997,8 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return true; } - ThreeState equalByConstants = equalByConstant(c1Index, c2Index); - if (equalByConstants != ThreeState.UNSURE) return equalByConstants.toBoolean() != isNegated; if (!isNegated) { //Equals + if (c1Index.equals(c2Index) || areCompatibleConstants(c1Index, c2Index)) return true; if (isUnstableValue(dfaLeft) || isUnstableValue(dfaRight)) return true; if (!uniteClasses(c1Index, c2Index)) return false; @@ -1015,6 +1013,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { myCachedNonTrivialEqClasses = null; } else { // Not Equals + if (c1Index.equals(c2Index) || areCompatibleConstants(c1Index, c2Index)) return false; if (isNull(dfaLeft) && isPrimitive(dfaRight) || isNull(dfaRight) && isPrimitive(dfaLeft)) return true; myDistinctClasses.addUnordered(c1Index, c2Index); } @@ -1035,8 +1034,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return true; } - ThreeState equalByConstants = equalByConstant(c1Index, c2Index); - if (equalByConstants != ThreeState.UNSURE) return !equalByConstants.toBoolean(); + if (c1Index.equals(c2Index) || areCompatibleConstants(c1Index, c2Index)) return false; if (isNull(dfaLeft) && isPrimitive(dfaRight) || isNull(dfaRight) && isPrimitive(dfaLeft)) return true; myCachedHash = null; return myDistinctClasses.addOrdered(c1Index, c2Index); @@ -1069,35 +1067,17 @@ public class DfaMemoryStateImpl implements DfaMemoryState { c2 == null && c1 instanceof PsiVariable; } - @NotNull - private ThreeState equalByConstant(int i1, int i2) { - if (i1 == i2) return ThreeState.YES; - EqClass ec1 = myEqClasses.get(i1); - EqClass ec2 = myEqClasses.get(i2); - if (ec1 == null || ec2 == null) return ThreeState.UNSURE; - DfaValue constOrBox1 = ec1.findConstant(true); - DfaValue constOrBox2 = ec2.findConstant(true); - if (constOrBox1 == null || constOrBox2 == null) return ThreeState.UNSURE; - if (constOrBox1 instanceof DfaConstValue && constOrBox2 instanceof DfaConstValue) { - return areConstantsEqual((DfaConstValue)constOrBox1, (DfaConstValue)constOrBox2); - } - if (constOrBox1 instanceof DfaBoxedValue && constOrBox2 instanceof DfaBoxedValue) { - DfaValue wrapped1 = ((DfaBoxedValue)constOrBox1).getWrappedValue(); - DfaValue wrapped2 = ((DfaBoxedValue)constOrBox2).getWrappedValue(); - if (wrapped1 instanceof DfaConstValue && wrapped2 instanceof DfaConstValue && - areConstantsEqual((DfaConstValue)wrapped1, (DfaConstValue)wrapped2) == ThreeState.NO) { - return ThreeState.NO; - } - } - return ThreeState.UNSURE; + private boolean areCompatibleConstants(int i1, int i2) { + Double dv1 = getDoubleValue(i1); + return dv1 != null && dv1.equals(getDoubleValue(i2)); } - private static ThreeState areConstantsEqual(DfaConstValue const1, DfaConstValue const2) { - Number value1 = ObjectUtils.tryCast(const1.getValue(), Number.class); - Number value2 = ObjectUtils.tryCast(const2.getValue(), Number.class); - if (value1 == null || value2 == null) return ThreeState.UNSURE; - if (value1 instanceof Long && value2 instanceof Long) return ThreeState.fromBoolean(value1.equals(value2)); - return ThreeState.fromBoolean(value1.doubleValue() == value2.doubleValue()); + @Nullable + private Double getDoubleValue(int eqClassIndex) { + EqClass ec = myEqClasses.get(eqClassIndex); + DfaValue dfaConst = ec == null ? null : ec.findConstant(false); + Object constValue = dfaConst instanceof DfaConstValue ? ((DfaConstValue)dfaConst).getValue() : null; + return constValue instanceof Number ? ((Number)constValue).doubleValue() : null; } boolean isUnknownState(DfaValue val) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java index f057bf796824..266f27ea27e2 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java @@ -14,7 +14,11 @@ import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.*; import com.intellij.psi.impl.source.resolve.JavaResolveUtil; import com.intellij.psi.tree.IElementType; -import com.intellij.psi.util.*; +import com.intellij.psi.util.CachedValueProvider; +import com.intellij.psi.util.CachedValuesManager; +import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.util.PsiUtil; +import com.intellij.ui.treeStructure.NullNode; import com.intellij.util.IncorrectOperationException; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.FList; @@ -24,6 +28,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; /** @@ -142,59 +147,57 @@ public class DfaUtil { @NotNull public static Nullability inferMethodNullability(PsiMethod method) { - if (PsiUtil.resolveClassInType(method.getReturnType()) == null) { + final PsiCodeBlock body = method.getBody(); + if (body == null || PsiUtil.resolveClassInType(method.getReturnType()) == null) { return Nullability.UNKNOWN; } - return inferBlockNullability(method, InferenceFromSourceUtil.suppressNullable(method)); + return inferBlockNullability(body, InferenceFromSourceUtil.suppressNullable(method)); } @NotNull public static Nullability inferLambdaNullability(PsiLambdaExpression lambda) { - if (LambdaUtil.getFunctionalInterfaceReturnType(lambda) == null) { + final PsiElement body = lambda.getBody(); + if (body == null || LambdaUtil.getFunctionalInterfaceReturnType(lambda) == null) { return Nullability.UNKNOWN; } - return inferBlockNullability(lambda, false); + return inferBlockNullability(body, false); } @NotNull - private static Nullability inferBlockNullability(PsiParameterListOwner owner, boolean suppressNullable) { - PsiElement body = owner.getBody(); - if (body == null) return Nullability.UNKNOWN; + private static Nullability inferBlockNullability(PsiElement body, boolean suppressNullable) { + final AtomicBoolean hasNulls = new AtomicBoolean(); + final AtomicBoolean hasNotNulls = new AtomicBoolean(); + final AtomicBoolean hasUnknowns = new AtomicBoolean(); final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); - class BlockNullabilityVisitor extends StandardInstructionVisitor { - boolean hasNulls = false; - boolean hasNotNulls = false; - boolean hasUnknowns = false; - + final RunnerResult rc = dfaRunner.analyzeMethod(body, new StandardInstructionVisitor() { @Override - protected void checkReturnValue(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @NotNull PsiParameterListOwner context, - @NotNull DfaMemoryState state) { - if (context == owner) { - if (TypeConversionUtil.isPrimitiveAndNotNull(expression.getType()) || state.isNotNull(value)) { - hasNotNulls = true; + public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + if(PsiTreeUtil.isAncestor(body, instruction.getReturn(), false)) { + DfaValue returned = memState.peek(); + if (memState.isNull(returned)) { + hasNulls.set(true); } - else if (state.isNull(value)) { - hasNulls = true; + else if (memState.isNotNull(returned)) { + hasNotNulls.set(true); } else { - hasUnknowns = true; + hasUnknowns.set(true); } } + return super.visitCheckReturnValue(instruction, runner, memState); } - } - BlockNullabilityVisitor visitor = new BlockNullabilityVisitor(); - final RunnerResult rc = dfaRunner.analyzeMethod(body, visitor); + }); if (rc == RunnerResult.OK) { - if (visitor.hasNulls) { + if (hasNulls.get()) { return suppressNullable ? Nullability.UNKNOWN : Nullability.NULLABLE; } - if (visitor.hasNotNulls && !visitor.hasUnknowns) { + if (hasNotNulls.get() && !hasUnknowns.get()) { return Nullability.NOT_NULL; } } @@ -378,7 +381,7 @@ public class DfaUtil { @Override public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - PsiExpression place = instruction.getExpression(); + PsiExpression place = instruction.getPlace(); if (place != null) { PlaceResult result = myResults.computeIfAbsent(place, __ -> new PlaceResult()); ((ValuableDataFlowRunner.MyDfaMemoryState)memState).forVariableStates((variableValue, value) -> { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java index 41a9af388f2d..7c69a0b4b1f6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java @@ -16,7 +16,6 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint; -import com.intellij.codeInspection.dataFlow.value.DfaRelationValue; import com.intellij.codeInspection.dataFlow.value.DfaRelationValue.RelationType; import com.intellij.lang.injection.InjectedLanguageManager; import com.intellij.psi.*; @@ -36,7 +35,6 @@ import java.util.List; import java.util.function.Supplier; import static com.intellij.codeInspection.dataFlow.ContractReturnValue.*; -import static com.intellij.codeInspection.dataFlow.MethodContract.singleConditionContract; import static com.intellij.codeInspection.dataFlow.MethodContract.trivialContract; import static com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint.*; import static com.intellij.codeInspection.dataFlow.StandardMethodContract.createConstraintArray; @@ -50,11 +48,11 @@ public class HardcodedContracts { private static final List ARRAY_RANGE_CONTRACTS = ContainerUtil.immutableList( nonnegativeArgumentContract(1), nonnegativeArgumentContract(2), - singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), - fail()), - singleConditionContract(ContractValue.argument(2), RelationType.GT, ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), - fail()), - singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(2), fail()) + MethodContract.singleConditionContract(ContractValue.argument(1), RelationType.GT, + ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), fail()), + MethodContract.singleConditionContract(ContractValue.argument(2), RelationType.GT, + ContractValue.argument(0).specialField(SpecialField.ARRAY_LENGTH), fail()), + MethodContract.singleConditionContract(ContractValue.argument(1), RelationType.GT, ContractValue.argument(2), fail()) ); private static final CallMatcher QUEUE_POLL = instanceCall("java.util.Queue", "poll").parameterCount(0); @@ -105,17 +103,17 @@ public class HardcodedContracts { .register(instanceCall(JAVA_UTIL_MAP, "equals").parameterTypes(JAVA_LANG_OBJECT), ContractProvider.list(SpecialField.MAP_SIZE::getEqualsContracts)) .register(instanceCall(JAVA_UTIL_COLLECTION, "contains").parameterCount(1), - ContractProvider.single(() -> singleConditionContract( + ContractProvider.single(() -> MethodContract.singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), returnFalse()))) .register(instanceCall(JAVA_UTIL_MAP, "containsKey", "containsValue").parameterCount(1), - ContractProvider.single(() -> singleConditionContract( + ContractProvider.single(() -> MethodContract.singleConditionContract( ContractValue.qualifier().specialField(SpecialField.MAP_SIZE), RelationType.EQ, ContractValue.zero(), returnFalse()))) .register(instanceCall(JAVA_UTIL_LIST, "get").parameterTypes("int"), ContractProvider.list(() -> Arrays.asList(nonnegativeArgumentContract(0), specialFieldRangeContract(0, RelationType.LT, SpecialField.COLLECTION_SIZE)))) .register(instanceCall("java.util.SortedSet", "first", "last").parameterCount(0), - ContractProvider.single(() -> singleConditionContract( + ContractProvider.single(() -> MethodContract.singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), fail()))) // All these methods take array as 1st parameter, from index as 2nd and to index as 3rd @@ -125,7 +123,7 @@ public class HardcodedContracts { .register(staticCall("org.mockito.ArgumentMatchers", "argThat").parameterCount(1), ContractProvider.single(() -> new StandardMethodContract(new ValueConstraint[]{ANY_VALUE}, returnAny()))) .register(instanceCall("java.util.Queue", "peek", "poll").parameterCount(0), - (call, paramCount) -> Arrays.asList(singleConditionContract( + (call, paramCount) -> Arrays.asList(MethodContract.singleConditionContract( ContractValue.qualifier().specialField(SpecialField.COLLECTION_SIZE), RelationType.EQ, ContractValue.zero(), returnNull()), trivialContract(returnAny()))) .register(anyOf(staticCall(JAVA_LANG_MATH, "max").parameterTypes("int", "int"), @@ -139,11 +137,9 @@ public class HardcodedContracts { staticCall(JAVA_LANG_LONG, "min").parameterTypes("long", "long")), (call, paramCount) -> mathMinMax(false)) .register(instanceCall(JAVA_LANG_STRING, "startsWith", "endsWith", "contains"), - ContractProvider.single(() -> singleConditionContract( + ContractProvider.single(() -> MethodContract.singleConditionContract( ContractValue.qualifier().specialField(SpecialField.STRING_LENGTH), RelationType.LT, - ContractValue.argument(0).specialField(SpecialField.STRING_LENGTH), returnFalse()))) - .register(instanceCall(JAVA_LANG_OBJECT, "equals").parameterTypes(JAVA_LANG_OBJECT), - (call, paramCount) -> equalsContracts(call)); + ContractValue.argument(0).specialField(SpecialField.STRING_LENGTH), returnFalse()))); public static List getHardcodedContracts(@NotNull PsiMethod method, @Nullable PsiMethodCallExpression call) { PsiClass owner = method.getContainingClass(); @@ -208,49 +204,34 @@ public class HardcodedContracts { if (endLimited) { contracts.add(nonnegativeArgumentContract(1)); contracts.add(specialFieldRangeContract(1, RelationType.LE, SpecialField.STRING_LENGTH)); - contracts.add(singleConditionContract(ContractValue.argument(0), RelationType.LE.getNegated(), ContractValue.argument(1), fail())); + contracts.add(MethodContract + .singleConditionContract(ContractValue.argument(0), RelationType.LE.getNegated(), + ContractValue.argument(1), fail())); } return contracts; } static MethodContract optionalAbsentContract(ContractReturnValue returnValue) { - return singleConditionContract(ContractValue.qualifier(), RelationType.IS, ContractValue.optionalValue(false), returnValue); + return MethodContract + .singleConditionContract(ContractValue.qualifier(), RelationType.IS, ContractValue.optionalValue(false), returnValue); } static MethodContract nonnegativeArgumentContract(int argNumber) { - return singleConditionContract(ContractValue.argument(argNumber), RelationType.LT, ContractValue.zero(), fail()); + return MethodContract + .singleConditionContract(ContractValue.argument(argNumber), RelationType.LT, ContractValue.zero(), fail()); } static MethodContract specialFieldRangeContract(int index, RelationType type, SpecialField specialField) { - return singleConditionContract(ContractValue.argument(index), type.getNegated(), ContractValue.qualifier().specialField(specialField), - fail()); + return MethodContract.singleConditionContract(ContractValue.argument(index), type.getNegated(), + ContractValue.qualifier().specialField(specialField), fail()); } static List mathMinMax(boolean isMax) { - return Arrays.asList(singleConditionContract( + return Arrays.asList(MethodContract.singleConditionContract( ContractValue.argument(0), isMax ? RelationType.GT : RelationType.LT, ContractValue.argument(1), returnParameter(0)), trivialContract(returnParameter(1))); } - private static List equalsContracts(PsiMethodCallExpression call) { - PsiExpression qualifier = call == null ? null : call.getMethodExpression().getQualifierExpression(); - if (qualifier != null && knownAsEqualByReference(qualifier.getType())) { - return Arrays.asList( - singleConditionContract(ContractValue.qualifier(), RelationType.EQ, ContractValue.argument(0), returnTrue()), - trivialContract(returnFalse()) - ); - } - return Arrays.asList(new StandardMethodContract(new StandardMethodContract.ValueConstraint[]{NULL_VALUE}, returnFalse()), - singleConditionContract(ContractValue.qualifier(), DfaRelationValue.RelationType.EQ, - ContractValue.argument(0), returnTrue())); - } - - private static boolean knownAsEqualByReference(PsiType type) { - if (type instanceof PsiArrayType) return true; - PsiClass psiClass = PsiUtil.resolveClassInClassTypeOnly(type); - return psiClass != null && (psiClass.isEnum() || JAVA_LANG_CLASS.equals(psiClass.getQualifiedName())); - } - private static boolean isJunit(String className) { return className.startsWith("junit.framework.") || className.startsWith("org.junit.") || className.equals("org.testng.AssertJUnit"); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java index 2ea578cb5e4c..7f90e1f2fa17 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/InstructionVisitor.java @@ -18,89 +18,20 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.Nullability; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.value.*; -import com.intellij.openapi.util.TextRange; -import com.intellij.psi.*; -import com.intellij.psi.util.PsiTreeUtil; +import com.intellij.psi.PsiArrayAccessExpression; +import com.intellij.psi.PsiExpression; +import com.intellij.psi.PsiType; import com.intellij.psi.util.PsiUtil; -import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; import java.util.ArrayList; -import java.util.Objects; /** * @author peter */ public abstract class InstructionVisitor { - protected void beforeExpressionPush(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @Nullable TextRange range, - @NotNull DfaMemoryState state) { - - } - - protected void beforeMethodReferenceResultPush(@NotNull DfaValue value, - @NotNull PsiMethodReferenceExpression methodRef, - @NotNull DfaMemoryState state) { - - } - - protected void checkReturnValue(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @NotNull PsiParameterListOwner context, - @NotNull DfaMemoryState state) { - - } - - void pushExpressionResult(@NotNull DfaValue value, - @NotNull ExpressionPushingInstruction instruction, - @NotNull DfaMemoryState state) { - PsiExpression anchor = instruction.getExpression(); - if (anchor != null - && !(instruction instanceof MethodCallInstruction && - (((MethodCallInstruction)instruction).getMethodType() == MethodCallInstruction.MethodType.BOXING || - ((MethodCallInstruction)instruction).getMethodType() == MethodCallInstruction.MethodType.UNBOXING)) - && !(instruction instanceof PushInstruction && ((PushInstruction)instruction).isReferenceWrite())) { - if (anchor instanceof PsiMethodReferenceExpression && !(instruction instanceof PushInstruction)) { - beforeMethodReferenceResultPush(value, (PsiMethodReferenceExpression)anchor, state); - } - else { - callBeforeExpressionPush(value, instruction, state, anchor); - } - } - state.push(value); - } - - private void callBeforeExpressionPush(@NotNull DfaValue value, - @NotNull ExpressionPushingInstruction instruction, - @NotNull DfaMemoryState state, PsiExpression anchor) { - beforeExpressionPush(value, anchor, instruction.getExpressionRange(), state); - PsiElement parent = PsiUtil.skipParenthesizedExprUp(anchor.getParent()); - if (parent instanceof PsiLambdaExpression) { - checkReturnValue(value, Objects.requireNonNull(instruction.getExpression()), (PsiLambdaExpression)parent, state); - } - else if (parent instanceof PsiReturnStatement) { - PsiParameterListOwner context = PsiTreeUtil.getParentOfType(parent, PsiMethod.class, PsiLambdaExpression.class); - if (context != null) { - checkReturnValue(value, Objects.requireNonNull(instruction.getExpression()), context, state); - } - } - else if (parent instanceof PsiConditionalExpression && - !PsiTreeUtil.isAncestor(((PsiConditionalExpression)parent).getCondition(), anchor, false)) { - callBeforeExpressionPush(value, instruction, state, (PsiConditionalExpression)parent); - } - else if (parent instanceof PsiPolyadicExpression) { - PsiPolyadicExpression polyadic = (PsiPolyadicExpression)parent; - if ((polyadic.getOperationTokenType().equals(JavaTokenType.ANDAND) || polyadic.getOperationTokenType().equals(JavaTokenType.OROR)) && - PsiTreeUtil.isAncestor(ArrayUtil.getLastElement(polyadic.getOperands()), anchor, false)) { - callBeforeExpressionPush(value, instruction, state, polyadic); - } - } - } - public DfaInstructionState[] visitAssign(AssignInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { memState.pop(); DfaValue dest = memState.pop(); @@ -147,11 +78,6 @@ public abstract class InstructionVisitor { return nextInstruction(instruction, runner, state); } - public DfaInstructionState[] visitResultOf(ResultOfInstruction instruction, DataFlowRunner runner, DfaMemoryState state) { - pushExpressionResult(state.pop(), instruction, state); - return nextInstruction(instruction, runner, state); - } - protected static DfaInstructionState[] nextInstruction(Instruction instruction, DataFlowRunner runner, DfaMemoryState memState) { return new DfaInstructionState[]{new DfaInstructionState(runner.getInstruction(instruction.getIndex() + 1), memState)}; } @@ -163,7 +89,7 @@ public abstract class InstructionVisitor { public DfaInstructionState[] visitBinop(BinopInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { memState.pop(); memState.pop(); - pushExpressionResult(DfaUnknownValue.getInstance(), instruction, memState); + memState.push(DfaUnknownValue.getInstance()); return nextInstruction(instruction, runner, memState); } @@ -177,6 +103,11 @@ public abstract class InstructionVisitor { return nextInstruction(instruction, runner, state); } + public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + memState.pop(); + return nextInstruction(instruction, runner, memState); + } + public DfaInstructionState[] visitLambdaExpression(LambdaInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { return nextInstruction(instruction, runner, memState); } @@ -252,7 +183,7 @@ public abstract class InstructionVisitor { } memState.pop(); //qualifier - pushExpressionResult(DfaUnknownValue.getInstance(), instruction, memState); + memState.push(DfaUnknownValue.getInstance()); return nextInstruction(instruction, runner, memState); } @@ -264,24 +195,23 @@ public abstract class InstructionVisitor { DfaValue dfaValue = memState.pop(); dfaValue = dfaValue.createNegated(); - pushExpressionResult(dfaValue, instruction, memState); + memState.push(dfaValue); return nextInstruction(instruction, runner, memState); } public DfaInstructionState[] visitPush(PushInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - pushExpressionResult(instruction.getValue(), instruction, memState); + memState.push(instruction.getValue()); return nextInstruction(instruction, runner, memState); } public DfaInstructionState[] visitArrayAccess(ArrayAccessInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { memState.pop(); // index memState.pop(); // array reference - pushExpressionResult(instruction.getValue(), instruction, memState); + memState.push(instruction.getValue()); return nextInstruction(instruction, runner, memState); } public DfaInstructionState[] visitTypeCast(TypeCastInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - pushExpressionResult(memState.pop(), instruction, memState); return nextInstruction(instruction, runner, memState); } 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 9ee7914ff677..72901ec078f9 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,11 +2,13 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; -import com.intellij.psi.*; +import com.intellij.psi.PsiAnnotation; +import com.intellij.psi.PsiExpression; +import com.intellij.psi.PsiMethod; +import com.intellij.psi.PsiMethodCallExpression; 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; @@ -28,18 +30,6 @@ 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) * @@ -50,9 +40,8 @@ public class JavaMethodContractUtil { */ @NotNull public static List getMethodCallContracts(@NotNull final PsiMethod method, - @Nullable PsiCallExpression call) { - List contracts = - HardcodedContracts.getHardcodedContracts(method, ObjectUtils.tryCast(call, PsiMethodCallExpression.class)); + @Nullable PsiMethodCallExpression call) { + List contracts = HardcodedContracts.getHardcodedContracts(method, call); return !contracts.isEmpty() ? contracts : getMethodContracts(method); } @@ -211,7 +200,9 @@ public class JavaMethodContractUtil { @Contract("null -> null") public static PsiExpression findReturnedValue(@Nullable PsiMethodCallExpression call) { if (call == null) return null; - List contracts = getMethodCallContracts(call); + PsiMethod method = call.resolveMethod(); + if (method == null) return null; + List contracts = getMethodCallContracts(method, 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 40e2d26b289f..9093cc51d192 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 @@ -32,10 +32,12 @@ import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.MethodUtils; import com.siyeh.ig.psiutils.TypeUtils; import gnu.trove.THashSet; +import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; +import java.util.stream.Stream; /** * @author peter @@ -101,7 +103,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { checkNotNullable(memState, dfaSource, kind.problem(rValue)); } - pushExpressionResult(dfaDest, instruction, memState); + memState.push(dfaDest); flushArrayOnUnknownAssignment(instruction, runner.getFactory(), dfaDest, memState); return nextInstruction(instruction, runner, memState); @@ -155,6 +157,15 @@ public class StandardInstructionVisitor extends InstructionVisitor { } } + @Override + public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + final DfaValue retValue = memState.pop(); + checkNotNullable(memState, retValue, NullabilityProblemKind.nullableReturn.problem(instruction.getReturn())); + return nextInstruction(instruction, runner, memState); + } + @Override public DfaInstructionState[] visitArrayAccess(ArrayAccessInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { PsiArrayAccessExpression arrayExpression = instruction.getExpression(); @@ -184,7 +195,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (arrayElementValue != DfaUnknownValue.getInstance()) { result = arrayElementValue; } - pushExpressionResult(result, instruction, memState); + memState.push(result); return nextInstruction(instruction, runner, memState); } @@ -223,13 +234,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { if (contracts.isEmpty()) return; PsiType returnType = substitutor.substitute(method.getReturnType()); DfaValue defaultResult = runner.getFactory().createTypeValue(returnType, DfaPsiUtil.getElementNullability(returnType, method)); - Set currentStates = Collections.singleton(new DfaCallState(state.createClosureState(), callArguments)); - for (MethodContract contract : contracts) { - currentStates = addContractResults(contract, currentStates, runner.getFactory(), new HashSet<>(), defaultResult, methodRef); - } - for (DfaCallState currentState: currentStates) { - pushExpressionResult(defaultResult, () -> methodRef, currentState.myMemoryState); - } + Stream returnValues = possibleReturnValues(callArguments, state, contracts, runner.getFactory(), defaultResult); + returnValues.forEach(res -> processMethodReferenceResult(methodRef, contracts, res)); } @NotNull @@ -262,6 +268,24 @@ public class StandardInstructionVisitor extends InstructionVisitor { return new DfaCallArguments(qualifier, arguments, JavaMethodContractUtil.isPure(method)); } + private static Stream possibleReturnValues(DfaCallArguments callArguments, + DfaMemoryState state, + List contracts, + DfaValueFactory factory, DfaValue defaultResult) { + Set currentStates = Collections.singleton(new DfaCallState(state.createClosureState(), callArguments)); + Set finalStates = ContainerUtil.newLinkedHashSet(); + for (MethodContract contract : contracts) { + currentStates = addContractResults(contract, currentStates, factory, finalStates, defaultResult); + } + return StreamEx.of(finalStates).map(DfaMemoryState::peek) + .append(currentStates.isEmpty() ? StreamEx.empty() : StreamEx.of(defaultResult)).distinct(); + } + + protected void processMethodReferenceResult(PsiMethodReferenceExpression methodRef, + List contracts, + DfaValue res) { + } + @Override public DfaInstructionState[] visitTypeCast(TypeCastInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { PsiType type = instruction.getCastTo(); @@ -271,11 +295,9 @@ public class StandardInstructionVisitor extends InstructionVisitor { onInstructionProducesCCE(instruction); } - DfaValue value = memState.pop(); if (type instanceof PsiPrimitiveType) { - value = factory.getBoxedFactory().createUnboxed(value); + memState.push(factory.getBoxedFactory().createUnboxed(memState.pop())); } - pushExpressionResult(value, instruction, memState); return nextInstruction(instruction, runner, memState); } @@ -294,7 +316,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { DfaValue defaultResult = getMethodResultValue(instruction, callArguments.myQualifier, memState, runner.getFactory()); if (callArguments.myArguments != null) { for (MethodContract contract : instruction.getContracts()) { - currentStates = addContractResults(contract, currentStates, runner.getFactory(), finalStates, defaultResult, instruction.getExpression()); + currentStates = addContractResults(contract, currentStates, runner.getFactory(), finalStates, defaultResult); if (currentStates.size() + finalStates.size() > DataFlowRunner.MAX_STATES_PER_BRANCH) { if (LOG.isDebugEnabled()) { LOG.debug("Too complex contract on " + instruction.getContext() + ", skipping contract processing"); @@ -306,17 +328,22 @@ public class StandardInstructionVisitor extends InstructionVisitor { } } for (DfaCallState callState : currentStates) { - pushExpressionResult(defaultResult, instruction, callState.myMemoryState); + callState.myMemoryState.push(defaultResult); finalStates.add(callState.myMemoryState); } } + PsiMethodReferenceExpression methodRef = instruction.getMethodType() == MethodCallInstruction.MethodType.METHOD_REFERENCE_CALL ? + (PsiMethodReferenceExpression)instruction.getContext() : null; DfaInstructionState[] result = new DfaInstructionState[finalStates.size()]; int i = 0; for (DfaMemoryState state : finalStates) { if (instruction.shouldFlushFields()) { state.flushFields(); } + if (methodRef != null) { + processMethodReferenceResult(methodRef, instruction.getContracts(), state.peek()); + } result[i++] = new DfaInstructionState(runner.getInstruction(instruction.getIndex() + 1), state); } return result; @@ -324,18 +351,13 @@ public class StandardInstructionVisitor extends InstructionVisitor { @NotNull private List handleKnownMethods(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - PsiMethod method = instruction.getTargetMethod(); - if (method == null) return Collections.emptyList(); - CustomMethodHandlers.CustomMethodHandler handler = CustomMethodHandlers.find(method); + if (instruction.getTargetMethod() == null) return Collections.emptyList(); + CustomMethodHandlers.CustomMethodHandler handler = CustomMethodHandlers.find(instruction); if (handler == null) return Collections.emptyList(); memState = memState.createCopy(); DfaCallArguments callArguments = popCall(instruction, runner, memState, false); - DfaValue result = callArguments.myArguments == null ? null : handler.getMethodResult(callArguments, memState, runner.getFactory()); - if (result != null) { - pushExpressionResult(result, instruction, memState); - return Collections.singletonList(memState); - } - return Collections.emptyList(); + return callArguments.myArguments == null ? Collections.emptyList() : + handler.handle(callArguments, memState, runner.getFactory()); } @NotNull @@ -425,16 +447,15 @@ public class StandardInstructionVisitor extends InstructionVisitor { return value; } - private Set addContractResults(MethodContract contract, - Set states, - DfaValueFactory factory, - Set finalStates, - DfaValue defaultResult, - PsiExpression expression) { + private static Set addContractResults(MethodContract contract, + Set states, + DfaValueFactory factory, + Set finalStates, + DfaValue defaultResult) { if(contract.isTrivial()) { for (DfaCallState callState : states) { DfaValue result = contract.getReturnValue().getDfaValue(factory, defaultResult, callState); - pushExpressionResult(result, () -> expression, callState.myMemoryState); + callState.myMemoryState.push(result); finalStates.add(callState.myMemoryState); } return Collections.emptySet(); @@ -463,7 +484,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { } if(state != null) { DfaValue result = contract.getReturnValue().getDfaValue(factory, defaultResult, new DfaCallState(state, arguments)); - pushExpressionResult(result, () -> expression, state); + state.push(result); finalStates.add(state); } } @@ -588,12 +609,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { @Override public DfaInstructionState[] visitCheckNotNull(CheckNotNullInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - NullabilityProblemKind.NullabilityProblem problem = instruction.getProblem(); - if (NullabilityProblemKind.nullableReturn.isMyProblem(problem)) { - checkNotNullable(memState, memState.peek(), problem); - } else { - memState.push(dereference(memState, memState.pop(), problem)); - } + DfaValue result = dereference(memState, memState.pop(), instruction.getProblem()); + memState.push(result); return super.visitCheckNotNull(instruction, runner, memState); } @@ -613,7 +630,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { return states; } } - DfaValue result = DfaUnknownValue.getInstance(); + DfaValue result = null; PsiType type = instruction.getResultType(); if (PsiType.INT.equals(type) || PsiType.LONG.equals(type)) { LongRangeSet left = memState.getValueFact(dfaLeft, DfaFactType.RANGE); @@ -625,10 +642,10 @@ public class StandardInstructionVisitor extends InstructionVisitor { } } } - if (result == DfaUnknownValue.getInstance() && JavaTokenType.PLUS == opSign && TypeUtils.isJavaLangString(type)) { + if (result == null && JavaTokenType.PLUS == opSign && TypeUtils.isJavaLangString(type)) { result = runner.getFactory().createTypeValue(type, Nullability.NOT_NULL); } - pushExpressionResult(result, instruction, memState); + memState.push(result == null ? DfaUnknownValue.getInstance() : result); instruction.setTrueReachable(); // Not a branching instruction actually. instruction.setFalseReachable(); @@ -637,12 +654,12 @@ public class StandardInstructionVisitor extends InstructionVisitor { } @Nullable - private DfaInstructionState[] handleRelationBinop(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - DfaValue dfaRight, - DfaValue dfaLeft, - RelationType relationType) { + private static DfaInstructionState[] handleRelationBinop(BinopInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + DfaValue dfaRight, + DfaValue dfaLeft, + RelationType relationType) { DfaValueFactory factory = runner.getFactory(); RelationType[] relations = splitRelation(relationType); @@ -750,11 +767,11 @@ public class StandardInstructionVisitor extends InstructionVisitor { } @Nullable - private DfaInstructionState[] handleConstantComparison(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - DfaValue dfaRight, - DfaValue dfaLeft, RelationType relationType) { + private static DfaInstructionState[] handleConstantComparison(BinopInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + DfaValue dfaRight, + DfaValue dfaLeft, RelationType relationType) { if (dfaLeft instanceof DfaVariableValue && dfaRight instanceof DfaVariableValue) { Number leftValue = getKnownNumberValue(memState, (DfaVariableValue)dfaLeft); Number rightValue = getKnownNumberValue(memState, (DfaVariableValue)dfaRight); @@ -782,7 +799,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { } if (dfaLeft instanceof DfaConstValue && dfaRight instanceof DfaConstValue || - DfaConstValue.isContractFail(dfaLeft) || DfaConstValue.isContractFail(dfaRight)) { + dfaLeft == runner.getFactory().getConstFactory().getContractFail() || + dfaRight == runner.getFactory().getConstFactory().getContractFail()) { boolean negated = (relationType == RelationType.NE) ^ (DfaMemoryStateImpl.isNaN(dfaLeft) || DfaMemoryStateImpl.isNaN(dfaRight)); boolean result = dfaLeft == dfaRight ^ negated; return makeBooleanResultArray(instruction, runner, memState, result); @@ -792,11 +810,11 @@ public class StandardInstructionVisitor extends InstructionVisitor { } @Nullable - private DfaInstructionState[] checkComparingWithConstant(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - DfaVariableValue var, - RelationType opSign, Number comparedWith) { + private static DfaInstructionState[] checkComparingWithConstant(BinopInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + DfaVariableValue var, + RelationType opSign, Number comparedWith) { Number knownValue = getKnownNumberValue(memState, var); if (knownValue != null) { return checkComparisonWithKnownValue(instruction, runner, memState, opSign, knownValue, comparedWith); @@ -810,12 +828,12 @@ public class StandardInstructionVisitor extends InstructionVisitor { return knownConstantValue != null && knownConstantValue.getValue() instanceof Number ? (Number)knownConstantValue.getValue() : null; } - private DfaInstructionState[] checkComparisonWithKnownValue(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - RelationType opSign, - Number leftValue, - Number rightValue) { + private static DfaInstructionState[] checkComparisonWithKnownValue(BinopInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + RelationType opSign, + Number leftValue, + Number rightValue) { int cmp = compare(leftValue, rightValue); Boolean result = null; boolean hasNaN = DfaUtil.isNaN(leftValue) || DfaUtil.isNaN(rightValue); @@ -849,19 +867,12 @@ public class StandardInstructionVisitor extends InstructionVisitor { return Double.compare(a.doubleValue(), b.doubleValue()); } - private DfaInstructionState[] makeBooleanResultArray(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - boolean result) { + private static DfaInstructionState[] makeBooleanResultArray(BinopInstruction instruction, DataFlowRunner runner, DfaMemoryState memState, boolean result) { return new DfaInstructionState[]{makeBooleanResult(instruction, runner, memState, ThreeState.fromBoolean(result))}; } - private DfaInstructionState makeBooleanResult(BinopInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - @NotNull ThreeState result) { - DfaValue value = result == ThreeState.UNSURE ? DfaUnknownValue.getInstance() : runner.getFactory().getBoolean(result.toBoolean()); - pushExpressionResult(value, instruction, memState); + private static DfaInstructionState makeBooleanResult(BinopInstruction instruction, DataFlowRunner runner, DfaMemoryState memState, @NotNull ThreeState result) { + memState.push(result == ThreeState.UNSURE ? DfaUnknownValue.getInstance() : runner.getFactory().getBoolean(result.toBoolean())); if (result != ThreeState.NO) { instruction.setTrueReachable(); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java index 9328cae7a859..2c1b2e355788 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/OptionalChainInliner.java @@ -81,10 +81,11 @@ public class OptionalChainInliner implements CallInliner { .ifNotNull() .swap() // stack: .. optValue, elseValue .end() - .pop() - .resultOf(call); + .pop(); + }) + .register(OPTIONAL_OR_NULL, (builder, call) -> { + // no op! }) - .register(OPTIONAL_OR_NULL, CFGBuilder::resultOf) .register(OPTIONAL_OR_ELSE_GET, (builder, call) -> { PsiExpression fn = call.getArgumentList().getExpressions()[0]; builder @@ -93,8 +94,7 @@ public class OptionalChainInliner implements CallInliner { .ifNull() .pop() .invokeFunction(0, fn) - .end() - .resultOf(call); + .end(); }) .register(OPTIONAL_IF_PRESENT, (builder, call) -> { PsiExpression fn = call.getArgumentList().getExpressions()[0]; @@ -106,8 +106,7 @@ public class OptionalChainInliner implements CallInliner { .elseBranch() .pop() .pushUnknown() - .end() - .resultOf(call); + .end(); }); private static final CallMapper> INTERMEDIATE_MAPPER = @@ -257,23 +256,14 @@ public class OptionalChainInliner implements CallInliner { private static void inlineOf(CFGBuilder builder, PsiType optionalElementType, PsiMethodCallExpression qualifierCall) { PsiExpression argument = qualifierCall.getArgumentList().getExpressions()[0]; - builder - .pushExpression(argument) - .boxUnbox(argument, optionalElementType); + builder.pushExpression(argument) + .boxUnbox(argument, optionalElementType) + .pushUnknown() // ... arg, ? + .splice(2, 1, 0, 1) // ... arg, ?, arg + .invoke(qualifierCall) // ... arg, opt -- keep original call in CFG so some warnings like "ofNullable for null" can work + .pop(); // ... arg if ("of".equals(qualifierCall.getMethodExpression().getReferenceName())) { - builder.checkNotNull(argument, NullabilityProblemKind.passingNullableToNotNullParameter) - .push(builder.getFactory().getFactValue(DfaFactType.OPTIONAL_PRESENCE, true), qualifierCall) - .pop(); - } - else { - builder - .dup() - .ifNull() - .push(builder.getFactory().getFactValue(DfaFactType.OPTIONAL_PRESENCE, false), qualifierCall) - .elseBranch() - .push(builder.getFactory().getFactValue(DfaFactType.OPTIONAL_PRESENCE, true), qualifierCall) - .end() - .pop(); + builder.checkNotNull(argument, NullabilityProblemKind.passingNullableToNotNullParameter); } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/StreamChainInliner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/StreamChainInliner.java index 0a505291c213..b9c1a4e64c59 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/StreamChainInliner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/inliner/StreamChainInliner.java @@ -163,10 +163,8 @@ public class StreamChainInliner implements CallInliner { myNext.pushResult(builder); } else { - DfaValue resultValue = - builder.getFactory().createTypeValue(myCall.getType(), - DfaPsiUtil.getElementNullability(myCall.getType(), myCall.resolveMethod())); - builder.push(resultValue, myCall); + builder.push(builder.getFactory() + .createTypeValue(myCall.getType(), DfaPsiUtil.getElementNullability(myCall.getType(), myCall.resolveMethod()))); } } @@ -216,7 +214,7 @@ public class StreamChainInliner implements CallInliner { @Override void pushResult(CFGBuilder builder) { - builder.push(myResult, myCall); + builder.push(myResult); } } @@ -632,7 +630,7 @@ public class StreamChainInliner implements CallInliner { .ifConditionIs(true) .chain(b -> buildStreamCFG(b, firstStep, originalQualifier)) .end() - .push(builder.getFactory().createTypeValue(call.getType(), Nullability.NOT_NULL), call); + .push(builder.getFactory().createTypeValue(call.getType(), Nullability.NOT_NULL)); return true; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ArrayAccessInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ArrayAccessInstruction.java index b926a805dc91..f6731b89308a 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ArrayAccessInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ArrayAccessInstruction.java @@ -23,7 +23,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.psi.PsiArrayAccessExpression; import org.jetbrains.annotations.NotNull; -public class ArrayAccessInstruction extends Instruction implements ExpressionPushingInstruction { +public class ArrayAccessInstruction extends Instruction { private final @NotNull DfaValue myValue; private final @NotNull PsiArrayAccessExpression myExpression; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/AssignInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/AssignInstruction.java index 8826a33fda71..9066a3a52f23 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/AssignInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/AssignInstruction.java @@ -24,11 +24,10 @@ import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.psi.PsiAssignmentExpression; import com.intellij.psi.PsiExpression; import com.intellij.psi.PsiVariable; -import com.intellij.util.ObjectUtils; import org.jetbrains.annotations.Contract; import org.jetbrains.annotations.Nullable; -public class AssignInstruction extends Instruction implements ExpressionPushingInstruction { +public class AssignInstruction extends Instruction { private final PsiExpression myRExpression; private final PsiExpression myLExpression; @Nullable private final DfaValue myAssignedValue; @@ -71,13 +70,6 @@ public class AssignInstruction extends Instruction implements ExpressionPushingI return "ASSIGN"; } - @Nullable - @Override - public PsiAssignmentExpression getExpression() { - if(myRExpression== null) return null; - return ObjectUtils.tryCast(myRExpression.getParent(), PsiAssignmentExpression.class); - } - @Contract("null -> null") @Nullable private static PsiExpression getLeftHandOfAssignment(PsiExpression rExpression) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java index 6960b25103c7..6741bd4bfdf1 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/BinopInstruction.java @@ -21,6 +21,7 @@ import com.intellij.codeInspection.dataFlow.DfaInstructionState; import com.intellij.codeInspection.dataFlow.DfaMemoryState; import com.intellij.codeInspection.dataFlow.InstructionVisitor; import com.intellij.openapi.util.TextRange; +import com.intellij.psi.PsiElement; import com.intellij.psi.PsiExpression; import com.intellij.psi.PsiPolyadicExpression; import com.intellij.psi.PsiType; @@ -30,18 +31,18 @@ import org.jetbrains.annotations.Nullable; import static com.intellij.psi.JavaTokenType.*; -public class BinopInstruction extends BranchingInstruction implements ExpressionPushingInstruction { +public class BinopInstruction extends BranchingInstruction { private static final TokenSet ourSignificantOperations = TokenSet.create(EQEQ, NE, LT, GT, LE, GE, INSTANCEOF_KEYWORD, PLUS, MINUS, AND, PERC, DIV, GTGT, GTGTGT); private final IElementType myOperationSign; private final @Nullable PsiType myResultType; private final int myLastOperand; - public BinopInstruction(IElementType opSign, @Nullable PsiExpression psiAnchor, @Nullable PsiType resultType) { + public BinopInstruction(IElementType opSign, @Nullable PsiElement psiAnchor, @Nullable PsiType resultType) { this(opSign, psiAnchor, resultType, -1); } - public BinopInstruction(IElementType opSign, @Nullable PsiExpression psiAnchor, @Nullable PsiType resultType, int lastOperand) { + public BinopInstruction(IElementType opSign, @Nullable PsiElement psiAnchor, @Nullable PsiType resultType, int lastOperand) { super(psiAnchor); myResultType = resultType; myOperationSign = ourSignificantOperations.contains(opSign) ? opSign : null; @@ -52,7 +53,7 @@ public class BinopInstruction extends BranchingInstruction implements Expression * @return range inside the anchor which evaluates this instruction, or null if the whole anchor evaluates this instruction */ @Nullable - public TextRange getExpressionRange() { + public TextRange getAnchorRange() { if (myLastOperand != -1 && getPsiAnchor() instanceof PsiPolyadicExpression) { PsiPolyadicExpression anchor = (PsiPolyadicExpression)getPsiAnchor(); PsiExpression[] operands = anchor.getOperands(); @@ -63,12 +64,6 @@ public class BinopInstruction extends BranchingInstruction implements Expression return null; } - @Nullable - @Override - public PsiExpression getExpression() { - return (PsiExpression)getPsiAnchor(); - } - @Override public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { return visitor.visitBinop(this, runner, stateBefore); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java new file mode 100644 index 000000000000..ce406aedf616 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java @@ -0,0 +1,48 @@ +/* + * Copyright 2000-2009 JetBrains s.r.o. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.intellij.codeInspection.dataFlow.instructions; + +import com.intellij.codeInspection.dataFlow.DataFlowRunner; +import com.intellij.codeInspection.dataFlow.DfaInstructionState; +import com.intellij.codeInspection.dataFlow.DfaMemoryState; +import com.intellij.codeInspection.dataFlow.InstructionVisitor; +import com.intellij.psi.PsiExpression; +import org.jetbrains.annotations.NotNull; + +/** + * @author max + */ +public class CheckReturnValueInstruction extends Instruction { + private final @NotNull PsiExpression myReturnValue; + + public CheckReturnValueInstruction(@NotNull PsiExpression returnValue) { + myReturnValue = returnValue; + } + + @Override + public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { + return visitor.visitCheckReturnValue(this, runner, stateBefore); + } + + @NotNull + public PsiExpression getReturn() { + return myReturnValue; + } + + public String toString() { + return "CheckReturnValue"; + } +} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ExpressionPushingInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ExpressionPushingInstruction.java deleted file mode 100644 index b2605afd260b..000000000000 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ExpressionPushingInstruction.java +++ /dev/null @@ -1,25 +0,0 @@ -// Copyright 2000-2018 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.instructions; - -import com.intellij.openapi.util.TextRange; -import com.intellij.psi.PsiExpression; -import org.jetbrains.annotations.Nullable; - -/** - * An instruction which pushes a result of {@link PsiExpression} (or its part) evaluation to the stack - * - */ -public interface ExpressionPushingInstruction { - /** - * @return a PsiExpression which result is pushed to the stack, or null if this instruction is not bound to any particular PsiExpression - */ - @Nullable - PsiExpression getExpression(); - - /** - * @return if non-null, a part of PsiExpression, returned by {@link #getExpression()} which this instruction actually evaluates. - * Usable for polyadic expressions like {@code a == b == c}: here instruction may evaluate only {@code a == b} part. - */ - @Nullable - default TextRange getExpressionRange() {return null;} -} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/InstanceofInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/InstanceofInstruction.java index 01b51c0cce90..2631084367e6 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/InstanceofInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/InstanceofInstruction.java @@ -19,10 +19,7 @@ import com.intellij.codeInspection.dataFlow.DataFlowRunner; import com.intellij.codeInspection.dataFlow.DfaInstructionState; import com.intellij.codeInspection.dataFlow.DfaMemoryState; import com.intellij.codeInspection.dataFlow.InstructionVisitor; -import com.intellij.psi.JavaTokenType; -import com.intellij.psi.PsiExpression; -import com.intellij.psi.PsiMethodCallExpression; -import com.intellij.psi.PsiType; +import com.intellij.psi.*; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -33,7 +30,7 @@ public class InstanceofInstruction extends BinopInstruction { @Nullable private final PsiExpression myLeft; @Nullable private final PsiType myCastType; - public InstanceofInstruction(PsiExpression psiAnchor, @Nullable PsiExpression left, @NotNull PsiType castType) { + public InstanceofInstruction(PsiElement psiAnchor, @Nullable PsiExpression left, @NotNull PsiType castType) { super(JavaTokenType.INSTANCEOF_KEYWORD, psiAnchor, PsiType.BOOLEAN); myLeft = left; myCastType = castType; diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java index 51344892dbd8..3ae7a6bf8116 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/MethodCallInstruction.java @@ -21,13 +21,14 @@ import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.psi.*; import com.intellij.util.ObjectUtils; +import com.siyeh.ig.callMatcher.CallMatcher; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; -public class MethodCallInstruction extends Instruction implements ExpressionPushingInstruction { +public class MethodCallInstruction extends Instruction { private static final Nullability[] EMPTY_NULLABILITY_ARRAY = new Nullability[0]; @Nullable private final PsiType myType; @@ -119,10 +120,15 @@ public class MethodCallInstruction extends Instruction implements ExpressionPush myReturnNullability = call instanceof PsiNewExpression ? Nullability.NOT_NULL : DfaPsiUtil.getElementNullability(myType, myTargetMethod); } - @Nullable - @Override - public PsiExpression getExpression() { - return ObjectUtils.tryCast(myContext, PsiExpression.class); + public boolean matches(CallMatcher matcher) { + switch (myMethodType) { + case REGULAR_METHOD_CALL: + return myContext instanceof PsiMethodCallExpression && matcher.test((PsiMethodCallExpression)myContext); + case METHOD_REFERENCE_CALL: + return matcher.methodReferenceMatches((PsiMethodReferenceExpression)myContext); + default: + return false; + } } /** diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/NotInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/NotInstruction.java index 090112be3243..5cb17b0facee 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/NotInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/NotInstruction.java @@ -16,18 +16,10 @@ package com.intellij.codeInspection.dataFlow.instructions; -import com.intellij.codeInspection.dataFlow.DataFlowRunner; -import com.intellij.codeInspection.dataFlow.DfaInstructionState; -import com.intellij.codeInspection.dataFlow.DfaMemoryState; -import com.intellij.codeInspection.dataFlow.InstructionVisitor; -import com.intellij.psi.PsiPrefixExpression; +import com.intellij.codeInspection.dataFlow.*; +import com.intellij.codeInspection.dataFlow.value.DfaValue; -public class NotInstruction extends Instruction implements ExpressionPushingInstruction { - private final PsiPrefixExpression myAnchor; - - public NotInstruction(PsiPrefixExpression anchor) { - myAnchor = anchor; - } +public class NotInstruction extends Instruction { @Override public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { @@ -37,9 +29,4 @@ public class NotInstruction extends Instruction implements ExpressionPushingInst public String toString() { return "NOT"; } - - @Override - public PsiPrefixExpression getExpression() { - return myAnchor; - } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/PushInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/PushInstruction.java index 2b0a0c559ed6..b282d9cb4c9f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/PushInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/PushInstruction.java @@ -26,7 +26,7 @@ import com.intellij.psi.PsiExpression; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public class PushInstruction extends Instruction implements ExpressionPushingInstruction { +public class PushInstruction extends Instruction { private final DfaValue myValue; private final PsiExpression myPlace; private final boolean myReferenceWrite; @@ -50,8 +50,7 @@ public class PushInstruction extends Instruction implements ExpressionPushingIns return myValue; } - @Override - public PsiExpression getExpression() { + public PsiExpression getPlace() { return myPlace; } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java deleted file mode 100644 index a240dfd7b84e..000000000000 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2000-2018 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.instructions; - -import com.intellij.codeInspection.dataFlow.DataFlowRunner; -import com.intellij.codeInspection.dataFlow.DfaInstructionState; -import com.intellij.codeInspection.dataFlow.DfaMemoryState; -import com.intellij.codeInspection.dataFlow.InstructionVisitor; -import com.intellij.psi.PsiExpression; -import org.jetbrains.annotations.NotNull; - -public class ResultOfInstruction extends Instruction implements ExpressionPushingInstruction { - @NotNull - private final PsiExpression myExpression; - - public ResultOfInstruction(@NotNull PsiExpression expression) { - myExpression = expression; - } - - @Override - public DfaInstructionState[] accept(DataFlowRunner runner, DfaMemoryState stateBefore, InstructionVisitor visitor) { - return visitor.visitResultOf(this, runner, stateBefore); - } - - public String toString() { - return "RESULT_OF "+myExpression.getText(); - } - - @NotNull - @Override - public PsiExpression getExpression() { - return myExpression; - } -} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/TypeCastInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/TypeCastInstruction.java index 480d3a0cb913..efd48577e09d 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/TypeCastInstruction.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/TypeCastInstruction.java @@ -23,9 +23,8 @@ import com.intellij.codeInspection.dataFlow.InstructionVisitor; import com.intellij.psi.PsiExpression; import com.intellij.psi.PsiType; import com.intellij.psi.PsiTypeCastExpression; -import org.jetbrains.annotations.NotNull; -public class TypeCastInstruction extends Instruction implements ExpressionPushingInstruction { +public class TypeCastInstruction extends Instruction { private final PsiTypeCastExpression myCastExpression; private final PsiExpression myCasted; private final PsiType myCastTo; @@ -36,6 +35,10 @@ public class TypeCastInstruction extends Instruction implements ExpressionPushin myCastTo = castTo; } + public PsiTypeCastExpression getCastExpression() { + return myCastExpression; + } + public PsiExpression getCasted() { return myCasted; } @@ -53,10 +56,4 @@ public class TypeCastInstruction extends Instruction implements ExpressionPushin public String toString() { return "CAST_TO "+myCastTo.getCanonicalText(); } - - @NotNull - @Override - public PsiTypeCastExpression getExpression() { - return myCastExpression; - } } 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 193ad45ba12d..f1454dcd55e5 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,7 +24,6 @@ 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; @@ -191,25 +190,4 @@ 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; - } - - /** - * Checks whether given value is a special internal sentinel value returned by {@link Factory#getSentinel()}. - * - * @param value value to check - * @return true if specified value is a sentinel value - */ - public static boolean isSentinel(DfaValue value) { - return value instanceof DfaConstValue && ((DfaConstValue)value).getValue() == SENTINEL; - } } diff --git a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java index bc965da5d756..97146c0d3c59 100644 --- a/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java +++ b/java/java-impl/src/com/intellij/refactoring/extractMethod/ExtractMethodProcessor.java @@ -11,8 +11,8 @@ import com.intellij.codeInsight.intention.AddAnnotationPsiFix; import com.intellij.codeInsight.navigation.NavigationUtil; import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.instructions.BranchingInstruction; +import com.intellij.codeInspection.dataFlow.instructions.CheckReturnValueInstruction; import com.intellij.codeInspection.dataFlow.instructions.Instruction; -import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.ide.DataManager; import com.intellij.ide.util.PropertiesComponent; import com.intellij.ide.util.PsiClassListCellRenderer; @@ -399,8 +399,11 @@ public class ExtractMethodProcessor implements MatchProvider { */ private boolean getReturnsNullability(boolean nullsExpected) { PsiElement body = null; - if (myCodeFragmentMember instanceof PsiParameterListOwner) { - body = ((PsiParameterListOwner)myCodeFragmentMember).getBody(); + if (myCodeFragmentMember instanceof PsiMethod) { + body = ((PsiMethod)myCodeFragmentMember).getBody(); + } + else if (myCodeFragmentMember instanceof PsiLambdaExpression) { + body = ((PsiLambdaExpression)myCodeFragmentMember).getBody(); } if (body == null) return false; @@ -431,23 +434,25 @@ public class ExtractMethodProcessor implements MatchProvider { } if (returnedExpressions.isEmpty()) return true; - final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); - final StandardInstructionVisitor returnChecker = new StandardInstructionVisitor() { + class ReturnChecker extends StandardInstructionVisitor { + boolean myResult = true; + @Override - protected void checkReturnValue(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @NotNull PsiParameterListOwner context, - @NotNull DfaMemoryState state) { - if (context == myCodeFragmentMember && - returnedExpressions.stream().anyMatch(ret -> PsiTreeUtil.isAncestor(ret, expression, false))) { - boolean result = nullsExpected ? state.isNull(value) : state.isNotNull(value); - if (!result) { - dfaRunner.cancel(); - } + public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + if (returnedExpressions.contains(instruction.getReturn())) { + myResult &= nullsExpected ? memState.isNull(memState.peek()) : memState.isNotNull(memState.peek()); } + return super.visitCheckReturnValue(instruction, runner, memState); } - }; - return dfaRunner.analyzeMethod(body, returnChecker) == RunnerResult.OK; + } + final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); + final ReturnChecker returnChecker = new ReturnChecker(); + if (dfaRunner.analyzeMethod(body, returnChecker) == RunnerResult.OK) { + return returnChecker.myResult; + } + return false; } protected boolean insertNotNullCheckIfPossible() { diff --git a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml new file mode 100644 index 000000000000..f0b6066c0c00 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml @@ -0,0 +1,45 @@ + + + + Test.java + 14 + Condition <code>c</code> is always <code>false</code>. + + + + Test.java + 20 + Condition <code>c</code> is always <code>true</code>. + + + + Test.java + 33 + Condition <code>o</code> is always <code>true</code>. + + + + Test.java + 39 + Condition <code>o</code> is always <code>false</code>. + + + + Test.java + 45 + Condition <code>o</code> is always <code>true</code>. + + + + Test.java + 51 + Condition <code>o</code> at the left side of assignment expression is always <code>false</code>. Can be simplified. + + + + Test.java + 62 + Condition <code>o</code> is always <code>true</code> + + + diff --git a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/src/Test.java b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/src/Test.java new file mode 100644 index 000000000000..635c2922e449 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/boxingBoolean/src/Test.java @@ -0,0 +1,65 @@ +public class S { + void f(Boolean override) { + + if (override == null) { + //doSomething(); + } else if (override) { // always false? + //doOverride(); + } + + } + public void te0(boolean b){ + Boolean c = false; + // if (b) c = true; + if (c) { + } + } + public void te1(boolean b){ + Boolean c = true; + // if (b) c = true; + if (c) { + } + } + public void te2(boolean b){ + Boolean c = false; + if (b) c = true; + if (c) { + } + } + + public void te3(boolean b){ + Boolean c = Boolean.FALSE; + boolean o = !c; + if (o) { + } + } + public void te4(boolean b){ + Boolean c = Boolean.FALSE; + boolean o = c; + if (o) { + } + } + public void te5(boolean b){ + Boolean c = Boolean.TRUE; + boolean o = b||c; + if (o) { + } + } + public void te6(boolean b){ + Boolean c = Boolean.TRUE; + boolean o = !c; + o |= c&b; + if (o) { + } + } + + public void flushOriginal(boolean b){ + boolean o; + { + Boolean c = Boolean.FALSE; + o = !c; + } + if (o) { + } + } +} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/AndAndWithOr.java b/java/java-tests/testData/inspection/dataFlow/fixture/AndAndWithOr.java deleted file mode 100644 index 62bc894f9812..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/fixture/AndAndWithOr.java +++ /dev/null @@ -1,9 +0,0 @@ -import java.util.List; - -class Test { - void test(String type) { - if(type != null && (type.equals("foo") | type.equals("bar"))) { - System.out.println("Who knows"); - } - } -} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/AndEquals.java b/java/java-tests/testData/inspection/dataFlow/fixture/AndEquals.java index 88131122fbe1..068c494eab29 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/AndEquals.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/AndEquals.java @@ -3,7 +3,7 @@ import java.util.List; class Some { public static void appendTokenTypes(StringBuilder sb, List tokenTypes) { for (int count = 0, line = 0, size = tokenTypes.size(); count < size; count++) { - boolean newLine = count == 2 || line > 0 && (count - 2) % 6 == 0; + boolean newLine = count == 2 || line > 0 && (count - 2) % 6 == 0; newLine &= (size - count) > 2; } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/BoxingBoolean.java b/java/java-tests/testData/inspection/dataFlow/fixture/BoxingBoolean.java deleted file mode 100644 index 2b02403f0de9..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/fixture/BoxingBoolean.java +++ /dev/null @@ -1,65 +0,0 @@ -class S { - void f(Boolean override) { - - if (override == null) { - //doSomething(); - } else if (override) { // always false? - //doOverride(); - } - - } - public void te0(boolean b){ - Boolean c = false; - // if (b) c = true; - if (c) { - } - } - public void te1(boolean b){ - Boolean c = true; - // if (b) c = true; - if (c) { - } - } - public void te2(boolean b){ - Boolean c = false; - if (b) c = true; - if (c) { - } - } - - public void te3(boolean b){ - Boolean c = Boolean.FALSE; - boolean o = !c; - if (o) { - } - } - public void te4(boolean b){ - Boolean c = Boolean.FALSE; - boolean o = c; - if (o) { - } - } - public void te5(boolean b){ - Boolean c = Boolean.TRUE; - boolean o = b||c; - if (o) { - } - } - public void te6(boolean b){ - Boolean c = Boolean.TRUE; - boolean o = !c; - o |= c&b; - if (o) { - } - } - - public void flushOriginal(boolean b){ - boolean o; - { - Boolean c = Boolean.FALSE; - o = !c; - } - if (o) { - } - } -} diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/BuildRegexpNotComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/BuildRegexpNotComplex.java index bf54081769cd..344a78fe2d18 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/BuildRegexpNotComplex.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/BuildRegexpNotComplex.java @@ -8,7 +8,7 @@ class X { final char c = pattern.charAt(i); if (c == '*') { } else if (c == ' ') { } - else if (c == ':' || prevIsUppercase) { } + else if (c == ':' || prevIsUppercase) { } } System.out.println(forCompletion); System.out.println(exactPrefixLen); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/DoubleNaN.java b/java/java-tests/testData/inspection/dataFlow/fixture/DoubleNaN.java index 1ba7fd6343e8..78738b3d2967 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/DoubleNaN.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/DoubleNaN.java @@ -32,6 +32,6 @@ public class DoubleNaN { void test2() { System.out.println(1.0 == Double.NaN); - System.out.println(!(1.0 < Double.NaN)); + System.out.println(!(1.0 < Double.NaN)); } } \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java deleted file mode 100644 index 6ed439c2022b..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsInLoopNotTooComplex.java +++ /dev/null @@ -1,29 +0,0 @@ -import java.util.List; - -class Test { - enum A {X, Y, Z}; - List list1, list2; - - void test(List tokens) { - String t = "int"; - A s = A.X; - A l = A.X; - for (String token : tokens) { - if ("unsigned".equals(token)) { - s = A.Z; - } - else if ("signed".equals(token)) { - s = A.Y; - } - else if ("short".equals(token)) { - l = A.Y; - } - else if ("long".equals(token)) { - l = A.Z; - } - else if (list1.contains(token) || list2.contains(token)) { - t = token; - } - } - } -} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java deleted file mode 100644 index 025704da1eaf..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java +++ /dev/null @@ -1,18 +0,0 @@ -import java.util.List; - -class Test { - void test(Object x, Object y) { - if(x.equals(x)) { - // do not report here; reported by EqualsWithItselfInspection - System.out.println("always"); - } - if(!x.equals(x)) { - // do not report here; reported by EqualsWithItselfInspection - System.out.println("never"); - } - y = x; - if(x.equals(y)) { - System.out.println("always"); - } - } -} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java index 6107147b267e..522662b6c9a7 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java @@ -29,8 +29,8 @@ class Test { maybe = Optional.empty(); System.out.println(maybe.get()); } - boolean b = ((maybe.isPresent())) && maybe.get() == 1; - boolean c = (!maybe.isPresent()) || maybe.get() == 1; + boolean b = ((maybe.isPresent())) && maybe.get() == 1; + boolean c = (!maybe.isPresent()) || maybe.get() == 1; Integer value = !maybe.isPresent() ? 0 : maybe.get(); } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OrWithAssignment.java b/java/java-tests/testData/inspection/dataFlow/fixture/OrWithAssignment.java deleted file mode 100644 index 3c854fea62da..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OrWithAssignment.java +++ /dev/null @@ -1,10 +0,0 @@ -import java.util.List; - -class Test { - void test(String type) { - boolean uint = false; - if ("int".equals(type) || (uint = "uint".equals(type))) { - System.out.println("possible"); - } - } -} \ No newline at end of file diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/ReturningConstantExpression.java b/java/java-tests/testData/inspection/dataFlow/fixture/ReturningConstantExpression.java index 6780c2ce8c0e..ba3ad71b6a0c 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/ReturningConstantExpression.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/ReturningConstantExpression.java @@ -2,7 +2,7 @@ class BrokenAlignment { boolean smth() { if (2 == 2) { - System.out.println("True"); + return true; } boolean b = 3 == 3; diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/SkipAssertions.java b/java/java-tests/testData/inspection/dataFlow/fixture/SkipAssertions.java index 7ebaf9c94c87..72994b72f10c 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/SkipAssertions.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/SkipAssertions.java @@ -38,7 +38,7 @@ class Test { private static void testOrNotFail(boolean a, boolean b, boolean c) { if(b) { - assert !(a || b || c); + assert !(a || b || c); } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/Xor.java b/java/java-tests/testData/inspection/dataFlow/fixture/Xor.java index a13e5bf8048e..31b8f906a7cd 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/Xor.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/Xor.java @@ -4,7 +4,7 @@ class Some { public static void main(String[] args) { boolean x = true, y = true, z = true, t = true; - boolean r = x ^ y ^ z ^ t; + boolean r = x ^ y ^ z ^ t; System.out.println("r: " + r); } diff --git a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java index 8d9b5d079fd8..0cef6476a847 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java @@ -105,6 +105,7 @@ public class DataFlowInspectionAncientTest extends InspectionTestCase { public void testIDEADEV2605() { doTest15(); } public void testConstantsDifferentTypes() { doTest15(); } public void testBoxingNaN() { doTest15(); } + public void testBoxingBoolean() { doTest15(true); } public void testCheckedExceptionDominance() { doTest15(); } public void testIDEADEV10489() { doTest15(); } public void testPlusOnStrings() { doTest15(); } 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 e8d7c909eb1c..0a9665af2521 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -619,9 +619,4 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testNanComparisonWrong() { doTest(); } public void testConstantMethods() { doTest(); } public void testPolyadicEquality() { doTest(); } - public void testEqualsInLoopNotTooComplex() { doTest(); } - public void testEqualsWithItself() { doTest(); } - public void testBoxingBoolean() { doTest(); } - public void testOrWithAssignment() { doTest(); } - public void testAndAndWithOr() { doTest(); } } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java index 7ea9d1a8a19e..53c00cbcdc98 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/EqualsWithItselfInspection.java @@ -53,31 +53,31 @@ public class EqualsWithItselfInspection extends BaseInspection { @Override public void visitMethodCallExpression(PsiMethodCallExpression expression) { super.visitMethodCallExpression(expression); - if (isEqualsWithItself(expression)) { - registerMethodCallError(expression); + if (!MethodCallUtils.isEqualsCall(expression) && + !MethodCallUtils.isEqualsIgnoreCaseCall(expression) && + !MethodCallUtils.isCompareToCall(expression) && + !MethodCallUtils.isCompareToIgnoreCaseCall(expression)) { + return; } + final PsiReferenceExpression methodExpression = expression.getMethodExpression(); + final PsiExpressionList argumentList = expression.getArgumentList(); + final PsiExpression[] arguments = argumentList.getExpressions(); + if (arguments.length != 1) { + return; + } + final PsiExpression argument = ParenthesesUtils.stripParentheses(arguments[0]); + final PsiExpression qualifier = methodExpression.getQualifierExpression(); + if (qualifier == null) { + if (!(argument instanceof PsiThisExpression)) { + return; + } + } else { + if (!EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(qualifier, argument) || + SideEffectChecker.mayHaveSideEffects(qualifier)) { + return; + } + } + registerMethodCallError(expression); } } - - public static boolean isEqualsWithItself(PsiMethodCallExpression expression) { - if (!MethodCallUtils.isEqualsCall(expression) && - !MethodCallUtils.isEqualsIgnoreCaseCall(expression) && - !MethodCallUtils.isCompareToCall(expression) && - !MethodCallUtils.isCompareToIgnoreCaseCall(expression)) { - return false; - } - final PsiReferenceExpression methodExpression = expression.getMethodExpression(); - final PsiExpressionList argumentList = expression.getArgumentList(); - final PsiExpression[] arguments = argumentList.getExpressions(); - if (arguments.length != 1) { - return false; - } - final PsiExpression argument = ParenthesesUtils.stripParentheses(arguments[0]); - final PsiExpression qualifier = methodExpression.getQualifierExpression(); - if (qualifier != null) { - return EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(qualifier, argument) && - !SideEffectChecker.mayHaveSideEffects(qualifier); - } - return argument instanceof PsiThisExpression; - } } 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 07bd9737da4f..f729249f4d74 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java @@ -16,6 +16,7 @@ package com.siyeh.ig.bugs; import com.intellij.codeInspection.dataFlow.*; +import com.intellij.codeInspection.dataFlow.instructions.CheckReturnValueInstruction; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; import com.intellij.codeInspection.dataFlow.value.DfaRelationValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; @@ -75,7 +76,7 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { if (!MethodUtils.isComparatorCompare(method) || ControlFlowUtils.methodAlwaysThrowsException(method)) { return; } - check(method); + check(method.getParameterList(), method.getBody()); } @Override @@ -86,12 +87,10 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { ControlFlowUtils.lambdaExpressionAlwaysThrowsException(lambda)) { return; } - check(lambda); + check(lambda.getParameterList(), lambda.getBody()); } - private void check(PsiParameterListOwner owner) { - PsiParameterList parameterList = owner.getParameterList(); - PsiElement body = owner.getBody(); + private void check(PsiParameterList parameterList, PsiElement body) { if (body == null || parameterList.getParametersCount() != 2) return; // comparator like "(a, b) -> 0" fulfills the comparator contract, so no need to warn its parameters are not used if (body instanceof PsiExpression && ExpressionUtils.isZero((PsiExpression)body)) return; @@ -101,12 +100,15 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } PsiMethodCallExpression soleCall = ObjectUtils.tryCast(LambdaUtil.extractSingleExpressionFromBody(body), PsiMethodCallExpression.class); if (soleCall != null) { - MethodContract contract = ContainerUtil.getOnlyItem(JavaMethodContractUtil.getMethodCallContracts(soleCall)); - if (contract != null && contract.isTrivial() && contract.getReturnValue().isFail()) return; + PsiMethod method = soleCall.resolveMethod(); + if (method != null) { + MethodContract contract = ContainerUtil.getOnlyItem(JavaMethodContractUtil.getMethodCallContracts(method, soleCall)); + if (contract != null && contract.isTrivial() && contract.getReturnValue().isFail()) return; + } } PsiParameter[] parameters = parameterList.getParameters(); checkParameterList(parameters, body); - checkReflexivity(owner, parameters, body); + checkReflexivity(parameters, body); } private void checkParameterList(PsiParameter[] parameters, PsiElement context) { @@ -118,7 +120,7 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } } - private void checkReflexivity(PsiParameterListOwner owner, PsiParameter[] parameters, PsiElement body) { + private void checkReflexivity(PsiParameter[] parameters, PsiElement body) { StandardDataFlowRunner runner = new StandardDataFlowRunner(false, body) { @NotNull @Override @@ -131,25 +133,20 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { return state; } }; - ComparatorVisitor visitor = new ComparatorVisitor(owner); + ComparatorVisitor visitor = new ComparatorVisitor(); if (runner.analyzeMethod(body, visitor) != RunnerResult.OK) return; - if (visitor.myRange.contains(0) || visitor.myContexts.isEmpty()) return; + if (visitor.myRange.contains(0)) return; PsiElement context = null; if (visitor.myContexts.size() == 1) { context = visitor.myContexts.iterator().next(); } else { - PsiElement commonParent = PsiTreeUtil.findCommonParent(visitor.myContexts.toArray(PsiElement.EMPTY_ARRAY)); - if (commonParent instanceof PsiExpression) { - context = commonParent; - } else { - PsiParameterListOwner parent = PsiTreeUtil.getParentOfType(body, PsiMethod.class, PsiLambdaExpression.class); - if (parent instanceof PsiMethod) { - context = ((PsiMethod)parent).getNameIdentifier(); - } - else if (parent instanceof PsiLambdaExpression) { - context = parent.getParameterList(); - } + PsiElement parent = PsiTreeUtil.getParentOfType(body, PsiMethod.class, PsiLambdaExpression.class); + if (parent instanceof PsiMethod) { + context = ((PsiMethod)parent).getNameIdentifier(); + } + else if (parent instanceof PsiLambdaExpression) { + context = ((PsiLambdaExpression)parent).getParameterList(); } } registerError(context != null ? context : body, @@ -157,23 +154,18 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } private static class ComparatorVisitor extends StandardInstructionVisitor { - private final PsiParameterListOwner myOwner; - private final Set myContexts = new HashSet<>(); LongRangeSet myRange = LongRangeSet.empty(); - - public ComparatorVisitor(PsiParameterListOwner owner) { - myOwner = owner; - } + Set myContexts = new HashSet<>(); @Override - protected void checkReturnValue(@NotNull DfaValue value, - @NotNull PsiExpression expression, - @NotNull PsiParameterListOwner owner, - @NotNull DfaMemoryState state) { - if (owner != myOwner) return; - myContexts.add(expression); - LongRangeSet range = state.getValueFact(value, DfaFactType.RANGE); + public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState) { + myContexts.add(instruction.getReturn()); + DfaValue value = memState.peek(); + LongRangeSet range = memState.getValueFact(value, DfaFactType.RANGE); myRange = range == null ? LongRangeSet.all() : myRange.union(range); + return super.visitCheckReturnValue(instruction, runner, memState); } } diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/suspicious_comparator_compare/ComparatorIsNotReflexive.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/suspicious_comparator_compare/ComparatorIsNotReflexive.java index eb14a1df7ed7..e1bc02b720b9 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/suspicious_comparator_compare/ComparatorIsNotReflexive.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/suspicious_comparator_compare/ComparatorIsNotReflexive.java @@ -21,17 +21,11 @@ class ComparatorIsNotReflexive implements Comparator { return -1; } - Comparator lambda = (a, b) -> a.length() > b.length() ? 1 : -1; - - Comparator lambda2 = (a, b) -> a.length() > b.length() ? 1 : - (a.length() < b.length() ? 0 : -1); - - Comparator lambda3 = (a, b) -> (a.length() > b.length() ? 0 : - Math.random() > 0.5 ? (-1) : (1)); + Comparator lambda = (a, b) -> a.length() > b.length() ? 1 : -1; Comparator arrayComparator = (b1, b2) -> { if(b1.length != b2.length) return 0; // typo: == was intended - return b1.length > b2.length ? 1 : -1; + return b1.length > b2.length ? 1 : -1; }; Comparator cmp = (a,b) -> test();