NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION erroneously issued on equals(@Nullable Object)

[marquiswang]

If I add an equals override with a @javax.annotation.Nullable annotation (from com.google.code.findbugs:jsr305), I get an NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION

"M D NP: Method TestObject.equals(Object) overrides the nullness annotation of parameter other in an incompatible way At TestObject.java:[lines 6-12]"

import javax.annotation.Nullable;

public final class TestObject {
    @Override
    public boolean equals(@Nullable Object other) {
        if (other == this) {
            return true;
        }
        if (!(other instanceof TestObject)) {
            return true;
        }
        return false;
    }

    @Override
    public int hashCode() {
        return getClass().hashCode();
    }
}

The description on the error says:

Method tightens nullness annotation on parameter
A method should always implement the contract of a method it overrides. Thus, if a method takes a parameter that is marked as @nullable, you shouldn't override that method in a subclass with a method where that parameter is @nonnull. Doing so violates the contract that the method should handle a null parameter.

This shouldn't be applying, since we're overriding with @nullable.

[ThrawnCA]

I can reproduce this. There's no annotation on Object.equals, so yeah, looks like a false positive. Will take a look.

[ThrawnCA]

OK, having dug around a bit, this stems from the fact that FindBugs (since 2007) implicitly adds a CheckForNull annotation on the equals method parameter (along with a bunch of other methods; see edu.cmd.cs.findbugs.ba.DefaultNullnessAnnotations). There doesn't seem to be information in the history about why that was applied, but it does make a lot of sense; an equals method does need to be able to compare the object to null. It's even in the JDK documentation: "For any non-null reference value x, x.equals(null) should return false."

So I don't understand why anyone would annotate an equals method with @Nullable, which is supposed to counteract all other nullness annotations.

The resulting message could be better, however; perhaps annotating equals with anything less than @CheckForNull should have its own detector?

[marquiswang]

I have to admit I was not aware of @CheckForNull. I'd been using @Nullable incorrectly for years, it seems. (While Googling around for the difference, I found this thread which indicates that I'm not the only one: https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000214944-Why-javax-annotation-CheckForNull-is-not-included-in-nullable-annotations-set-by-default- )

I would be happy switching to annotating my equals methods with @CheckForNull instead.

[ThrawnCA]

I'd been using @nullable incorrectly for years, it seems.

Yeah, apparently Nullable is supposed to behave as if there were no annotation at all. Though in this case there's a conflict.

I won't close this yet, because we should do some better handling of this case (and perhaps similar ones).

[marquiswang]

I have a somewhat radical suggestion. How about we treat @Nullable on parameters exactly like @CheckForNull?

As I understand it, @Nullable only really makes sense for return values. An method implementation with @Nullable or @CheckForNull parameters will always have to do a null check to be safe.

This will also work for all the people who have been using @Nullable like how jetbrains defines it.

[JLLeitschuh]

I 100% agree that this is a non-intuitive error.

Especially when combined with this issue here in the Kotlin compiler:
https://youtrack.jetbrains.net/issue/KT-27194

[rmalani]

+1 This is a non-intuitive error, please resolve.

[marquiswang]

@ThrawnCA Any objections to my suggestion above to treat @Nullable on parameters the same as @CheckForNull? I could try a PR if that is acceptable.

[ThrawnCA]

@marquiswang That would need to be discussed with @spotbugs/everyone, not just me.

[marquiswang]

Is there a process for that discussion?

[KengoTODA]

No well-defined process we have, just have discussion at here then it's OK.

To keep backward compatibility (at least in 3.1.x release), I prefer not changing behaviour but adding another bug pattern that detects misuse of @Nullable in parameter.

I know JSR305 and spotbugs-annotations are not intuitive. However changing their behaviour will introduce more and different complexity. So if we need different behaviour, it's better to migrate to another solution like annotation provided by Checker Framework, I think.

[iloveeclipse]

I can't remember if I've asked this few years ago on the FB list, what is the rationale behind @Nullable and @CheckForNull existing at same time to represent possibly null values but being treated differently.

The javadoc on them explains it:

@CheckForNull

@Nonnull(when = When.MAYBE)
The annotated element might be null, and uses of the element should check for null

@Nullable

@Nonnull(when = When.UNKNOWN)
The annotated element could be null under some circumstances. 
In general, this means developers will have to read the documentation to determine 
when a null value is acceptable and whether it is necessary to check for a null value.

The presence of two different annotations in FB is highly surprising for everyone, especially because Eclipse (and the rest of the world) used their versions of @Nullable in the same sense as @CheckForNull, and for example one must teach Eclipse compiler to use @CheckForNull instead of @Nullable on projects using FindBugs to avoid wrong compiler warnings generated.

So, in my POV this was an unfortunate decision in the very early days of NP analysis invented by JSR-305 authors.

As a result, very surprising, if I remember it right, FB will not warn about possible NPE on dereferencing values marked as @Nullable. I personally can't understand this, but I have not spent that amount of time thinking about NP issues like the JSR-305 authors did, so my opinion is not an expert opinion, but more an opinion of a user and practically thinking engineer. For me this is a nonsense, unfortunately very old one.

Ideally we would change @CheckForNull to @Nullable and treat them in the same way, to avoid user confusion on the "duality of null annotations".

However, this will be a breaking change for all FindBugs/SpotBugs users, and probably also to the FB engine itself. I have now no real insights how deep in the FB engine the difference is rooted and how much code we need to change to remove this "duality of null annotations".

So if we would do this, we should do this on 4.x branch.

See also https://wiki.eclipse.org/JDT_Core/Null_Analysis

[marquiswang]

I too was surprised to find that there is both @CheckForNull and @Nullable. IntelliJ and the jetbrains nullability annotations also treat @Nullable as @CheckForNull, so that makes two of the major Java IDEs that use @Nullable differently. I would definitely support changing the SpotBugs version to be consistent (and updating JSR-305? I'm not sure how that works).

Conceptually, I kind of like the concept of having a third nullability annotation that covers some of the weird edge cases where technically a field or a method can be null but you can be confident that it never will. On example is two fields for which you know that exactly one is always null. They are both @Nullable, but once you've checked one you don't need to check the other. That said, this is a rare enough edge case that I definitely don't think it's worth the additional confusion given what most people think @Nullable means.

[ngeor]

First time spotbugs user here. I have this problem with auto-generated source code from the Immutables library.

A bit of googling shows that Findbugs had this issue as well back in 2017 https://stackoverflow.com/questions/46760970/immutables-lib-adds-nullable-to-equals-method

Is there any progress on this issue?

[jglick]

FWIW I do sometimes use @Nullable in its documented sense—that the conditions under which null might be accepted are too subtle for static analysis to bother with.