Feature Request: Use an annotation like @MonotonicNonNull for better reasoning about field nullability inside lambdas

[agrieve]

Similar to #570, but not stream-specific.

We've hit this scenario 3 or 4 times in chrome so far:

import org.jspecify.annotations.Nullable;
import org.jspecify.annotations.NullMarked;

@NullMarked
public class Foo {
  @Nullable Object mThing;

  void method(Object a, @Nullable Object b) {
    mThing = new Object();
    Runnable r = () -> mThing.hashCode();
    r.run();
  }
}

Foo.java:10: warning: [NullAway] dereferenced expression mThing is @Nullable
    Runnable r = () -> mThing.hashCode();
                             ^
    (see http://t.uber.com/nullaway )

• When the field in question starts as null, but has only non-null assignments, then it is safe to assume it is non-null for dereferences that occur after an assignment (since it can never return to being null).
• For lambdas that are constructed after an assignment, we should not warn about dereferences made by lambdas.

[msridhar]

When the field in question starts as null, but has only non-null assignments, then it is safe to assume it is non-null for dereferences that occur after an assignment (since it can never return to being null).

Within a single method, we already have this level of reasoning, and it could be extended to callee methods via @RequiresNonNull.

For lambdas that are constructed after an assignment, we should not warn about dereferences made by lambdas.

In the example the code is safe because of the r.run() call that happens in the same method. But in a case where run() is invoked later or asynchronously, it's not clear from a local analysis that mThing wasn't set to null again before the lambda executes. If there are only non-null assignments then even an asynchronous invocation is safe. But proving that requires a combination of global reasoning (only non-null assignments) and local reasoning (that the lambda is only created after such an assignment) that we don't do right now, and may not be straightforward to add.

Does the real code use a Runnable? Or is it something where it is more obvious that the lambda executes immediately and not asynchronously?

[agrieve]

Here's the most recent example: https://chromium-review.googlesource.com/c/chromium/src/+/6264464/6/components/messages/android/java/src/org/chromium/components/messages/MessageWrapper.java#140

I think all of the real examples are for code that is run asynchronously, which I guess makes this different from streams.

I don't love it, but maybe could use a new annotation to remove the need for global reasoning. E.g. @OnceNullable.

I thought this one might be hard :P. It's certainly something we can live without.

[msridhar]

Thinking more this seems like it could be handled if we had better support for checking a @MonotonicNonNull annotation (like that from Checker Framework). If we have a @MonotonicNonNull field f, and we know locally within a method m that f is initialized before a lambda l is created, then we can analyze the body of l assuming f is @NonNull. That part wouldn't be hard at all. The only more substantial thing to implement would be checking that only @NonNull values are assigned to @MonotonicNonNull fields; and that might not be too hard either. So maybe adding this support is doable, assuming folks are willing to add @MonotonicNonNull annotations to the relevant fields.

[agrieve]

Well that's a much better name than I came up with :P. I'm sure we'd use it if support were added.

[agrieve]

Wow, great stuff! Looking forward to trying this out!

[msridhar]

We should have a new release out soon!

[msridhar]

@agrieve NullAway 0.12.4 is now released with this fix

[agrieve]

Just tried this out. Working great for non-static fields, but it looks like it's treated the same as @Nullable for static fields.

[msridhar]

@agrieve sorry I didn't test static fields. I think I know what needs to be done though. Can you open a separate issue to make it easier to track?