Skip to content

Some NonNull annotations not recognized#2694

Merged
hazendaz merged 12 commits intospotbugs:masterfrom
gtoison:issue-2558
Mar 1, 2024
Merged

Some NonNull annotations not recognized#2694
hazendaz merged 12 commits intospotbugs:masterfrom
gtoison:issue-2558

Conversation

@gtoison
Copy link
Copy Markdown
Contributor

@gtoison gtoison commented Nov 10, 2023

Added support for:

  • edu.umd.cs.findbugs.annotations.NonNull
  • edu.umd.cs.findbugs.annotations.Nullable
  • org.netbeans.api.annotations.common.NonNull
  • org.netbeans.api.annotations.common.NullAllowed
  • lombok.NonNull

ernest-emmanuel-utibe

This comment was marked as off-topic.


static final ClassDescriptor androidNullable = DescriptorFactory.createClassDescriptor("android/support/annotation/Nullable");

static final ClassDescriptor androidNonNull = DescriptorFactory.createClassDescriptor("android/support/annotation/NonNull");

This comment was marked as resolved.

@JuditKnoll
Copy link
Copy Markdown
Collaborator

I looked into what causes the build fail. There is an NP_NULL_ON_SOME_PATH error in line 291 of edu.umd.cs.findbugs.sarif.Location class (inside the inner class LogicalLocation) of the method toJsonObject():

properties.forEach((k, v) -> propertiesBag.addProperty(k, v));

because the properties field has the annotation edu.umd.cs.findbugs.annotation.Nullable which is added in this PR:
@Nullable
final Map<String, String> properties = new HashMap<>();

@gtoison
Copy link
Copy Markdown
Contributor Author

gtoison commented Nov 16, 2023

Thank you very much for looking into it, I did not have much time to investigate but I saw that the code is handling the @TypeQualifierNickname on edu.umd.cs.findbugs.annotation.Nullable so, indeed, it is not necessary to add that one.
I probably made a mistake but when trying to remove it and building on my computer I still had the failing unit tests though, I couldn't understand why

@gtoison
Copy link
Copy Markdown
Contributor Author

gtoison commented Nov 20, 2023

Hello @JuditKnoll,
How did you see that issue with edu.umd.cs.findbugs.sarif.Location?
I'm probably doing it the wrong way but I use to build with the gradlew test command and then I look at tests results

The error I see is:

org.opentest4j.AssertionFailedError: Unexpected bugs (6):
missing NP_STORE_INTO_NONNULL_FIELD In TestNonNull1.java
missing NP_NONNULL_PARAM_VIOLATION At TestNonNull1.java:[line 30]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull1.java:[line 18]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull1.java:[lines 24-25]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull2.java:[lines 33-34]
missing NP_NONNULL_PARAM_VIOLATION At TestNonNull2.java:[lines 13-14]
	at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:38)
	at app//org.junit.jupiter.api.Assertions.fail(Assertions.java:138)
	at app//edu.umd.cs.findbugs.DetectorsTest.checkForUnexpectedBugs(DetectorsTest.java:142)
	at java.base@17.0.8.1/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base@17.0.8.1/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base@17.0.8.1/java.util.ArrayList.forEach(ArrayList.java:1511)

@JuditKnoll
Copy link
Copy Markdown
Collaborator

I simply ran ./gradlew build or the same command as the github action does ./gradlew build smokeTest, which gives the following warning at the end* (this is from the github action check) :

Execution failed for task ':spotbugs:spotbugsMain'.
> A failure occurred while executing com.github.spotbugs.snom.internal.SpotBugsRunnerForHybrid$SpotBugsExecutor
   > Verification failed: SpotBugs ended with exit code 1. See the report at: file:///home/runner/work/spotbugs/spotbugs/spotbugs/build/reports/spotbugs/main.html

I looked at the report which contained that the error is at edu.umd.cs.findbugs.sarif.Location$LogicalLocation, line 291.
I have my doubts about whether the properties field really need to have the annotation edu.umd.cs.findbugs.annotation.Nullable, but I haven't checked it extensively.

The strange thing is, that earlier my output was the same as that of the github action check, but now, without any modification to the earlier version, the :spotbugs-tests:test task fails too, and get the same error as well as you wrote about. So there may be multiple problems.


*There are some other messages in connection with DumbMethods and IllegalArgumentEception when analyzing Issue2640, but those are not relevant here, since it's thrown even when the build is successful ( see the successful build on master ), and I think it's already solved #2643, just the maven plugin doesn't contain it yet.

@gtoison
Copy link
Copy Markdown
Contributor Author

gtoison commented Nov 20, 2023

I think that these warnings are from the spotbugs gradle plugin running on spotbugs' own code, so the analysis itself is done from an already released version, not from the freshly compiled version

EDIT: the issue was SpotBug flagging an issue in it's own code (in edu.umd.cs.findbugs.sarif.Location) thanks to the change

@JuditKnoll
Copy link
Copy Markdown
Collaborator

Hello @JuditKnoll, How did you see that issue with edu.umd.cs.findbugs.sarif.Location? I'm probably doing it the wrong way but I use to build with the gradlew test command and then I look at tests results

The error I see is:

org.opentest4j.AssertionFailedError: Unexpected bugs (6):
missing NP_STORE_INTO_NONNULL_FIELD In TestNonNull1.java
missing NP_NONNULL_PARAM_VIOLATION At TestNonNull1.java:[line 30]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull1.java:[line 18]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull1.java:[lines 24-25]
missing NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE At TestNonNull2.java:[lines 33-34]
missing NP_NONNULL_PARAM_VIOLATION At TestNonNull2.java:[lines 13-14]
	at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:38)
	at app//org.junit.jupiter.api.Assertions.fail(Assertions.java:138)
	at app//edu.umd.cs.findbugs.DetectorsTest.checkForUnexpectedBugs(DetectorsTest.java:142)
	at java.base@17.0.8.1/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base@17.0.8.1/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base@17.0.8.1/java.util.ArrayList.forEach(ArrayList.java:1511)

It looks like the error at edu.umd.cs.findbugs.sarif.Location$LogicalLocation is only hiding the one you got. Maybe converting these failing tests to AbstractIntegrationTest could help, or at least the debuging could be a bit easier.

@gtoison gtoison marked this pull request as ready for review February 24, 2024 20:46
Comment thread spotbugsTestCases/src/java/ghIssues/Issue2558.java Outdated
@hazendaz
Copy link
Copy Markdown
Member

@gtoison Can you fix merge conflict?

@gtoison
Copy link
Copy Markdown
Contributor Author

gtoison commented Feb 29, 2024

I have merged the conflict

@hazendaz hazendaz self-assigned this Mar 1, 2024
@hazendaz hazendaz added this to the SpotBugs 4.8.4 milestone Mar 1, 2024
@hazendaz hazendaz merged commit e1825c0 into spotbugs:master Mar 1, 2024
PatrikScully pushed a commit to PatrikScully/spotbugs that referenced this pull request Jun 14, 2024
* test: reproducer for issue spotbugs#2558

* Recognize more nullability annotations

* fix: added back check for Eclipse's NonNullByDefault

* fix: made eclipseNonNullByDefault a field again since used externally

* fix: with the other changes the smoke tests flag that field as NonNull

LogicalLocation.properties should be annotated NonNull

* doc: added changelog entry

* fix: updated the comment now that the false positive is fixed
@JuditKnoll JuditKnoll linked an issue Oct 13, 2025 that may be closed by this pull request
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.

False positive about the rule NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT

5 participants