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 b0398fe710a8..35592734846e 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 @@ -207,6 +207,16 @@ 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/ContractChecker.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ContractChecker.java index 7f244611f770..dfbd6aca3e3b 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,8 +3,7 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.NullableNotNullManager; import com.intellij.codeInspection.dataFlow.StandardMethodContract.ValueConstraint; -import com.intellij.codeInspection.dataFlow.instructions.CheckReturnValueInstruction; -import com.intellij.codeInspection.dataFlow.instructions.Instruction; +import com.intellij.codeInspection.dataFlow.instructions.ControlTransferInstruction; import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; import com.intellij.codeInspection.dataFlow.instructions.ReturnInstruction; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; @@ -26,32 +25,111 @@ import java.util.Set; /** * @author peter */ -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; +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; - private ContractChecker(PsiMethod method, StandardMethodContract contract, boolean ownContract) { - super(false, null); - myMethod = method; - myContract = contract; - myOwnContract = ownContract; + 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) 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); + } } static Map checkContractClause(PsiMethod method, StandardMethodContract contract, boolean ownContract) { - PsiCodeBlock body = method.getBody(); if (body == null) return Collections.emptyMap(); - ContractChecker checker = new ContractChecker(method, contract, ownContract); + DataFlowRunner runner = new StandardDataFlowRunner(false, null); PsiParameter[] parameters = method.getParameterList().getParameters(); - final DfaMemoryState initialState = checker.createMemoryState(); - final DfaValueFactory factory = checker.getFactory(); + final DfaMemoryState initialState = runner.createMemoryState(); + final DfaValueFactory factory = runner.getFactory(); for (int i = 0; i < contract.getParameterCount(); i++) { ValueConstraint constraint = contract.getParameterConstraint(i); DfaConstValue comparisonValue = constraint.getComparisonValue(factory); @@ -62,89 +140,8 @@ class ContractChecker extends DataFlowRunner { } } - 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(); - Instruction instruction = instructionState.getInstruction(); - if (instruction instanceof ReturnInstruction) { - if (((ReturnInstruction)instruction).isViaException()) { - ContainerUtil.addIfNotNull(myFailures, ((ReturnInstruction)instruction).getAnchor()); - } else { - myMayReturnNormally = true; - } - } - - if (memState.isEphemeral()) { - return super.acceptInstruction(visitor, instructionState); - } - 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 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; + ContractCheckerVisitor visitor = new ContractCheckerVisitor(method, contract, ownContract); + runner.analyzeMethod(body, visitor, false, Collections.singletonList(initialState)); + return visitor.getErrors(); } } 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 071e2b95b199..97a9982bc7c4 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,7 +140,8 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { if (parent instanceof PsiLambdaExpression && myCodeFragment instanceof PsiExpression) { generateBoxingUnboxingInstructionFor((PsiExpression)myCodeFragment, LambdaUtil.getFunctionalInterfaceReturnType((PsiLambdaExpression)parent)); - addInstruction(new CheckReturnValueInstruction((PsiExpression)myCodeFragment)); + addInstruction(new CheckNotNullInstruction(NullabilityProblemKind.nullableReturn.problem((PsiExpression)myCodeFragment))); + addInstruction(new PopInstruction()); } addInstruction(new ReturnInstruction(myFactory.controlTransfer(ReturnTransfer.INSTANCE, FList.emptyList()), null)); @@ -831,7 +832,8 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { generateBoxingUnboxingInstructionFor(returnValue, LambdaUtil.getFunctionalInterfaceReturnType(lambdaExpression)); } } - addInstruction(new CheckReturnValueInstruction(returnValue)); + addInstruction(new CheckNotNullInstruction(NullabilityProblemKind.nullableReturn.problem(returnValue))); + addInstruction(new PopInstruction()); } addInstruction(new ReturnInstruction(myFactory.controlTransfer(ReturnTransfer.INSTANCE, myTrapStack), statement)); @@ -1311,19 +1313,19 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } PsiType type = expression.getType(); if (op == JavaTokenType.ANDAND) { - generateAndExpression(operands, type, true); + generateAndOrExpression(expression, operands, type, true, true); } else if (op == JavaTokenType.OROR) { - generateOrExpression(operands, type, true); + generateAndOrExpression(expression, operands, type, false, true); } else if (op == JavaTokenType.XOR && PsiType.BOOLEAN.equals(type)) { generateXorExpression(expression, operands, type, false); } else if (op == JavaTokenType.AND && PsiType.BOOLEAN.equals(type)) { - generateAndExpression(operands, type, false); + generateAndOrExpression(expression, operands, type, true, false); } else if (op == JavaTokenType.OR && PsiType.BOOLEAN.equals(type)) { - generateOrExpression(operands, type, false); + generateAndOrExpression(expression, operands, type, false, false); } else if (isBinaryDivision(op) && operands.length == 2 && type != null && PsiType.LONG.isAssignableFrom(type)) { @@ -1482,27 +1484,6 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { } } - 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]))); - } - } - } - private void generateBooleanAssignmentExpression(boolean and, PsiExpression lExpression, PsiExpression rExpression, PsiType exprType) { lExpression.accept(this); generateBoxingUnboxingInstructionFor(lExpression, exprType); @@ -1531,39 +1512,32 @@ public class ControlFlowAnalyzer extends JavaElementVisitor { overPushSuccess.setOffset(pushSuccess.getIndex() + 1); } - private void generateAndExpression(PsiExpression[] operands, final PsiType exprType, boolean shortCircuit) { - List branchToFail = new ArrayList<>(); + private void generateAndOrExpression(PsiExpression expression, + PsiExpression[] operands, + final PsiType exprType, + boolean and, + 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(true, operand); + combineStackBooleans(and, operand); } continue; } - ConditionalGotoInstruction onFail = new ConditionalGotoInstruction(null, true, operand); - branchToFail.add(onFail); - addInstruction(onFail); + 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]))); + } } - - if (!shortCircuit) { - return; + if (shortCircuit) { + addInstruction(new ResultOfInstruction(expression)); } - - 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) { 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 dd6ee7296257..8d8e72496f4e 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,7 +14,6 @@ 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; @@ -254,7 +253,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool for (Instruction instruction : allProblems) { if (instruction instanceof TypeCastInstruction && - reportedAnchors.add(((TypeCastInstruction)instruction).getCastExpression().getCastType())) { + reportedAnchors.add(((TypeCastInstruction)instruction).getExpression().getCastType())) { reportCastMayFail(holder, (TypeCastInstruction)instruction); } else if (instruction instanceof BranchingInstruction) { @@ -264,8 +263,6 @@ 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) { @@ -275,9 +272,9 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool reportOptionalOfNullableImprovements(holder, reportedAnchors, visitor.getOfNullableCalls()); - visitor.getBooleanCalls().forEach((call, state) -> { - if (state != ThreeState.UNSURE && reportedAnchors.add(call)) { - reportConstantCondition(holder, call, state.toBoolean()); + visitor.getBooleanExpressions().forEach((expression, state) -> { + if (state != ThreeState.UNSURE && reportedAnchors.add(expression)) { + reportConstantBoolean(holder, expression, state.toBoolean()); } }); @@ -476,21 +473,6 @@ 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).getExpression(); - 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) { @@ -616,7 +598,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool } private static void reportCastMayFail(ProblemsHolder holder, TypeCastInstruction instruction) { - PsiTypeCastExpression typeCast = instruction.getCastExpression(); + PsiTypeCastExpression typeCast = instruction.getExpression(); PsiExpression operand = typeCast.getOperand(); PsiTypeElement castType = typeCast.getCastType(); assert castType != null; @@ -889,7 +871,7 @@ public class DataFlowInspectionBase extends AbstractBaseJavaLocalInspectionTool return false; } - private static boolean isFlagCheck(PsiElement element) { + 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 bff41c52aa5a..55a929705418 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 @@ -9,6 +9,7 @@ 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.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.ExpressionUtils; @@ -26,7 +27,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { private final Map, StateInfo> myStateInfos = new LinkedHashMap<>(); private final Set myCCEInstructions = ContainerUtil.newHashSet(); private final Map myFailingCalls = new HashMap<>(); - private final Map myBooleanCalls = new HashMap<>(); + private final Map myBooleanExpressions = new HashMap<>(); private final Map myOfNullableCalls = new HashMap<>(); private final Map> myArrayStoreProblems = new HashMap<>(); private final Map myMethodReferenceResults = new HashMap<>(); @@ -127,8 +128,8 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { return myOfNullableCalls; } - Map getBooleanCalls() { - return myBooleanCalls; + Map getBooleanExpressions() { + return myBooleanExpressions; } Map getMethodReferenceResults() { @@ -166,6 +167,7 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { @Nullable TextRange range, @NotNull DfaMemoryState memState) { expression.accept(new ExpressionVisitor(value, memState)); + handleBooleanResults(value, memState, expression); } @Override @@ -221,13 +223,48 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { contracts.stream().anyMatch(contract -> contract.getReturnValue().isFail() && !contract.isTrivial()); } - private static boolean shouldCollectBooleanCallResult(PsiMethodCallExpression call) { - if (ExpressionUtils.isVoidContext(call)) return false; - PsiMethod method = call.resolveMethod(); - if (method == null || !PsiType.BOOLEAN.equals(method.getReturnType()) || !JavaMethodContractUtil.isPure(method)) return false; - List contracts = JavaMethodContractUtil.getMethodCallContracts(method, call); - return CustomMethodHandlers.find(method) != null || - !contracts.isEmpty() && contracts.stream().anyMatch(contract -> contract.getReturnValue().isBoolean() && !contract.isTrivial()); + 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; + } + } + } + if (curState != null || shouldCollectBooleanResult(expression)) { + myBooleanExpressions.put(expression, nextState); + } + } + + 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; } @Override @@ -290,7 +327,6 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { if (DfaOptionalSupport.OPTIONAL_OF_NULLABLE.test(call)) { processOfNullableResult(myValue, myMemState, call.getArgumentList().getExpressions()[0]); } - handleBooleanCalls(call); } @Override @@ -319,23 +355,5 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor { myValues.put(expression, newValue); } } - - private void handleBooleanCalls(PsiMethodCallExpression call) { - ThreeState curState = myBooleanCalls.get(call); - if (curState == ThreeState.UNSURE) return; - ThreeState nextState = ThreeState.UNSURE; - if (myValue instanceof DfaConstValue) { - Object val = ((DfaConstValue)myValue).getValue(); - if (val instanceof Boolean) { - nextState = ThreeState.fromBoolean((Boolean)val); - if (curState != null && curState != nextState) { - nextState = ThreeState.UNSURE; - } - } - } - if (curState != null || shouldCollectBooleanCallResult(call)) { - myBooleanCalls.put(call, nextState); - } - } } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaUtil.java index cdeef9777f23..f057bf796824 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,10 +14,7 @@ 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.CachedValueProvider; -import com.intellij.psi.util.CachedValuesManager; -import com.intellij.psi.util.PsiTreeUtil; -import com.intellij.psi.util.PsiUtil; +import com.intellij.psi.util.*; import com.intellij.util.IncorrectOperationException; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.FList; @@ -27,7 +24,6 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.*; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Predicate; /** @@ -146,57 +142,59 @@ public class DfaUtil { @NotNull public static Nullability inferMethodNullability(PsiMethod method) { - final PsiCodeBlock body = method.getBody(); - if (body == null || PsiUtil.resolveClassInType(method.getReturnType()) == null) { + if (PsiUtil.resolveClassInType(method.getReturnType()) == null) { return Nullability.UNKNOWN; } - return inferBlockNullability(body, InferenceFromSourceUtil.suppressNullable(method)); + return inferBlockNullability(method, InferenceFromSourceUtil.suppressNullable(method)); } @NotNull public static Nullability inferLambdaNullability(PsiLambdaExpression lambda) { - final PsiElement body = lambda.getBody(); - if (body == null || LambdaUtil.getFunctionalInterfaceReturnType(lambda) == null) { + if (LambdaUtil.getFunctionalInterfaceReturnType(lambda) == null) { return Nullability.UNKNOWN; } - return inferBlockNullability(body, false); + return inferBlockNullability(lambda, false); } @NotNull - private static Nullability inferBlockNullability(PsiElement body, boolean suppressNullable) { - final AtomicBoolean hasNulls = new AtomicBoolean(); - final AtomicBoolean hasNotNulls = new AtomicBoolean(); - final AtomicBoolean hasUnknowns = new AtomicBoolean(); + private static Nullability inferBlockNullability(PsiParameterListOwner owner, boolean suppressNullable) { + PsiElement body = owner.getBody(); + if (body == null) return Nullability.UNKNOWN; final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); - final RunnerResult rc = dfaRunner.analyzeMethod(body, new StandardInstructionVisitor() { + class BlockNullabilityVisitor extends StandardInstructionVisitor { + boolean hasNulls = false; + boolean hasNotNulls = false; + boolean hasUnknowns = false; + @Override - 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); + 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; } - else if (memState.isNotNull(returned)) { - hasNotNulls.set(true); + else if (state.isNull(value)) { + hasNulls = true; } else { - hasUnknowns.set(true); + hasUnknowns = true; } } - return super.visitCheckReturnValue(instruction, runner, memState); } - }); + } + BlockNullabilityVisitor visitor = new BlockNullabilityVisitor(); + final RunnerResult rc = dfaRunner.analyzeMethod(body, visitor); if (rc == RunnerResult.OK) { - if (hasNulls.get()) { + if (visitor.hasNulls) { return suppressNullable ? Nullability.UNKNOWN : Nullability.NULLABLE; } - if (hasNotNulls.get() && !hasUnknowns.get()) { + if (visitor.hasNotNulls && !visitor.hasUnknowns) { return Nullability.NOT_NULL; } } 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 7b475f99a2c3..2ea578cb5e4c 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 @@ -19,16 +19,16 @@ 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.PsiArrayAccessExpression; -import com.intellij.psi.PsiExpression; -import com.intellij.psi.PsiMethodReferenceExpression; -import com.intellij.psi.PsiType; +import com.intellij.psi.*; +import com.intellij.psi.util.PsiTreeUtil; 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 @@ -48,6 +48,13 @@ public abstract class InstructionVisitor { } + 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) { @@ -61,12 +68,39 @@ public abstract class InstructionVisitor { beforeMethodReferenceResultPush(value, (PsiMethodReferenceExpression)anchor, state); } else { - beforeExpressionPush(value, anchor, instruction.getExpressionRange(), state); + 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(); @@ -113,6 +147,11 @@ 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)}; } @@ -138,11 +177,6 @@ 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); } @@ -247,6 +281,7 @@ public abstract class InstructionVisitor { } 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/StandardInstructionVisitor.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java index d273a0928f60..f28eb5b764cd 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 @@ -101,7 +101,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { checkNotNullable(memState, dfaSource, kind.problem(rValue)); } - memState.push(dfaDest); + pushExpressionResult(dfaDest, instruction, memState); flushArrayOnUnknownAssignment(instruction, runner.getFactory(), dfaDest, memState); return nextInstruction(instruction, runner, memState); @@ -155,15 +155,6 @@ 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(); @@ -280,9 +271,11 @@ public class StandardInstructionVisitor extends InstructionVisitor { onInstructionProducesCCE(instruction); } + DfaValue value = memState.pop(); if (type instanceof PsiPrimitiveType) { - memState.push(factory.getBoxedFactory().createUnboxed(memState.pop())); + value = factory.getBoxedFactory().createUnboxed(value); } + pushExpressionResult(value, instruction, memState); return nextInstruction(instruction, runner, memState); } @@ -597,8 +590,12 @@ public class StandardInstructionVisitor extends InstructionVisitor { @Override public DfaInstructionState[] visitCheckNotNull(CheckNotNullInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { - DfaValue result = dereference(memState, memState.pop(), instruction.getProblem()); - memState.push(result); + NullabilityProblemKind.NullabilityProblem problem = instruction.getProblem(); + if (NullabilityProblemKind.nullableReturn.isMyProblem(problem)) { + checkNotNullable(memState, memState.peek(), problem); + } else { + memState.push(dereference(memState, memState.pop(), problem)); + } return super.visitCheckNotNull(instruction, runner, memState); } 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 1c0e3e06c80f..9328cae7a859 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,11 +81,10 @@ public class OptionalChainInliner implements CallInliner { .ifNotNull() .swap() // stack: .. optValue, elseValue .end() - .pop(); - }) - .register(OPTIONAL_OR_NULL, (builder, call) -> { - // no op! + .pop() + .resultOf(call); }) + .register(OPTIONAL_OR_NULL, CFGBuilder::resultOf) .register(OPTIONAL_OR_ELSE_GET, (builder, call) -> { PsiExpression fn = call.getArgumentList().getExpressions()[0]; builder @@ -94,7 +93,8 @@ public class OptionalChainInliner implements CallInliner { .ifNull() .pop() .invokeFunction(0, fn) - .end(); + .end() + .resultOf(call); }) .register(OPTIONAL_IF_PRESENT, (builder, call) -> { PsiExpression fn = call.getArgumentList().getExpressions()[0]; @@ -106,7 +106,8 @@ public class OptionalChainInliner implements CallInliner { .elseBranch() .pop() .pushUnknown() - .end(); + .end() + .resultOf(call); }); private static final CallMapper> INTERMEDIATE_MAPPER = 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 9066a3a52f23..8826a33fda71 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,10 +24,11 @@ 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 { +public class AssignInstruction extends Instruction implements ExpressionPushingInstruction { private final PsiExpression myRExpression; private final PsiExpression myLExpression; @Nullable private final DfaValue myAssignedValue; @@ -70,6 +71,13 @@ public class AssignInstruction extends Instruction { 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/CheckReturnValueInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java deleted file mode 100644 index ce406aedf616..000000000000 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/CheckReturnValueInstruction.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * 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/ResultOfInstruction.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java new file mode 100644 index 000000000000..a240dfd7b84e --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/instructions/ResultOfInstruction.java @@ -0,0 +1,34 @@ +// 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 efd48577e09d..480d3a0cb913 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,8 +23,9 @@ 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 { +public class TypeCastInstruction extends Instruction implements ExpressionPushingInstruction { private final PsiTypeCastExpression myCastExpression; private final PsiExpression myCasted; private final PsiType myCastTo; @@ -35,10 +36,6 @@ public class TypeCastInstruction extends Instruction { myCastTo = castTo; } - public PsiTypeCastExpression getCastExpression() { - return myCastExpression; - } - public PsiExpression getCasted() { return myCasted; } @@ -56,4 +53,10 @@ public class TypeCastInstruction extends Instruction { public String toString() { return "CAST_TO "+myCastTo.getCanonicalText(); } + + @NotNull + @Override + public PsiTypeCastExpression getExpression() { + return myCastExpression; + } } 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 0feddc7d8081..496ef6e553fa 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; @@ -403,11 +403,8 @@ public class ExtractMethodProcessor implements MatchProvider { */ private boolean getReturnsNullability(boolean nullsExpected) { PsiElement body = null; - if (myCodeFragmentMember instanceof PsiMethod) { - body = ((PsiMethod)myCodeFragmentMember).getBody(); - } - else if (myCodeFragmentMember instanceof PsiLambdaExpression) { - body = ((PsiLambdaExpression)myCodeFragmentMember).getBody(); + if (myCodeFragmentMember instanceof PsiParameterListOwner) { + body = ((PsiParameterListOwner)myCodeFragmentMember).getBody(); } if (body == null) return false; @@ -438,25 +435,23 @@ public class ExtractMethodProcessor implements MatchProvider { } if (returnedExpressions.isEmpty()) return true; - class ReturnChecker extends StandardInstructionVisitor { - boolean myResult = true; - - @Override - 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); - } - } final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(); - final ReturnChecker returnChecker = new ReturnChecker(); - if (dfaRunner.analyzeMethod(body, returnChecker) == RunnerResult.OK) { - return returnChecker.myResult; - } - return false; + final StandardInstructionVisitor returnChecker = new StandardInstructionVisitor() { + @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(); + } + } + } + }; + return dfaRunner.analyzeMethod(body, returnChecker) == RunnerResult.OK; } 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 deleted file mode 100644 index f0b6066c0c00..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/expected.xml +++ /dev/null @@ -1,45 +0,0 @@ - - - - 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 deleted file mode 100644 index 635c2922e449..000000000000 --- a/java/java-tests/testData/inspection/dataFlow/boxingBoolean/src/Test.java +++ /dev/null @@ -1,65 +0,0 @@ -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 new file mode 100644 index 000000000000..62bc894f9812 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/AndAndWithOr.java @@ -0,0 +1,9 @@ +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 068c494eab29..88131122fbe1 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 new file mode 100644 index 000000000000..2b02403f0de9 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/BoxingBoolean.java @@ -0,0 +1,65 @@ +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 344a78fe2d18..bf54081769cd 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 78738b3d2967..1ba7fd6343e8 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/EqualsWithItself.java b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java index ff72840bb442..025704da1eaf 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/EqualsWithItself.java @@ -11,7 +11,7 @@ class Test { System.out.println("never"); } y = x; - if(x.equals(y)) { + if(x.equals(y)) { System.out.println("always"); } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java index 522662b6c9a7..6107147b267e 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 new file mode 100644 index 000000000000..3c854fea62da --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OrWithAssignment.java @@ -0,0 +1,10 @@ +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 ba3ad71b6a0c..6780c2ce8c0e 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) { - return true; + System.out.println("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 72994b72f10c..7ebaf9c94c87 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/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java index 0cef6476a847..8d9b5d079fd8 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionAncientTest.java @@ -105,7 +105,6 @@ 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 521475180221..d1025e01f95b 100644 --- a/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java +++ b/java/java-tests/testSrc/com/intellij/java/codeInspection/DataFlowInspectionTest.java @@ -622,6 +622,9 @@ public class DataFlowInspectionTest extends DataFlowInspectionTestCase { public void testPolyadicEquality() { doTest(); } public void testEqualsInLoopNotTooComplex() { doTest(); } public void testEqualsWithItself() { doTest(); } + public void testBoxingBoolean() { doTest(); } + public void testOrWithAssignment() { doTest(); } + public void testAndAndWithOr() { doTest(); } public void testBoxUnboxArrayElement() { doTest(); } public void testExactInstanceOf() { doTest(); } public void testNullFlushed() { doTest(); } 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 26bdfd8fa0e7..07bd9737da4f 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/SuspiciousComparatorCompareInspection.java @@ -16,7 +16,6 @@ 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; @@ -76,7 +75,7 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { if (!MethodUtils.isComparatorCompare(method) || ControlFlowUtils.methodAlwaysThrowsException(method)) { return; } - check(method.getParameterList(), method.getBody()); + check(method); } @Override @@ -87,10 +86,12 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { ControlFlowUtils.lambdaExpressionAlwaysThrowsException(lambda)) { return; } - check(lambda.getParameterList(), lambda.getBody()); + check(lambda); } - private void check(PsiParameterList parameterList, PsiElement body) { + private void check(PsiParameterListOwner owner) { + PsiParameterList parameterList = owner.getParameterList(); + PsiElement body = owner.getBody(); 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; @@ -105,7 +106,7 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } PsiParameter[] parameters = parameterList.getParameters(); checkParameterList(parameters, body); - checkReflexivity(parameters, body); + checkReflexivity(owner, parameters, body); } private void checkParameterList(PsiParameter[] parameters, PsiElement context) { @@ -117,7 +118,7 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { } } - private void checkReflexivity(PsiParameter[] parameters, PsiElement body) { + private void checkReflexivity(PsiParameterListOwner owner, PsiParameter[] parameters, PsiElement body) { StandardDataFlowRunner runner = new StandardDataFlowRunner(false, body) { @NotNull @Override @@ -130,20 +131,25 @@ public class SuspiciousComparatorCompareInspection extends BaseInspection { return state; } }; - ComparatorVisitor visitor = new ComparatorVisitor(); + ComparatorVisitor visitor = new ComparatorVisitor(owner); if (runner.analyzeMethod(body, visitor) != RunnerResult.OK) return; - if (visitor.myRange.contains(0)) return; + if (visitor.myRange.contains(0) || visitor.myContexts.isEmpty()) return; PsiElement context = null; if (visitor.myContexts.size() == 1) { context = visitor.myContexts.iterator().next(); } else { - 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(); + 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(); + } } } registerError(context != null ? context : body, @@ -151,18 +157,23 @@ 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(); - Set myContexts = new HashSet<>(); + + public ComparatorVisitor(PsiParameterListOwner owner) { + myOwner = owner; + } @Override - public DfaInstructionState[] visitCheckReturnValue(CheckReturnValueInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState) { - myContexts.add(instruction.getReturn()); - DfaValue value = memState.peek(); - LongRangeSet range = memState.getValueFact(value, DfaFactType.RANGE); + 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); 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 e1bc02b720b9..eb14a1df7ed7 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,11 +21,17 @@ class ComparatorIsNotReflexive implements Comparator { return -1; } - Comparator lambda = (a, b) -> a.length() > b.length() ? 1 : -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 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();