From b0f41a8d624763cc6443557bed3abda7eaf8e0b4 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Tue, 25 Apr 2017 11:23:15 +0700 Subject: [PATCH] Report always true/always false condition on boolean returning methods with contracts --- .../dataFlow/DataFlowInspectionBase.java | 62 ++++++++++++++++--- .../fixture/OptionalGetWithoutIsPresent.java | 16 ++--- .../dataFlow/fixture/OptionalIsPresent.java | 2 +- .../src/com/intellij/util/ThreeState.java | 4 ++ 4 files changed, 66 insertions(+), 18 deletions(-) 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 112d5f47a3c3..6f63f00340d2 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 @@ -346,6 +346,13 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { reportUncheckedOptionalGet(holder, visitor.getOptionalCalls(), visitor.getOptionalQualifiers()); + Map calls = visitor.getBooleanCalls(); + calls.forEach((call, state) -> { + if (state != ThreeState.UNSURE && reportedAnchors.add(call)) { + reportConstantCondition(holder, visitor, call, state.toBoolean()); + } + }); + if (REPORT_CONSTANT_REFERENCE_VALUES) { reportConstantReferenceValues(holder, visitor, reportedAnchors); } @@ -462,7 +469,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { holder.registerProblem(expr, "Passing a non-null argument to Optional", DfaOptionalSupport.createReplaceOptionalOfNullableWithOfFix()); } - + } } } @@ -700,12 +707,12 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { PsiType returnType = method.getReturnType(); // no warnings in void lambdas, where the expression is not returned anyway if (block instanceof PsiExpression && block.getParent() instanceof PsiLambdaExpression && PsiType.VOID.equals(returnType)) return; - + // no warnings for Void methods, where only null can be possibly returned if (returnType == null || returnType.equalsToText(CommonClassNames.JAVA_LANG_VOID)) return; for (PsiElement statement : visitor.getProblems(NullabilityProblem.nullableReturn)) { - assert statement instanceof PsiExpression; + assert statement instanceof PsiExpression; final PsiExpression expr = (PsiExpression)statement; if (!reportedAnchors.add(expr)) continue; @@ -962,6 +969,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { private final Set myCCEInstructions = ContainerUtil.newHashSet(); private final Map myFailingCalls = new HashMap<>(); private final Map myOptionalCalls = new HashMap<>(); + private final Map myBooleanCalls = new HashMap<>(); private final List myOptionalQualifiers = new ArrayList<>(); private boolean myAlwaysReturnsNotNull = true; @@ -969,7 +977,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { protected void onInstructionProducesCCE(TypeCastInstruction instruction) { myCCEInstructions.add(instruction); } - + Collection getProblems(final NullabilityProblem kind) { return ContainerUtil.filter(myProblems.get(kind), psiElement -> { StateInfo info = myStateInfos.get(Pair.create(kind, psiElement)); @@ -984,6 +992,10 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { return myOptionalCalls; } + Map getBooleanCalls() { + return myBooleanCalls; + } + List getOptionalQualifiers() { return myOptionalQualifiers; } @@ -1010,25 +1022,57 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { } else if (DfaOptionalSupport.isOptionalGetMethodName(methodName)) { ThreeState state = memState.checkOptional(memState.peek()); - myOptionalCalls.merge(call, state, (s1, s2) -> s1 == s2 ? s1 : ThreeState.UNSURE); + myOptionalCalls.merge(call, state, ThreeState::merge); } } } DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); if (hasNonTrivialFailingContracts(instruction)) { - boolean allFail = Arrays.stream(states).allMatch(s -> s.getMemoryState().peek() == runner.getFactory().getConstFactory().getContractFail()); + DfaConstValue fail = runner.getFactory().getConstFactory().getContractFail(); + boolean allFail = Arrays.stream(states).allMatch(s -> s.getMemoryState().peek() == fail); myFailingCalls.merge(instruction, allFail, Boolean::logicalAnd); } + handleBooleanCalls(instruction, states); return states; } + void handleBooleanCalls(MethodCallInstruction instruction, DfaInstructionState[] states) { + if (!hasNonTrivialBooleanContracts(instruction)) return; + PsiMethod method = instruction.getTargetMethod(); + if (method == null || !ControlFlowAnalyzer.isPure(method)) return; + PsiMethodCallExpression call = ObjectUtils.tryCast(instruction.getCallExpression(), PsiMethodCallExpression.class); + if (call == null || myBooleanCalls.get(call) == ThreeState.UNSURE) return; + PsiElement parent = call.getParent(); + if (parent instanceof PsiExpressionStatement) return; + if (parent instanceof PsiLambdaExpression && + PsiType.VOID.equals(LambdaUtil.getFunctionalInterfaceReturnType((PsiLambdaExpression)parent))) { + 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 static boolean hasNonTrivialFailingContracts(MethodCallInstruction instruction) { List contracts = instruction.getContracts(); - return !contracts.isEmpty() && contracts.stream().anyMatch(DataFlowInstructionVisitor::isNonTrivialFailingContract); + return !contracts.isEmpty() && contracts.stream().anyMatch( + contract -> contract.getReturnValue() == MethodContract.ValueConstraint.THROW_EXCEPTION && !contract.isTrivial()); } - private static boolean isNonTrivialFailingContract(MethodContract contract) { - return contract.getReturnValue() == MethodContract.ValueConstraint.THROW_EXCEPTION && !contract.isTrivial(); + private static boolean hasNonTrivialBooleanContracts(MethodCallInstruction instruction) { + List contracts = instruction.getContracts(); + return !contracts.isEmpty() && contracts.stream().anyMatch( + contract -> (contract.getReturnValue() == MethodContract.ValueConstraint.FALSE_VALUE || + contract.getReturnValue() == MethodContract.ValueConstraint.TRUE_VALUE) + && !contract.isTrivial()); } @Override diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java index 7de03e46e8e2..7b5584c51066 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java @@ -84,9 +84,9 @@ class OptionalWithoutIsPresent { maybe = Optional.empty(); System.out.println(maybe.get()); } - boolean b = ((maybe.isPresent())) && maybe.get() == 1; - boolean c = (!maybe.isPresent()) || maybe.get() == 1; - Integer value = !maybe.isPresent() ? 0 : maybe.get(); + boolean b = ((maybe.isPresent())) && maybe.get() == 1; + boolean c = (!maybe.isPresent()) || maybe.get() == 1; + Integer value = !maybe.isPresent() ? 0 : maybe.get(); } Optional getIntegerOptional() { @@ -95,7 +95,7 @@ class OptionalWithoutIsPresent { private static void a() { Optional optional = Optional.empty(); - final boolean present = optional.isPresent(); + final boolean present = optional.isPresent(); // optional = Optional.empty(); if (present) { final String string = optional.get(); @@ -105,7 +105,7 @@ class OptionalWithoutIsPresent { private static void b() { Optional optional = Optional.empty(); - final boolean present = optional.isPresent(); + final boolean present = optional.isPresent(); optional = Optional.empty(); if (present) { final String string = optional.get(); @@ -156,7 +156,7 @@ class OptionalWithoutIsPresent { private void checkAsserts2() { Optional o3 = Optional.empty(); - org.testng.Assert.assertTrue(o3.isPresent()); + org.testng.Assert.assertTrue(o3.isPresent()); System.out.println(o3.get()); } @@ -193,9 +193,9 @@ class OptionalWithoutIsPresent { public static String demo() { Optional holder = Optional.empty(); - if (! holder.isPresent()) { + if (! holder.isPresent()) { holder = Optional.of("hello world"); - if (!holder.isPresent()) { + if (!holder.isPresent()) { return null; } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java index 2e38bcb7bc74..05a70685824a 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java @@ -7,7 +7,7 @@ class Test { test = Optional.of("x"); } else { test = Optional.empty(); - if(!test.isPresent()) { + if(!test.isPresent()) { System.out.println("Always"); } } diff --git a/platform/util/src/com/intellij/util/ThreeState.java b/platform/util/src/com/intellij/util/ThreeState.java index d87372a57027..46764d003378 100644 --- a/platform/util/src/com/intellij/util/ThreeState.java +++ b/platform/util/src/com/intellij/util/ThreeState.java @@ -26,6 +26,10 @@ public enum ThreeState { return value ? YES : NO; } + public ThreeState merge(ThreeState other) { + return this == other ? this : UNSURE; + } + public boolean toBoolean() { if (this == UNSURE) { throw new IllegalStateException("Must be or YES, or NO");