Reason about iterating over a map's key set using an enhanced for loop #554

msridhar · 2022-01-15T01:00:23Z

Fixes #535

Uses the strategy outlined in this comment: #535 (comment)

coveralls · 2022-01-15T01:05:25Z

Pull Request Test Coverage Report for Build #720

0 of 0 changed or added relevant lines in 0 files are covered.
44 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.1%) to 89.495%

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java	6	96.17%
../nullaway/src/main/java/com/uber/nullaway/dataflow/NullnessStore.java	9	89.02%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java	29	90.07%

Totals
Change from base Build #716:	0.1%
Covered Lines:	4200
Relevant Lines:	4693

💛 - Coveralls

lazaroclapp

A few comments and questions, but overall looks good!

Did we run our performance benchmarks on this change?

lazaroclapp · 2022-01-18T22:09:19Z

nullaway/src/test/java/com/uber/nullaway/NullAwayKeySetIteratorTests.java

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import java.util.*;",


I know this is test code, so arguably this doesn't matter, but shouldn't this be just import java.util.Map?

lazaroclapp · 2022-01-18T22:09:36Z

nullaway/src/test/java/com/uber/nullaway/NullAwayKeySetIteratorTests.java

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import java.util.*;",


lazaroclapp · 2022-01-18T22:10:36Z

nullaway/src/test/java/com/uber/nullaway/NullAwayKeySetIteratorTests.java

+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import java.util.*;",


Even here, I'd just import Map and HashMap.

Just, in general, mild vote for listing the imports individually as if this were real code. But I am open to the opposite argument.

Agreed, fixed in 4257191

lazaroclapp · 2022-01-18T22:13:10Z

nullaway/src/test/java/com/uber/nullaway/NullAwayKeySetIteratorTests.java

+            "public class Test {",
+            "  public void keySetStuff(Map<Object, Object> m) {",
+            "    // BUG: Diagnostic contains: dereferenced expression",
+            "    m.get(m.keySet().iterator().next()).toString();",


Is this unhandled or a true positive? Technically the map could be empty....

If the Map were empty, this would cause a NoSuchElementException, not an NPE. So I would say our report is a false positive, since it refers to the possibility of an NPE.

lazaroclapp · 2022-01-18T22:17:41Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java

+     * as this class is designed specifically for reasoning about iterating over map keys using an
+     * enhanced-for loop over a {@code keySet()}, and for such cases the iterator is always stored
+     * locally
+     */


If we only support locals, is there a more precise type than Element?

Yup, tightened to VariableElement in 76ddc4c

lazaroclapp · 2022-01-18T22:31:13Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

+   * {@code null}.
+   */
+  @Nullable
+  private Node getMapNodeForKeySetIteratorCall(MethodInvocationNode invocationNode) {


Worth a preconditions check that the method being called is Set::iterator()? Or do worry about the extra performance hit of those checks and feel the isEnhancedForIteratorVariable(...) check on the caller and our assumptions about CF's CFG generation are enough of a check?

In general, I was trying to minimize the overhead from unnecessary checks. I am very confident in the invariants we rely on in terms of the CF CFG right now. My hope was that if we bump the CF version and something changes, our tests would fail. I guess there is the possibility that CF would change in a way that would cause us to start thinking other types of expressions were non-null erroneously, introducing a new unsoundness. That seems pretty unlikely to me, but if you'd like, I can introduce some extra sanity checking. I would want to limit that checking to not run for every assignment statement, though, instead only for cases where we are pretty sure we're in an enhanced for loop. WDYT?

Your call. I think you are way more familiar with the CF internals than I am, if you feel the invariant is pretty much set in stone and requires no runtime checking, I am fine with that. That said, if you think there is a reasonable value in adding sanity checking, I definitely agree we can defer it until right before the updates.set(...) call. Though, again, if you think our assumptions are unlikely to break, I am also fine leaving this as is.

lazaroclapp · 2022-01-18T22:32:56Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

+          updates.set(mapWithIteratorContentsKey, NONNULL);
+        }
+      }
+    } else {


Why else { if {...} } instead of else if { ... }?

Good catch, fixed in e628368

lazaroclapp · 2022-01-18T22:37:03Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

+    } else {
+      // Check for an assignment lhs = iter#numX.next().  From the structure of Checker Framework
+      // CFGs, we know that if iter#numX is the receiver of a call on the rhs of an assignment, it
+      // must be a call to next().


Wonder if we should trust that CFG is stable enough here, or if we should be checking the method symbol for Iterator::next(). We can do this after verifying isEnhancedForIteratorVariable(...) but before generating the AP, if we are worried about the performance hit from doing so on every method invocation.

See my comment above. If you'd like a check here, is it ok to do it only if mapGetPath != null? That would limit the perf impact even more (we will likely have many enhanced-for loops that are not over the key set of a map).

lazaroclapp · 2022-01-18T22:46:54Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java

+   * Creates an access path identical to {@code accessPath} (which must represent a map), but with
+   * {@code mapKey} as its map {@code get()} argument
+   */
+  public static AccessPath withMapKey(AccessPath accessPath, MapKey mapKey) {


So, the way we are using this method is that we have an AP of the form ap1 = x.y.m.get(iter#numX) (which is not real, more of a virtual map get) and see v = iter#numX.next() so we call this method with (ap1, v) to obtain ap1 = x.y.m.get(v). Correct?

If so, maybe this should be replaceMapKey(...)? Or, if that's still missing some usage where this is called an access path without an existing map key, then the javadoc should mention it nonetheless "{@code mapKey} as its map {@code get()} argument, replacing any map key information in the original {@code accessPath}."

You are correct. We currently only expect this method to be used when accessPath already represents a map get. Renamed the method and clarified Javadoc in 0fca272. I don't think a further precondition check is needed here, but I can add if you like

msridhar · 2022-01-21T17:17:32Z

Did we run our performance benchmarks on this change?

No, not yet, I'll do that now.

Thanks for the review @lazaroclapp! I addressed most of the comments. There is still an open question about how many well-formedness checks we should add on the CFG; see my inline comments.

lazaroclapp

See comment below. I'd be fine with this landing as is after performance benchmarking. I leave adding well-formedness checks up to what you think makes sense. If you are pretty sure the CFG invariant will not change in an unexpected way for future Java constructs, then it makes sense to omit those checks for performance reasons (but having them and deferring them to the last possible moment is always an option).

lazaroclapp · 2022-01-21T18:28:13Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

+   * {@code null}.
+   */
+  @Nullable
+  private Node getMapNodeForKeySetIteratorCall(MethodInvocationNode invocationNode) {


Your call. I think you are way more familiar with the CF internals than I am, if you feel the invariant is pretty much set in stone and requires no runtime checking, I am fine with that. That said, if you think there is a reasonable value in adding sanity checking, I definitely agree we can defer it until right before the updates.set(...) call. Though, again, if you think our assumptions are unlikely to break, I am also fine leaving this as is.

msridhar · 2022-01-21T18:56:13Z

Here are the perf benchmarking results. master branch:

Benchmark                      Mode  Cnt   Score   Error  Units
AutodisposeBenchmark.compile  thrpt   25  10.837 ± 0.078  ops/s
CaffeineBenchmark.compile     thrpt   25   2.390 ± 0.013  ops/s

With this PR:

Benchmark                      Mode  Cnt   Score   Error  Units
AutodisposeBenchmark.compile  thrpt   25  10.851 ± 0.070  ops/s
CaffeineBenchmark.compile     thrpt   25   2.362 ± 0.019  ops/s

The changes seem to be within the noise.

msridhar · 2022-01-21T19:19:29Z

I decided to add the sanity checks, just to be safe. Will re-run perf benchmarks now. If the checks do end up causing overhead we care about, we can remove them later (or possibly find something else small to optimize).

lazaroclapp

Approved iff no performance regression :)

lazaroclapp · 2022-01-21T19:32:30Z

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java

        // receiver represents the map
        return baseInvocation.getTarget().getReceiver();
      }
    }
    return null;
  }

