Better handling of iterating over a map's key set

[msridhar]

Example:

   void foo(Map<String, Object> m){
     for(String k: m.keySet()) {
       m.get(k).toString(); // NullAway thinks it's a null deref
     }
   }

NullAway currently warns on the above code, but it does not warn for the following code:

   void foo2(Map<String, Object> m){
     String k = "baz";
     for(m.containsKey(k)) {
       m.get(k).toString(); 
     }
   }

Ideally, we would generalize our handling of the second case to handle the first case as well. Developers may find it strange that the second case is handled but not the first.

[msridhar]

Some thoughts here. Consider the following code:

import java.util.*;
class MyClass {
    void test(Map m) {
	for (Object o: m.keySet()) {
	    m.get(o);
	}
    }
}

The Checker Framework generates the following control-flow graph for the test() method.
MyClass-test-Map.dot.pdf

This is the key part of it:

Based on this CFG, I think we could pattern-match to handle the common case of code like the above that uses an enhanced for loop, as follows. During dataflow analysis, at an assignment node:

1. Check if the RHS is of the form m.keySet().iterator() and the LHS is a Checker-generated temp var like iter#num0. If so, this is an iterator() call corresponding to an enhanced for loop. In the successor store, we could add an access path of the form m.get(iter#num0.next()) as @NonNull.
2. Check if the RHS is of the form iter#num0.next() and the LHS is a local variable like o. If m.get(iter#num0.next()) is in the predecessor store, we could add m.get(o) to the successor store. This is essentially a very limited form of local must-alias tracking.

Checking for temporary variables like iter#num0 is important since for such variables created for enhanced for loops, we know they will never be re-assigned, which makes step 2 a safe-ish operation. It is still possible that m gets re-assigned to a different Map between the two assignments matched above, leading to unsoundness. But we are already unsound for such cases; e.g., we do not report an error for the following case:

void mapReassign(Map m, Object o) {
  if (m.containsKey(o)) {
    m = new HashMap();
    m.get(o).toString(); // null dereference!
  }
}

So the proposed handling would not add a new source of unsoundness.

@lazaroclapp thoughts?

[lazaroclapp]

This makes perfect sense to me. I assume we don't know of any other case where CF uses iter#num[...] as a naming pattern (other than enhanced for loops)? Also, no variable in the code itself can have a name clashing with those temporaries, right?

In that case, this seems perfectly safe to me (where safe is "as safe as our existing handling of .containsKey(...) or better, as noted above).