Special handling for equals(Object)

[cpovirk]

A sentiment that I've heard in past discussions of nullness is: "Object.equals(Object) is known to be required to handle a null input, so implementations of equals shouldn't need to annotate the parameter explicitly."

I personally believe we should require people to annotate it anyway. (This is part of my general dislike for special cases [edit: link goes to a discussion of java.lang.Void]. FWIW, I believe the Checker Framework requires it to be annotated.) But I'm filing this bug in anticipation that we'll get such a request someday -- maybe from someone who is reading this right now and will tell me that I have it all wrong :) Even if not, we will eventually want to write up a justification of why we're going this direction.

[note to self / other Googlers: internal issue 12964927]

[cpovirk]

See also google/guava#1819

Also: It looks like the need to annotate equals caused some specific problems internally because of our NullPointerTester. Fortunately, NullPointerTester should have little reason to exist once we have standardize nullness annotations.

[cpovirk]

(This came up in the context of code generation: The code generator was smart enough to copy @Nullable annotations from methods that were declared with them, but it naturally didn't copy @Nullable from boolean equals(Object obj) because there is no @Nullable in the source. This isn't necessarily a big problem, though: The implementation code just looks like it has an unnecessary null check (unlikely to be an error?), and callers won't have a reference whose static type is the generated code, so they'll see the inherited Object.equals with its implicit annotations (or some other supertype, which is hopefully annotated correctly). Again, our problem was with NullPointerTester, which is a strange beast.)

[ edit: also about generated code: #34 (comment) ]

[kevinb9n]

(Re: NullPointerTester, there is wording in the requirements doc to the effect that we will throw it under the bus whenever we have to.)

I think we should be able to agree that there should be no special treatment of equals. The top method Object.equals will have its parameter marked @Nullable and therefore any override that changes it to @NotNull (explicitly or implicitly) will get a warning. People will over time develop the same habit that we did, hitting @ almost automatically after equals(. The best way to improve on that situation is really just to keep eliminating the reasons why people have to write those methods by hand in the first place.

(If the subparam is of "unspecified nullness" that's a whole other discussion that probably belongs in an issue that is specific to issues of unspecified nullness)

[cpovirk]

I am personally in favor of no special case. But I am open to hearing that the fallout for, say AutoValue, is more complex than we'd hope. Ideally AutoValue comes to recognize us as the One True Nullness Annotation and then include it on the equals method in all cases that it's present on the classpath. (But does this have implications for strict-deps enforcement? I guess it's not the end of the world for users to have to add another dep to use AutoValue. Maybe AutoValue would even export it? Or maybe the right rule is to add the annotation if @DefaultNotNull is in effect? Probably that?)

(I still don't see a need to Do Anything about this at the moment.)

[kevinb9n]

(I wonder if this is an issue for AutoValue at all. If it just doesn't output an annotation at all, it would generate a warning upon compiling the generated code, but imho users should not be shown warnings that come from compiling generated code anyway. Then everyone who access equals will be doing it via a supertype anyway, so no warnings there either.)

[cpovirk]

Oh, nice. I hadn't considered that warnings would probably be off for generated code.

[cpovirk]

Optimistically closing, since we at least have a story here now. (I guess "we" always did, but I'm just now coming to understand it :))

[cpovirk]

In #301, I just mentioned that uber/NullAway#619 points out that record classes end up with an equals method whose parameter is not annotated @Nullable.

I don't think that's an argument for special cases for equals in general: Record classes are unusual in that you don't write the source code for them. So I'd apply any special case only for record classes.

[kevinb9n]

JSpecify can spin this in several ways:

• Since the MyRecord.equals parameter comes from somewhere that is not a null-marked context, it has unspecified nullness. As such tools can treat is as they see fit, and smart ones know to treat it as nullable.
• If your record class is null-marked then the parameter in question is officially non-null, yet smart tools will still treat it as nullable anyway.
• JSpecify specifies that such parameters are always considered nullable