+  private boolean isCallToMethod(


msridhar · 2022-01-21T20:22:10Z

Approved iff no performance regression :)

Re-ran and perf differences are within noise. Landing 🙂

msridhar added 11 commits January 12, 2022 09:20

WIP

11e1206

Merge branch 'master' into keyset-iterator

e18e7a6

very hacky

b399dc6

cleanup

88ff350

cleanup

d716751

cleanup

32c8e8c

cleanup

451c75e

docs

d66047b

Merge branch 'master' into keyset-iterator

c94b27e

Merge branch 'master' into keyset-iterator

f7fcb98

more comments

72bc1f0

msridhar added 2 commits January 15, 2022 09:22

more tests

ea75a46

another test

a546ce2

msridhar marked this pull request as ready for review January 15, 2022 18:34

msridhar requested a review from lazaroclapp January 15, 2022 18:34

msridhar changed the title ~~[WIP] Reason about iterating over a map's key set using an enhanced for loop~~ Reason about iterating over a map's key set using an enhanced for loop Jan 15, 2022

msridhar added 5 commits January 15, 2022 15:01

Merge branch 'master' into keyset-iterator

ca9655d

Extend new base test class

5c32698

Merge branch 'master' into keyset-iterator

722911c

add some braces

7f9a385

Merge branch 'master' into keyset-iterator

1739ee2

lazaroclapp reviewed Jan 18, 2022

View reviewed changes

msridhar added 4 commits January 21, 2022 08:54

Use specific imports in tests

4257191

Tighten type to VariableElement

76ddc4c

Change nested if to else if

e628368

Rename method

0fca272

msridhar requested a review from lazaroclapp January 21, 2022 17:16

lazaroclapp reviewed Jan 21, 2022

View reviewed changes

Add sanity checks, minimizing perf impact

3c989fb

lazaroclapp approved these changes Jan 21, 2022

View reviewed changes

msridhar merged commit 4ec519c into master Jan 21, 2022

msridhar deleted the keyset-iterator branch January 21, 2022 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reason about iterating over a map's key set using an enhanced for loop #554

Reason about iterating over a map's key set using an enhanced for loop #554

msridhar commented Jan 15, 2022 •

edited

Loading

coveralls commented Jan 15, 2022 •

edited

Loading

lazaroclapp left a comment

lazaroclapp Jan 18, 2022

lazaroclapp Jan 18, 2022

lazaroclapp Jan 18, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

lazaroclapp Jan 18, 2022

msridhar Jan 21, 2022

msridhar commented Jan 21, 2022

lazaroclapp left a comment

lazaroclapp Jan 21, 2022

msridhar commented Jan 21, 2022

msridhar commented Jan 21, 2022 •

edited

Loading

lazaroclapp left a comment

lazaroclapp Jan 21, 2022

msridhar commented Jan 21, 2022

Reason about iterating over a map's key set using an enhanced for loop #554

Reason about iterating over a map's key set using an enhanced for loop #554

Conversation

msridhar commented Jan 15, 2022 • edited Loading

coveralls commented Jan 15, 2022 • edited Loading

Pull Request Test Coverage Report for Build #720

💛 - Coveralls

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Jan 21, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Jan 21, 2022

msridhar commented Jan 21, 2022 • edited Loading

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Jan 21, 2022

msridhar commented Jan 15, 2022 •

edited

Loading

coveralls commented Jan 15, 2022 •

edited

Loading

msridhar commented Jan 21, 2022 •

edited

Loading