From 1cb1966489d5514110666bbe328b534999bd2bec Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 10 Feb 2017 14:28:29 +0700 Subject: [PATCH] OptionalGetWithoutIsPresentInspection: optional state integrated into main dataflow (IDEA-CR-18052) --- .../dataFlow/DfaMemoryState.java | 5 + .../dataFlow/DfaMemoryStateImpl.java | 25 ++- .../dataFlow/DfaVariableState.java | 50 ++++-- .../NullParameterConstraintChecker.java | 6 +- .../dataFlow/StandardInstructionVisitor.java | 86 +++++++++-- .../dataFlow/ValuableDataFlowRunner.java | 26 ++-- .../dataFlow/value/DfaOptionalValue.java | 46 ++++++ .../dataFlow/value/DfaValueFactory.java | 7 + .../dataFlow/fixture/OptionalIsPresent.java | 19 +++ .../DataFlowInspection8Test.java | 1 + ...OptionalGetWithoutIsPresentInspection.java | 144 +----------------- 11 files changed, 232 insertions(+), 183 deletions(-) create mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaOptionalValue.java create mode 100644 java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java index 326a7c76cf21..f78f66a45858 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryState.java @@ -19,6 +19,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaConstValue; import com.intellij.codeInspection.dataFlow.value.DfaRelationValue; import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; +import com.intellij.util.ThreeState; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -43,8 +44,12 @@ public interface DfaMemoryState { boolean applyInstanceofOrNull(@NotNull DfaRelationValue dfaCond); + void applyIsPresentCheck(boolean present, DfaValue qualifier); + boolean applyCondition(DfaValue dfaCond); + ThreeState checkOptional(DfaValue value); + void flushFields(); void flushVariable(DfaVariableValue variable); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java index 6e1cc3f64056..e37f3130f3d9 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaMemoryStateImpl.java @@ -36,6 +36,7 @@ import com.intellij.psi.PsiType; import com.intellij.psi.util.TypeConversionUtil; import com.intellij.util.ArrayUtil; import com.intellij.util.ObjectUtils; +import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.Stack; import gnu.trove.*; @@ -614,6 +615,13 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return false; } + @Override + public void applyIsPresentCheck(boolean present, DfaValue qualifier) { + if (qualifier instanceof DfaVariableValue) { + setVariableState((DfaVariableValue)qualifier, getVariableState((DfaVariableValue)qualifier).withOptionalPresense(present)); + } + } + static DfaValue unwrap(DfaValue value) { if (value instanceof DfaBoxedValue) { return ((DfaBoxedValue)value).getWrappedValue(); @@ -766,6 +774,9 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return false; } } + if (!isNegated && dfaRight instanceof DfaOptionalValue) { + applyIsPresentCheck(((DfaOptionalValue)dfaRight).isPresent(), dfaLeft); + } return true; } @@ -907,13 +918,23 @@ public class DfaMemoryStateImpl implements DfaMemoryState { return true; } + @Override + public ThreeState checkOptional(DfaValue value) { + if (value instanceof DfaVariableValue) { + DfaVariableValue var = (DfaVariableValue)value; + DfaVariableState state = getVariableState(var); + return state.getOptionalPresense(); + } + return value instanceof DfaOptionalValue ? ThreeState.fromBoolean(((DfaOptionalValue)value).isPresent()) : ThreeState.UNSURE; + } + @Nullable private DfaRelationValue compareToNull(DfaValue dfaVar, boolean negated) { DfaConstValue dfaNull = myFactory.getConstFactory().getNull(); return myFactory.getRelationFactory().createRelation(dfaVar, dfaNull, JavaTokenType.EQEQ, negated); } - protected void setVariableState(DfaVariableValue dfaVar, DfaVariableState state) { + void setVariableState(DfaVariableValue dfaVar, DfaVariableState state) { assert !myUnknownVariables.contains(dfaVar); if (state.equals(myDefaultVariableStates.get(dfaVar))) { myVariableStates.remove(dfaVar); @@ -923,7 +944,7 @@ public class DfaMemoryStateImpl implements DfaMemoryState { myCachedHash = null; } - protected DfaVariableState getVariableState(DfaVariableValue dfaVar) { + DfaVariableState getVariableState(DfaVariableValue dfaVar) { DfaVariableState state = myVariableStates.get(dfaVar); if (state == null) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java index 54a87aa4b6f4..e61e10abf342 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/DfaVariableState.java @@ -30,6 +30,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.PsiPrimitiveType; +import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -40,23 +41,27 @@ import java.util.HashSet; import java.util.List; import java.util.Set; -public class DfaVariableState { +class DfaVariableState { @NotNull final Set myInstanceofValues; @NotNull final Set myNotInstanceofValues; @NotNull final Nullness myNullability; + @NotNull final ThreeState myOptionalPresence; private final int myHash; - protected DfaVariableState(@NotNull DfaVariableValue dfaVar) { - this(Collections.emptySet(), Collections.emptySet(), dfaVar.getInherentNullability()); + DfaVariableState(@NotNull DfaVariableValue dfaVar) { + this(Collections.emptySet(), Collections.emptySet(), dfaVar.getInherentNullability(), ThreeState.UNSURE); } - protected DfaVariableState(@NotNull Set instanceofValues, + DfaVariableState(@NotNull Set instanceofValues, @NotNull Set notInstanceofValues, - @NotNull Nullness nullability) { + @NotNull Nullness nullability, + @NotNull ThreeState optionalPresence) { myInstanceofValues = instanceofValues; myNotInstanceofValues = notInstanceofValues; myNullability = nullability; - myHash = (myInstanceofValues.hashCode() * 31 + myNotInstanceofValues.hashCode()) * 31 + myNullability.hashCode(); + myOptionalPresence = optionalPresence; + myHash = ((myInstanceofValues.hashCode() * 31 + myNotInstanceofValues.hashCode()) * 31 + myNullability.hashCode()) * 31 + + myOptionalPresence.hashCode(); } public boolean isNullable() { @@ -96,7 +101,7 @@ public class DfaVariableState { HashSet newInstanceof = ContainerUtil.newHashSet(myInstanceofValues); newInstanceof.removeAll(moreGeneric); newInstanceof.add(dfaType.getDfaType()); - result = createCopy(newInstanceof, myNotInstanceofValues, result.myNullability); + result = createCopy(newInstanceof, myNotInstanceofValues, result.myNullability, myOptionalPresence); return result; } @@ -124,7 +129,7 @@ public class DfaVariableState { HashSet newNotInstanceof = ContainerUtil.newHashSet(myNotInstanceofValues); newNotInstanceof.removeAll(moreSpecific); newNotInstanceof.add(dfaType.getDfaType()); - return createCopy(myInstanceofValues, newNotInstanceof, myNullability); + return createCopy(myInstanceofValues, newNotInstanceof, myNullability, myOptionalPresence); } @NotNull @@ -132,12 +137,12 @@ public class DfaVariableState { if (myInstanceofValues.contains(type)) { HashSet newInstanceof = ContainerUtil.newHashSet(myInstanceofValues); newInstanceof.remove(type); - return createCopy(newInstanceof, myNotInstanceofValues, myNullability); + return createCopy(newInstanceof, myNotInstanceofValues, myNullability, myOptionalPresence); } if (myNotInstanceofValues.contains(type)) { HashSet newNotInstanceof = ContainerUtil.newHashSet(myNotInstanceofValues); newNotInstanceof.remove(type); - return createCopy(myInstanceofValues, newNotInstanceof, myNullability); + return createCopy(myInstanceofValues, newNotInstanceof, myNullability, myOptionalPresence); } return this; } @@ -152,13 +157,16 @@ public class DfaVariableState { DfaVariableState aState = (DfaVariableState) obj; return myHash == aState.myHash && myNullability == aState.myNullability && + myOptionalPresence == aState.myOptionalPresence && myInstanceofValues.equals(aState.myInstanceofValues) && myNotInstanceofValues.equals(aState.myNotInstanceofValues); } @NotNull - protected DfaVariableState createCopy(@NotNull Set instanceofValues, @NotNull Set notInstanceofValues, @NotNull Nullness nullability) { - return new DfaVariableState(instanceofValues, notInstanceofValues, nullability); + protected DfaVariableState createCopy(@NotNull Set instanceofValues, + @NotNull Set notInstanceofValues, + @NotNull Nullness nullability, ThreeState optionalPresent) { + return new DfaVariableState(instanceofValues, notInstanceofValues, nullability, optionalPresent); } public String toString() { @@ -172,11 +180,15 @@ public class DfaVariableState { if (!myNotInstanceofValues.isEmpty()) { buf.append(" not instanceof ").append(StringUtil.join(myNotInstanceofValues, ",")); } + + if (myOptionalPresence != ThreeState.UNSURE) { + buf.append(myOptionalPresence == ThreeState.YES ? " Optional with value" : " empty Optional"); + } return buf.toString(); } @NotNull - protected Nullness getNullability() { + Nullness getNullability() { return myNullability; } @@ -186,7 +198,7 @@ public class DfaVariableState { @NotNull DfaVariableState withNullability(@NotNull Nullness nullness) { - return myNullability == nullness ? this : createCopy(myInstanceofValues, myNotInstanceofValues, nullness); + return myNullability == nullness ? this : createCopy(myInstanceofValues, myNotInstanceofValues, nullness, myOptionalPresence); } @NotNull @@ -194,6 +206,13 @@ public class DfaVariableState { return myNullability != Nullness.NOT_NULL ? withNullability(nullable ? Nullness.NULLABLE : Nullness.UNKNOWN) : this; } + DfaVariableState withOptionalPresense(final boolean presense) { + ThreeState optionalPresent = ThreeState.fromBoolean(presense); + return myOptionalPresence != optionalPresent + ? createCopy(myInstanceofValues, myNotInstanceofValues, myNullability, optionalPresent) + : this; + } + @NotNull public DfaVariableState withValue(DfaValue value) { return this; @@ -212,4 +231,7 @@ public class DfaVariableState { return myNotInstanceofValues; } + public ThreeState getOptionalPresense() { + return myOptionalPresence; + } } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullParameterConstraintChecker.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullParameterConstraintChecker.java index 344453335e00..00a28620bb0f 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullParameterConstraintChecker.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/NullParameterConstraintChecker.java @@ -29,6 +29,7 @@ import com.intellij.psi.PsiParameter; import com.intellij.psi.PsiPrimitiveType; import com.intellij.psi.impl.search.JavaNullMethodArgumentUtil; import com.intellij.util.SmartList; +import com.intellij.util.ThreeState; import gnu.trove.THashSet; import org.jetbrains.annotations.NotNull; @@ -128,9 +129,8 @@ class NullParameterConstraintChecker extends DataFlowRunner { protected MyDfaMemoryState(DfaValueFactory factory) { super(factory); for (PsiParameter parameter : myPossiblyViolatedParameters) { - setVariableState(getFactory().getVarFactory().createVariableValue(parameter, false), new DfaVariableState(Collections.emptySet(), - Collections.emptySet(), - Nullness.NULLABLE)); + setVariableState(getFactory().getVarFactory().createVariableValue(parameter, false), + new DfaVariableState(Collections.emptySet(), Collections.emptySet(), Nullness.NULLABLE, ThreeState.UNSURE)); } } 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 30fbafd4cd04..5bad45e3d83c 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 @@ -24,9 +24,12 @@ import com.intellij.psi.tree.IElementType; 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; +import com.siyeh.ig.psiutils.TypeUtils; import gnu.trove.THashSet; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -180,24 +183,28 @@ public class StandardInstructionVisitor extends InstructionVisitor { DfaValue[] argValues = popCallArguments(instruction, runner, memState); final DfaValue qualifier = popQualifier(instruction, runner, memState); - LinkedHashSet currentStates = ContainerUtil.newLinkedHashSet(memState); Set finalStates = ContainerUtil.newLinkedHashSet(); - if (argValues != null) { - for (MethodContract contract : instruction.getContracts()) { - currentStates = addContractResults(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"); + finalStates.addAll(handleOptionalMethods(instruction, runner, memState, qualifier, argValues)); + + if (finalStates.isEmpty()) { + LinkedHashSet currentStates = ContainerUtil.newLinkedHashSet(memState); + if (argValues != null) { + for (MethodContract contract : instruction.getContracts()) { + currentStates = addContractResults(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"); + } + finalStates.clear(); + currentStates = ContainerUtil.newLinkedHashSet(memState); + break; } - finalStates.clear(); - currentStates = ContainerUtil.newLinkedHashSet(memState); - break; } } - } - for (DfaMemoryState state : currentStates) { - state.push(getMethodResultValue(instruction, qualifier, runner.getFactory())); - finalStates.add(state); + for (DfaMemoryState state : currentStates) { + state.push(getMethodResultValue(instruction, qualifier, runner.getFactory())); + finalStates.add(state); + } } DfaInstructionState[] result = new DfaInstructionState[finalStates.size()]; @@ -211,6 +218,57 @@ public class StandardInstructionVisitor extends InstructionVisitor { return result; } + @NotNull + private static List handleOptionalMethods(MethodCallInstruction instruction, + DataFlowRunner runner, + DfaMemoryState memState, + DfaValue qualifierValue, @Nullable DfaValue[] argValues) { + PsiMethodCallExpression call = ObjectUtils.tryCast(instruction.getCallExpression(), PsiMethodCallExpression.class); + if (call == null) return Collections.emptyList(); + String methodName = call.getMethodExpression().getReferenceName(); + if ("isPresent".equals(methodName)) { + PsiMethod method = call.resolveMethod(); + if (method != null && TypeUtils.isOptional(method.getContainingClass())) { + ThreeState state = memState.checkOptional(qualifierValue); + DfaConstValue.Factory constFactory = runner.getFactory().getConstFactory(); + if (state == ThreeState.UNSURE) { + DfaMemoryState falseState = memState.createCopy(); + memState.push(constFactory.getTrue()); + memState.applyIsPresentCheck(true, qualifierValue); + falseState.push(constFactory.getFalse()); + falseState.applyIsPresentCheck(false, qualifierValue); + return Arrays.asList(memState, falseState); + } + else { + DfaValue result = state == ThreeState.YES ? constFactory.getTrue() : constFactory.getFalse(); + memState.push(result); + return Collections.singletonList(memState); + } + } + } + if ("of".equals(methodName)) { + PsiMethod method = call.resolveMethod(); + if (method != null && TypeUtils.isOptional(method.getContainingClass())) { + memState.push(runner.getFactory().getOptionalFactory().getOptional(true)); + return Collections.singletonList(memState); + } + } + if (DfaOptionalSupport.resolveOfNullable(call) != null) { + if (argValues != null && argValues.length == 1 && memState.isNotNull(argValues[0])) { + memState.push(runner.getFactory().getOptionalFactory().getOptional(true)); + return Collections.singletonList(memState); + } + } + if ("empty".equals(methodName) || "absent".equals(methodName)) { + PsiMethod method = call.resolveMethod(); + if (method != null && TypeUtils.isOptional(method.getContainingClass())) { + memState.push(runner.getFactory().getOptionalFactory().getOptional(false)); + return Collections.singletonList(memState); + } + } + return Collections.emptyList(); + } + @Nullable private DfaValue[] popCallArguments(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { final PsiExpression[] args = instruction.getArgs(); diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java index dd49478ac35e..addf17873a3c 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/ValuableDataFlowRunner.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2017 JetBrains s.r.o. + * Copyright 2000-2015 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. @@ -21,6 +21,7 @@ import com.intellij.codeInspection.dataFlow.value.DfaValue; import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.psi.PsiExpression; +import com.intellij.util.ThreeState; import com.intellij.util.containers.FList; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -30,7 +31,7 @@ import java.util.Set; /** * @author Gregory.Shrago */ -public class ValuableDataFlowRunner extends DataFlowRunner { +class ValuableDataFlowRunner extends DataFlowRunner { @NotNull @Override protected DfaMemoryState createMemoryState() { @@ -64,11 +65,11 @@ public class ValuableDataFlowRunner extends DataFlowRunner { } } - public static class ValuableDfaVariableState extends DfaVariableState { + static class ValuableDfaVariableState extends DfaVariableState { private final DfaValue myValue; @NotNull final FList myConcatenation; - public ValuableDfaVariableState(@NotNull DfaVariableValue psiVariable) { + private ValuableDfaVariableState(@NotNull DfaVariableValue psiVariable) { super(psiVariable); myValue = null; myConcatenation = FList.emptyList(); @@ -77,28 +78,33 @@ public class ValuableDataFlowRunner extends DataFlowRunner { private ValuableDfaVariableState(Set instanceofValues, Set notInstanceofValues, Nullness nullability, DfaValue value, - @NotNull FList concatenation) { - super(instanceofValues, notInstanceofValues, nullability); + @NotNull FList concatenation, ThreeState optionalPresence) { + super(instanceofValues, notInstanceofValues, nullability, optionalPresence); myValue = value; myConcatenation = concatenation; } @NotNull @Override - protected DfaVariableState createCopy(@NotNull Set instanceofValues, @NotNull Set notInstanceofValues, @NotNull Nullness nullability) { - return new ValuableDfaVariableState(instanceofValues, notInstanceofValues, nullability, myValue, myConcatenation); + protected DfaVariableState createCopy(@NotNull Set instanceofValues, + @NotNull Set notInstanceofValues, + @NotNull Nullness nullability, + ThreeState optionalPresence) { + return new ValuableDfaVariableState(instanceofValues, notInstanceofValues, nullability, myValue, myConcatenation, optionalPresence); } @NotNull @Override public DfaVariableState withValue(@Nullable final DfaValue value) { if (value == myValue) return this; - return new ValuableDfaVariableState(myInstanceofValues, myNotInstanceofValues, myNullability, value, myConcatenation); + return new ValuableDfaVariableState(myInstanceofValues, myNotInstanceofValues, myNullability, value, myConcatenation, + myOptionalPresence); } ValuableDfaVariableState withExpression(@NotNull final FList concatenation) { if (concatenation == myConcatenation) return this; - return new ValuableDfaVariableState(myInstanceofValues, myNotInstanceofValues, myNullability, myValue, concatenation); + return new ValuableDfaVariableState(myInstanceofValues, myNotInstanceofValues, myNullability, myValue, concatenation, + myOptionalPresence); } @Override diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaOptionalValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaOptionalValue.java new file mode 100644 index 000000000000..b327f4389e83 --- /dev/null +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaOptionalValue.java @@ -0,0 +1,46 @@ +/* + * 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. + * 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.value; + +public class DfaOptionalValue extends DfaValue { + final boolean myPresent; + + protected DfaOptionalValue(DfaValueFactory factory, boolean isPresent) { + super(factory); + myPresent = isPresent; + } + + public boolean isPresent() { + return myPresent; + } + + public String toString() { + return myPresent ? "Optional with value" : "Empty optional"; + } + + public static class Factory { + private final DfaOptionalValue myPresent, myAbsent; + + Factory(DfaValueFactory factory) { + myPresent = new DfaOptionalValue(factory, true); + myAbsent = new DfaOptionalValue(factory, false); + } + + public DfaOptionalValue getOptional(boolean present) { + return present ? myPresent : myAbsent; + } + } +} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java index c8e4d60f3d24..dc5ccaf45280 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaValueFactory.java @@ -59,6 +59,7 @@ public class DfaValueFactory { myTypeFactory = new DfaTypeValue.Factory(this); myRelationFactory = new DfaRelationValue.Factory(this); myExpressionFactory = new DfaExpressionFactory(this); + myOptionalFactory = new DfaOptionalValue.Factory(this); } public boolean isHonorFieldInitializers() { @@ -135,6 +136,7 @@ public class DfaValueFactory { private final DfaTypeValue.Factory myTypeFactory; private final DfaRelationValue.Factory myRelationFactory; private final DfaExpressionFactory myExpressionFactory; + private final DfaOptionalValue.Factory myOptionalFactory; @NotNull public DfaVariableValue.Factory getVarFactory() { @@ -159,4 +161,9 @@ public class DfaValueFactory { public DfaRelationValue.Factory getRelationFactory() { return myRelationFactory; } + + @NotNull + public DfaOptionalValue.Factory getOptionalFactory() { + return myOptionalFactory; + } } diff --git a/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java new file mode 100644 index 000000000000..2e38bcb7bc74 --- /dev/null +++ b/java/java-tests/testData/inspection/dataFlow/fixture/OptionalIsPresent.java @@ -0,0 +1,19 @@ +import java.util.Optional; + +class Test { + private void checkIsPresent(boolean b) { + Optional test; + if (b) { + test = Optional.of("x"); + } else { + test = Optional.empty(); + if(!test.isPresent()) { + System.out.println("Always"); + } + } + Optional other = test; + if(test.isPresent() && other.isPresent()) { + System.out.println(test.get()); + } + } +} diff --git a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspection8Test.java b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspection8Test.java index f4e0df8426a6..2dac136b94a3 100644 --- a/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspection8Test.java +++ b/java/java-tests/testSrc/com/intellij/codeInspection/DataFlowInspection8Test.java @@ -47,6 +47,7 @@ public class DataFlowInspection8Test extends DataFlowInspectionTestCase { public void testNullableForeachVariable() { doTestWithCustomAnnotations(); } public void testGenericParameterNullity() { doTestWithCustomAnnotations(); } public void testOptionalOfNullable() { doTest(); } + public void testOptionalIsPresent() { doTest(); } public void testPrimitiveInVoidLambda() { doTest(); } public void testNotNullLambdaParameter() { doTest(); } public void testNotNullOptionalLambdaParameter() { doTest(); } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java index 3f73df8e12ef..b94cc58c970d 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java @@ -17,9 +17,6 @@ package com.siyeh.ig.bugs; import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; -import com.intellij.codeInspection.dataFlow.value.DfaValue; -import com.intellij.codeInspection.dataFlow.value.DfaValueFactory; -import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.psi.*; import com.intellij.psi.util.PsiTreeUtil; import com.intellij.util.ObjectUtils; @@ -92,16 +89,7 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { } private void analyze(PsiElement context) { - final DataFlowRunner dfaRunner = new StandardDataFlowRunner(false, true, isOnTheFly()) { - private final DfaOptionalValue myPresentValue = new DfaOptionalValue(getFactory(), true); - private final DfaOptionalValue myAbsentValue = new DfaOptionalValue(getFactory(), false); - - @NotNull - @Override - protected DfaMemoryState createMemoryState() { - return new OptionalMemoryState(getFactory(), myPresentValue, myAbsentValue); - } - }; + final DataFlowRunner dfaRunner = new StandardDataFlowRunner(false, true, isOnTheFly()); dfaRunner.analyzeMethod(context, new StandardInstructionVisitor() { @Override public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { @@ -109,66 +97,13 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { if (call != null) { String methodName = call.getMethodExpression().getReferenceName(); PsiExpression qualifier = call.getMethodExpression().getQualifierExpression(); - OptionalMemoryState optionalMemState = (OptionalMemoryState)memState; - if (qualifier != null && TypeUtils.isOptional(qualifier.getType())) { - if ("isPresent".equals(methodName)) { - DfaValue qualifierValue = optionalMemState.peek(); - ThreeState state = optionalMemState.checkOptional(qualifierValue); - if(state == ThreeState.UNSURE) { - DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); - if(states.length != 1) return states; - OptionalMemoryState trueState = (OptionalMemoryState)states[0].getMemoryState(); - trueState.pop(); - OptionalMemoryState falseState = trueState.createCopy(); - trueState.push(optionalMemState.getBoolean(true)); - trueState.applyIsPresentCheck(true, qualifierValue); - falseState.push(optionalMemState.getBoolean(false)); - falseState.applyIsPresentCheck(false, qualifierValue); - return new DfaInstructionState[] {states[0], new DfaInstructionState(states[0].getInstruction(), falseState)}; - } else { - return replaceResult(instruction, runner, memState, optionalMemState.getBoolean(state.toBoolean())); - } - } - else if (isOptionalGetMethodName(methodName)) { - ThreeState state = optionalMemState.checkOptional(memState.peek()); - seen.merge(call, state, (s1, s2) -> s1 == s2 ? s1 : ThreeState.UNSURE); - } - } - if ("of".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && TypeUtils.isOptional(method.getContainingClass())) { - return replaceResult(instruction, runner, memState, optionalMemState.getOptional(true)); - } - } - if ("ofNullable".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && - TypeUtils.isOptional(method.getContainingClass()) && - call.getArgumentList().getExpressions().length == 1) { - if(memState.isNotNull(memState.peek())) { - return replaceResult(instruction, runner, memState, optionalMemState.getOptional(true)); - } - } - } - if ("empty".equals(methodName)) { - PsiMethod method = call.resolveMethod(); - if (method != null && TypeUtils.isOptional(method.getContainingClass())) { - return replaceResult(instruction, runner, memState, optionalMemState.getOptional(false)); - } + if (qualifier != null && TypeUtils.isOptional(qualifier.getType()) && isOptionalGetMethodName(methodName)) { + ThreeState state = memState.checkOptional(memState.peek()); + seen.merge(call, state, (s1, s2) -> s1 == s2 ? s1 : ThreeState.UNSURE); } } return super.visitMethodCall(instruction, runner, memState); } - - private DfaInstructionState[] replaceResult(MethodCallInstruction instruction, - DataFlowRunner runner, - DfaMemoryState memState, - DfaValue result) { - DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); - memState.pop(); - memState.push(result); - return states; - } }); } @@ -190,76 +125,5 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { private static boolean isOptionalGetMethodName(String name) { return "get".equals(name) || "getAsDouble".equals(name) || "getAsInt".equals(name) || "getAsLong".equals(name); } - - static class OptionalMemoryState extends DfaMemoryStateImpl { - private final DfaOptionalValue myPresentValue; - private final DfaOptionalValue myAbsentValue; - - public OptionalMemoryState(DfaValueFactory factory, - DfaOptionalValue presentValue, - DfaOptionalValue absentValue) { - super(factory); - myPresentValue = presentValue; - myAbsentValue = absentValue; - } - - public OptionalMemoryState(OptionalMemoryState state) { - super(state); - myPresentValue = state.myPresentValue; - myAbsentValue = state.myAbsentValue; - } - - public DfaValue getBoolean(boolean val) { - return val ? getFactory().getConstFactory().getTrue() : getFactory().getConstFactory().getFalse(); - } - - public DfaValue getOptional(boolean present) { - return present ? myPresentValue : myAbsentValue; - } - - @NotNull - @Override - protected DfaVariableState createVariableState(@NotNull DfaVariableValue var) { - return new ValuableDataFlowRunner.ValuableDfaVariableState(var); - } - - public ThreeState checkOptional(DfaValue value) { - if (value instanceof DfaVariableValue) { - DfaVariableValue var = (DfaVariableValue)value; - DfaValue newCond = getVariableState(var).getValue(); - if (newCond == null) { - return checkOptional(getVariableState(var.createNegated()).getValue()); - } - return checkOptional(newCond); - } - return value instanceof DfaOptionalValue ? ThreeState.fromBoolean(((DfaOptionalValue)value).myPresent) : ThreeState.UNSURE; - } - - void applyIsPresentCheck(boolean present, DfaValue qualifier) { - if (qualifier instanceof DfaVariableValue) { - DfaVariableValue optionalVar = (DfaVariableValue)qualifier; - setVariableState(optionalVar, getVariableState(optionalVar).withValue(getOptional(present))); - } - } - - @NotNull - @Override - public OptionalMemoryState createCopy() { - return new OptionalMemoryState(this); - } - } - } - - static class DfaOptionalValue extends DfaValue { - final boolean myPresent; - - protected DfaOptionalValue(DfaValueFactory factory, boolean isPresent) { - super(factory); - myPresent = isPresent; - } - - public String toString() { - return myPresent ? "Optional with value" : "Empty optional"; - } } }