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 dc9c9b15194b..95195332aaf7 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java @@ -15,14 +15,16 @@ */ package com.siyeh.ig.bugs; +import com.intellij.codeInspection.dataFlow.*; +import com.intellij.codeInspection.dataFlow.instructions.*; +import com.intellij.codeInspection.dataFlow.value.DfaValue; +import com.intellij.codeInspection.dataFlow.value.DfaVariableValue; import com.intellij.psi.*; -import com.intellij.psi.tree.IElementType; -import com.intellij.psi.util.PsiTreeUtil; -import com.intellij.psi.util.PsiUtil; import com.siyeh.InspectionGadgetsBundle; import com.siyeh.ig.BaseInspection; import com.siyeh.ig.BaseInspectionVisitor; -import com.siyeh.ig.psiutils.*; +import com.siyeh.ig.psiutils.ParenthesesUtils; +import com.siyeh.ig.psiutils.TypeUtils; import org.jetbrains.annotations.Nls; import org.jetbrains.annotations.NotNull; @@ -40,9 +42,7 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { @NotNull @Override protected String buildErrorString(Object... infos) { - final PsiType type = (PsiType)infos[0]; - final PsiClass aClass = PsiUtil.resolveClassInClassTypeOnly(type); - assert aClass != null; + final PsiClass aClass = (PsiClass)infos[0]; return InspectionGadgetsBundle.message("optional.get.without.is.present.problem.descriptor", aClass.getName()); } @@ -54,221 +54,102 @@ public class OptionalGetWithoutIsPresentInspection extends BaseInspection { private static class OptionalGetWithoutIsPresentVisitor extends BaseInspectionVisitor { @Override - public void visitMethodCallExpression(PsiMethodCallExpression expression) { - super.visitMethodCallExpression(expression); - final PsiReferenceExpression methodExpression = expression.getMethodExpression(); - final String name = methodExpression.getReferenceName(); - if (!"get".equals(name) && !"getAsDouble".equals(name) && !"getAsInt".equals(name) && !"getAsLong".equals(name)) { - return; - } - final PsiExpression qualifier = ParenthesesUtils.stripParentheses(methodExpression.getQualifierExpression()); - if (qualifier == null) { - return; - } - final PsiType type = qualifier.getType(); - if (!TypeUtils.isOptional(type)) { - return; - } - if (qualifier instanceof PsiReferenceExpression) { - final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)qualifier; - if (isSurroundedByIsPresentGuard(referenceExpression, expression)) { - return; - } - } - registerMethodCallError(expression, type); + public void visitMethod(PsiMethod method) { + check(method.getBody()); } - } - private static boolean isSurroundedByIsPresentGuard(PsiReferenceExpression optionalReference, PsiElement context) { - PsiStatement sibling = PsiTreeUtil.getParentOfType(context, PsiStatement.class); - sibling = PsiTreeUtil.getPrevSiblingOfType(sibling, PsiStatement.class); - final IsPresentChecker checker = new IsPresentChecker(optionalReference); - while (sibling != null) { - if (sibling instanceof PsiIfStatement) { - final PsiIfStatement ifStatement = (PsiIfStatement)sibling; - final PsiExpression condition = ifStatement.getCondition(); - if (condition != null) { - final PsiElement target = optionalReference.resolve(); - if (!(target instanceof PsiVariable)) { - return true; - } - final PsiVariable variable = (PsiVariable)target; - final PsiStatement thenBranch = ifStatement.getThenBranch(); - if (!ControlFlowUtils.statementMayCompleteNormally(thenBranch) || VariableAccessUtils.variableIsAssigned(variable, thenBranch)) { - checker.negate = true; - if (checker.checkExpression(condition)) { - return true; - } - } - final PsiStatement elseBranch = ifStatement.getElseBranch(); - if (!ControlFlowUtils.statementMayCompleteNormally(elseBranch) || VariableAccessUtils.variableIsAssigned(variable, elseBranch)) { - checker.negate = false; - if (checker.checkExpression(condition)) { - return true; + @Override + public void visitClassInitializer(PsiClassInitializer initializer) { + check(initializer.getBody()); + } + + @Override + public void visitField(PsiField field) { + check(field.getInitializer()); + } + + private void check(PsiElement element) { + if (!containsOptionalGetCall(element)) { + return; + } + final StandardDataFlowRunner dfaRunner = new StandardDataFlowRunner(false, true, isOnTheFly()); + dfaRunner.analyzeMethod(element, new InstructionVisitor() { + @Override + public DfaInstructionState[] visitAssign(AssignInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + DfaValue dfaSource = memState.pop(); + DfaValue dfaDest = memState.pop(); + if (dfaDest instanceof DfaVariableValue) { + DfaVariableValue var = (DfaVariableValue)dfaDest; + final PsiModifierListOwner psi = var.getPsiVariable(); + if (!(psi instanceof PsiField) || !psi.hasModifierProperty(PsiModifier.VOLATILE)) { + memState.setVarValue(var, dfaSource); } } + memState.push(dfaDest); + return nextInstruction(instruction, runner, memState); } - } - else if (sibling instanceof PsiWhileStatement) { - final PsiWhileStatement whileStatement = (PsiWhileStatement)sibling; - final PsiExpression condition = whileStatement.getCondition(); - checker.negate = true; - if (checker.checkExpression(condition)) { - return true; - } - } - else if (sibling instanceof PsiAssertStatement) { - final PsiAssertStatement assertStatement = (PsiAssertStatement)sibling; - final PsiExpression condition = assertStatement.getAssertCondition(); - checker.negate = false; - if (checker.checkExpression(condition)) { - return true; - } - } - else if (sibling instanceof PsiExpressionStatement) { - final PsiExpressionStatement expressionStatement = (PsiExpressionStatement)sibling; - final PsiExpression expression = expressionStatement.getExpression(); - if (expression instanceof PsiMethodCallExpression) { - final PsiMethodCallExpression methodCallExpression = (PsiMethodCallExpression)expression; - if (MethodCallUtils.isCallToMethod(methodCallExpression, "org.junit.Assert", PsiType.VOID, "assertTrue", null) || - MethodCallUtils.isCallToMethod(methodCallExpression, "junit.framework.Assert", PsiType.VOID, "assertTrue", null) || - MethodCallUtils.isCallToMethod(methodCallExpression, "org.testng.Assert", PsiType.VOID, "assertTrue", null) || - MethodCallUtils.isCallToMethod(methodCallExpression, "org.testng.AssertJUnit", PsiType.VOID, "assertTrue", null)) { - checker.negate = false; - final PsiExpressionList argumentList = methodCallExpression.getArgumentList(); - final PsiExpression[] arguments = argumentList.getExpressions(); - for (PsiExpression argument : arguments) { - if (checker.checkExpression(argument)) { - return true; + + @Override + public DfaInstructionState[] visitMethodCall(MethodCallInstruction instruction, DataFlowRunner runner, DfaMemoryState memState) { + final DfaInstructionState[] states = super.visitMethodCall(instruction, runner, memState); + + final PsiMethod targetMethod = instruction.getTargetMethod(); + if (targetMethod != null) { + final PsiClass aClass = targetMethod.getContainingClass(); + if (TypeUtils.isOptional(aClass)) { + final String name = targetMethod.getName(); + if (name.equals("isPresent")) { + memState.pop(); + memState.push(runner.getFactory().getConstFactory().getFalse()); + } + else if (name.equals("get") || name.equals("getAsDouble") || name.equals("getAsInt") || name.equals("getAsLong")) { + final PsiMethodCallExpression methodCallExpression = (PsiMethodCallExpression)instruction.getCallExpression(); + if (methodCallExpression != null) { + registerMethodCallError(methodCallExpression, aClass); + } } } } + return states; } - } - sibling = PsiTreeUtil.getPrevSiblingOfType(sibling, PsiStatement.class); - } - checker.negate = false; - PsiElement parent = PsiTreeUtil.getParentOfType(context, PsiIfStatement.class, PsiWhileStatement.class, PsiConditionalExpression.class, - PsiPolyadicExpression.class); - while (parent != null) { - if (parent instanceof PsiPolyadicExpression) { - final PsiPolyadicExpression polyadicExpression = (PsiPolyadicExpression)parent; - if (JavaTokenType.OROR.equals(polyadicExpression.getOperationTokenType())) { - checker.negate = true; - } - } - parent.accept(checker); - if (checker.hasIsPresentCall()) { - return true; - } - parent = PsiTreeUtil.getParentOfType(parent, PsiIfStatement.class, PsiWhileStatement.class, PsiConditionalExpression.class, - PsiPolyadicExpression.class); - } - return checker.hasIsPresentCall(); - } - - private static class IsPresentChecker extends JavaElementVisitor { - - private final PsiReferenceExpression referenceExpression; - private boolean negate = false; - private boolean hasIsPresentCall = false; - - - IsPresentChecker(PsiReferenceExpression referenceExpression) { - this.referenceExpression = referenceExpression; + }); } - @Override - public void visitReferenceExpression(PsiReferenceExpression expression) { - visitExpression(expression); + private static boolean containsOptionalGetCall(PsiElement element) { + if (element == null) return false; + final OptionalGetCallChecker checker = new OptionalGetCallChecker(); + element.acceptChildren(checker); + return checker.containsOptionalGetCall(); } - @Override - public void visitPolyadicExpression(PsiPolyadicExpression expression) { - final IElementType tokenType = expression.getOperationTokenType(); - if (tokenType != JavaTokenType.ANDAND && tokenType != JavaTokenType.OROR) { - return; - } - for (PsiExpression operand : expression.getOperands()) { - if (PsiTreeUtil.isAncestor(operand, referenceExpression, false)) { + private static class OptionalGetCallChecker extends JavaRecursiveElementWalkingVisitor { + private boolean result = false; + + @Override + public void visitMethodCallExpression(PsiMethodCallExpression expression) { + if (result) { return; } - checkExpression(operand); - if (hasIsPresentCall) { - return; - } - } - } - - @Override - public void visitWhileStatement(PsiWhileStatement statement) { - checkExpression(statement.getCondition()); - } - - @Override - public void visitIfStatement(PsiIfStatement ifStatement) { - final PsiStatement elseBranch = ifStatement.getElseBranch(); - negate = elseBranch != null && PsiTreeUtil.isAncestor(elseBranch, referenceExpression, true); - final PsiStatement statement = negate ? elseBranch : ifStatement.getThenBranch(); - if (statement instanceof PsiBlockStatement) { - final PsiBlockStatement blockStatement = (PsiBlockStatement)statement; - if (VariableAccessUtils.variableIsAssignedBeforeReference(referenceExpression, blockStatement)) { - return; - } - } - checkExpression(ifStatement.getCondition()); - } - - @Override - public void visitConditionalExpression(PsiConditionalExpression expression) { - final PsiExpression elseExpression = expression.getElseExpression(); - negate = elseExpression != null && PsiTreeUtil.isAncestor(elseExpression, referenceExpression, true); - checkExpression(expression.getCondition()); - } - - private boolean checkExpression(PsiExpression expression) { - expression = PsiUtil.deparenthesizeExpression(expression); - if (expression instanceof PsiPrefixExpression) { - final PsiPrefixExpression prefixExpression = (PsiPrefixExpression)expression; - final IElementType tokenType = prefixExpression.getOperationTokenType(); - if (tokenType != JavaTokenType.EXCL) { - return false; - } - negate = !negate; - return checkExpression(prefixExpression.getOperand()); - } - else if (expression instanceof PsiPolyadicExpression) { - final PsiPolyadicExpression binaryExpression = (PsiPolyadicExpression)expression; - visitPolyadicExpression(binaryExpression); - } - else if (expression instanceof PsiMethodCallExpression) { - final PsiMethodCallExpression methodCallExpression = (PsiMethodCallExpression)expression; - final PsiReferenceExpression methodExpression = methodCallExpression.getMethodExpression(); + super.visitMethodCallExpression(expression); + final PsiReferenceExpression methodExpression = expression.getMethodExpression(); final String name = methodExpression.getReferenceName(); - if (!"isPresent".equals(name)) { - return false; + if (!"get".equals(name) && !"getAsDouble".equals(name) && !"getAsInt".equals(name) && !"getAsLong".equals(name)) { + return; } final PsiExpression qualifier = ParenthesesUtils.stripParentheses(methodExpression.getQualifierExpression()); - if (!(qualifier instanceof PsiReferenceExpression)) { - return false; + if (qualifier == null) { + return; } - final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)qualifier; - hasIsPresentCall = !negate && EquivalenceChecker.getCanonicalPsiEquivalence().expressionsAreEquivalent(referenceExpression, this.referenceExpression); - } - else if (expression instanceof PsiReferenceExpression) { - final PsiReferenceExpression referenceExpression = (PsiReferenceExpression)expression; - final PsiExpression definition = DeclarationSearchUtils.findDefinition(referenceExpression, null); - final PsiExpression optionalDefinition = DeclarationSearchUtils.findDefinition(this.referenceExpression, null); - if (definition == null || optionalDefinition == null || optionalDefinition.getTextOffset() > definition.getTextOffset()) { - return false; + final PsiType type = qualifier.getType(); + if (!TypeUtils.isOptional(type)) { + return; } - return checkExpression(definition); + result = true; } - return hasIsPresentCall; - } - public boolean hasIsPresentCall() { - return hasIsPresentCall; + public boolean containsOptionalGetCall() { + return result; + } } } } diff --git a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/TypeUtils.java b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/TypeUtils.java index eeb18b99eb87..7e9dcd7007c1 100644 --- a/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/TypeUtils.java +++ b/plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/TypeUtils.java @@ -85,7 +85,10 @@ public class TypeUtils { } public static boolean isOptional(@Nullable PsiType type) { - final PsiClass aClass = PsiUtil.resolveClassInClassTypeOnly(type); + return isOptional(PsiUtil.resolveClassInClassTypeOnly(type)); + } + + public static boolean isOptional(PsiClass aClass) { if (aClass == null) { return false; } 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 0c1e79aac2b9..caf7d267fc1c 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 @@ -12,7 +12,7 @@ class OptionalWithoutIsPresent { } if (maybe.isPresent()) { maybe = Optional.empty(); - System.out.println(maybe.get()); + System.out.println(maybe.get()); } boolean b = ((maybe.isPresent())) && maybe.get() == 1; boolean c = (!maybe.isPresent()) || maybe.get() == 1; @@ -34,7 +34,7 @@ class OptionalWithoutIsPresent { final boolean present = optional.isPresent(); optional = Optional.empty(); if (present) { - final String string = optional.get(); + final String string = optional.get(); System.out.println(string); } } @@ -56,8 +56,119 @@ class OptionalWithoutIsPresent { if (! holder.isPresent()) { holder = Optional.of("hello world"); + if (!holder.isPresent()) { + return null; + } } return holder.get(); } + + //public Collection findCategories(Long articleId) { + // return java.util.stream.Stream.of(Optional.of("asdf")).filter(Optional::isPresent).map(x -> x.get()).collect( + // java.util.stream.Collectors.toList()) ; + //} + + public static void main(String[] args) { + Optional stringOpt; + + if((stringOpt = getOptional()).isPresent()) { + stringOpt.get(); + } + } + + public static void main2(String[] args) { + if(getOptional().isPresent()) { + getOptional().get(); //'Optional.get()' without 'isPresent()' check + } + } + + public static Optional getOptional() { + return Optional.empty(); + } + + //void order(Optional order) { + // order.ifPresent(o -> System.out.println(order.get())); + //} + + public static void two(Optional o1,Optional o2) { + if (!o1.isPresent() && !o2.isPresent()) { + return; + } + System.out.println(o1.isPresent() ? o1.get() : o2.get()); + } + + public static void two2(Optional o1,Optional o2) { + if (!o2.isPresent()) { + return; + } + System.out.println(o1.isPresent() ? o1.get() : o2.get()); + } + + public static void two3(Optional o1,Optional o2) { + System.out.println(o1.isPresent() ? o1.get() : o2.get()); + } + + void def(Optional def) { + if (!def.isPresent()) { + throw new RuntimeException(); + } + { + def.get(); + } + } + + private void orred(Optional opt1, Optional opt2) { + if (!opt1.isPresent() || !opt2.isPresent()) { + return; + } + opt1.get(); + opt2.get(); + } + + class Range { + Optional min = Optional.of(1); + Optional max = Optional.of(2); + + Optional getMax() { + return max; + } + Optional getMin() { + return min; + } + } + void useRange(Range a, Range b) { + if (!a.getMax().isPresent() || !b.getMin().isPresent()) { + + } + else if (a.getMax().get() <= b.getMin().get()) { + + } + } + + public void foo(Optional value) { + if (!value.isPresent()) { + return; + } + + try { + System.out.println(value.get()); // <- warning appears here + } finally { + System.out.println("hi"); + } + } + + void shortIf(Optional o) { + if (true || o.isPresent()) { + o.get(); + } + } + + void test(Optional aOpt, Optional bOpt) { + if (!aOpt.isPresent() || !bOpt.isPresent()) { + throw new RuntimeException(); + } + String a = aOpt.get(); + String b = bOpt.get(); + } } \ No newline at end of file diff --git a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspectionTest.java b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspectionTest.java index 08893781ff51..a69c870972ef 100644 --- a/plugins/InspectionGadgets/testsrc/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspectionTest.java +++ b/plugins/InspectionGadgets/testsrc/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspectionTest.java @@ -111,6 +111,24 @@ public class OptionalGetWithoutIsPresentInspectionTest extends LightInspectionTe "}"); } + public void testNested() { + doTest("import java.util.Optional;" + + "class X {" + + " void test(Optional opt, String action) {" + + " if(!opt.isPresent()) {" + + " throw new IllegalArgumentException();" + + " }" + + " switch(action) {" + + " case \"case\":" + + " System.out.println(opt.get());" + + " break;" + + " default:" + + " System.err.println(opt.get());" + + " }" + + " }" + + "}"); + } + public void testOptionalGetWithoutIsPresent() { doTest(); }