MDS/openide
1
0
Fork 0
mirror of https://gitflic.ru/project/openide/openide.git synced 2026-09-27 10:03:11 +07:00
Code Issues Packages Projects Releases Wiki Activity

Improved optional.get/orElseThrow handling: now it's a special contract

Browse Source
This commit is contained in:
Tagir Valeev
2017-03-17 12:24:05 +07:00
parent 2676e31e9f
commit 1b533c8f4d
7 changed files with 144 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DataFlowInspectionBase.java
+13 -24
View File
@@ -355,30 +355,24 @@ public class DataFlowInspectionBase extends BaseJavaBatchLocalInspectionTool {
private void reportUncheckedOptionalGet(ProblemsHolder holder,
Map<PsiMethodCallExpression, ThreeState> calls,
List<PsiExpression> qualifiers) {
if (!REPORT_UNCHECKED_OPTIONALS) return;
for (Map.Entry<PsiMethodCallExpression, ThreeState> 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<MethodContract> 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
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaOptionalSupport.java
+5 -1
View File
@@ -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;
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/HardcodedContracts.java
+53 -1
View File
@@ -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<MethodContract> 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();
}
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/MethodContract.java
+24 -2
View File
@@ -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);
}
}
java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/StandardInstructionVisitor.java
+26 -31
View File
@@ -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<String> OPTIONAL_METHOD_NAMES =
ContainerUtil.set("isPresent", "of", "ofNullable", "fromNullable", "empty", "absent",
"or", "orElseGet", "orElseThrow", "ifPresent", "map", "flatMap", "filter", "transform");
private static final Set<String> OPTIONAL_METHOD_NAMES = ContainerUtil
.set("of", "ofNullable", "fromNullable", "empty", "absent", "or", "orElseGet", "ifPresent", "map", "flatMap", "filter", "transform");
private static final CallMapper<LongRangeSet> KNOWN_METHOD_RANGES = new CallMapper<LongRangeSet>()
.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<DfaMemoryState> 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<DfaMemoryState> addContractResults(DfaValue[] argValues,
MethodContract contract,
LinkedHashSet<DfaMemoryState> states,
MethodCallInstruction instruction,
DfaValueFactory factory,
Set<DfaMemoryState> finalStates) {
private LinkedHashSet<DfaMemoryState> addContractResults(DfaValue qualifier,
DfaValue[] argValues,
MethodContract contract,
LinkedHashSet<DfaMemoryState> states,
MethodCallInstruction instruction,
DfaValueFactory factory,
Set<DfaMemoryState> finalStates) {
DfaConstValue.Factory constFactory = factory.getConstFactory();
LinkedHashSet<DfaMemoryState> 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<DfaMemoryState> 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);
java/java-tests/testData/inspection/dataFlow/fixture/OptionalGetWithoutIsPresent.java
+23 -7
View File
@@ -82,7 +82,7 @@ class OptionalWithoutIsPresent {
}
if (maybe.isPresent()) {
maybe = Optional.empty();
System.out.println(maybe.<warning descr="'Optional.get()' will definitely fail as Optional is empty here">get</warning>());
System.out.println(maybe.<warning descr="The call to get always fails, according to its method contracts">get</warning>());
}
boolean b = <warning descr="Condition '((maybe.isPresent()))' is always 'false'">((maybe.isPresent()))</warning> && maybe.get() == 1;
boolean c = <warning descr="Condition '(!maybe.isPresent())' is always 'true'">(!maybe.isPresent())</warning> || maybe.get() == 1;
@@ -118,7 +118,7 @@ class OptionalWithoutIsPresent {
boolean absent = !present;
boolean otherAbsent = !!absent;
if(otherAbsent) {
System.out.println(opt.<warning descr="'Optional.get()' will definitely fail as Optional is empty here">get</warning>());
System.out.println(opt.<warning descr="The call to get always fails, according to its method contracts">get</warning>());
} else {
System.out.println(opt.get());
}
@@ -151,7 +151,7 @@ class OptionalWithoutIsPresent {
o2 = getOptional();
org.junit.Assert.assertTrue(!o2.isPresent());
System.out.println(o2.<warning descr="'Optional.get()' will definitely fail as Optional is empty here">get</warning>());
System.out.println(o2.<warning descr="The call to get always fails, according to its method contracts">get</warning>());
}
private void checkAsserts2() {
@@ -175,12 +175,12 @@ class OptionalWithoutIsPresent {
test = Optional.empty();
}
System.out.println(test.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
if(b) {
if(<warning descr="Condition 'b' is always 'true'">b</warning>) {
test = Optional.empty();
} else {
test = Optional.empty();
}
System.out.println(test.<warning descr="'Optional.get()' will definitely fail as Optional is empty here">get</warning>());
System.out.println(test.<warning descr="The call to get always fails, according to its method contracts">get</warning>());
}
@@ -228,7 +228,7 @@ class OptionalWithoutIsPresent {
void order(Optional<String> order, boolean b) {
order.ifPresent(o -> System.out.println(order.get()));
System.out.println(order.orElseGet(() -> order.<warning descr="'Optional.get()' will definitely fail as Optional is empty here">get</warning>().trim()));
System.out.println(order.orElseGet(() -> order.<warning descr="The call to get always fails, according to its method contracts">get</warning>().trim()));
}
public static void two(Optional<Object> o1,Optional<Object> o2) {
@@ -318,7 +318,7 @@ class OptionalWithoutIsPresent {
void guavaTest(com.google.common.base.Optional<String> opt, String s) {
System.out.println(opt.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
if(opt.isPresent()) {
if(<warning descr="Condition 'opt.isPresent()' is always 'true'">opt.isPresent()</warning>) {
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<String> opt) {
try {
opt.orElseThrow(RuntimeException::new);
System.out.println("Ok: " + opt.get());
} catch (RuntimeException ex) {
System.out.println("Fail: " + opt.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
}
}
public void testThrowFail(Optional<String> arg) {
if(!arg.isPresent()) {
System.out.println(arg.<warning descr="The call to orElseThrow always fails, according to its method contracts">orElseThrow</warning>(IllegalAccessError::new));
}
String res = Optional.<String>empty().<warning descr="The call to orElseThrow always fails, according to its method contracts">orElseThrow</warning>(RuntimeException::new);
}
}
platform/platform-resources-en/src/messages/InspectionsBundle.properties
-1
View File
@@ -92,7 +92,6 @@ dataflow.message.unboxing.method.reference=Use of <code>#ref</code> #loc would n
dataflow.too.complex=Method <code>#ref</code> 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=<code>{0}.#ref()</code> without ''isPresent()'' check
dataflow.message.optional.get.definitely.absent=<code>{0}.#ref()</code> will definitely fail as {0} is empty here
#deprecated
inspection.deprecated.display.name=Deprecated API usage