uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.63k stars 293 forks source link

Initializer method field state not correctly detected #1040

Closed protocol7 closed 1 month ago

protocol7 commented 1 month ago

With the following (somewhat contrived minimised example from our code base), NullAway does not detect that the field state for state is initialized:

package com.hello.nullaway;

public class Foo {
    enum State {
        SOME_STATE
    }

    private State state;

    public Foo() {
        foo();
    }

    private void foo() {
        bar();
    }

    private void bar() {
        state = State.SOME_STATE;
    }
}

Resulting in the following error:

src/main/java/com/hello/nullaway/Foo.java:10: error: [NullAway] initializer method does not guarantee @NonNull field state (line 8) is initialized along all control-flow paths (remember to check for exceptions or early returns).
    public Foo() {
           ^
    (see http://t.uber.com/nullaway )

NullAway is configured with -XepOpt:NullAway:AnnotatedPackages=com.hello.

When the field is of a non-enum type, e.g. an int or a String, this works as expected. If bar() is called directly from the constructor, it works as expected.

msridhar commented 1 month ago

Hi there, this is expected behavior. In the docs here:

NullAway only detects cases where a field is initialized via a direct call to another method from a constructor, not via a chain of calls.

This was a deliberate decision trading off some convenience for improved checker performance (following full call trees could get expensive).

I don't see any different when I make the field a String; I see the same error.

Are there initialization patterns in your code base where this limitation makes things very inconvenient?

protocol7 commented 1 month ago

Thanks for the quick response and for the pointer to the documentation, I had missed this. Fully understand the tradeoff.

I don't see any different when I make the field a String; I see the same error.

You are right on changing the type to String, I must have made a mistake in my testing. The error does go away for me if changing to an int though.

Are there initialization patterns in your code base where this limitation makes things very inconvenient?

No, not necessarily. We are starting to add NullAway analysis to a very large codebase, so I expect we will run into a few of these. I suspect most can be refactored, although that of course adds some friction and confusion (as proven here).

msridhar commented 1 month ago

The error does go away for me if changing to an int though.

Yes, NullAway does not do any checking around initialization of primitive type fields, since they can't be de-referenced (so no NPE possible).

Are there initialization patterns in your code base where this limitation makes things very inconvenient?

No, not necessarily. We are starting to add NullAway analysis to a very large codebase, so I expect we will run into a few of these. I suspect most can be refactored, although that of course adds some friction and confusion (as proven here).

Understood. Please let us know if you have any other questions or issues.

We've also developed the NullAway Annotator to help in adding initial @Nullable annotations to a code base, in case it's of interest. Happy to help in support of that tool also.