IG: rewrite "'Optional.get()' without 'isPresent()' check" inspection using data...
authorBas Leijdekkers <basleijdekkers@gmail.com>
Tue, 20 Sep 2016 15:35:54 +0000 (17:35 +0200)
committerBas Leijdekkers <basleijdekkers@gmail.com>
Tue, 20 Sep 2016 15:37:17 +0000 (17:37 +0200)
plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspection.java
plugins/InspectionGadgets/InspectionGadgetsAnalysis/src/com/siyeh/ig/psiutils/TypeUtils.java
plugins/InspectionGadgets/test/com/siyeh/igtest/bugs/optional_get_without_is_present/OptionalGetWithoutIsPresent.java
plugins/InspectionGadgets/testsrc/com/siyeh/ig/bugs/OptionalGetWithoutIsPresentInspectionTest.java

index dc9c9b15194b148e28329df14d914b3add9b8409..95195332aaf75ac6d4deaffc7d196ae344affcb3 100644 (file)
  */
 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;
-            }
-          }
-        }
-      }
-      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;
-              }
-            }
-          }
-        }
-      }
-      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 visitClassInitializer(PsiClassInitializer initializer) {
+      check(initializer.getBody());
     }
 
     @Override
-    public void visitReferenceExpression(PsiReferenceExpression expression) {
-      visitExpression(expression);
+    public void visitField(PsiField field) {
+      check(field.getInitializer());
     }
 
-    @Override
-    public void visitPolyadicExpression(PsiPolyadicExpression expression) {
-      final IElementType tokenType = expression.getOperationTokenType();
-      if (tokenType != JavaTokenType.ANDAND && tokenType != JavaTokenType.OROR) {
+    private void check(PsiElement element) {
+      if (!containsOptionalGetCall(element)) {
         return;
       }
-      for (PsiExpression operand : expression.getOperands()) {
-        if (PsiTreeUtil.isAncestor(operand, referenceExpression, false)) {
-          return;
-        }
-        checkExpression(operand);
-        if (hasIsPresentCall) {
-          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);
         }
-      }
-    }
-
-    @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;
+        @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;
         }
-      }
-      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 static boolean containsOptionalGetCall(PsiElement element) {
+      if (element == null) return false;
+      final OptionalGetCallChecker checker = new OptionalGetCallChecker();
+      element.acceptChildren(checker);
+      return checker.containsOptionalGetCall();
     }
 
-    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;
+    private static class OptionalGetCallChecker extends JavaRecursiveElementWalkingVisitor {
+      private boolean result = false;
+
+      @Override
+      public void visitMethodCallExpression(PsiMethodCallExpression expression) {
+        if (result) {
+          return;
         }
-        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;
+      }
     }
   }
 }
index eeb18b99eb87e8389e2bdb6a6d81730601ed10e8..7e9dcd7007c1226ecc94ee3472aeb99c28b6e1b7 100644 (file)
@@ -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;
     }
index 0c1e79aac2b9b89ced12f24e8c0c59acf7ffe75c..caf7d267fc1cd77a91056d3cf849083649b0784c 100644 (file)
@@ -12,7 +12,7 @@ class OptionalWithoutIsPresent {
     }
     if (maybe.isPresent()) {
       maybe = Optional.empty();
-      System.out.println(maybe.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
+      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.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>();
+      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<String> 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<String> 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<String> getOptional() {
+    return Optional.empty();
+  }
+
+  //void order(Optional<String> order) {
+  //  order.ifPresent(o -> System.out.println(order.get()));
+  //}
+
+  public static void two(Optional<Object> o1,Optional<Object> o2) {
+    if (!o1.isPresent() && !o2.isPresent()) {
+      return;
+    }
+    System.out.println(o1.isPresent() ? o1.get() : o2.get());
+  }
+
+  public static void two2(Optional<Object> o1,Optional<Object> o2) {
+    if (!o2.isPresent()) {
+      return;
+    }
+    System.out.println(o1.isPresent() ? o1.get() : o2.get());
+  }
+
+  public static void two3(Optional<Object> o1,Optional<Object> o2) {
+    System.out.println(o1.isPresent() ? o1.get() : o2.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>());
+  }
+
+  void def(Optional<String> def) {
+    if (!def.isPresent()) {
+      throw new RuntimeException();
+    }
+    {
+      def.get();
+    }
+  }
+
+  private void orred(Optional<Integer> opt1, Optional<Integer> opt2) {
+    if (!opt1.isPresent() || !opt2.isPresent()) {
+      return;
+    }
+    opt1.get();
+    opt2.get();
+  }
+
+  class Range {
+    Optional<Integer> min = Optional.of(1);
+    Optional<Integer> max = Optional.of(2);
+
+    Optional<Integer> getMax() {
+      return max;
+    }
+    Optional<Integer> 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<Long> value) {
+    if (!value.isPresent()) {
+      return;
+    }
+
+    try {
+      System.out.println(value.get()); // <- warning appears here
+    } finally {
+      System.out.println("hi");
+    }
+  }
+
+  void shortIf(Optional<String> o) {
+    if (true || o.isPresent()) {
+      o.<warning descr="'Optional.get()' without 'isPresent()' check">get</warning>();
+    }
+  }
+
+  void test(Optional<String> aOpt, Optional<String> bOpt) {
+    if (!aOpt.isPresent() || !bOpt.isPresent()) {
+      throw new RuntimeException();
+    }
+    String a = aOpt.get();
+    String b = bOpt.get();
+  }
 }
\ No newline at end of file
index 08893781ff51477fa45143811428941e25d1cc58..a69c870972ef311750f754fc08e4d837bcd80d3e 100644 (file)
@@ -111,6 +111,24 @@ public class OptionalGetWithoutIsPresentInspectionTest extends LightInspectionTe
            "}");
   }
 
+  public void testNested() {
+    doTest("import java.util.Optional;" +
+           "class X {" +
+           "  void test(Optional<String> 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();
   }