Introducing improved Optional Checker

checker-qual/src/main/java/org/checkerframework/checker/nonempty/qual/NonEmpty.java

@@ -8,8 +8,9 @@
 import org.checkerframework.framework.qual.SubtypeOf;
 
 /**
- * The {@link java.util.Collection Collection}, {@link java.util.Iterator Iterator}, {@link
- * java.lang.Iterable Iterable}, or {@link java.util.Map Map} is definitely non-empty.
+ * The {@link java.util.Collection Collection}, {@link java.util.Iterator Iterator}, {@link Iterable
+ * Iterable}, or {@link java.util.Map Map}, or {@link java.util.stream.Stream Stream} is definitely
+ * non-empty.
  *
  * @checker_framework.manual #non-empty-checker Non-Empty Checker
  */

checker-qual/src/main/java/org/checkerframework/checker/nonempty/qual/UnknownNonEmpty.java

@@ -10,7 +10,8 @@
 
 /**
  * The {@link java.util.Collection Collection}, {@link java.util.Iterator Iterator}, {@link
- * java.lang.Iterable Iterable}, or {@link java.util.Map Map} may or may not be empty.
+ * java.lang.Iterable Iterable}, {@link java.util.Map Map}, or {@link java.util.stream.Stream
+ * Stream} may or may not be empty.
  *
  * @checker_framework.manual #non-empty-checker Non-Empty Checker
  */

checker/src/main/java/org/checkerframework/checker/nonempty/NonEmptyChecker.java

@@ -1,17 +1,70 @@
 package org.checkerframework.checker.nonempty;
 
+import com.sun.source.tree.MethodTree;
+import java.util.Set;
+import org.checkerframework.checker.optional.OptionalChecker;
+import org.checkerframework.checker.optional.OptionalImplChecker;
+import org.checkerframework.checker.optional.OptionalImplVisitor;
 import org.checkerframework.common.basetype.BaseTypeChecker;
+import org.checkerframework.framework.source.SourceChecker;
 
 /**
  * A type-checker that prevents {@link java.util.NoSuchElementException} in the use of container
  * classes.
  *
+ * <p>Runs the {@link org.checkerframework.checker.optional.OptionalImplChecker} (as a subchecker)
+ * by default. This checker should not yet be run as a standalone checker. The Non-Empty Checker
+ * uses explicitly-written (i.e., programmer-written) annotations from the Non-Empty type system to
+ * refine the analysis of operations on containers (e.g., Streams, Collections) that result in
+ * values of type {@link java.util.Optional}.
+ *
  * @checker_framework.manual #non-empty-checker Non-Empty Checker
  */
 public class NonEmptyChecker extends BaseTypeChecker {
 
+  /** A cached instance of {@link OptionalImplVisitor} */
+  private OptionalImplVisitor optionalImplVisitor;
+
   /** Creates a NonEmptyChecker. */
   public NonEmptyChecker() {
     super();
   }
+
+  @Override
+  protected Set<Class<? extends SourceChecker>> getImmediateSubcheckerClasses() {
+    Set<Class<? extends SourceChecker>> checkers = super.getImmediateSubcheckerClasses();
+    // Ideally, the OptionalImplChecker should only be added as a subchecker if this Non-Empty
+    // Checker is being run in aggregate with the top-level Optional Checker
+    // However, it appears that this information (via SourceChecker#getParentChecker()) is not
+    // available at the time when this method is called.
+    checkers.add(OptionalImplChecker.class);
+    return checkers;
+  }
+
+  @Override
+  public boolean shouldSkipDefs(MethodTree tree) {
+    if (!(this.parentChecker instanceof OptionalChecker)) {
+      // The parent checker being null, or not being an instance of the top-level Optional Checker
+      // indicates that this Non-Empty Checker is being run independently.
+      // Check all definitions in this case, not just the ones relevant to the Optional Checker's
+      // guarantee.
+      return false;
+    }
+    return !getMethodsToVerify().contains(tree.getName().toString());
+  }
+
+  /**
+   * Obtains the methods to verify w.r.t. the Non-Empty type system from the Optional Checker.
+   *
+   * @return the set of names of the methods to be verified by the Non-Empty Checker
+   * @throws AssertionError if the {@link OptionalImplChecker} is not a subchecker of this checker
+   */
+  private Set<String> getMethodsToVerify() {
+    if (optionalImplVisitor == null) {
+      OptionalImplChecker optionalCheckerImpl = getSubchecker(OptionalImplChecker.class);
+      assert optionalCheckerImpl != null;
+      optionalImplVisitor = (OptionalImplVisitor) optionalCheckerImpl.getVisitor();
+    }
+    return optionalImplVisitor.getNamesOfMethodsToVerifyWithNonEmptyChecker();
+  }
 }

