Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Control flow analysis fails within a stream collector #934

Closed
wbadam opened this issue Mar 15, 2024 · 5 comments · Fixed by #938
Closed

Control flow analysis fails within a stream collector #934

wbadam opened this issue Mar 15, 2024 · 5 comments · Fixed by #938

Comments

@wbadam
Copy link

wbadam commented Mar 15, 2024

I think this may be the same as #332 which was closed recently.

NullAway is unable to infer nullity on a dereference made inside a stream collector.

In the example below, streams1() passes when the filter is added, and fails otherwise. In streams2(), there is a dereference warning for foo.bar even though the filter makes sure the value is non-null.

public class Streams {
    record Foo(@Nullable String bar, String baz) {}

    // NullAway check passes ✅ 
    List<Integer> streams1() {
        List<Foo> foos = new ArrayList<>();
        return foos.stream()
                .filter(foo -> foo.bar != null)
                .map(foo -> foo.bar.length())
                .toList();
    }

    // NullAway check fails on dereferencing `foo.bar` within the collector ❌ 
    Map<Integer, String> streams2() {
        List<Foo> foos = new ArrayList<>();
        return foos.stream()
                .filter(foo -> foo.bar != null)
                .collect(Collectors.toMap(foo -> foo.bar.length(), foo -> foo.baz));
    }
}

NullAway version: 0.10.24

@msridhar
Copy link
Collaborator

Hi @wbadam I was able to reproduce this. Support for this kind of reasoning on streams is limited to particular constructs where we were confident the handling was correct. I think we could probably extend this to handle Collectors.toMap, but it doesn't quite fit the patterns we were matching before, so it may take a bit of work. @lazaroclapp do you have any thoughts here?

@msridhar
Copy link
Collaborator

@wbadam I have an initial PR up for this case in #938. Can you think of any related stream scenarios involving filters where NullAway is reporting false positives for you all?

@wbadam
Copy link
Author

wbadam commented Mar 25, 2024

@msridhar After a quick search I've found a couple of similar cases:

  • .collect(Collectors.groupingBy(foo -> foo.bar.length()));
  • .collect(ImmutableMap.toImmutableMap(foo -> foo.bar.length(), foo -> foo.baz)); (less important I guess as it could be replaced by native api)

There may be more that I couldn't immediately find, but these I reckon would cover most of our cases.

@msridhar
Copy link
Collaborator

@wbadam I've added support for those cases to #938.

msridhar added a commit that referenced this issue Mar 27, 2024
Fixes #934

The key new thing with the support here is we have further nesting.
Rather than a `map` method, where the relevant lambda is passed
directly:
```java
stream.filter(foo -> foo.bar != null).map(foo -> foo.bar.baz)
```
In this case we have a `collect` call, which gets as its argument the
result of `Collectors.toMap`, and the relevant lambdas are passed to
`toMap`:
```java
stream
  .filter(foo -> foo.bar != null)
  .collect(Collectors.toMap(foo -> foo.bar.baz, foo -> foo.bar.other))
```
Supporting this requires some new types of logic in our streams handler
(particularly because there are multiple relevant lambdas for a single
`collect` call). We do also handle anonymous inner classes.
@msridhar
Copy link
Collaborator

FYI @wbadam this fix was part of the 0.10.25 release from today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants