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

False positive on equals(nullable) when using NullAway with Java 21. #897

Closed vrasidas-at-b2c2 closed 8 months ago

vrasidas-at-b2c2 commented 9 months ago

We are attempting to migrate our codebase from Java 17 to 21. We have run into a peculiar issue where NullAway started producing false positives when calling on equals() with a nullable parameter. This did not use to be the case.

NullAway version: 0.10.19 and 0.10.21

Repro:

import javax.annotation.Nullable;

public class Repro {

    public interface AnInterface {}

    public static boolean foo(AnInterface a, @Nullable AnInterface b) {
        return a.equals(b);
    }
}

The above when built with Java 21 (OpenJDK 64-Bit Server VM Zulu21.28+85-CA (build 21+35, mixed mode, sharing) results in error:

/Repro.java:10: error: [NullAway] passing @Nullable parameter 'b' where @NonNull is required
        return a.equals(b);
                        ^
    (see http://t.uber.com/nullaway )

When built with Java 17 (OpenJDK 64-Bit Server VM Zulu17.32+13-CA (build 17.0.2+8-LTS, mixed mode, sharing) ) no error is generated.

msridhar commented 9 months ago

I've reproduced this bug, thanks for reporting it! It's a strange one. I'll work on a fix.

msridhar commented 9 months ago

This is due to the fix for https://bugs.openjdk.org/browse/JDK-8272564 landing in JDK 18. Further discussion of technical details here: https://bugs.openjdk.org/browse/JDK-8272715. I'll continue working on a fix here; I think we'll need to somehow special case implicit methods in interfaces that come from java.lang.Object.

vrasidas-at-b2c2 commented 8 months ago

Thank you for the quick turnaround @msridhar! Much appreciated!

msridhar commented 8 months ago

Sure thing, we will try to get this landed soon. Once it lands would it be helpful to you if we cut a release with the change?

cpovirk commented 8 months ago

(I'm wondering if we can fix this on the Error Prone side inside ASTHelpers.getSymbol. I'm doing some initial testing now.... Thanks for the report, @vrasidas-at-b2c2, and for the JDK links, @msridhar! This might also help with https://github.com/uber/NullAway/issues/764?)

vrasidas-at-b2c2 commented 8 months ago

Once again thank you so much for the quick turnaround!

Once it lands would it be helpful to you if we cut a release with the change?

@msridhar extremely so! 😁

Right now, as a workaround we are forced to use Objects.equals() which adds unnecessary noise to the code. The fix should enable us to move to Java 21 without having to do these code changes.

yuxincs commented 8 months ago

@vrasidas-at-b2c2 New release cut, please wait a few hours for it to be propagated :)

cpovirk commented 8 months ago

It's possible that https://github.com/google/error-prone/commit/e5a6d0d8f9f96bda8e9952b7817cd0d2b63e51be (which will be in the next Error Prone release) will eliminate the need for a NullAway workaround. However:

msridhar commented 8 months ago

Thanks @cpovirk! Yes we will need to keep our workaround until we decide to require the next Error Prone release from all users. But I can try to remember to add a comment once the next release comes out so we remember to remove our workaround eventually :-)