Optional refactoring: as special value now; fixed IDEA-204165

This commit is contained in:
Tagir Valeev
2018-12-18 11:30:14 +07:00
parent eae13a7a7f
commit 7cad465cd2
17 changed files with 146 additions and 91 deletions

View File

@@ -3,6 +3,7 @@ package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.instructions.EndOfInitializerInstruction;
import com.intellij.codeInspection.dataFlow.value.DfaConstValue;
import com.intellij.codeInspection.dataFlow.value.DfaFactMapValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.codeInspection.dataFlow.value.DfaVariableValue;
import com.intellij.openapi.util.TextRange;
@@ -73,16 +74,19 @@ public class CommonDataflow {
private void addFacts(PsiExpression expression, DfaMemoryStateImpl memState, DfaValue value) {
DfaFactMap existing = myFacts.get(expression);
if(existing != DfaFactMap.EMPTY) {
DfaFactMap newMap = memState.getFactMap(value);
if (!DfaNullability.isNotNull(newMap) && memState.isNotNull(value)) {
newMap = newMap.with(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL);
}
DfaFactMap newMap = getFactMap(memState, value);
if (value instanceof DfaVariableValue) {
SpecialField field = SpecialField.fromQualifierType(value.getType());
if (field != null) {
DfaConstValue constValue = memState.getConstantValue(field.createValue(value.getFactory(), value));
if (constValue != null) {
newMap = newMap.with(DfaFactType.SPECIAL_FIELD_VALUE, field.withValue(constValue));
DfaValue specialField = field.createValue(value.getFactory(), value);
if (specialField instanceof DfaVariableValue) {
DfaConstValue constantValue = memState.getConstantValue(specialField);
specialField = constantValue != null
? constantValue
: specialField.getFactory().getFactFactory().createValue(getFactMap(memState, specialField));
}
if (specialField instanceof DfaConstValue || specialField instanceof DfaFactMapValue) {
newMap = newMap.with(DfaFactType.SPECIAL_FIELD_VALUE, field.withValue(specialField));
}
}
}
@@ -96,6 +100,16 @@ public class CommonDataflow {
}
}
@NotNull
private static DfaFactMap getFactMap(DfaMemoryStateImpl memState, DfaValue value) {
DfaFactMap newMap = memState.getFactMap(value);
DfaNullability nullability = newMap.get(DfaFactType.NULLABILITY);
if (nullability != DfaNullability.NOT_NULL && memState.isNotNull(value)) {
newMap = newMap.with(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL);
}
return newMap;
}
/**
* Returns true if given expression was visited by dataflow. Note that dataflow usually tracks deparenthesized expressions only,
* so you should deparenthesize it in advance if necessary.

View File

@@ -108,10 +108,6 @@ public abstract class ContractValue {
return value ? IndependentValue.TRUE : IndependentValue.FALSE;
}
public static ContractValue optionalValue(boolean present) {
return present ? IndependentValue.OPTIONAL_PRESENT : IndependentValue.OPTIONAL_ABSENT;
}
public static ContractValue nullValue() {
return IndependentValue.NULL;
}
@@ -178,10 +174,6 @@ public abstract class ContractValue {
return other == TRUE;
}
};
static final IndependentValue OPTIONAL_PRESENT =
new IndependentValue(factory -> DfaOptionalSupport.getOptionalValue(factory, true), "present");
static final IndependentValue OPTIONAL_ABSENT =
new IndependentValue(factory -> DfaOptionalSupport.getOptionalValue(factory, false), "empty");
static final IndependentValue ZERO = new IndependentValue(factory -> factory.getInt(0), "0");
private final Function<? super DfaValueFactory, ? extends DfaValue> mySupplier;

View File

@@ -214,8 +214,16 @@ final class DataFlowInstructionVisitor extends StandardInstructionVisitor {
}
private void processOfNullableResult(@NotNull DfaValue value, @NotNull DfaMemoryState memState, PsiElement anchor) {
Boolean fact = memState.getValueFact(value, DfaFactType.OPTIONAL_PRESENCE);
ThreeState present = fact == null ? ThreeState.UNSURE : ThreeState.fromBoolean(fact);
DfaValueFactory factory = value.getFactory();
DfaValue optionalValue = factory == null ? DfaUnknownValue.getInstance() : SpecialField.OPTIONAL_VALUE.createValue(factory, value);
ThreeState present;
if (memState.isNull(optionalValue)) {
present = ThreeState.NO;
}
else if (memState.isNotNull(optionalValue)) {
present = ThreeState.YES;
}
else present = ThreeState.UNSURE;
myOfNullableCalls.merge(anchor, present, ThreeState::merge);
}

View File

@@ -87,15 +87,12 @@ public abstract class DfaFactType<T> extends Key<T> {
@Nullable
@Override
DfaNullability fromDfaValue(DfaValue value) {
public DfaNullability fromDfaValue(DfaValue value) {
if (value instanceof DfaConstValue) {
return ((DfaConstValue)value).getValue() == null ? DfaNullability.NULLABLE : DfaNullability.NOT_NULL;
}
if (value instanceof DfaBoxedValue) return DfaNullability.NOT_NULL;
if (value instanceof DfaFactMapValue) {
DfaFactMapValue factValue = (DfaFactMapValue)value;
if (factValue.get(OPTIONAL_PRESENCE) != null || factValue.get(RANGE) != null) return DfaNullability.NOT_NULL;
}
if (value instanceof DfaFactMapValue && ((DfaFactMapValue)value).get(RANGE) != null) return DfaNullability.NOT_NULL;
return super.fromDfaValue(value);
}
@@ -126,20 +123,6 @@ public abstract class DfaFactType<T> extends Key<T> {
}
};
/**
* This fact is applied to the Optional values (like {@link java.util.Optional} or Guava Optional).
* When its value is true, then optional is known to be present.
* When its value is false, then optional is known to be empty (absent).
*/
public static final DfaFactType<Boolean> OPTIONAL_PRESENCE = new DfaFactType<Boolean>("Optional") {
@NotNull
@Override
public String toString(@NotNull Boolean fact) {
return fact ? "present Optional" : "absent Optional";
}
};
/**
* This fact is applied to the integral values (of types byte, char, short, int, long).
* Its value represents a range of possible values.
@@ -157,7 +140,7 @@ public abstract class DfaFactType<T> extends Key<T> {
@Nullable
@Override
LongRangeSet fromDfaValue(DfaValue value) {
public LongRangeSet fromDfaValue(DfaValue value) {
if(value instanceof DfaVariableValue) {
return calcFromVariable((DfaVariableValue)value);
}
@@ -264,7 +247,7 @@ public abstract class DfaFactType<T> extends Key<T> {
@NotNull
@Override
public String getPresentationText(@NotNull SpecialFieldValue fact, @Nullable PsiType type) {
return String.valueOf(fact.getValue());
return fact.getPresentationText(type);
}
};
@@ -284,7 +267,7 @@ public abstract class DfaFactType<T> extends Key<T> {
}
@Nullable
T fromDfaValue(DfaValue value) {
public T fromDfaValue(DfaValue value) {
return value instanceof DfaFactMapValue ? ((DfaFactMapValue)value).get(this) : null;
}

View File

@@ -818,10 +818,6 @@ public class DfaMemoryStateImpl implements DfaMemoryState {
case IS:
return applyFacts(dfaVar, factValue.getFacts());
case IS_NOT: {
Boolean optionalPresence = factValue.get(DfaFactType.OPTIONAL_PRESENCE);
if(optionalPresence != null) {
return applyFact(dfaVar, DfaFactType.OPTIONAL_PRESENCE, !optionalPresence);
}
boolean isNotNull = DfaNullability.isNotNull(factValue.getFacts());
TypeConstraint constraint = factValue.get(DfaFactType.TYPE_CONSTRAINT);
if (constraint != null && constraint.getNotInstanceofValues().isEmpty()) {
@@ -864,29 +860,27 @@ public class DfaMemoryStateImpl implements DfaMemoryState {
}
private void updateVarStateOnComparison(@NotNull DfaVariableValue dfaVar, DfaValue value, boolean isNegated) {
if (!(dfaVar.getType() instanceof PsiPrimitiveType)) {
if (isNegated) {
if (isNull(value)) {
setVariableState(dfaVar, getVariableState(dfaVar).withFact(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL));
if (isNegated) {
if (isNull(value)) {
setVariableState(dfaVar, getVariableState(dfaVar).withFact(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL));
}
} else {
if (value instanceof DfaConstValue) {
Object constValue = ((DfaConstValue)value).getValue();
if (constValue == null) {
setVariableState(dfaVar, getVariableState(dfaVar).withFact(DfaFactType.NULLABILITY, DfaNullability.NULLABLE));
return;
}
} else {
if (value instanceof DfaConstValue) {
Object constValue = ((DfaConstValue)value).getValue();
if (constValue == null) {
setVariableState(dfaVar, getVariableState(dfaVar).withFact(DfaFactType.NULLABILITY, DfaNullability.NULLABLE));
return;
}
DfaPsiType dfaType = myFactory.createDfaType(((DfaConstValue)value).getType());
DfaVariableState state = getVariableState(dfaVar).withInstanceofValue(dfaType);
if (state != null) {
setVariableState(dfaVar, state);
}
}
if (isNotNull(value) && !isNotNull(dfaVar)) {
setVariableState(dfaVar, getVariableState(dfaVar).withoutFact(DfaFactType.NULLABILITY));
applyRelation(dfaVar, myFactory.getConstFactory().getNull(), true);
DfaPsiType dfaType = myFactory.createDfaType(((DfaConstValue)value).getType());
DfaVariableState state = getVariableState(dfaVar).withInstanceofValue(dfaType);
if (state != null) {
setVariableState(dfaVar, state);
}
}
if (isNotNull(value) && !isNotNull(dfaVar)) {
setVariableState(dfaVar, getVariableState(dfaVar).withoutFact(DfaFactType.NULLABILITY));
applyRelation(dfaVar, myFactory.getConstFactory().getNull(), true);
}
}
}

View File

@@ -84,7 +84,9 @@ public class DfaOptionalSupport {
*/
@NotNull
public static DfaValue getOptionalValue(DfaValueFactory factory, boolean present) {
DfaFactMap facts = DfaFactMap.EMPTY.with(DfaFactType.OPTIONAL_PRESENCE, present).with(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL);
DfaValue value = present ? factory.getFactValue(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL) : factory.getConstFactory().getNull();
DfaFactMap facts = DfaFactMap.EMPTY.with(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL)
.with(DfaFactType.SPECIAL_FIELD_VALUE, SpecialField.OPTIONAL_VALUE.withValue(value));
return factory.getFactFactory().createValue(facts);
}

View File

@@ -229,7 +229,8 @@ public class HardcodedContracts {
}
static MethodContract optionalAbsentContract(ContractReturnValue returnValue) {
return singleConditionContract(ContractValue.qualifier(), RelationType.IS, ContractValue.optionalValue(false), returnValue);
return singleConditionContract(ContractValue.qualifier().specialField(SpecialField.OPTIONAL_VALUE), RelationType.EQ,
ContractValue.nullValue(), returnValue);
}
static MethodContract nonnegativeArgumentContract(int argNumber) {

View File

@@ -29,6 +29,9 @@ public class NullabilityUtil {
if (value.getDescriptor() instanceof DfaExpressionFactory.ThisDescriptor) {
return DfaNullability.NOT_NULL;
}
if (value.getDescriptor() == SpecialField.OPTIONAL_VALUE) {
return DfaNullability.NULLABLE;
}
PsiModifierListOwner var = value.getPsiVariable();
Nullability nullability = DfaPsiUtil.getElementNullabilityIgnoringParameterInference(value.getType(), var);
if (nullability != Nullability.UNKNOWN) {

View File

@@ -3,6 +3,7 @@ package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.rangeSet.LongRangeSet;
import com.intellij.codeInspection.dataFlow.value.*;
import com.intellij.codeInspection.util.OptionalUtil;
import com.intellij.psi.*;
import com.intellij.psi.util.InheritanceUtil;
import com.intellij.psi.util.PsiUtil;
@@ -10,6 +11,7 @@ import com.intellij.psi.util.TypeConversionUtil;
import com.siyeh.ig.callMatcher.CallMatcher;
import com.siyeh.ig.psiutils.ExpressionUtils;
import com.siyeh.ig.psiutils.MethodUtils;
import com.siyeh.ig.psiutils.TypeUtils;
import one.util.streamex.StreamEx;
import org.jetbrains.annotations.Contract;
import org.jetbrains.annotations.NotNull;
@@ -112,6 +114,43 @@ public enum SpecialField implements VariableDescriptor {
boolean isMyAccessor(PsiMember accessor) {
return accessor instanceof PsiMethod && UNBOXING_CALL.methodMatches((PsiMethod)accessor);
}
},
OPTIONAL_VALUE(null, "value", true) {
@Override
PsiType getType(DfaVariableValue variableValue) {
return OptionalUtil.getOptionalElementType(variableValue.getType());
}
@NotNull
@Override
public DfaValue getDefaultValue(DfaValueFactory factory) {
return factory.getFactValue(DfaFactType.NULLABILITY, DfaNullability.NULLABLE);
}
@Override
boolean isMyQualifierType(PsiType type) {
return TypeUtils.isOptional(type);
}
@Override
public String getPresentationText(@NotNull DfaValue value, @Nullable PsiType type) {
if (value instanceof DfaConstValue && ((DfaConstValue)value).getValue() == null) {
return "empty Optional";
}
if (value instanceof DfaFactMapValue) {
DfaNullability nullability = ((DfaFactMapValue)value).get(DfaFactType.NULLABILITY);
if (nullability == DfaNullability.NOT_NULL) {
return "present Optional";
}
return "";
}
return super.getPresentationText(value, type);
}
@Override
boolean isMyAccessor(PsiMember accessor) {
return accessor instanceof PsiMethod && OptionalUtil.OPTIONAL_GET.methodMatches((PsiMethod)accessor);
}
};
private final String myClassName;
@@ -147,6 +186,10 @@ public enum SpecialField implements VariableDescriptor {
return accessor instanceof PsiMethod && MethodUtils.methodMatches((PsiMethod)accessor, myClassName, null, myMethodName);
}
public String getPresentationText(@NotNull DfaValue value, @Nullable PsiType type) {
return value.toString();
}
/**
* Finds a special field which corresponds to given accessor (method or field)
* @param accessor accessor to find a special field for
@@ -220,7 +263,7 @@ public enum SpecialField implements VariableDescriptor {
return factory.getFactValue(DfaFactType.RANGE, LongRangeSet.indexRange());
}
PsiPrimitiveType getType(DfaVariableValue variableValue) {
PsiType getType(DfaVariableValue variableValue) {
return PsiType.INT;
}
@@ -253,6 +296,17 @@ public enum SpecialField implements VariableDescriptor {
return new SpecialFieldValue(this, value);
}
/**
* Returns a value from given SpecialFieldValue if it's bound to this special field
* @param sfValue {@link SpecialFieldValue} to extract the value from
* @return en extracted value, or null if argument is null or it's bound to different special field
*/
@Contract("null -> null")
@Nullable
public DfaValue extract(@Nullable SpecialFieldValue sfValue) {
return sfValue != null && sfValue.getField() == this ? sfValue.getValue() : null;
}
/**
* Returns a special field which corresponds to given qualifier type
* (currently it's assumed that only one special field may exist for given qualifier type)

View File

@@ -4,6 +4,7 @@ package com.intellij.codeInspection.dataFlow;
import com.intellij.codeInspection.dataFlow.value.DfaConstValue;
import com.intellij.codeInspection.dataFlow.value.DfaFactMapValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.psi.PsiType;
import org.jetbrains.annotations.NotNull;
import java.util.Objects;
@@ -55,4 +56,8 @@ public final class SpecialFieldValue {
public String toString() {
return myField + " = " + myValue;
}
public String getPresentationText(PsiType type) {
return myField.getPresentationText(myValue, type);
}
}

View File

@@ -19,7 +19,7 @@ import com.intellij.codeInsight.Nullability;
import com.intellij.codeInspection.dataFlow.CFGBuilder;
import com.intellij.codeInspection.dataFlow.DfaOptionalSupport;
import com.intellij.codeInspection.dataFlow.NullabilityProblemKind;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.codeInspection.dataFlow.SpecialField;
import com.intellij.codeInspection.dataFlow.value.DfaValueFactory;
import com.intellij.psi.*;
import com.intellij.psi.util.PsiUtil;
@@ -221,15 +221,9 @@ public class OptionalChainInliner implements CallInliner {
return true;
}
}
DfaValue presentOptional = DfaOptionalSupport.getOptionalValue(builder.getFactory(), true);
builder
.pushExpression(expression, problem)
.push(presentOptional)
.ifCondition(JavaTokenType.INSTANCEOF_KEYWORD)
.push(builder.getFactory().createTypeValue(optionalElementType, Nullability.NOT_NULL))
.elseBranch()
.pushNull()
.end()
.unwrap(SpecialField.OPTIONAL_VALUE, optionalElementType)
.assignTo(builder.createTempVariable(optionalElementType));
return true;
}

View File

@@ -267,17 +267,16 @@ public class StreamChainInliner implements CallInliner {
@Override
void iteration(CFGBuilder builder) {
DfaValue presentOptional = DfaOptionalSupport.getOptionalValue(builder.getFactory(), true);
if (myFunction != null) {
builder.push(myResult)
.push(presentOptional)
.ifCondition(JavaTokenType.INSTANCEOF_KEYWORD)
DfaVariableValue optValue = (DfaVariableValue)SpecialField.OPTIONAL_VALUE.createValue(builder.getFactory(), myResult);
builder.push(optValue)
.ifNotNull()
.push(builder.getFactory().getFactValue(DfaFactType.NULLABILITY, DfaNullability.NOT_NULL))
.swap()
.invokeFunction(2, myFunction, Nullability.NOT_NULL)
.end();
}
builder.assign(myResult, presentOptional).splice(2);
builder.assign(myResult, DfaOptionalSupport.getOptionalValue(builder.getFactory(), true)).splice(2);
}
}

View File

@@ -73,6 +73,7 @@ public class OptionalUtil {
case OPTIONAL_DOUBLE:
return PsiType.DOUBLE;
case JAVA_UTIL_OPTIONAL:
case GUAVA_OPTIONAL:
PsiType[] parameters = ((PsiClassType)type).getParameters();
if (parameters.length != 1) return null;
PsiType streamType = parameters[0];

View File

@@ -1,8 +1,7 @@
// 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;
import com.intellij.codeInspection.dataFlow.CommonDataflow;
import com.intellij.codeInspection.dataFlow.DfaFactType;
import com.intellij.codeInspection.dataFlow.*;
import com.intellij.codeInspection.util.LambdaGenerationUtil;
import com.intellij.codeInspection.util.OptionalRefactoringUtil;
import com.intellij.openapi.project.Project;
@@ -130,9 +129,9 @@ public class SimplifyOptionalCallChainsInspection extends AbstractBaseJavaLocalI
!EquivalenceChecker.getCanonicalPsiEquivalence().typesAreEquivalent(qualifier.getType(), parentCall.getType())) {
return;
}
if ("get".equals(call.getMethodExpression().getReferenceName()) &&
!Boolean.TRUE.equals(CommonDataflow.getExpressionFact(qualifier, DfaFactType.OPTIONAL_PRESENCE))) {
return;
if ("get".equals(call.getMethodExpression().getReferenceName())) {
SpecialFieldValue fact = CommonDataflow.getExpressionFact(qualifier, DfaFactType.SPECIAL_FIELD_VALUE);
if (DfaFactType.NULLABILITY.fromDfaValue(SpecialField.OPTIONAL_VALUE.extract(fact)) != DfaNullability.NOT_NULL) return;
}
SimplifyOptionalChainFix fix = new SimplifyOptionalChainFix(qualifier.getText(), "Unwrap", "Unnecessary Optional rewrapping");
handleSimplification(Objects.requireNonNull(parentCall.getMethodExpression().getReferenceNameElement()), fix);

View File

@@ -3,8 +3,9 @@ package com.intellij.codeInspection.java18api;
import com.intellij.codeInsight.PsiEquivalenceUtil;
import com.intellij.codeInspection.*;
import com.intellij.codeInspection.dataFlow.CommonDataflow;
import com.intellij.codeInspection.dataFlow.DfaFactType;
import com.intellij.codeInspection.dataFlow.*;
import com.intellij.codeInspection.dataFlow.value.DfaFactMapValue;
import com.intellij.codeInspection.dataFlow.value.DfaValue;
import com.intellij.codeInspection.util.LambdaGenerationUtil;
import com.intellij.codeInspection.util.OptionalUtil;
import com.intellij.openapi.project.Project;
@@ -37,9 +38,13 @@ public class OptionalGetWithoutIsPresentInspection extends AbstractBaseJavaLocal
PsiClass optionalClass = PsiUtil.resolveClassInClassTypeOnly(qualifier.getType());
if (optionalClass == null) return;
CommonDataflow.DataflowResult result = CommonDataflow.getDataflowResult(qualifier);
if (result != null &&
result.expressionWasAnalyzed(qualifier) &&
result.getExpressionFact(qualifier, DfaFactType.OPTIONAL_PRESENCE) == null &&
if (result == null || !result.expressionWasAnalyzed(qualifier)) return;
SpecialFieldValue fact = result.getExpressionFact(qualifier, DfaFactType.SPECIAL_FIELD_VALUE);
DfaValue value = SpecialField.OPTIONAL_VALUE.extract(fact);
if (value != null && !(value instanceof DfaFactMapValue)) return;
DfaNullability nullability = value != null ? ((DfaFactMapValue)value).get(DfaFactType.NULLABILITY) : null;
if (nullability != DfaNullability.NOT_NULL &&
nullability != DfaNullability.FLUSHED &&
!isPresentCallWithSameQualifierExists(qualifier)) {
holder.registerProblem(nameElement,
InspectionsBundle.message("inspection.optional.get.without.is.present.message", optionalClass.getName()),

View File

@@ -366,7 +366,8 @@ class CtorTest {
CtorTest() {
System.out.println(test.get());
something();
System.out.println(test.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
// Conservatively skip the warning: people rarely do this
System.out.println(test.get());
}
<error descr="Missing method body, or declare abstract">CtorTest(String noBody);</error>

View File

@@ -32,7 +32,7 @@ public class JavaTypeProviderTest extends LightCodeInsightTestCase {
"<table>" +
"<tr><td align='left' valign='top' style='color:#909090'>Type:</td><td>Optional&lt;String&gt;</td></tr>" +
"<tr><td align='left' valign='top' style='color:#909090'>Nullability:</td><td>non-null</td></tr>" +
"<tr><td align='left' valign='top' style='color:#909090'>Optional:</td><td>present Optional</td></tr>" +
"<tr><td align='left' valign='top' style='color:#909090'>Value:</td><td>present Optional</td></tr>" +
"</table>");
}