Propagate more nullability info to lambdas known to be invoked synchronously #952

msridhar · 2024-04-24T21:33:37Z

Fixes #941

We propagate full nullability info from the enclosing context to callbacks passed to Map.forEach, Iterable.forEach, List.removeIf, and all methods on java.util.stream.Stream

codecov · 2024-04-24T21:40:28Z

Codecov Report

Attention: Patch coverage is 77.14286% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 86.06%. Comparing base (719b167) to head (b8b8078).

Files	Patch %	Lines
.../nullaway/handlers/SynchronousCallbackHandler.java	83.33%	1 Missing and 6 partials ⚠️
...er/nullaway/handlers/OptionalEmptinessHandler.java	14.28%	6 Missing ⚠️
...a/com/uber/nullaway/handlers/CompositeHandler.java	77.77%	1 Missing and 1 partial ⚠️
...m/uber/nullaway/handlers/AccessPathPredicates.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #952      +/-   ##
============================================
- Coverage     86.08%   86.06%   -0.02%     
- Complexity     2018     2032      +14     
============================================
  Files            79       81       +2     
  Lines          6612     6669      +57     
  Branches       1280     1294      +14     
============================================
+ Hits           5692     5740      +48     
- Misses          510      516       +6     
- Partials        410      413       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lazaroclapp · 2024-04-26T21:00:25Z

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

+            "        if (this.target == null) {",
+            "            throw new IllegalArgumentException();",
+            "        }",
+            "        this.resolved = new MyMap<>();",


Maybe use a type other than MyMap for this.resolved here? Got a bit confused think that was what changed the test case, but it doesn't, right? What matters is that target is MyMap.

See 8a03676

lazaroclapp · 2024-04-26T21:01:28Z

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

+            "        List<Object> l = new ArrayList<>();",
+            "        l.forEach(v -> System.out.println(v + this.f.toString()));",
+            "        Iterable<Object> l2 = l;",
+            "        l2.forEach(v -> System.out.println(v + this.f.toString()));",


Worth adding a case where we set this.f = null then do one of these and show it now reports an error?

See 26bc3a0

lazaroclapp · 2024-04-26T21:04:04Z

nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayStreamSupportPositiveCases.java

@@ -191,7 +191,8 @@ private Stream<T> test1(Stream<T> stream) {

    private Stream<T> test2(Stream<T> stream) {
      Preconditions.checkNotNull(ref);
-      // BUG: Diagnostic contains: dereferenced expression ref is @Nullable
+      // no error since we propagate nullability facts to stream callbacks, which
+      // in sane code are invoked soon after the stream is created


I guess I go a bit back and forth between "this is probably safe" and "restricting it to cases where the stream doesn't escape the method will remove most false positives too and be safer". Your call.

Let me think through a bit more what it would take to implement the safer version.

@lazaroclapp I thought about this more and I think a reasonably precise safer version would be kind of a pain to implement. My thought was to try to detect if the overall chain of stream calls ended in a terminal operation. But there are a bunch of terminal operations on Stream itself, and then there are methods to convert Stream to IntStream, DoubleStream, etc., each with their own terminal operations. (As an aside, in a follow-up we should probably handle those primitive stream types in the same way that we handle Streams themselves.) We could also just check if the enclosing method has a return type of Stream, but that is imprecise, and also misses cases like passing as a parameter or storing in a field.

Bottom line, my feeling is at this point, we are most likely ok going with the less safe version. But, I think it's time we add a wiki page on our stream handling in general, documenting how we propagate nullability facts from filter() methods to subsequent callbacks and also this new propagation of nullability facts from the enclosing context. On that page we could give caveats on cases where our handling could be unsound, and why we decided to do what we did.

Let me know what you think.

Of course, what this really needs is a new type system about the state of Stream objects... but in the absence of that, just eagerly propagating nullability of APs in the closure is probably safe enough (kidding about the Stream type-state system... mostly)

To close the loop here I added a wiki page with some docs:

https://github.com/uber/NullAway/wiki/Stream-Handling

nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java

lazaroclapp · 2024-04-26T21:08:19Z

nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java

+  /**
+   * An AccessPath predicate that always returns false. Used for optimizing
+   * getAccessPathPredicateForNestedMethod.
+   */
+  static final Predicate<AccessPath> FALSE_AP_PREDICATE = ap -> false;
+
+  /**
+   * An AccessPath predicate that always returns true. Used for optimizing
+   * getAccessPathPredicateForNestedMethod.
+   */
+  static final Predicate<AccessPath> TRUE_AP_PREDICATE = ap -> true;


Why part of CompositeHandler vs Handler? Also, maybe we want Handler.AccessPathPredicates.[TRUE|FALSE]_AP_PREDICATE? (We can always static import to avoid repetition)

See my comment above, I'm happy with making this change if you think it's better

lazaroclapp · 2024-04-26T21:11:27Z

nullaway/src/main/java/com/uber/nullaway/handlers/OptionalEmptinessHandler.java

-    }
-    return false;
+      return false;
+    };


