Soundly handle must call obligations with unknown must call methods

checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/MustCall.java

@@ -27,7 +27,7 @@
 @Target({ElementType.TYPE_USE, ElementType.TYPE_PARAMETER})
 @SubtypeOf({MustCallUnknown.class})
 @DefaultQualifierInHierarchy
-@DefaultFor({TypeUseLocation.EXCEPTION_PARAMETER})
+@DefaultFor({TypeUseLocation.EXCEPTION_PARAMETER, TypeUseLocation.UPPER_BOUND})
 public @interface MustCall {
   /**
    * Methods that might need to be called on the expression whose type is annotated.

checker-util/src/main/java/org/checkerframework/checker/regex/util/RegexUtil.java

@@ -450,8 +450,8 @@ public static boolean noStringMatchesAnyRegex(
    * @return a list of the results of applying {@code f} to the elements of {@code iterable}
    */
   public static <
-          @KeyForBottom FROM extends @Nullable @UnknownKeyFor @MustCallUnknown Object,
-          @KeyForBottom TO extends @Nullable @UnknownKeyFor @MustCallUnknown Object>
+          @KeyForBottom FROM extends @Nullable @UnknownKeyFor Object,
+          @KeyForBottom TO extends @Nullable @UnknownKeyFor Object>
       List<TO> mapList(
           @MustCallUnknown Function<@MustCallUnknown ? super FROM, ? extends TO> f,
           Iterable<FROM> iterable) {

checker/build.gradle

@@ -286,6 +286,12 @@ task allCalledMethodsTests(type: Test, group: 'Verification') {
   }
 }
 
+task allResourceLeakTests(type: Test, group: 'Verification') {
+  description 'Run all Junit tests for the Resource Leak Checker.'
+  include '**/ResourceLeak*.class'
+  include '**/MustCall*.class'
+}
+
 // These are tests that should only be run with JDK 11+.
 task jtregJdk11Tests(group: 'Verification') {
   description 'Run the jtreg tests made for JDK 11+.'

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

@@ -336,6 +336,13 @@ private static AnnotationMirror getMustCallValue(
           return result;
         }
       }
+      AnnotationMirror result =
+          mcAtf
+              .getAnnotatedType(reference.getElement())
+              .getEffectiveAnnotationInHierarchy(mcAtf.TOP);
+      if (result != null) {
+        return result;
+      }
       // There wasn't an @MustCall annotation for it in the store, so fall back to the default
       // must-call type for the class.
       // TODO: we currently end up in this case when checking a call to the return type
@@ -353,6 +360,7 @@ private static AnnotationMirror getMustCallValue(
         // Void types can't have methods called on them, so returning bottom is safe.
         return mcAtf.BOTTOM;
       }
+
       return mcAtf.getAnnotatedType(typeElt).getPrimaryAnnotationInHierarchy(mcAtf.TOP);
     }
 
@@ -866,8 +874,8 @@ private boolean shouldTrackInvocationResult(Set<Obligation> obligations, Node no
       ExecutableElement executableElement = TreeUtils.elementFromUse(newClassTree);
       TypeElement typeElt = TypesUtils.getTypeElement(ElementUtils.getType(executableElement));
       return typeElt == null
-          || !typeFactory.getMustCallValue(typeElt).isEmpty()
-          || !typeFactory.getMustCallValue(newClassTree).isEmpty();
+          || !typeFactory.hasEmptyMustCallValue(typeElt)
+          || !typeFactory.hasEmptyMustCallValue(newClassTree);
     }
 
     // Now callTree.getKind() == Tree.Kind.METHOD_INVOCATION.
