Skip to content

Conversation

@basil
Copy link
Member

@basil basil commented Jun 25, 2024

Recent versions of spotbugs-maven-plugin no longer ignore Nullable and treat the presence of this annotation as a reason to log a new warning whenever anyone calls this method and doesn't check the result for null. Since our plugins do this in dozens of places this creates a lot of new warnings. This PR restores the old behavior by removing the Nullable annotation.

Testing done

  • mvn clean verify -DskipTests
  • Reproduced the problem in workflow-durable-task-step plugin with SpotBugs 4.8.6.0; verified that after this PR most of the new warnings went away.

@jglick
Copy link
Member

jglick commented Jul 5, 2024

log a new warning whenever anyone calls this method and doesn't check the result for null

Hmm, that is supposed to be what @CheckForNull is for. Whereas IIUC @Nullable is supposed to be an explicit declaration that the conditions under which the value is null are too complex for mechanical analysis and you should trust the human author.

public abstract @Nullable <T> T get(Class<T> key) throws IOException, InterruptedException;
public abstract <T> T get(Class<T> key) throws IOException, InterruptedException;

@Override public abstract void onSuccess(@Nullable Object result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has not caused me any issues so far in the handful of tests I ran with the new version of SpotBugs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let us leave it for now, but it seems that SpotBugs devs have picked an interpretation of @Nullable which basically makes it a confusing synonym for @CheckForNull which should probably just be avoided generally.

@jglick jglick merged commit 3ee58b4 into jenkinsci:master Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants