infra: Enabling Spot-Bug in CircleCI#16665
Conversation
0577627 to
35e7e58
Compare
|
@romani can you review please. CI error is unrelated to the changes. |
|
@romani Ping. |
| <Class name="com.puppycrawl.tools.checkstyle.checks.sizes.RecordComponentNumberCheck"/> | ||
| <Bug pattern="AT_STALE_THREAD_WRITE_OF_PRIMITIVE"/> | ||
| </Match> | ||
| <!-- New Suppressions --> |
There was a problem hiding this comment.
I am confused , why this violations appears?
It should be same as execution of mvn clean verify
There was a problem hiding this comment.
Hey @romani I suspect the difference arises because mvn verify runs later build phases that optimize/modify bytecode , while mvn test-compile spotbugs:check analyzes raw, unprocessed bytecode, exposing violations(e.g., redundant null checks) may be due to incomplete plugin/annotation processing.
There was a problem hiding this comment.
Also I have referenced the Issue correctly now.
There was a problem hiding this comment.
ok, please analyze if this suppressins should stay forever, or we need to fix them in followup PRs.
There was a problem hiding this comment.
Hey @romani , No need of fixing them, they should be there in the exclusion list until there is a newer version of spotbug with a fix for this flase positives. I analyzed some of the High Confidence Checks done by SpotBug, Almost all of them are false positives. I will give a example in Description to clarify why this is happening.
There was a problem hiding this comment.
please analyze all of them, if there is already defect, find link to it and place comment <!-- until .... -->
if issue not existing, please create and place same "until" comment.
please create issue on spotbugs plugin to get to know why validation by mvn clean verify is different from mvn spotbugs:check, already existing spotbugs/spotbugs-maven-plugin#806
There was a problem hiding this comment.
Ok, we need to slice and dice current suppressions by :
- Suppression until spotbug issue is fixed
- point to improve in checkstyle codebase, separate issue.
We need enablement to be merged quicker to prevent new leaks of violations.
There was a problem hiding this comment.
Done. Apparently changes are needed in Build file which might lead to less false positives according to comment : spotbugs/spotbugs-maven-plugin#806 (comment). So until then we will suppress the false positives.
There was a problem hiding this comment.
Please remove <!-- New Suppressions --> line, it has no value
|
please change commit message to have prefix "infra:" , we reference issues in prefix only to our issue, not others, you can put link to spotbugs in commit message after prefix. |
15c7dad to
48f40e8
Compare
| </Match> | ||
|
|
||
| <!-- Suppress AFBR_ABNORMAL_FINALLY_BLOCK_RETURN --> | ||
| <!-- until https://github.com/spotbugs/spotbugs-maven-plugin/issues/806 --> |
There was a problem hiding this comment.
Please create new issue on checkstyle to fix all such violations.
Suppression file should have links to checkstyle issue until which suppression stays.
It will be separate activities to resolve them.
| <Class name="com.puppycrawl.tools.checkstyle.checks.sizes.RecordComponentNumberCheck"/> | ||
| <Bug pattern="AT_STALE_THREAD_WRITE_OF_PRIMITIVE"/> | ||
| </Match> | ||
| <!-- New Suppressions --> |
There was a problem hiding this comment.
Please remove <!-- New Suppressions --> line, it has no value
|
please recheck suppressions, some already fixed. |
73affde to
2268b91
Compare
2268b91 to
8a3c8a1
Compare
Earlier Disabled : #14863
Due to SpotBug Issue Mentioned in :spotbugs/spotbugs-maven-plugin#806
Local Results on : mvn clean test-compile spotbugs
Most of the Bugs Flagged by SpotBugs are False Positives
For Example Lets Take This High-Confidence Error Flagges by SpotBug :
This is the part of the code where Error is Flagged :
What is the Error?
Ans:The error RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE occurs because Spotbugs detects a redundant null check on a variable (stream in your code) that is guaranteed to be non-null due to prior dereferencing. In this case, this is likely a false positive caused by Spotbugs analyzing the bytecode generated for the try-with-resources block.
What’s happening:
-Files.find(...) returns a Stream, which will never be null (it throws IOException on failure).
-The try-with-resources block auto-closes the stream, and the generated bytecode includes an implicit if (stream != null) check to avoid closing a null resource. Spotbugs flags this as redundant because stream cannot logically be null here.
Why it’s a false positive:
The redundant check is in the bytecode (generated by try-with-resources), not our source code. Spotbugs is technically correct but overly strict here.
Solution
<threshold>Medium</threshold>[ But in this case we might miss out on some small actual bugs but the flase positives will be drastically reduced]