address trivial coderabbit PR comments

Author: Sascha Kehrli

checker/src/main/java/org/checkerframework/checker/collectionownership/CollectionOwnershipChecker.java

@@ -1,5 +1,6 @@
 package org.checkerframework.checker.collectionownership;
 
+import java.util.LinkedHashSet;
 import java.util.Set;
 import org.checkerframework.checker.rlccalledmethods.RLCCalledMethodsChecker;
 import org.checkerframework.common.basetype.BaseTypeChecker;
@@ -17,7 +18,8 @@ public CollectionOwnershipChecker() {}
 
   @Override
   protected Set<Class<? extends SourceChecker>> getImmediateSubcheckerClasses() {
-    Set<Class<? extends SourceChecker>> checkers = super.getImmediateSubcheckerClasses();
+    Set<Class<? extends SourceChecker>> checkers =
+        new LinkedHashSet<>(super.getImmediateSubcheckerClasses());
     checkers.add(RLCCalledMethodsChecker.class);
     return checkers;
   }

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

@@ -1,2 +1,2 @@
-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
-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 of return type.%nfound   : no ownership annotation or @Owning%nrequired: @NotOwning%nConsequence: method %s in %s cannot override method %s in %s
+owning.override.param=Incompatible ownership of parameter %s.%nfound   : no ownership annotation or @NotOwning%nrequired: @Owning%nConsequence: method %s in %s cannot override method %s in %s

checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakCollections.java

@@ -6,7 +6,7 @@
 import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest;
 import org.junit.runners.Parameterized.Parameters;
 
-/** Tests for the Resource Leak Checker. */
+/** Tests for the Resource Leak Checker for Collections. */
 public class ResourceLeakCollections extends CheckerFrameworkPerDirectoryTest {
   public ResourceLeakCollections(List<File> testFiles) {
     super(

checker/tests/resourceleak/EnhancedFor.java

@@ -26,9 +26,9 @@ void test2(List<@MustCall Socket> list) {
   }
 
   void test3(List<Socket> list) {
-    // With the extension of the RLC to collections, this is no longer an error.
-    // The return type of Iterator#next is NotOwning, because the iterator does not
-    // own the elements, but instead the host collection the iterator is associated with.
+    // With the RLC collection extension, this is no longer an error.
+    // Iterator#next returns @NotOwning: iterators do not own their elements; ownership
+    // stays with the host collection associated with the iterator.
     for (Socket s : list) {}
   }
 

checker/tests/resourceleak/Issue4815.java

@@ -10,7 +10,7 @@ public <T extends Component> void initialize(
     object.initialize();
     // `list` resolves to List<@MustCall Component> and thus cannot accept
     // an element of non-empty @MustCall type enforced by the Java
-    // typechecker. This is an unfortunate consequence of the otherwise
+    // type checker. This is an unfortunate consequence of the otherwise
     // elegant extension of the RLC to collections, which doesn't detect
     // that object already fulfilled its obligation here.
     // :: error: argument

checker/tests/resourceleak/SocketIntoList.java

@@ -23,10 +23,9 @@ public List<Socket> test2(@OwningCollection List<Socket> l) {
   }
 
   public void test3(List<@MustCall({}) Socket> l) throws Exception {
-    // although s is illegally assigned into the list l, an error
-    // for required.method.not.called is not additionally reported at
-    // this declaration site of s. list#add(@Owning E) takes on
-    // the obligation of the argument.
+    // although s is illegally added to l, a required.method.not.called error
+    // is not additionally reported at this declaration site. List#add(@Owning E) takes on
+    // the obligation of its argument.
     Socket s = new Socket();
     s.bind(new InetSocketAddress("192.168.0.1", 0));
     // l cannot hold elements with non-empty @MustCall values

dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizer.java

@@ -108,8 +108,8 @@ public interface CFGVisualizer<
    * element.
    *
    * @param ice the iterated collection element
-   * @param value the value of the local variable
-   * @return the String representation of the local variable
+   * @param value the value associated with {@code ice}
+   * @return the String representation of this entry
    */
   String visualizeStoreIteratedCollectionElt(IteratedCollectionElement ice, V value);
 

dataflow/src/main/java/org/checkerframework/dataflow/expression/IteratedCollectionElement.java

@@ -20,12 +20,12 @@ public class IteratedCollectionElement extends JavaExpression {
   /**
    * Creates a new IteratedCollectionElement.
    *
-   * @param var a CFG node
+   * @param node CFG node
    * @param tree an AST tree
    */
-  public IteratedCollectionElement(Node var, Tree tree) {
-    super(var.getType());
-    this.node = var;
+  public IteratedCollectionElement(Node node, Tree tree) {
+    super(node.getType());
+    this.node = node;
     this.tree = tree;
   }
 

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java

@@ -3020,8 +3020,8 @@ protected void checkExceptionParameter(CatchTree tree) {
 
   /**
    * Returns a set of AnnotationMirrors that is a lower bound for exception parameters. If a user
-   * writes a more specific (lower) annotation that this on an exception parameter, the Checker
-   * Framework issues a warning saying that the written annotation cannot be verified.
+   * writes a more specific (lower) annotation than this on an exception parameter, the Checker
+   * Framework issues a warning indicating that the written annotation cannot be verified.
    *
    * <p>This implementation returns top; subclasses can change this behavior.
    *
@@ -3086,7 +3086,7 @@ protected void checkThrownExpression(ThrowTree tree) {
    * Returns a set of AnnotationMirrors that is an upper bound for thrown exceptions.
    *
    * <p>Note: by default this method returns {@code getExceptionParameterLowerBoundAnnotations()},
-   * so that this annotation is enforced.
+   * so that the same bound is enforced.
    *
    * <p>(Default is top.)
    *

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java

@@ -1238,7 +1238,9 @@ private S upperBound(S other, boolean shouldWiden) {
       if (thisVal != null) {
         V otherVal = e.getValue();
         V mergedVal = upperBoundOfValues(otherVal, thisVal, shouldWiden);
-        newStore.iteratedCollectionElements.put(ice, mergedVal);
+        if (mergedVal != null) {
+          newStore.iteratedCollectionElements.put(ice, mergedVal);
+        }
       }
     }