Added Optional emptiness handler (#278)

Added Optional Emptiness handler which does couple of things 1. on `optionalFoo.get()` hints that it can be Nullable. 2. on `optionalFoo.isPresent()` sets the `optionalFoo.get()` access path to Non-null. This allows ``` if (fooOptional.isPresent()) { fooOptional.get().bar(); // Implicitly understood to be safe } ``` and returns a warning if not implied safe. Fixes #274 Added unit tests Handler is gated by a configuration flag.
uber · Mar 6, 2019 · 4a4b591 · 4a4b591
1 parent baf7fbb
commit 4a4b591
Show file tree

Hide file tree

Showing 17 changed files with 395 additions and 25 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,14 @@ public interface Config {
    */
   boolean acknowledgeRestrictiveAnnotations();
 
+  /**
+   * @return true if Optional Emptiness Handler is to be used. When Optional.get() method is called
+   *     on an empty optional, program will crash with an exception. This handler warns on possible
+   *     cases where Optional.get() call is made on an empty optional. Nullaway determines if an
+   *     optional is non-empty based on Optional.isPresent() call.
+   */
+  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/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
@@ -0,0 +1,33 @@
+package com.uber.nullaway;
+
+/** Contains error message string to be displayed and the message type from {@link MessageTypes}. */
+public class ErrorMessage {
+  MessageTypes messageType;
+  String message;
+
+  ErrorMessage(MessageTypes messageType, String message) {
+    this.messageType = messageType;
+    this.message = message;
+  }
+
+  public void updateErrorMessage(MessageTypes messageType, String message) {
+    this.messageType = messageType;
+    this.message = message;
+  }
+
+  public enum MessageTypes {
+    DEREFERENCE_NULLABLE,
+    RETURN_NULLABLE,
+    PASS_NULLABLE,
+    ASSIGN_FIELD_NULLABLE,
+    WRONG_OVERRIDE_RETURN,
+    WRONG_OVERRIDE_PARAM,
+    METHOD_NO_INIT,
+    FIELD_NO_INIT,
+    UNBOX_NULLABLE,
+    NONNULL_FIELD_READ_BEFORE_INIT,
+    ANNOTATION_VALUE_INVALID,
+    CAST_TO_NONNULL_ARG_NONNULL,
+    GET_ON_EMPTY_OPTIONAL;
+  }
+}
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
@@ -88,6 +88,7 @@
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.processing.JavacProcessingEnvironment;
 import com.sun.tools.javac.tree.JCTree;
+import com.uber.nullaway.ErrorMessage.MessageTypes;
 import com.uber.nullaway.dataflow.AccessPathNullnessAnalysis;
 import com.uber.nullaway.dataflow.EnclosingEnvironmentNullness;
 import com.uber.nullaway.handlers.Handler;
@@ -366,7 +367,7 @@ private void updateEnvironmentMapping(Tree tree, VisitorState state) {
     // 2. we keep info on all locals rather than just effectively final ones for simplicity
     EnclosingEnvironmentNullness.instance(state.context)
         .addEnvironmentMapping(
-            tree, analysis.getLocalVarInfoBefore(state.getPath(), state.context));
+            tree, analysis.getNullnessInfoBeforeNewContext(state.getPath(), state, handler));
   }
 
   private Symbol.MethodSymbol getSymbolOfSuperConstructor(
@@ -1933,11 +1934,16 @@ private Description matchDereference(
       return Description.NO_MATCH;
     }
     if (mayBeNullExpr(state, baseExpression)) {
-      String message = "dereferenced expression " + baseExpression.toString() + " is @Nullable";
+      final String message =
+          "dereferenced expression " + baseExpression.toString() + " is @Nullable";
+      ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message);
+
+      handler.onPrepareErrorMessage(baseExpression, state, errorMessage);
+
       return createErrorDescriptionForNullAssignment(
-          MessageTypes.DEREFERENCE_NULLABLE,
+          errorMessage.messageType,
           derefExpression,
-          message,
+          errorMessage.message,
           baseExpression,
           state.getPath());
     }
@@ -2325,21 +2331,6 @@ public void setComputedNullness(ExpressionTree e, Nullness nullness) {
     computedNullnessMap.put(e, nullness);
   }
 
-  public enum MessageTypes {
-    DEREFERENCE_NULLABLE,
-    RETURN_NULLABLE,
-    PASS_NULLABLE,
-    ASSIGN_FIELD_NULLABLE,
-    WRONG_OVERRIDE_RETURN,
-    WRONG_OVERRIDE_PARAM,
-    METHOD_NO_INIT,
-    FIELD_NO_INIT,
-    UNBOX_NULLABLE,
-    NONNULL_FIELD_READ_BEFORE_INIT,
-    ANNOTATION_VALUE_INVALID,
-    CAST_TO_NONNULL_ARG_NONNULL;
-  }
-
   @AutoValue
   abstract static class FieldInitEntities {
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java
@@ -311,7 +311,7 @@ public static boolean isMapPut(Symbol.MethodSymbol symbol, Types types) {
    * root of an access path; either a variable {@link javax.lang.model.element.Element} or <code>
    * this</code> (enclosing method receiver)
    */
-  static final class Root {
+  public static final class Root {
 
     /** does this represent the receiver? */
     private final boolean isMethodReceiver;

diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessAnalysis.java
@@ -19,6 +19,7 @@
 package com.uber.nullaway.dataflow;
 
 import com.google.common.collect.ImmutableList;
+import com.google.errorprone.VisitorState;
 import com.sun.source.util.TreePath;
 import com.sun.tools.javac.util.Context;
 import com.uber.nullaway.Config;
@@ -47,6 +48,8 @@ public final class AccessPathNullnessAnalysis {
 
   private final DataFlow dataFlow;
 
+  private static String OPTIONAL_PATH = "java.util.Optional";
+
   // Use #instance to instantiate
   private AccessPathNullnessAnalysis(
       Predicate<MethodInvocationNode> methodReturnsNonNull,
@@ -151,11 +154,12 @@ public Set<Element> getNonnullStaticFieldsBefore(TreePath path, Context context)
    * Get nullness info for local variables before some node
    *
    * @param path tree path to some AST node within a method / lambda / initializer
-   * @param context Javac context
+   * @param state visitor state
    * @return nullness info for local variables just before the node
    */
-  public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) {
-    NullnessStore store = dataFlow.resultBefore(path, context, nullnessPropagation);
+  public NullnessStore getNullnessInfoBeforeNewContext(
+      TreePath path, VisitorState state, Handler handler) {
+    NullnessStore store = dataFlow.resultBefore(path, state.context, nullnessPropagation);
     if (store == null) {
       return NullnessStore.empty();
     }
@@ -169,7 +173,8 @@ public NullnessStore getLocalVarInfoBefore(TreePath path, Context context) {
                   || e.getKind().equals(ElementKind.LOCAL_VARIABLE);
             }
           }
-          return false;
+
+          return handler.includeApInfoInSavedContext(ap, state);
         });
   }
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java
@@ -34,7 +34,9 @@
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.util.Context;
+import com.uber.nullaway.ErrorMessage;
 import com.uber.nullaway.NullAway;
+import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
 import com.uber.nullaway.dataflow.NullnessStore;
 import java.util.List;
@@ -166,4 +168,15 @@ public void onDataflowVisitLambdaResultExpression(
       ExpressionTree tree, NullnessStore thenStore, NullnessStore elseStore) {
     // NoOp
   }
+
+  @Override
+  public void onPrepareErrorMessage(
+      ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) {
+    // NoOp
+  }
+
+  @Override
+  public boolean includeApInfoInSavedContext(AccessPath accessPath, 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
@@ -35,7 +35,9 @@
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.util.Context;
+import com.uber.nullaway.ErrorMessage;
 import com.uber.nullaway.NullAway;
+import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
 import com.uber.nullaway.dataflow.NullnessStore;
 import java.util.List;
@@ -207,4 +209,21 @@ public void onDataflowVisitLambdaResultExpression(
       h.onDataflowVisitLambdaResultExpression(tree, thenStore, elseStore);
     }
   }
+
+  @Override
+  public void onPrepareErrorMessage(
+      ExpressionTree expr, VisitorState state, ErrorMessage errorMessage) {
+    for (Handler h : handlers) {
+      h.onPrepareErrorMessage(expr, state, errorMessage);
+    }
+  }
+
+  @Override
+  public boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state) {
+    boolean shouldFilter = false;
+    for (Handler h : handlers) {
+      shouldFilter |= h.includeApInfoInSavedContext(accessPath, state);
+    }
+    return shouldFilter;
+  }
 }
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/ContractHandler.java
@@ -30,6 +30,7 @@
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.util.Context;
+import com.uber.nullaway.ErrorMessage;
 import com.uber.nullaway.NullAway;
 import com.uber.nullaway.Nullness;
 import com.uber.nullaway.dataflow.AccessPath;
@@ -221,7 +222,10 @@ private void reportMatch(Tree errorLocTree, String message) {
     if (this.analysis != null && this.state != null) {
       this.state.reportMatch(
           this.analysis.createErrorDescription(
-              NullAway.MessageTypes.ANNOTATION_VALUE_INVALID, errorLocTree, message, errorLocTree));
+              ErrorMessage.MessageTypes.ANNOTATION_VALUE_INVALID,
+              errorLocTree,
+              message,
+              errorLocTree));
     }
   }
 

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
@@ -34,8 +34,10 @@
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Types;
 import com.sun.tools.javac.util.Context;
