Bump SpotBugs to 4.10.3 - #40
Conversation
Updates the wrapped SpotBugs version from 4.8.4 to 4.10.3 (latest release) and regenerates docs/patterns.json and docs/description/* accordingly. SpotBugs 4.9.0+ raises its minimum runtime requirement to Java 11, so dockerBaseImage is bumped from amazoncorretto:8-alpine3.18-jre to amazoncorretto:11-alpine3.18 (no JRE-only alpine variant is published for Corretto 11+, only jdk/full/plain, which are all identical in size). Also bumps scalaVersion from 2.12.15 to 2.12.21 to work around a Scala 2.12 compiler bug (scala/bug#12783) where the classfile parser crashes reading MethodParameters attributes with unnamed parameters, present in SpotBugs classes starting at 4.8.5 (compiled with a newer javac). Without this the build fails with "bad constant pool index: 0" for any SpotBugs version above 4.8.4. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.github.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades SpotBugs to 4.10.3 and transitions the runtime to Java 11 via Amazon Corretto, which is a necessary step for maintaining compatibility with modern bytecode. The Codacy analysis indicates the changes are generally up to standards. However, the documentation regeneration process has introduced several formatting and syntax issues. Specifically, Java annotations in the documentation are incorrectly prefixed with '&' instead of '@', and there are multiple typos in specification references and descriptions. These documentation issues should be addressed to ensure clarity for end-users, though they do not block the core functional upgrade.
About this PR
- Several regenerated documentation files exhibit systemic issues: Java annotations are incorrectly prefixed with '&' instead of '@', likely due to HTML escaping, and several JSON description strings contain trailing whitespace and newlines. The regeneration process should be reviewed to ensure correct character encoding and formatting.
Test suggestions
- Verify that the project compiles successfully using the new Scala and SpotBugs versions.\n- [ ] Verify the integrity of the regenerated documentation files (patterns.json and markdown descriptions).\n- [ ] Ensure the Docker container initializes and executes basic analysis successfully under Java 11.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that the project compiles successfully using the new Scala and SpotBugs versions.\n- [ ] Verify the integrity of the regenerated documentation files (patterns.json and markdown descriptions).\n- [ ] Ensure the Docker container initializes and executes basic analysis successfully under Java 11.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| Suppressing annotations `&SuppressFBWarnings` should be removed from the source code as soon as they are no more needed. | ||
| Leaving them may result in accidental warnings suppression. | ||
| The unnecessary annotation is not reported when the class is annotated with an `&Generated` annotation with retention CLASS or RUNTIME. No newline at end of file |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Annotations in Java are prefixed with @, not &. This appears to be a typo from an HTML-escaped source or copy-paste error. Using standard Java notation ensures the documentation is clear for users.\n\nsuggestion\n Suppressing annotations `@SuppressFBWarnings` should be removed from the source code as soon as they are no more needed.\n Leaving them may result in accidental warnings suppression.\n The unnecessary annotation is not reported when the class is annotated with an `@Generated` annotation with retention CLASS or RUNTIME.\n
| an indication of some misunderstanding or some other logic error. | ||
| If you really want to test the value for being null, perhaps it would be clearer to do | ||
| better to do a null test rather than an instanceof test. No newline at end of file | ||
| better to do a null test rather than an instanceof test. |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The sentence structure here is repetitive. It can be simplified to improve clarity.\n\nsuggestion\na null-check rather than an instanceof test.\n
| }, { | ||
| "patternId" : "UNS_UNSAFE_CALL", | ||
| "title" : "Call to Unsafe.", | ||
| "description" : "Do not use {4}. As the class name indicates, its methods are not safe to use.\n ", |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Remove the trailing newline and extra spaces from the end of the description string to ensure consistent UI rendering.\n\nsuggestion\n "description" : "Do not use {4}. As the class name indicates, its methods are not safe to use.",\n
| # [This write of this 64-bit primitive variable may not be atomic](https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#AT_NONATOMIC_64BIT_PRIMITIVE) | ||
|
|
||
| The long and the double are 64-bit primitive types, and depending on the Java Virtual Machine implementation assigning a value to them can be treated as two separate 32-bit writes, and as such it's not atomic. | ||
| See [JSL 17.7\. Non-Atomic Treatment of double and long](https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.7). |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The standard acronym for the Java Language Specification is JLS.\n\nsuggestion\n See [JLS 17.7\\. Non-Atomic Treatment of double and long](https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.7).\n
2 participants
Summary
4.8.4to4.10.3(latest release) inbuild.sbt, and regeneratesdocs/patterns.jsonanddocs/description/*viaDocumentationGenerator(24 patterns added, 4 removed/renamed by upstream SpotBugs).dockerBaseImagefromamazoncorretto:8-alpine3.18-jretoamazoncorretto:11-alpine3.18, since SpotBugs 4.9.0+ raises its minimum runtime requirement to Java 11. Note: Corretto no longer publishes a slim JRE-only Alpine variant for Java 11+ (only-jdk/-full/plain, all identical in size), so the resulting image is larger than before — this is unavoidable given the runtime requirement.scalaVersionfrom2.12.15to2.12.21to work around scala/bug#12783: the Scala 2.12 classfile parser crashes (FatalError: bad constant pool index: 0) readingMethodParametersattributes with unnamed parameters, which are present in SpotBugs classes from4.8.5onward (compiled with a newer javac). This isn't an incidental/opportunistic bump — the build cannot compile against any SpotBugs version newer than4.8.4without it. Confirmed via bisection (4.8.4 compiles, 4.8.5+ all fail with the identical stack trace regardless of the JDK running sbt) and confirmed the fix by testing 2.12.21 against 4.10.3.findsecbugsVersionandsbContribVersionwere left untouched, per the playbook (only bump when the task specifically targets those plugins).codacy/base,codacy/plugins-test) were left untouched — out of scope and previously reverted after breaking CI when bumped opportunistically.Validation performed locally
sbt compile— passes (JDK 8 for the Scala compiler bug fix verification; JDK 11 for anything touching SpotBugs classes at runtime)sbt "runMain com.codacy.tools.spotbugs.DocumentationGenerator"— regenerated docs, diff reviewedsbt scalafmtCheckAll scalafmtSbtCheck test— 18/18 tests passsbt "set version := \"latest\"; docker:publishLocal"— image builds and runscodacy-plugins-testagainst the local image:runMain codacy.plugins.DockerTest json codacy-spotbugs:latest— passrunMain codacy.plugins.DockerTest pattern codacy-spotbugs:latest— passrunMain codacy.plugins.DockerTest multiple codacy-spotbugs:latest— pass (4/4 fixture cases)Test plan