Unrelated to this PR, but... are we missing unit tests for this handler? Or is Codecov wrong?

We are in fact missing coverage! It turns out that #678 made the pseudo-field simulating contents of an Optional final, which means we already (safely) propagate nullability facts about that pseudo-field into lambdas, which means we no longer need this handler method! I'll go ahead and delete this method in a separate PR. Nice to have the test coverage exposed!

lazaroclapp · 2024-04-26T21:14:47Z

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

   */
-  boolean includeApInfoInSavedContext(AccessPath accessPath, VisitorState state);
+  Predicate<AccessPath> getAccessPathPredicateForNestedMethod(TreePath path, VisitorState state);


Interesting that our existing stream handling support doesn't really use this Handler method, it seems... I assume because of how handlers work that we just end up composing the information about x we get from the stream handler when looking at s.filter(x -> ...).map(x -> ...) with the info this gives us about the APs from the closure?

Yes, that's exactly right. This handler method is used to decide which access paths from the enclosure should be maintained when analyzing a lambda, beyond the standard ones (paths involving effectively-final locals). Before I think we only used this for the OptionalEmptinessHandler

lazaroclapp · 2024-04-26T21:22:04Z

nullaway/src/main/java/com/uber/nullaway/handlers/SynchronousCallbackHandler.java

+    Preconditions.checkArgument(
+        leafNode instanceof ClassTree || leafNode instanceof LambdaExpressionTree,
+        "Unexpected leaf type: %s",
+        leafNode.getClass());


Why do we know the leafNode is a ClassTree or LambdaExpressionTree here? (I assume at some point we are filtering out method references and the like?

Edit: Guess it has to do with where we call updateEnvironmentMapping. But by the time we get to getNullnessInfoBeforeNewContext that's no longer very clear in our docs/checks.

Good point, tweaked the docs and method name in 34bb9f3

msridhar added 7 commits March 30, 2024 09:26

test case

b3da601

Merge branch 'master' into issue-941

7913e5b

WIP

343cb29

WIP

bddd3be

another test

c344998

more

282c55a

removeIf

f139ef7

msridhar mentioned this pull request Apr 24, 2024

Variable checked non-null outside a forEach or Stream lambda triggers an error #941

Closed

msridhar added 4 commits April 25, 2024 12:55

stream methods

c364624

fix test case

75a9626

tweak comment

589bb6d

docs

0b46478

msridhar marked this pull request as ready for review April 25, 2024 23:13

msridhar requested review from lazaroclapp and yuxincs April 25, 2024 23:13

handle forEach on Iterable

beab844

msridhar changed the title ~~[WIP] Propagate more nullability info to lambdas known to be invoked synchronously~~ Propagate more nullability info to lambdas known to be invoked synchronously Apr 25, 2024

lazaroclapp reviewed Apr 26, 2024

View reviewed changes

msridhar added 4 commits April 27, 2024 17:48

tweak test

8a03676

test another case

26bc3a0

update docs and rename method

34bb9f3

fix test

1ee4ee0

msridhar mentioned this pull request Apr 28, 2024

Remove OptionalEmptinessHandler method for propagating access paths to lambdas #953

Closed

msridhar requested a review from lazaroclapp April 29, 2024 17:45

lazaroclapp approved these changes Apr 30, 2024

View reviewed changes

msridhar added 2 commits April 29, 2024 19:51

factor out fields

bfd77cd

tweak docs

b8b8078

msridhar merged commit edcfe49 into uber:master Apr 30, 2024
10 of 12 checks passed

msridhar deleted the issue-941 branch April 30, 2024 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate more nullability info to lambdas known to be invoked synchronously #952

Propagate more nullability info to lambdas known to be invoked synchronously #952

msridhar commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024 •

edited

Loading

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024 •

edited

Loading

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024

msridhar Apr 29, 2024

lazaroclapp Apr 30, 2024

msridhar May 17, 2024

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024

lazaroclapp Apr 26, 2024

msridhar Apr 28, 2024

Propagate more nullability info to lambdas known to be invoked synchronously #952

Propagate more nullability info to lambdas known to be invoked synchronously #952

Conversation

msridhar commented Apr 24, 2024 • edited Loading

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

msridhar Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar Apr 28, 2024 • edited Loading

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 Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 24, 2024 •

edited

Loading

msridhar Apr 28, 2024 •

edited

Loading

msridhar Apr 28, 2024 •

edited

Loading