+import com.uber.nullaway.ErrorMessage;
 import com.uber.nullaway.NullAway;
 import com.uber.nullaway.Nullness;
+import com.uber.nullaway.dataflow.AccessPath;
 import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
 import com.uber.nullaway.dataflow.NullnessStore;
 import java.util.List;
@@ -270,6 +272,27 @@ 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.
+   * @param errorMessage error message string and type of the error wrapped in {@link
+   *     com.uber.nullaway.NullAway.ErrorMessage}.
+   */
+  void onPrepareErrorMessage(ExpressionTree expr, VisitorState state, ErrorMessage errorMessage);
+
+  /**
+   * Called when the store access paths are filtered for local variable information before an
+   * expression.
+   *
+   * @param accessPath The access path that needs to be checked if filtered.
+   * @param state The current visitor state.
+   * @return true if the nullability information for this accesspath should be treated as part of
+   *     the surrounding context when processing a lambda expression or anonymous class declaration.
+   */
+  boolean includeApInfoInSavedContext(AccessPath accessPath, 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,6 +50,9 @@ public static Handler buildDefault(Config config) {
     handlerListBuilder.add(new RxNullabilityPropagator());
     handlerListBuilder.add(new ContractHandler());
     handlerListBuilder.add(new ApacheThriftIsSetHandler());
+    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
@@ -293,6 +293,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)