checker/src/main/java/org/checkerframework/checker/nullness/NullnessTransfer.java

@@ -430,7 +430,7 @@ public TransferResult<NullnessValue, NullnessStore> visitMethodInvocation(
       }
       if (isKeyFor) {
         AnnotatedTypeMirror receiverType = nullnessTypeFactory.getReceiverType(n.getTree());
-        if (!hasNullableValueType(receiverType)) {
+        if (!isValueTypeNullable(receiverType)) {
           makeNonNull(result, n);
           refineToNonNull(result);
         }
@@ -446,7 +446,7 @@ public TransferResult<NullnessValue, NullnessStore> visitMethodInvocation(
    * @param mapOrSubtype the Map type, or a subtype
    * @return true if mapType's value type is @Nullable
    */
-  private boolean hasNullableValueType(AnnotatedTypeMirror mapOrSubtype) {
+  private boolean isValueTypeNullable(AnnotatedTypeMirror mapOrSubtype) {
     AnnotatedDeclaredType mapType =
         AnnotatedTypes.asSuper(nullnessTypeFactory, mapOrSubtype, MAP_TYPE);
     int numTypeArguments = mapType.getTypeArguments().size();

checker/src/main/java/org/checkerframework/checker/optional/OptionalChecker.java

@@ -1,32 +1,26 @@
 package org.checkerframework.checker.optional;
 
-import java.util.Optional;
-import java.util.Set;
-import org.checkerframework.common.aliasing.AliasingChecker;
-import org.checkerframework.common.basetype.BaseTypeChecker;
-import org.checkerframework.framework.qual.RelevantJavaTypes;
-import org.checkerframework.framework.qual.StubFiles;
+import java.util.ArrayList;
+import java.util.Collection;
+import org.checkerframework.checker.nonempty.NonEmptyChecker;
+import org.checkerframework.framework.source.AggregateChecker;
 import org.checkerframework.framework.source.SourceChecker;
-import org.checkerframework.framework.source.SupportedOptions;
 
 /**
- * A type-checker that prevents misuse of the {@link java.util.Optional} class.
+ * A type-checker that prevents {@link java.util.NoSuchElementException} in the use of the {@link
+ * java.util.Optional} class.
  *
  * @checker_framework.manual #optional-checker Optional Checker
  */
-// TODO: For a call to `@Optional#ofNullable`, if the argument has type
-// @NonNull, make the return type have type @Present.
-@RelevantJavaTypes(Optional.class)
-@StubFiles({"javaparser.astub"})
-@SupportedOptions("optionalMapAssumeNonNull")
-public class OptionalChecker extends BaseTypeChecker {
-  /** Create an OptionalChecker. */
+public class OptionalChecker extends AggregateChecker {
+
+  /** Creates a new {@link org.checkerframework.checker.optional.OptionalChecker} */
   public OptionalChecker() {}
 
   @Override
-  protected Set<Class<? extends SourceChecker>> getImmediateSubcheckerClasses() {
-    Set<Class<? extends SourceChecker>> subcheckers = super.getImmediateSubcheckerClasses();
-    subcheckers.add(AliasingChecker.class);
-    return subcheckers;
+  protected Collection<Class<? extends SourceChecker>> getSupportedCheckers() {
+    Collection<Class<? extends SourceChecker>> checkers = new ArrayList<>(2);
+    checkers.add(NonEmptyChecker.class);
+    return checkers;
   }
 }

checker/src/main/java/org/checkerframework/checker/optional/OptionalAnnotatedTypeFactory.java → checker/src/main/java/org/checkerframework/checker/optional/OptionalImplAnnotatedTypeFactory.java

@@ -21,8 +21,8 @@
 import org.checkerframework.javacutil.AnnotationBuilder;
 import org.checkerframework.javacutil.TreeUtils;
 
-/** OptionalAnnotatedTypeFactory for the Optional Checker. */
-public class OptionalAnnotatedTypeFactory extends BaseAnnotatedTypeFactory {
+/** Type factory for the OptionalImplChecker. */
+public class OptionalImplAnnotatedTypeFactory extends BaseAnnotatedTypeFactory {
 
   /** The element for java.util.Optional.map(). */
   private final ExecutableElement optionalMap;
@@ -31,11 +31,12 @@ public class OptionalAnnotatedTypeFactory extends BaseAnnotatedTypeFactory {
   protected final AnnotationMirror PRESENT = AnnotationBuilder.fromClass(elements, Present.class);
 
   /**
-   * Creates an OptionalAnnotatedTypeFactory.
+   * Creates an OptionalImplAnnotatedTypeFactory.
    *
-   * @param checker the Optional Checker associated with this type factory
+   * @param checker the {@link org.checkerframework.checker.optional.OptionalImplChecker} associated
+   *     with this type factory
    */
-  public OptionalAnnotatedTypeFactory(BaseTypeChecker checker) {
+  public OptionalImplAnnotatedTypeFactory(BaseTypeChecker checker) {
     super(checker);
     postInit();
     optionalMap = TreeUtils.getMethodOrNull("java.util.Optional", "map", 1, getProcessingEnv());
@@ -49,13 +50,13 @@ protected void addComputedTypeAnnotations(Tree tree, AnnotatedTypeMirror type, b
 
   /**
    * If {@code tree} is a call to {@link java.util.Optional#map(Function)} whose argument is a
-   * method reference, then this method adds {@code @Present} to {@code type} if the following is
+   * method reference m, then this method adds {@code @Present} to {@code type} if the following is
    * true:
    *
    * <ul>
    *   <li>The type of the receiver to {@link java.util.Optional#map(Function)} is {@code @Present},
    *       and
-   *   <li>{@link #returnHasNullable(MemberReferenceTree)} returns false.
+   *   <li>{@link #isReturnTypeNullable(MemberReferenceTree)} returns false for m.
    * </ul>
    *
    * @param tree a tree
@@ -67,39 +68,41 @@ private void optionalMapNonNull(Tree tree, AnnotatedTypeMirror type) {
     }
     MethodInvocationTree mapTree = (MethodInvocationTree) tree;
     ExpressionTree argTree = mapTree.getArguments().get(0);
-    if (argTree.getKind() == Kind.MEMBER_REFERENCE) {
-      MemberReferenceTree memberReferenceTree = (MemberReferenceTree) argTree;
-      AnnotatedTypeMirror optType = getReceiverType(mapTree);
-      if (optType == null || !optType.hasEffectiveAnnotation(Present.class)) {
-        return;
-      }
-      if (!returnHasNullable(memberReferenceTree)) {
-        // The method still could have a @PolyNull on the return and might return null.
-        // If @PolyNull is the primary annotation on the parameter and not on any type
-        // arguments or array elements, then it is still safe to mark the optional type as
-        // present.
-        // TODO: Add the check for poly null on arguments.
-        type.replaceAnnotation(PRESENT);
-      }
+    if (argTree.getKind() != Kind.MEMBER_REFERENCE) {
+      return;
+    }
+    AnnotatedTypeMirror mapReceiver = getReceiverType(mapTree);
+    if (mapReceiver == null || !mapReceiver.hasEffectiveAnnotation(Present.class)) {
+      return;
+    }
+    MemberReferenceTree memberReferenceTree = (MemberReferenceTree) argTree;
+    if (!isReturnTypeNullable(memberReferenceTree)) {
+      // The method still could have a @PolyNull on the return and might return null.
+      // If @PolyNull is the primary annotation on the parameter and not on any type
+      // arguments or array elements, then it is still safe to mark the optional type as
+      // present.
+      // TODO: Add the check for @PolyNull on arguments.
+      type.replaceAnnotation(PRESENT);
     }
   }
 
   /**
    * Returns true if the return type of the function type of {@code memberReferenceTree} is
    * annotated with {@code @Nullable}.
    *
-   * @param memberReferenceTree a member reference
+   * @param memberReferenceTree a member reference (which is to a method or constructor)
    * @return true if the return type of the function type of {@code memberReferenceTree} is
    *     annotated with {@code @Nullable}
    */
-  private boolean returnHasNullable(MemberReferenceTree memberReferenceTree) {
+  private boolean isReturnTypeNullable(MemberReferenceTree memberReferenceTree) {
+    // A member reference refers to either a method or a constructor.
     if (TreeUtils.MemberReferenceKind.getMemberReferenceKind(memberReferenceTree)
         .isConstructorReference()) {
       return false;
     }
     ExecutableElement memberReferenceFuncType = TreeUtils.elementFromUse(memberReferenceTree);
     if (memberReferenceFuncType.getEnclosingElement().getKind() == ElementKind.ANNOTATION_TYPE) {
-      // Annotation element accessor are always non-null;
+      // Annotation element accessors are always non-null;
       return false;
     }
 
@@ -111,10 +114,10 @@ private boolean returnHasNullable(MemberReferenceTree memberReferenceTree) {
   }
 
   /**
-   * Returns true if {@code annos} contains a nullable annotation.
+   * Returns true if {@code annos} contains an annotation named "Nullable".
    *
    * @param annos a collection of annotations
-   * @return true if {@code annos} contains a nullable annotation
+   * @return true if {@code annos} contains an annotation named "Nullable".
    */
   private boolean containsNullable(Collection<? extends AnnotationMirror> annos) {
     for (AnnotationMirror anno : annos) {
@@ -128,6 +131,6 @@ private boolean containsNullable(Collection<? extends AnnotationMirror> annos) {
   @Override
   public CFTransfer createFlowTransferFunction(
       CFAbstractAnalysis<CFValue, CFStore, CFTransfer> analysis) {
-    return new OptionalTransfer(analysis);
+    return new OptionalImplTransfer(analysis);
   }
 }

checker/src/main/java/org/checkerframework/checker/optional/OptionalImplChecker.java

@@ -0,0 +1,42 @@
+package org.checkerframework.checker.optional;
+
+import java.util.NavigableSet;
+import java.util.Optional;
+import java.util.Set;
+import org.checkerframework.common.aliasing.AliasingChecker;
+import org.checkerframework.common.basetype.BaseTypeChecker;
+import org.checkerframework.framework.qual.RelevantJavaTypes;
+import org.checkerframework.framework.qual.StubFiles;
+import org.checkerframework.framework.source.SourceChecker;
+import org.checkerframework.framework.source.SupportedOptions;
+
+/**
+ * The implementation of a type-checker that prevents misuse of the {@link java.util.Optional}
+ * class.
+ *
+ * @checker_framework.manual #optional-checker Optional Checker
+ */
+// TODO: For a call to `@Optional#ofNullable`, if the argument has type
+// @NonNull, make the return type have type @Present.
+@RelevantJavaTypes(Optional.class)
+@StubFiles({"javaparser.astub"})
+@SupportedOptions("optionalMapAssumeNonNull")
+public class OptionalImplChecker extends BaseTypeChecker {
+  /** Create an OptionalImplChecker. */
+  public OptionalImplChecker() {}
+
+  @Override
+  protected Set<Class<? extends SourceChecker>> getImmediateSubcheckerClasses() {
+    Set<Class<? extends SourceChecker>> subcheckers = super.getImmediateSubcheckerClasses();
+    subcheckers.add(AliasingChecker.class);
+    return subcheckers;
+  }
+
+  /** Use "optional", rather than "optionalimpl", as the {@link SuppressWarnings} prefix. */
+  @Override
+  public NavigableSet<String> getSuppressWarningsPrefixes() {
+    NavigableSet<String> prefixes = super.getSuppressWarningsPrefixes();
+    prefixes.add("optional");
+    return prefixes;
+  }
+}