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

Suggest this == obj instead of super.equals(obj) when the two are equivalent. #4147

Merged
merged 0 commits into from
Oct 16, 2023

Conversation

copybara-service[bot]
Copy link
Contributor

Suggest this == obj instead of super.equals(obj) when the two are equivalent.

(inspired by Marcono1234's google/guava#6780)

@copybara-service copybara-service bot closed this Oct 16, 2023
@copybara-service copybara-service bot merged commit 3047300 into master Oct 16, 2023
@copybara-service copybara-service bot deleted the test_573814421 branch October 16, 2023 21:32
@Marcono1234
Copy link
Contributor

Thanks @cpovirk!

If you have time and energy for this, do you think it would also make sense to cover super.hashCode actually calling Object.hashCode? This also seems to happen now and then (e.g. elastic/elasticsearch#100871 or netty/netty#13659).

@cpovirk
Copy link
Member

cpovirk commented Oct 18, 2023

You're welcome. It has found way more problems than I expected, so thanks!

hashCode had occurred to me at one point, but I'd set it aside without much thought. I should have reconsidered after seeing how prevalent the equals problems were. I'm seeing what that turns up now. If it's as good as I'd expect, I'll productionize it.

@cpovirk
Copy link
Member

cpovirk commented Oct 18, 2023

The hashCode check turns up a bunch of bugs (many in the same file as the equals bugs, unsurprisingly) though maybe with a relatively larger fraction of inadvisable but not necessarily buggy methods. For example:

return id == null ? super.hashCode() : id.hashCode();

There are also some false-positive examples similar to what I'd seen with equals, in which people do some bit of work before delegating to super.

The checker is probably still worth doing, but it's not a complete no-brainer, so I'll try to come back to this when I have more brain to spare.

@cpovirk
Copy link
Member

cpovirk commented Oct 18, 2023

Eh, it looks like those are (mostly? entirely?) in classes whose equals methods we'd also be flagging. And really, in both cases, I think it makes sense to inline the == or System.identityHashCode operation instead of hiding it behind a super call that obscures things. So I'll mail out the review and see what happens.

copybara-service bot pushed a commit that referenced this pull request Oct 19, 2023
…o "`SuperCallToObjectMethod`."

...as suggested by @Marcono1234 in #4147 (comment).

Also, add docs.

PiperOrigin-RevId: 574501701
copybara-service bot pushed a commit that referenced this pull request Oct 19, 2023
…o "`SuperCallToObjectMethod`."

...as suggested by @Marcono1234 in #4147 (comment).

Also, add docs.

PiperOrigin-RevId: 574898389
@Marcono1234
Copy link
Contributor

Thanks a lot for extending the check and also for the extensive documentation!

@cpovirk
Copy link
Member

cpovirk commented Nov 22, 2023

(I just took a quick peek at super calls to Object.toString(), and they look less broken. I suspect that a few are bugs, but many look legitimate (e.g., delegating to super for prod builds but producing more verbose output for dev builds).)

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 this pull request may close these issues.

2 participants