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 8761d3c03acd..6d3e42b93c6f 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 @@ -355,30 +355,24 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { private void reportUncheckedOptionalGet(ProblemsHolder holder, Map calls, List qualifiers) { + if (!REPORT_UNCHECKED_OPTIONALS) return; for (Map.Entry entry : calls.entrySet()) { ThreeState state = entry.getValue(); - if (state == ThreeState.YES || state == ThreeState.UNSURE && !REPORT_UNCHECKED_OPTIONALS) { - continue; - } + if (state != ThreeState.UNSURE) continue; PsiMethodCallExpression call = entry.getKey(); PsiMethod method = call.resolveMethod(); if (method == null) continue; PsiClass optionalClass = method.getContainingClass(); if (optionalClass == null) continue; - if (state == ThreeState.NO) { - holder.registerProblem(getElementToHighlight(call), - InspectionsBundle.message("dataflow.message.optional.get.definitely.absent", optionalClass.getName())); - } else if (state == ThreeState.UNSURE) { - PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()); - if (qualifier instanceof PsiMethodCallExpression && - qualifiers.stream().anyMatch(q -> PsiEquivalenceUtil.areElementsEquivalent(q, qualifier))) { - // Conservatively do not report methodCall().get() cases if methodCall().isPresent() was found in the same method - // without deep correspondence analysis - continue; - } - holder.registerProblem(getElementToHighlight(call), - InspectionsBundle.message("dataflow.message.optional.get.without.is.present", optionalClass.getName())); + PsiExpression qualifier = PsiUtil.skipParenthesizedExprDown(call.getMethodExpression().getQualifierExpression()); + if (qualifier instanceof PsiMethodCallExpression && + qualifiers.stream().anyMatch(q -> PsiEquivalenceUtil.areElementsEquivalent(q, qualifier))) { + // Conservatively do not report methodCall().get() cases if methodCall().isPresent() was found in the same method + // without deep correspondence analysis + continue; } + holder.registerProblem(getElementToHighlight(call), + InspectionsBundle.message("dataflow.message.optional.get.without.is.present", optionalClass.getName())); } } @@ -980,7 +974,7 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { if ("isPresent".equals(methodName) && qualifier instanceof PsiMethodCallExpression) { myOptionalQualifiers.add(qualifier); } - else if (isOptionalGetMethodName(methodName)) { + else if (DfaOptionalSupport.isOptionalGetMethodName(methodName)) { ThreeState state = memState.checkOptional(memState.peek()); myOptionalCalls.merge(call, state, (s1, s2) -> s1 == s2 ? s1 : ThreeState.UNSURE); } @@ -994,18 +988,13 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool { return states; } - private static boolean isOptionalGetMethodName(String name) { - return "get".equals(name) || "getAsDouble".equals(name) || "getAsInt".equals(name) || "getAsLong".equals(name); - } - private static boolean hasNonTrivialFailingContracts(MethodCallInstruction instruction) { List contracts = instruction.getContracts(); - return !contracts.isEmpty() && contracts.stream().allMatch(DataFlowInstructionVisitor::isNonTrivialFailingContract); + return !contracts.isEmpty() && contracts.stream().anyMatch(DataFlowInstructionVisitor::isNonTrivialFailingContract); } private static boolean isNonTrivialFailingContract(MethodContract contract) { - return contract.returnValue == MethodContract.ValueConstraint.THROW_EXCEPTION && - Arrays.stream(contract.arguments).anyMatch(v -> v != MethodContract.ValueConstraint.ANY_VALUE); + return contract.returnValue == MethodContract.ValueConstraint.THROW_EXCEPTION && !contract.isTrivial(); } @Override diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java index dafc9fa0cfeb..380673d953f0 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2016 JetBrains s.r.o. + * Copyright 2000-2017 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. @@ -89,6 +89,10 @@ public class DfaOptionalSupport { return null; } + static boolean isOptionalGetMethodName(String name) { + return "get".equals(name) || "getAsDouble".equals(name) || "getAsInt".equals(name) || "getAsLong".equals(name); + } + private static class ReplaceOptionalCallFix implements LocalQuickFix { private final String myTargetMethodName; private final boolean myClearArguments; 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 5092bebd2dc2..4d53a925a89d 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 @@ -1,5 +1,5 @@ /* - * Copyright 2000-2014 JetBrains s.r.o. + * Copyright 2000-2017 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. @@ -15,14 +15,18 @@ */ package com.intellij.codeInspection.dataFlow; +import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.lang.injection.InjectedLanguageManager; import com.intellij.psi.*; import com.intellij.psi.util.PsiUtil; +import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.siyeh.ig.psiutils.ExpressionUtils; +import com.siyeh.ig.psiutils.TypeUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -33,6 +37,43 @@ import static com.intellij.codeInspection.dataFlow.MethodContract.createConstrai * @author peter */ public class HardcodedContracts { + static class OptionalPresenceContract extends MethodContract.QualifierBasedContract { + private final boolean myPresent; + + public OptionalPresenceContract(boolean mustPresent, ValueConstraint[] valueConstraints, ValueConstraint returnValue) { + super(valueConstraints, returnValue); + myPresent = mustPresent; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass() || !super.equals(o)) return false; + return myPresent == ((OptionalPresenceContract)o).myPresent; + } + + @Override + public int hashCode() { + return 31 * super.hashCode() + (myPresent ? 1 : 0); + } + + @Override + boolean applyContract(boolean matches, DfaValue qualifier, DfaMemoryState memoryState) { + boolean present = !matches ^ myPresent; + ThreeState state = memoryState.checkOptional(qualifier); + if(state == ThreeState.fromBoolean(!present)) return false; + if(state == ThreeState.UNSURE) { + memoryState.applyIsPresentCheck(present, qualifier); + } + return true; + } + + @Override + public String toString() { + return "[" + (myPresent ? "present" : "absent") + "] " + super.toString(); + } + } + public static List getHardcodedContracts(@NotNull PsiMethod method, @Nullable PsiMethodCallExpression call) { PsiClass owner = method.getContainingClass(); if (owner == null || @@ -85,6 +126,17 @@ public class HardcodedContracts { className.startsWith("org.assertj.core.api.")) { return handleTestFrameworks(paramCount, className, methodName, call); } + else if (TypeUtils.isOptional(owner)) { + MethodContract.ValueConstraint[] constraints = createConstraintArray(paramCount); + if (DfaOptionalSupport.isOptionalGetMethodName(methodName) || "orElseThrow".equals(methodName)) { + return Arrays.asList(new OptionalPresenceContract(false, constraints, THROW_EXCEPTION), + new OptionalPresenceContract(true, constraints, NOT_NULL_VALUE)); + } + else if ("isPresent".equals(methodName)) { + return Arrays.asList(new OptionalPresenceContract(false, constraints, FALSE_VALUE), + new OptionalPresenceContract(true, constraints, TRUE_VALUE)); + } + } return Collections.emptyList(); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java index c962fdd6a156..b61aa1015216 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2014 JetBrains s.r.o. + * Copyright 2000-2017 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. @@ -16,6 +16,7 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInspection.dataFlow.value.DfaConstValue; +import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; import com.intellij.openapi.util.text.StringUtil; import com.intellij.util.containers.ContainerUtil; @@ -24,6 +25,7 @@ import org.jetbrains.annotations.Nullable; import java.util.Arrays; import java.util.List; +import java.util.function.Predicate; /** * @author peter @@ -49,7 +51,7 @@ public class MethodContract { @Override public boolean equals(Object o) { if (this == o) return true; - if (!(o instanceof MethodContract)) return false; + if (o == null || o.getClass() != getClass()) return false; MethodContract contract = (MethodContract)o; @@ -74,6 +76,13 @@ public class MethodContract { return StringUtil.join(arguments, constraint -> constraint.toString(), ", ") + " -> " + returnValue; } + /** + * @return true if this contract result does not depend on arguments + */ + boolean isTrivial() { + return Arrays.stream(this.arguments).allMatch(Predicate.isEqual(ValueConstraint.ANY_VALUE)); + } + public enum ValueConstraint { ANY_VALUE("_"), NULL_VALUE("null"), NOT_NULL_VALUE("!null"), TRUE_VALUE("true"), FALSE_VALUE("false"), THROW_EXCEPTION("fail"); private final String myPresentableName; @@ -138,4 +147,17 @@ public class MethodContract { } } + abstract static class QualifierBasedContract extends MethodContract { + public QualifierBasedContract(@NotNull ValueConstraint[] arguments, + @NotNull ValueConstraint returnValue) { + super(arguments, returnValue); + } + + @Override + boolean isTrivial() { + return false; + } + + abstract boolean applyContract(boolean matches, DfaValue qualifier, DfaMemoryState memoryState); + } } 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 4ab3f253db32..6d1d14de2902 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 @@ -16,6 +16,7 @@ package com.intellij.codeInspection.dataFlow; import com.intellij.codeInsight.AnnotationUtil; +import com.intellij.codeInspection.dataFlow.MethodContract.QualifierBasedContract; import com.intellij.codeInspection.dataFlow.instructions.*; import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet; import com.intellij.codeInspection.dataFlow.value.*; @@ -27,7 +28,6 @@ import com.intellij.psi.util.PsiTreeUtil; import com.intellij.psi.util.PsiUtil; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.ObjectUtils; -import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.FactoryMap; import com.intellij.util.containers.MultiMap; @@ -50,9 +50,8 @@ public class StandardInstructionVisitor extends InstructionVisitor { private static final Logger LOG = Logger.getInstance("#com.intellij.codeInspection.dataFlow.StandardInstructionVisitor"); private static final Object ANY_VALUE = new Object(); - private static final Set OPTIONAL_METHOD_NAMES = - ContainerUtil.set("isPresent", "of", "ofNullable", "fromNullable", "empty", "absent", - "or", "orElseGet", "orElseThrow", "ifPresent", "map", "flatMap", "filter", "transform"); + private static final Set OPTIONAL_METHOD_NAMES = ContainerUtil + .set("of", "ofNullable", "fromNullable", "empty", "absent", "or", "orElseGet", "ifPresent", "map", "flatMap", "filter", "transform"); private static final CallMapper KNOWN_METHOD_RANGES = new CallMapper() .register(CallMatcher.instanceCall("java.time.LocalDateTime", "getHour"), LongRangeSet.range(0, 23)) .register(CallMatcher.instanceCall("java.time.LocalDateTime", "getMinute", "getSecond"), LongRangeSet.range(0, 59)) @@ -208,7 +207,7 @@ public class StandardInstructionVisitor extends InstructionVisitor { LinkedHashSet currentStates = ContainerUtil.newLinkedHashSet(memState); if (argValues != null) { for (MethodContract contract : instruction.getContracts()) { - currentStates = addContractResults(argValues, contract, currentStates, instruction, runner.getFactory(), finalStates); + currentStates = addContractResults(qualifier, argValues, contract, currentStates, instruction, runner.getFactory(), finalStates); if (currentStates.size() + finalStates.size() > DataFlowRunner.MAX_STATES_PER_BRANCH) { if (LOG.isDebugEnabled()) { LOG.debug("Too complex contract on " + instruction.getContext() + ", skipping contract processing"); @@ -266,26 +265,6 @@ public class StandardInstructionVisitor extends InstructionVisitor { DfaValue qualifier = popQualifier(instruction, runner, memState); DfaValue result = null; switch (methodName) { - case "isPresent": { - ThreeState state = memState.checkOptional(qualifier); - DfaConstValue.Factory constFactory = runner.getFactory().getConstFactory(); - if (state == ThreeState.UNSURE) { - DfaMemoryState falseState = memState.createCopy(); - memState.push(constFactory.getTrue()); - memState.applyIsPresentCheck(true, qualifier); - falseState.push(constFactory.getFalse()); - falseState.applyIsPresentCheck(false, qualifier); - return Arrays.asList(memState, falseState); - } - else { - result = state == ThreeState.YES ? constFactory.getTrue() : constFactory.getFalse(); - } - break; - } - case "orElseThrow": { - memState.applyIsPresentCheck(true, qualifier); - break; - } case "of": case "ofNullable": case "fromNullable": @@ -371,12 +350,13 @@ public class StandardInstructionVisitor extends InstructionVisitor { return qualifier; } - private LinkedHashSet addContractResults(DfaValue[] argValues, - MethodContract contract, - LinkedHashSet states, - MethodCallInstruction instruction, - DfaValueFactory factory, - Set finalStates) { + private LinkedHashSet addContractResults(DfaValue qualifier, + DfaValue[] argValues, + MethodContract contract, + LinkedHashSet states, + MethodCallInstruction instruction, + DfaValueFactory factory, + Set finalStates) { DfaConstValue.Factory constFactory = factory.getConstFactory(); LinkedHashSet falseStates = ContainerUtil.newLinkedHashSet(); for (int i = 0; i < argValues.length; i++) { @@ -424,6 +404,21 @@ public class StandardInstructionVisitor extends InstructionVisitor { states = nextStates; } + if (contract instanceof QualifierBasedContract) { + LinkedHashSet nextStates = ContainerUtil.newLinkedHashSet(); + QualifierBasedContract qualifierBasedContract = (QualifierBasedContract)contract; + for (DfaMemoryState state : states) { + DfaMemoryState falseCopy = state.createCopy(); + if (qualifierBasedContract.applyContract(true, qualifier, state)) { + nextStates.add(state); + } + if (qualifierBasedContract.applyContract(false, qualifier, falseCopy)) { + falseStates.add(falseCopy); + } + } + states = nextStates; + } + for (DfaMemoryState state : states) { state.push(getDfaContractReturnValue(contract, instruction, factory)); finalStates.add(state); diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java index dae9e9c3c9eb..9642644ebcf3 100644 --- a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java @@ -82,7 +82,7 @@ class OptionalWithoutIsPresent { } if (maybe.isPresent()) { maybe = Optional.empty(); - System.out.println(maybe.get()); + System.out.println(maybe.get()); } boolean b = ((maybe.isPresent())) && maybe.get() == 1; boolean c = (!maybe.isPresent()) || maybe.get() == 1; @@ -118,7 +118,7 @@ class OptionalWithoutIsPresent { boolean absent = !present; boolean otherAbsent = !!absent; if(otherAbsent) { - System.out.println(opt.get()); + System.out.println(opt.get()); } else { System.out.println(opt.get()); } @@ -151,7 +151,7 @@ class OptionalWithoutIsPresent { o2 = getOptional(); org.junit.Assert.assertTrue(!o2.isPresent()); - System.out.println(o2.get()); + System.out.println(o2.get()); } private void checkAsserts2() { @@ -175,12 +175,12 @@ class OptionalWithoutIsPresent { test = Optional.empty(); } System.out.println(test.get()); - if(b) { + if(b) { test = Optional.empty(); } else { test = Optional.empty(); } - System.out.println(test.get()); + System.out.println(test.get()); } @@ -228,7 +228,7 @@ class OptionalWithoutIsPresent { void order(Optional order, boolean b) { order.ifPresent(o -> System.out.println(order.get())); - System.out.println(order.orElseGet(() -> order.get().trim())); + System.out.println(order.orElseGet(() -> order.get().trim())); } public static void two(Optional o1,Optional o2) { @@ -318,7 +318,7 @@ class OptionalWithoutIsPresent { void guavaTest(com.google.common.base.Optional opt, String s) { System.out.println(opt.get()); - if(opt.isPresent()) { + if(opt.isPresent()) { System.out.println(opt.get()); } opt = com.google.common.base.Optional.fromNullable(s); @@ -347,4 +347,20 @@ class OptionalWithoutIsPresent { System.out.println("Yes"); } } + + void testThrowCatch(Optional opt) { + try { + opt.orElseThrow(RuntimeException::new); + System.out.println("Ok: " + opt.get()); + } catch (RuntimeException ex) { + System.out.println("Fail: " + opt.get()); + } + } + + public void testThrowFail(Optional arg) { + if(!arg.isPresent()) { + System.out.println(arg.orElseThrow(IllegalAccessError::new)); + } + String res = Optional.empty().orElseThrow(RuntimeException::new); + } } \ No newline at end of file diff --git a/platform/platform-resources-en/src/messages/InspectionsBundle.properties b/platform/platform-resources-en/src/messages/InspectionsBundle.properties index 8e29c2d72128..beb6de8f3886 100644 --- a/platform/platform-resources-en/src/messages/InspectionsBundle.properties +++ b/platform/platform-resources-en/src/messages/InspectionsBundle.properties @@ -92,7 +92,6 @@ dataflow.message.unboxing.method.reference=Use of #ref #loc would n dataflow.too.complex=Method #ref is too complex to analyze by data flow algorithm dataflow.method.fails.with.null.argument=Method will throw an exception when parameter is null dataflow.message.optional.get.without.is.present={0}.#ref() without ''isPresent()'' check -dataflow.message.optional.get.definitely.absent={0}.#ref() will definitely fail as {0} is empty here #deprecated inspection.deprecated.display.name=Deprecated API usage