From 123e339952300972bae9d81936aeb56808d319bf Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 10 Feb 2017 11:54:32 +0700 Subject: [PATCH] OptionalGetWithoutIsPresentInspection: removed IsPresentCheck (instead used two instruction states) (IDEA-CR-18052) --- .../dataFlow/value/DfaBoxedValue.java | 4 +- .../dataFlow/value/DfaComparableValue.java | 24 --- .../dataFlow/value/DfaRelationValue.java | 7 +- .../dataFlow/value/DfaUnboxedValue.java | 4 +- .../dataFlow/value/DfaVariableValue.java | 2 +- ...OptionalGetWithoutIsPresentInspection.java | 193 +++++------------- .../OptionalGetWithoutIsPresent.java | 10 +- 7 files changed, 67 insertions(+), 177 deletions(-) delete mode 100644 java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaComparableValue.java diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaBoxedValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaBoxedValue.java index e851f6817e88..efb2df995c48 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaBoxedValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaBoxedValue.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2017 JetBrains s.r.o. + * Copyright 2000-2009 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. @@ -23,7 +23,7 @@ import org.jetbrains.annotations.Nullable; import java.util.Map; -public class DfaBoxedValue extends DfaValue implements DfaComparableValue { +public class DfaBoxedValue extends DfaValue { private final DfaValue myWrappedValue; private DfaBoxedValue(DfaValue valueToWrap, DfaValueFactory factory) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaComparableValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaComparableValue.java deleted file mode 100644 index 75513aaf42a0..000000000000 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaComparableValue.java +++ /dev/null @@ -1,24 +0,0 @@ -/* - * 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; - -/** - * Marker interface for automatic relation creation - * - * @author Tagir Valeev - */ -public interface DfaComparableValue { -} diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaRelationValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaRelationValue.java index 47c571016cbb..cb2d4f1689ce 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaRelationValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaRelationValue.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2017 JetBrains s.r.o. + * Copyright 2000-2009 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. @@ -54,8 +54,9 @@ public class DfaRelationValue extends DfaValue { public DfaRelationValue createRelation(DfaValue dfaLeft, DfaValue dfaRight, IElementType relation, boolean negated) { if (PLUS == relation) return null; - if (dfaLeft instanceof DfaComparableValue || dfaRight instanceof DfaComparableValue) { - if (!(dfaLeft instanceof DfaComparableValue)) { + if (dfaLeft instanceof DfaVariableValue || dfaLeft instanceof DfaBoxedValue || dfaLeft instanceof DfaUnboxedValue + || dfaRight instanceof DfaVariableValue || dfaRight instanceof DfaBoxedValue || dfaRight instanceof DfaUnboxedValue) { + if (!(dfaLeft instanceof DfaVariableValue || dfaLeft instanceof DfaBoxedValue || dfaLeft instanceof DfaUnboxedValue)) { return createRelation(dfaRight, dfaLeft, getSymmetricOperation(relation), negated); } diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaUnboxedValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaUnboxedValue.java index 99bcbafba274..032272a687bd 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaUnboxedValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaUnboxedValue.java @@ -1,5 +1,5 @@ /* - * Copyright 2000-2017 JetBrains s.r.o. + * Copyright 2000-2009 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. @@ -17,7 +17,7 @@ package com.intellij.codeInspection.dataFlow.value; import org.jetbrains.annotations.NonNls; -public class DfaUnboxedValue extends DfaValue implements DfaComparableValue { +public class DfaUnboxedValue extends DfaValue { private final DfaVariableValue myVariable; DfaUnboxedValue(DfaVariableValue valueToWrap, DfaValueFactory factory) { diff --git a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java index 94443781367d..f8265fdaabbd 100644 --- a/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java +++ b/java/java-analysis-impl/src/com/intellij/codeInspection/dataFlow/value/DfaVariableValue.java @@ -46,7 +46,7 @@ import java.util.List; import static com.intellij.patterns.PsiJavaPatterns.*; -public class DfaVariableValue extends DfaValue implements DfaComparableValue { +public class DfaVariableValue extends DfaValue { private static final ElementPattern MEMBER_OR_METHOD_PARAMETER = or(psiMember(), psiParameter().withSuperParent(2, psiMember())); 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 888e3bc612e1..3f73df8e12ef 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java @@ -17,7 +17,9 @@ package com.siyeh.ig.bugs; import com.intellij.codeInspection.dataFlow.*; import com.intellij.codeInspection.dataFlow.instructions.MethodCallInstruction; -import com.intellij.codeInspection.dataFlow.value.*; +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; @@ -27,7 +29,6 @@ import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; import com.siyeh.ig.psiutils.ParenthesesUtils; import com.siyeh.ig.psiutils.TypeUtils; -import gnu.trove.TIntObjectHashMap; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -92,12 +93,13 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { private void analyze(PsiElement context) { final DataFlowRunner dfaRunner = new StandardDataFlowRunner(false, true, isOnTheFly()) { - private final OptionalValueFactory myOptionalFactory = new OptionalValueFactory(getFactory()); + 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(), myOptionalFactory); + return new OptionalMemoryState(getFactory(), myPresentValue, myAbsentValue); } }; dfaRunner.analyzeMethod(context, new StandardInstructionVisitor() { @@ -110,8 +112,22 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { OptionalMemoryState optionalMemState = (OptionalMemoryState)memState; if (qualifier != null && TypeUtils.isOptional(qualifier.getType())) { if ("isPresent".equals(methodName)) { - DfaValue result = optionalMemState.createIsPresentCheckResult(memState.peek()); - return replaceResult(instruction, runner, memState, result); + 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()); @@ -121,7 +137,7 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { if ("of".equals(methodName)) { PsiMethod method = call.resolveMethod(); if (method != null && TypeUtils.isOptional(method.getContainingClass())) { - return replaceResult(instruction, runner, memState, optionalMemState.getOptionalFactory().getOptional(true)); + return replaceResult(instruction, runner, memState, optionalMemState.getOptional(true)); } } if ("ofNullable".equals(methodName)) { @@ -130,14 +146,14 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { TypeUtils.isOptional(method.getContainingClass()) && call.getArgumentList().getExpressions().length == 1) { if(memState.isNotNull(memState.peek())) { - return replaceResult(instruction, runner, memState, optionalMemState.getOptionalFactory().getOptional(true)); + 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.getOptionalFactory().getOptional(false)); + return replaceResult(instruction, runner, memState, optionalMemState.getOptional(false)); } } } @@ -175,169 +191,62 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { return "get".equals(name) || "getAsDouble".equals(name) || "getAsInt".equals(name) || "getAsLong".equals(name); } - static class OptionalValueFactory { - private final DfaOptionalValue myPresentOptional, myAbsentOptional; - private final DfaValueFactory myFactory; - private final TIntObjectHashMap myPresentChecks = new TIntObjectHashMap<>(); - - OptionalValueFactory(DfaValueFactory factory) { - myPresentOptional = new DfaOptionalValue(factory, true); - myAbsentOptional = new DfaOptionalValue(factory, false); - myFactory = factory; - } - - IsPresentCheck getIsPresentCheck(DfaValue optional) { - int id = optional.getID(); - IsPresentCheck check = myPresentChecks.get(id); - if(check == null) { - myPresentChecks.put(id, check = new IsPresentCheck(myFactory, optional, null)); - } - return check; - } - - DfaOptionalValue getOptional(boolean present) { - return present ? myPresentOptional : myAbsentOptional; - } - } - static class OptionalMemoryState extends DfaMemoryStateImpl { - private final OptionalValueFactory myOptionalFactory; + private final DfaOptionalValue myPresentValue; + private final DfaOptionalValue myAbsentValue; - protected OptionalMemoryState(DfaValueFactory factory, - OptionalValueFactory optionalFactory) { + public OptionalMemoryState(DfaValueFactory factory, + DfaOptionalValue presentValue, + DfaOptionalValue absentValue) { super(factory); - myOptionalFactory = optionalFactory; + myPresentValue = presentValue; + myAbsentValue = absentValue; } - protected OptionalMemoryState(OptionalMemoryState toCopy) { - super(toCopy); - myOptionalFactory = toCopy.myOptionalFactory; + public OptionalMemoryState(OptionalMemoryState state) { + super(state); + myPresentValue = state.myPresentValue; + myAbsentValue = state.myAbsentValue; } - public OptionalValueFactory getOptionalFactory() { - return myOptionalFactory; + public DfaValue getBoolean(boolean val) { + return val ? getFactory().getConstFactory().getTrue() : getFactory().getConstFactory().getFalse(); } - @Override - public boolean applyCondition(final DfaValue dfaCond) { - if (dfaCond instanceof DfaRelationValue) { - DfaRelationValue relation = (DfaRelationValue)dfaCond; - if (relation.isEquality() || relation.isNonEquality()) { - DfaValue left = relation.getLeftOperand(); - DfaValue right = relation.getRightOperand(); - DfaConstValue constValue = null; - DfaValue nonConst = null; - if (left instanceof DfaConstValue) { - constValue = (DfaConstValue)left; - nonConst = right; - } - else if (right instanceof DfaConstValue) { - constValue = (DfaConstValue)right; - nonConst = left; - } - if (constValue != null) { - IsPresentCheck check = unwrapValue(nonConst, IsPresentCheck.class); - Object value = constValue.getValue(); - if (value instanceof Boolean && check != null) { - boolean present = ((Boolean)value).booleanValue() ^ relation.isNonEquality(); - applyIsPresentCheck(present ? check : check.createNegated()); - } - } - } - } - IsPresentCheck check = unwrapValue(dfaCond, IsPresentCheck.class); - if (check != null) { - return applyIsPresentCheck(check); - } - return super.applyCondition(dfaCond); - } - - @NotNull - DfaValue createIsPresentCheckResult(DfaValue qualifierValue) { - ThreeState state = checkOptional(qualifierValue); - switch (state) { - case YES: - return getFactory().getConstFactory().getTrue(); - case NO: - return getFactory().getConstFactory().getFalse(); - case UNSURE: - return myOptionalFactory.getIsPresentCheck(qualifierValue); - } - throw new IllegalStateException(); + public DfaValue getOptional(boolean present) { + return present ? myPresentValue : myAbsentValue; } @NotNull @Override protected DfaVariableState createVariableState(@NotNull DfaVariableValue var) { - if (var.isNegated()) { - DfaVariableState negatedState = getVariableState(var.createNegated()); - DfaValue negatedValue = negatedState.getValue(); - if (negatedValue != null) { - return negatedState.withValue(negatedValue.createNegated()); - } - } return new ValuableDataFlowRunner.ValuableDfaVariableState(var); } - private T unwrapValue(DfaValue dfaCond, Class aClass) { - if (dfaCond instanceof DfaVariableValue) { - DfaVariableValue var = (DfaVariableValue)dfaCond; + public ThreeState checkOptional(DfaValue value) { + if (value instanceof DfaVariableValue) { + DfaVariableValue var = (DfaVariableValue)value; DfaValue newCond = getVariableState(var).getValue(); if (newCond == null) { - newCond = getVariableState(var.createNegated()).getValue(); - T check = unwrapValue(newCond, aClass); - return check == null ? null : ObjectUtils.tryCast(check.createNegated(), aClass); + return checkOptional(getVariableState(var.createNegated()).getValue()); } - return unwrapValue(newCond, aClass); + return checkOptional(newCond); } - return ObjectUtils.tryCast(dfaCond, aClass); + return value instanceof DfaOptionalValue ? ThreeState.fromBoolean(((DfaOptionalValue)value).myPresent) : ThreeState.UNSURE; } - private boolean applyIsPresentCheck(IsPresentCheck check) { - DfaOptionalValue optional = unwrapValue(check.myOptional, DfaOptionalValue.class); - if (optional != null) { - return optional.myPresent == check.myNegated; + void applyIsPresentCheck(boolean present, DfaValue qualifier) { + if (qualifier instanceof DfaVariableValue) { + DfaVariableValue optionalVar = (DfaVariableValue)qualifier; + setVariableState(optionalVar, getVariableState(optionalVar).withValue(getOptional(present))); } - if (check.myOptional instanceof DfaVariableValue) { - DfaVariableValue optionalVar = (DfaVariableValue)check.myOptional; - setVariableState(optionalVar, getVariableState(optionalVar) - .withValue(myOptionalFactory.getOptional(!check.myNegated))); - } - return true; } @NotNull @Override - public DfaMemoryStateImpl createCopy() { + public OptionalMemoryState createCopy() { return new OptionalMemoryState(this); } - - public ThreeState checkOptional(DfaValue value) { - DfaOptionalValue optional = unwrapValue(value, DfaOptionalValue.class); - return optional == null ? ThreeState.UNSURE : ThreeState.fromBoolean(optional.myPresent); - } - } - } - - static class IsPresentCheck extends DfaValue implements DfaComparableValue { - final @NotNull DfaValue myOptional; - final boolean myNegated; - final IsPresentCheck myInverted; - - protected IsPresentCheck(DfaValueFactory factory, @NotNull DfaValue optional, IsPresentCheck positiveCheck) { - super(factory); - myOptional = optional; - myNegated = positiveCheck != null; - myInverted = positiveCheck == null ? new IsPresentCheck(factory, optional, this) : positiveCheck; - } - - @Override - public IsPresentCheck createNegated() { - return myInverted; - } - - public String toString() { - return (myNegated ? "!" : "") + myOptional + ".isPresent()"; } } diff --git a/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/optional_get_without_is_present/OptionalGetWithoutIsPresent.java b/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/optional_get_without_is_present/OptionalGetWithoutIsPresent.java index cfd085d9c94a..f4f8c2610bc7 100644 --- a/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/optional_get_without_is_present/OptionalGetWithoutIsPresent.java +++ b/plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/optional_get_without_is_present/OptionalGetWithoutIsPresent.java @@ -84,15 +84,19 @@ class OptionalWithoutIsPresent { } private void checkAsserts1() { - Optional o1 = Optional.empty(); + Optional o1 = getOptional(); assert o1.isPresent(); System.out.println(o1.get()); - Optional o2 = Optional.empty(); + Optional o2 = getOptional(); org.junit.Assert.assertTrue(o2.isPresent()); System.out.println(o2.get()); - Optional o3 = Optional.empty(); + Optional o3 = getOptional(); org.testng.Assert.assertTrue(o3.isPresent()); System.out.println(o3.get()); + + o2 = getOptional(); + org.junit.Assert.assertTrue(!o2.isPresent()); + System.out.println(o2.get()); } private void checkAsserts2() {