@@ -1697,8 +1705,8 @@ private boolean shouldTrackReturnType(MethodInvocationNode node) {
     TypeElement typeElt = TypesUtils.getTypeElement(type);
     // no need to track if type has no possible @MustCall obligation
     if (typeElt != null
-        && typeFactory.getMustCallValue(typeElt).isEmpty()
-        && typeFactory.getMustCallValue(methodInvocationTree).isEmpty()) {
+        && typeFactory.hasEmptyMustCallValue(typeElt)
+        && typeFactory.hasEmptyMustCallValue(methodInvocationTree)) {
       return false;
     }
     // check for absence of @NotOwning annotation
@@ -2065,8 +2073,28 @@ private void checkMustCall(
       Obligation obligation, AccumulationStore cmStore, CFStore mcStore, String outOfScopeReason) {
 
     List<String> mustCallValue = obligation.getMustCallMethods(typeFactory, mcStore);
+
+    // optimization: if mustCallValue is null, always issue a warning (there is no way to satisfy
+    // the check). A null mustCallValue occurs when the type is top (@MustCallUnknown).
+    if (mustCallValue == null) {
+      // Report the error at the first alias' definition. This choice is arbitrary but
+      // consistent.
+      ResourceAlias firstAlias = obligation.resourceAliases.iterator().next();
+      if (!reportedErrorAliases.contains(firstAlias)) {
+        if (!checker.shouldSkipUses(TreeUtils.elementFromTree(firstAlias.tree))) {
+          reportedErrorAliases.add(firstAlias);
+          checker.reportError(
+              firstAlias.tree,
+              "required.method.not.known",
+              firstAlias.reference.toString(),
+              firstAlias.reference.getType().toString(),
+              outOfScopeReason);
+        }
+      }
+      return;
+    }
     // optimization: if there are no must-call methods, do not need to perform the check
-    if (mustCallValue == null || mustCallValue.isEmpty()) {
+    if (mustCallValue.isEmpty()) {
       return;
     }
 

checker/src/main/java/org/checkerframework/checker/resourceleak/ResourceLeakAnnotatedTypeFactory.java

@@ -112,7 +112,7 @@ public ResourceLeakAnnotatedTypeFactory(BaseTypeChecker checker) {
   /*package-private*/ boolean isCandidateOwningField(Element element) {
     return (element.getKind().isField()
         && ElementUtils.isFinal(element)
-        && !getMustCallValue(element).isEmpty());
+        && !hasEmptyMustCallValue(element));
   }
 
   @Override
@@ -155,34 +155,69 @@ protected ResourceLeakAnalysis createFlowAnalysis() {
   }
 
   /**
-   * Returns the {@link MustCall#value} element/argument of the @MustCall annotation on the type of
-   * {@code tree}.
+   * Returns whether the {@link MustCall#value} element/argument of the @MustCall annotation on the
+   * type of {@code tree} is definitely empty.
    *
-   * <p>If possible, prefer {@link #getMustCallValue(Tree)}, which accounts for flow-sensitive
+   * <p>This method only considers the declared type: it does not consider flow-sensitive
    * refinement.
    *
    * @param tree a tree
-   * @return the strings in its must-call type
+   * @return true if the Must Call type is non-empty or top
    */
-  /*package-private*/ List<String> getMustCallValue(Tree tree) {
+  /*package-private*/ boolean hasEmptyMustCallValue(Tree tree) {
     MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory =
         getTypeFactoryOfSubchecker(MustCallChecker.class);
     AnnotatedTypeMirror mustCallAnnotatedType = mustCallAnnotatedTypeFactory.getAnnotatedType(tree);
     AnnotationMirror mustCallAnnotation =
         mustCallAnnotatedType.getPrimaryAnnotation(MustCall.class);
-    return getMustCallValues(mustCallAnnotation);
+    if (mustCallAnnotation != null) {
+      return getMustCallValues(mustCallAnnotation).isEmpty();
+    } else {
+      // Indicates @MustCallUnknown, which should be treated (conservatively) as if it contains
+      // some must call values.
+      return false;
+    }
+  }
+
+  /**
+   * Returns whether the {@link MustCall#value} element/argument of the @MustCall annotation on the
+   * type of {@code element} is definitely empty.
+   *
+   * <p>This method only considers the declared type: it does not consider flow-sensitive
+   * refinement.
+   *
+   * @param element an element
+   * @return true if the Must Call type is non-empty or top
+   */
+  /*package-private*/ boolean hasEmptyMustCallValue(Element element) {
+    MustCallAnnotatedTypeFactory mustCallAnnotatedTypeFactory =
+        getTypeFactoryOfSubchecker(MustCallChecker.class);
+    AnnotatedTypeMirror mustCallAnnotatedType =
+        mustCallAnnotatedTypeFactory.getAnnotatedType(element);
+    AnnotationMirror mustCallAnnotation =
+        mustCallAnnotatedType.getPrimaryAnnotation(MustCall.class);
+    if (mustCallAnnotation != null) {
+      return getMustCallValues(mustCallAnnotation).isEmpty();
+    } else {
+      // Indicates @MustCallUnknown, which should be treated (conservatively) as if it contains
+      // some must call values.
+      return false;
+    }
   }
 
   /**
    * Returns the {@link MustCall#value} element/argument of the @MustCall annotation on the class
-   * type of {@code element}.
+   * type of {@code element}. If there is no such annotation, returns the empty list.
    *
    * <p>Do not use this method to get the MustCall value of an {@link
    * org.checkerframework.checker.resourceleak.MustCallConsistencyAnalyzer.Obligation}. Instead, use
    * {@link
    * org.checkerframework.checker.resourceleak.MustCallConsistencyAnalyzer.Obligation#getMustCallMethods(ResourceLeakAnnotatedTypeFactory,
    * CFStore)}.
    *
+   * <p>Do not call {@link List#isEmpty()} on the result of this method: prefer to call {@link
+   * #hasEmptyMustCallValue(Element)}, which correctly accounts for @MustCallUnknown, instead.
+   *
    * @param element an element
    * @return the strings in its must-call type
    */
@@ -278,7 +313,7 @@ protected ResourceLeakAnalysis createFlowAnalysis() {
             || tree.getKind() == Tree.Kind.NEW_CLASS
             || tree.getKind() == Tree.Kind.METHOD_INVOCATION
         : "unexpected declaration tree kind: " + tree.getKind();
-    return !getMustCallValue(tree).isEmpty();
+    return !hasEmptyMustCallValue(tree);
   }
 
   /**

checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties

@@ -8,3 +8,4 @@ destructor.exceptional.postcondition=Method %s must resolve the must-call obliga
 mustcallalias.out.of.scope=This @MustCallAlias parameter might go out of scope without being assigned into an owning field of this object (if this is a constructor) or returned.\nReason for going out of scope: %s
 owning.override.param=Incompatible ownership for parameter %s.\nfound   : no ownership annotation or @NotOwning\nrequired: @Owning\nConsequence: method %s in %s cannot override method %s in %s
 owning.override.return=Incompatible ownership for return.\nfound   : no ownership annotation or @Owning\nrequired: @NotOwning\nConsequence: method %s in %s cannot override method %s in %s
+required.method.not.known=The checker cannot determine the must call methods of %s or any of its aliases, so it could not determine if they were called. Typically, this error indicates that you need to write an @MustCall annotation (often on an unconstrained generic type).\nThe type of object is: %s.\nReason for going out of scope: %s

checker/tests/mustcall/CommandResponse.java

@@ -8,12 +8,6 @@ class CommandResponse {
   Map<String, Object> data;
 
   public void putAll(Map<? extends String, ?> m) {
-    // This is a false positive. The fix is to change the declaration to match what's below.
-    // The cause is that implicit upper bounds are defaulted to top, to match the intuition
-    // that e.g. List<E> actually means List<E extends @Top Object>. In this case, that
-    // causes an incompatibility with putAll, whose type requires @Bottom Object as the second
-    // type parameter, because of the type of the data field.
-    // :: error: (argument)
     data.putAll(m);
   }
 

checker/tests/mustcall/EditLogInputStream.java

@@ -6,6 +6,9 @@ abstract class EditLogInputStream implements Closeable {
 }
 
 interface JournalSet extends Closeable {
-  static final Comparator<EditLogInputStream> LOCAL_LOG_PREFERENCE_COMPARATOR =
+  static final Comparator<? extends EditLogInputStream> LOCAL_LOG_PREFERENCE_COMPARATOR =
+      // This is an undesirable false positive that occurs because of the defaulting
+      // that the Must Call Checker uses for generics.
+      // :: error: type.argument
       Comparator.comparing(EditLogInputStream::isLocalLog).reversed();
 }

checker/tests/mustcall/ListOfMustCall.java

@@ -1,12 +1,18 @@
 // A test that checks that parameterized classes in the JDK don't cause false positives
 // when they are used with an @MustCall-annotated class.
+// Currently, two undesirable errors are issued on this test case, because it is not currently
+// possible to add an item with a non-empty must call type to a list without an error.
+// It is possible to call other List methods by using List<? extends ListOfMustCall>
+// (or equivalently List<? extends Socket>, etc.).
+// We should revisit this test if we ever revisit the idea of adding support for owning generics.
 
 import java.util.*;
 import org.checkerframework.checker.mustcall.qual.*;
 
 @InheritableMustCall("a")
 class ListOfMustCall {
   static void test(ListOfMustCall lm) {
+    // :: error: type.argument
     List<ListOfMustCall> l = new ArrayList<>();
     // add(E e) takes an object of the type argument's type
     l.add(lm);
@@ -15,6 +21,7 @@ static void test(ListOfMustCall lm) {
   }
 
   static void test2(ListOfMustCall lm) {
+    // :: error: type.argument
     List<@MustCall("a") ListOfMustCall> l = new ArrayList<>();
     l.add(lm);
     l.remove(lm);

checker/tests/mustcall/PlumeUtilRequiredAnnotations.java

@@ -1,3 +1,13 @@
+// 6/1/2023: We changed the defaulting rules despite the issues described below, because we
+// discovered
+// a soundness bug that demonstrated that the only reason the other defaulting scheme didn't
+// produce a lot of false positive errors was a mistake in our implementation: both defaulting
+// schemes produce (different) false positives, but defaulting to @MustCall({}) on the upper bound
+// of type variables limits the extra warnings to code that actually uses resources, which
+// is desirable. As part of this change, the expected warnings in this test case were removed,
+// but I have preserved their locations with "a type.argument error used to be issued here".
+// The following comment is the original note attached to this test case:
+
 // This is a test case that shows off some places in plume-util where
 // annotations were required, even though we'd have preferred the defaulting
 // rules to result in those annotations being the defaults.
@@ -17,13 +27,13 @@ class PlumeUtilRequiredAnnotations {
   // must be explicit rather than implicit (see that the eqR field never issue errors,
   // just like the eqS fields).
   class MultiRandSelector<T, S extends @Nullable @MustCall Object, R extends Object> {
-    // :: error: (type.argument)
+    // a type.argument error used to be issued here.
     private Partitioner<T, T> eqT;
     private Partitioner<S, S> eqS;
     private Partitioner<R, R> eqR;
 
     // Adding annotations to the definition of Partitioner doesn't fix this problem:
-    // :: error: (type.argument)
+    // a type.argument error used to be issued here.
     private Partitioner2<T, T> eqT2;
     private Partitioner2<S, S> eqS2;
     private Partitioner2<R, R> eqR2;

checker/tests/mustcall/TypeArgs.java

@@ -4,11 +4,9 @@ public class TypeArgs {
 
   static class A<Q extends @MustCall({"carly"}) Object> {}
 
-  // :: error: (type.argument)
   static class B<S> extends A<S> {}
 
   public <T> void f1(Generic<T> real, Generic<? super T> other, boolean flag) {
-    // :: error: (type.argument)
     f2(flag ? real : other);
   }
 

checker/tests/resourceleak/ACRegularExitPointTest.java

@@ -109,13 +109,17 @@ void innerfunc3() {
 
     Foo f = makeFoo();
     f.a();
-    Function<Foo, @CalledMethods("a") Foo> innerfunc =
+    Function<@MustCall Foo, @CalledMethods("a") @MustCall Foo> innerfunc =
         st -> {
           // :: error: (required.method.not.called)
           Foo fn1 = new Foo();
           Foo fn2 = makeFoo();
           fn2.a();
-          return fn2;
+          // The need for this cast is undesirable, but is a consequence of our approach to
+          // generic types. In this case, this cast is clearly safe (a() has already been called,
+          // so the obligation is satisfied on the returned value, as intended).
+          // :: warning: cast.unsafe
+          return ((@MustCall Foo) fn2);
         };
 
     innerfunc.apply(f);

checker/tests/resourceleak/ACSocketTest.java

@@ -151,6 +151,11 @@ void test(String address, int port) {
 
   protected Socket sock;
 
+  // This type.argument error is undesirable, but is a necessary consquence of our approach to
+  // handling generics in the Must Call Checker, which prevents containers from having
+  // @MustCall("close") type arguments without errors (in exchange for avoiding many false
+  // positives on containers that do not have must-call obligations on their component types).
+  // :: error: type.argument
   void connectToLeader(AtomicReference<Socket> socket) throws IOException {
     // :: error: (required.method.not.called)
     if (socket.get() == null) {

checker/tests/resourceleak/DuplicateError.java

@@ -1,6 +1,5 @@
 import java.util.List;
 import java.util.function.Function;
-import org.checkerframework.checker.mustcall.qual.MustCallUnknown;
 import org.checkerframework.checker.nullness.qual.KeyForBottom;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.checkerframework.checker.nullness.qual.UnknownKeyFor;
@@ -14,11 +13,9 @@ void m(List<?> values) {
 
 class DECollectionsPlume {
   public static <
-          @KeyForBottom FROM extends @Nullable @UnknownKeyFor @MustCallUnknown Object,
-          @KeyForBottom TO extends @Nullable @UnknownKeyFor @MustCallUnknown Object>
-      List<TO> mapList(
-          @MustCallUnknown Function<@MustCallUnknown ? super FROM, ? extends TO> f,
-          Iterable<FROM> iterable) {
+          @KeyForBottom FROM extends @Nullable @UnknownKeyFor Object,
+          @KeyForBottom TO extends @Nullable @UnknownKeyFor Object>
+      List<TO> mapList(Function<? super FROM, ? extends TO> f, Iterable<FROM> iterable) {
     return null;
   }
 }