Skip to content

Map subclass with @CheckForNull raises bugs on all Map.get() #3583

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 5, 2025
Merged

Map subclass with @CheckForNull raises bugs on all Map.get() #3583

merged 3 commits into from
Aug 5, 2025

Conversation

gtoison
Copy link
Contributor

@gtoison gtoison commented Aug 5, 2025 •
edited
Loading

See #3573
Guava's HashBiMap is annotated with @CheckForNull and implements Map.
Potentially all Map.get() are from a HashBiMap so they would need to be checked for nullness

@gtoison gtoison self-assigned this Aug 5, 2025
@iloveeclipse
Copy link
Member

I was 5 minutes too late :-)
See #3573 (comment)

@gtoison
Copy link
Contributor Author

gtoison commented Aug 5, 2025

This looks similar indeed 👍
I'm working on a fix to revert to the previous behavior but there might be a case to raise bugs for such unchecked calls, maybe in a later version of SpotBugs?

@iloveeclipse
Copy link
Member

I'm working on a fix to revert to the previous behavior

Thanks! Would be nice to have it restored.

but there might be a case to raise bugs for such unchecked calls, maybe in a later version of SpotBugs?

Sure. In fact, I highly dislike such sloppy style of programming, especially in MT related code it is always a nice surprise to developers that next get() can return not same value as before (independently on null/non null specs)...

But also with the null annotated implementation classes you never know if the given object that implement same interface would need extra checks or not, which is then again surprise at runtime.

gtoison added 2 commits August 5, 2025 11:25
@gtoison
fix: do not retain the "check for null" flag when merging IsNullValue
3e39e52 
When a call might be from different implementations (e.g. Map.get()
might be from HashMap or from Guava's HashBiMap) and when at least one
implementation has the CheckForNull annotation we currently don't want
the value to also have the flag.
We can revisit this later to raise potential bugs, because Map.get()
might indeed return null.
@gtoison
doc: updated changelog entry to link the regression caused by the change
8509c7d
@gtoison gtoison requested a review from iloveeclipse August 5, 2025 09:38
@gtoison gtoison linked an issue Aug 5, 2025 that may be closed by this pull request
Inconsistent & unexpected NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE reported #3573
Closed
@iloveeclipse iloveeclipse merged commit c2e18cf into spotbugs:master Aug 5, 2025
15 checks passed
@iloveeclipse
Copy link
Member

Thanks!

@JuditKnoll JuditKnoll added this to the Spotbugs 4.9.4 milestone Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveeclipse iloveeclipse iloveeclipse approved these changes

Assignees

@gtoison gtoison

Labels
bug regression
Projects
None yet
Milestone
Spotbugs 4.9.4
Development

Successfully merging this pull request may close these issues.

Inconsistent & unexpected NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE reported
3 participants
@gtoison @iloveeclipse @JuditKnoll