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

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

Closed
vrasidas-at-b2c2 opened this issue Jan 19, 2024 · 9 comments · Fixed by #898
Closed

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

vrasidas-at-b2c2 opened this issue Jan 19, 2024 · 9 comments · Fixed by #898
Assignees

Comments

@vrasidas-at-b2c2
Copy link

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
Copy link
Collaborator

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

@msridhar msridhar self-assigned this Jan 19, 2024
@msridhar
Copy link
Collaborator

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
Copy link
Author

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

@msridhar
Copy link
Collaborator

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
Copy link

cpovirk commented Jan 25, 2024

(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 #764?)

msridhar added a commit that referenced this issue Jan 25, 2024
Fixes #897 

This fixes a regression in our handling of implicit `equals()` methods
in interfaces when building on JDK 21. I see this as an interim fix,
until we can fix NullAway to properly always assume / enforce that the
parameter of `equals()` is `@Nullable`. But, I think we should fix the
regression in the short term.
@vrasidas-at-b2c2
Copy link
Author

vrasidas-at-b2c2 commented Jan 29, 2024

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
Copy link
Collaborator

yuxincs commented Jan 29, 2024

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

copybara-service bot pushed a commit to google/error-prone that referenced this issue Feb 1, 2024
…er` is a subclass.

We accomplish this by calling `baseSymbol()` on the initial `Symbol`.

(We likely should do this in more places in Error Prone, including the location in `ASTHelpers.getSymbol` that I've flagged and perhaps anywhere else that we directly access `tree.sym`.)

Thanks to @msridhar for links to relevant JDK discussions, notably [JDK-8293890](https://bugs.openjdk.org/browse/JDK-8293890).

Effects:
- This change definitely helps with [static imports](uber/NullAway#764), as demonstrated in a new test in `ParameterNameTest`, which fails before this change. (I didn't actually look into why the test fails before. Maybe the "fake" `Symbol` lacks the parameter names of the original?)
- I didn't check whether it helps with [`someSerializable.equals`](uber/NullAway#897). It seems likely to, but _shrug_ for now.
- I had expected this to help with the `TestCase.fail` handling in `ReturnMissingNullable`, but it turns out that I'm wrong on multiple levels. I updated its comments and tests accordingly:
   - `TestCase.fail` _does_ exist as a declared method nowadays; it's not merely inherited from `Assert`. I must have been looking at [quite an old version](junit-team/junit4@dde798f#diff-f9834c0e0ef1d54e1757e221b7b4248c2ba8e0de41d2f0fadac5927b906edd85R226).
   - The problem there comes not from the need for `ASTHelpers.getSymbol` to use `baseSymbol()` but instead for the need for `MethodMatchState.ownerType` to look at the actual `owner` instead of the type of the receiver tree. (If it started to do that, it would then benefit from this change to produce the correct `owner`.) I added a link to the appropriate bug there.
PiperOrigin-RevId: 601163959
copybara-service bot pushed a commit to google/error-prone that referenced this issue Feb 1, 2024
…er` is a subclass.

We accomplish this by calling `baseSymbol()` on the initial `Symbol`.

(We likely should do this in more places in Error Prone, including the location in `ASTHelpers.getSymbol` that I've flagged and perhaps anywhere else that we directly access `tree.sym`.)

Thanks to @msridhar for links to relevant JDK discussions, notably [JDK-8293890](https://bugs.openjdk.org/browse/JDK-8293890).

Effects:
- This change definitely helps with [static imports](uber/NullAway#764), as demonstrated in a new test in `ParameterNameTest`, which fails before this change. (I didn't actually look into why the test fails before. Maybe the "fake" `Symbol` lacks the parameter names of the original?)
- I didn't check whether it helps with [`someSerializable.equals`](uber/NullAway#897). It seems likely to, but _shrug_ for now.
- I had expected this to help with the `TestCase.fail` handling in `ReturnMissingNullable`, but it turns out that I'm wrong on multiple levels. I updated its comments and tests accordingly:
   - `TestCase.fail` _does_ exist as a declared method nowadays; it's not merely inherited from `Assert`. I must have been looking at [quite an old version](junit-team/junit4@dde798f#diff-f9834c0e0ef1d54e1757e221b7b4248c2ba8e0de41d2f0fadac5927b906edd85R226).
   - The problem there comes not from the need for `ASTHelpers.getSymbol` to use `baseSymbol()` but instead for the need for `MethodMatchState.ownerType` to look at the actual `owner` instead of the type of the receiver tree. (If it started to do that, it would then benefit from this change to produce the correct `owner`.) I added a link to the appropriate bug there.
PiperOrigin-RevId: 603374262
@cpovirk
Copy link

cpovirk commented Feb 1, 2024

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

  • That commit doesn't fix all parts of Error Prone, so it might not help here. (But from glancing at Fix bug with implicit equals() methods in interfaces #898, I'm guessing that it does help.)
  • You may want to keep your workaround in place even after the Error Prone release so as to support people who aren't using newer versions.

@msridhar
Copy link
Collaborator

msridhar commented Feb 1, 2024

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 :-)

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.

4 participants