Made changes according to the discussion

uber · Feb 19, 2019 · 884528b · 884528b
1 parent fd0fadd
commit 884528b
Show file tree

Hide file tree

Showing 13 changed files with 96 additions and 6 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java b/nullaway/src/main/java/com/uber/nullaway/AbstractConfig.java
@@ -68,6 +68,8 @@ public abstract class AbstractConfig implements Config {
 
   protected boolean isAcknowledgeRestrictive;
 
+  protected boolean checkOptionalEmptiness;
+
   protected boolean assertsEnabled;
 
   /**
@@ -182,6 +184,11 @@ public boolean acknowledgeRestrictiveAnnotations() {
     return isAcknowledgeRestrictive;
   }
 
+  @Override
+  public boolean checkOptionalEmptiness() {
+    return checkOptionalEmptiness;
+  }
+
   @Override
   public boolean assertsEnabled() {
     return assertsEnabled;

diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java
@@ -107,6 +107,9 @@ public interface Config {
    */
   boolean acknowledgeRestrictiveAnnotations();
 
+  /** @return true if Optional Emptiness Handler is to be used. */
+  boolean checkOptionalEmptiness();
+
   /**
    * @return the fully qualified name of a method which will take a @Nullable version of a value and
    *     return an @NonNull copy (likely through an unsafe downcast, but performing runtime checking

diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java
@@ -112,6 +112,11 @@ public boolean acknowledgeRestrictiveAnnotations() {
     throw new IllegalStateException(error_msg);
   }
 
+  @Override
+  public boolean checkOptionalEmptiness() {
+    throw new IllegalStateException(error_msg);
+  }
+
   @Override
   @Nullable
   public String getCastToNonNullMethod() {

diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
@@ -55,6 +55,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
   static final String FL_UNANNOTATED_CLASSES = EP_FL_NAMESPACE + ":UnannotatedClasses";
   static final String FL_ACKNOWLEDGE_RESTRICTIVE =
       EP_FL_NAMESPACE + ":AcknowledgeRestrictiveAnnotations";
+  static final String FL_CHECK_OPTIONAL_EMPTINESS = EP_FL_NAMESPACE + ":CheckOptionalEmptiness";
   static final String FL_SUPPRESS_COMMENT = EP_FL_NAMESPACE + ":AutoFixSuppressionComment";
   /** --- JarInfer configs --- */
   static final String FL_JI_ENABLED = EP_FL_NAMESPACE + ":JarInferEnabled";
@@ -132,6 +133,7 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig {
     isExhaustiveOverride = flags.getBoolean(FL_EXHAUSTIVE_OVERRIDE).orElse(false);
     isSuggestSuppressions = flags.getBoolean(FL_SUGGEST_SUPPRESSIONS).orElse(false);
     isAcknowledgeRestrictive = flags.getBoolean(FL_ACKNOWLEDGE_RESTRICTIVE).orElse(false);
+    checkOptionalEmptiness = flags.getBoolean(FL_CHECK_OPTIONAL_EMPTINESS).orElse(false);
     treatGeneratedAsUnannotated = flags.getBoolean(FL_GENERATED_UNANNOTATED).orElse(false);
     assertsEnabled = flags.getBoolean(FL_ASSERTS_ENABLED).orElse(false);
     fieldAnnotPattern =

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -1933,7 +1933,17 @@ private Description matchDereference(
       return Description.NO_MATCH;
     }
     if (mayBeNullExpr(state, baseExpression)) {
-      String message = "dereferenced expression " + baseExpression.toString() + " is @Nullable";
+      String message;
+      if (handler.checkIfOptionalGetCall(baseExpression, state)) {
+        final int exprStringSize = baseExpression.toString().length();
+        // Name of the optional is extracted from the expression
+        message =
+            "Optional "
+                + baseExpression.toString().substring(0, exprStringSize - 6)
+                + " can be empty, dereferenced get() call on it";
+      } else {
+        message = "dereferenced expression " + baseExpression.toString() + " is @Nullable";
+      }
       return createErrorDescriptionForNullAssignment(
           MessageTypes.DEREFERENCE_NULLABLE,
           derefExpression,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java
@@ -166,4 +166,9 @@ public void onDataflowVisitLambdaResultExpression(
       ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore) {
     // NoOp
   }
+
+  @Override
+  public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) {
+    return false;
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
@@ -207,4 +207,13 @@ public void onDataflowVisitLambdaResultExpression(
       h.onDataflowVisitLambdaResultExpression(tree, thenStore, elseStore);
     }
   }
+
+  @Override
+  public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) {
+    boolean ifOptionalGetCall = false;
+    for (Handler h : handlers) {
+      ifOptionalGetCall |= h.checkIfOptionalGetCall(expr, state);
+    }
+    return ifOptionalGetCall;
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
@@ -270,6 +270,15 @@ NullnessHint onDataflowVisitMethodInvocation(
   void onDataflowVisitLambdaResultExpression(
       ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore);
 
+  /**
+   * Called while creating the error message on a possible null/empty optional deference.
+   *
+   * @param expr The AST node for the expression being matched.
+   * @param state The current visitor state.
+   * @return If the method call in the expression is to Optional.get().
+   */
+  boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state);
+
   /**
    * A three value enum for handlers implementing onDataflowVisitMethodInvocation to communicate
    * their knowledge of the method return nullability to the the rest of NullAway.

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -50,7 +50,9 @@ public static Handler buildDefault(Config config) {
     handlerListBuilder.add(new RxNullabilityPropagator());
     handlerListBuilder.add(new ContractHandler());
     handlerListBuilder.add(new ApacheThriftIsSetHandler());
-    handlerListBuilder.add(new OptionalEmptinessHandler());
+    if (config.checkOptionalEmptiness()) {
+      handlerListBuilder.add(new OptionalEmptinessHandler());
+    }
     return new CompositeHandler(handlerListBuilder.build());
   }
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java
@@ -289,6 +289,7 @@ private static class DefaultLibraryModels implements LibraryModels {
                     "javax.lang.model.util.Elements",
                     "getDocComment(javax.lang.model.element.Element)"),
                 0)
+            .put(methodRef("java.util.Optional", "<T>of(T)"), 0)
             .put(methodRef("java.util.Deque", "addFirst(E)"), 0)
             .put(methodRef("java.util.Deque", "addLast(E)"), 0)
             .put(methodRef("java.util.Deque", "offerFirst(E)"), 0)

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java
@@ -99,6 +99,15 @@ public NullnessHint onDataflowVisitMethodInvocation(
     return NullnessHint.UNKNOWN;
   }
 
+  @Override
+  public boolean checkIfOptionalGetCall(ExpressionTree expr, VisitorState state) {
+    if (expr.getKind() == Tree.Kind.METHOD_INVOCATION
+        && optionalIsGetCall((Symbol.MethodSymbol) ASTHelpers.getSymbol(expr), state.getTypes())) {
+      return true;
+    }
+    return false;
+  }
+
   private void updateNonNullAPsForElement(
       AccessPathNullnessPropagation.Updates updates, @Nullable Element elem, Node base) {
     if (elem != null) {

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
@@ -1183,15 +1183,16 @@ public void OptionalEmptinessHandlerTest() {
                 "-d",
                 temporaryFolder.getRoot().getAbsolutePath(),
                 "-XepOpt:NullAway:AnnotatedPackages=com.uber",
-                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated"))
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:CheckOptionalEmptiness=true"))
         .addSourceLines(
             "TestNegative.java",
             "package com.uber;",
             "import java.util.Optional;",
             "import javax.annotation.Nullable;",
             "public class TestNegative {",
             "  void foo() {",
-            "    Optional<Object> a = Optional.of(null);",
+            "    Optional<Object> a = Optional.empty();",
             "      // no error since a.isPresent() is called",
             "      if(a.isPresent()){",
             "         a.get().toString();",
@@ -1205,8 +1206,32 @@ public void OptionalEmptinessHandlerTest() {
             "import javax.annotation.Nullable;",
             "public class TestPositive {",
             "  void foo() {",
-            "    Optional<Object> a = Optional.of(null);",
-            "    // BUG: Diagnostic contains: dereferenced expression a.get() is @Nullable",
+            "    Optional<Object> a = Optional.empty();",
+            "    // BUG: Diagnostic contains: Optional a can be empty",
+            "    a.get().toString();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void OptionalEmptinessUncheckedTest() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated"))
+        .addSourceLines(
+            "TestPositive.java",
+            "package com.uber;",
+            "import java.util.Optional;",
+            "import javax.annotation.Nullable;",
+            "public class TestPositive {",
+            "  void foo() {",
+            "    Optional<Object> a = Optional.empty();",
+            "    // no error since the handler is not attached",
             "    a.get().toString();",
             "  }",
             "}")

diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNativeModels.java
@@ -37,6 +37,7 @@
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Nullable;
 import javax.lang.model.element.Element;
@@ -229,6 +230,8 @@ static void nonNullParameters() {
     File f = new File(s);
     // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
     URLClassLoader.newInstance(null, NullAwayNativeModels.class.getClassLoader());
+    // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
+    Optional<Object> op = Optional.of(null);
   }
 
   static void elementStuff(Element e, Elements elems) {