Enhancement #5666: enhancing report with adding Sarif Report(POC) #7408
Conversation
Gaoyan1999
changed the title
📝 WalkthroughWalkthroughThe PR adds SARIF report generation to the Checker Framework. Two Gradle dependencies were added: com.fasterxml.jackson.core:jackson-databind:2.15.0 and com.contrastsecurity:java-sarif:2.0. A new Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
framework/build.gradle(1 hunks)framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java(1 hunks)framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (1)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
SarifReportGenerator(33-205)
🔇 Additional comments (8)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
122-153: LGTM!The
addResultmethod correctly filters diagnostic kinds, constructs SARIF Result objects with proper location information, and manages artifact registration. The implementation aligns with SARIF 2.1.0 specifications.framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (6)
623-630: LGTM!The SARIF-related fields are properly declared with appropriate nullability annotations and follow the existing field naming conventions in the class.
750-761: LGTM!The
getRootChecker()method correctly traverses the parent chain to find the root ancestor checker. The protected visibility appropriately allows subcheckers to access the root's SARIF generator.
1142-1150: LGTM!The SARIF initialization correctly ensures only the root checker creates the
SarifReportGenerator, validates the required output path argument, and throws a clearUserErrorwhen the path is missing.
1601-1621: LGTM!The updated
printOrStoreMessagemethod correctly propagates themessageKeyand ensures all subcheckers route their diagnostics to the root checker'sSarifReportGenerator. This properly implements the single-report-per-compilation requirement mentioned in the PR objectives.
3551-3576: LGTM!The
CheckerMessageclass is correctly extended with themessageKeyfield. The decision to not includemessageKeyinequals()/hashCode()is appropriate since it's metadata for SARIF reporting rather than part of the message's identity.
1519-1519: LGTM!The
messageKeyis correctly propagated through thereport()method to enable SARIF rule identification.framework/build.gradle (1)
56-57: Verify shadowJar configuration for these new dependencies and review jackson-databind version.Per the shadowing requirement noted in the context, confirm that the shadowJar configuration in the root build.gradle properly handles
jackson-databindandjava-sarif. Additionally, jackson-databind 2.15.0 is affected by CVE-2023-35116 (a disputed DoS vulnerability affecting versions through 2.15.2); consider upgrading to a patched version if available. Note thatjava-sarif:2.0is already the latest released version.
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (1)
1078-1092: Ensure SARIF write failures cannot break compilationThe PR description says SARIF generation is best‑effort and should not affect compilation, but
typeProcessingOvercurrently only catchesIOException. Other plausible failures fromwriteReport(for example,InvalidPathExceptionfromPaths.get, or unexpected runtime exceptions from Jackson) would propagate and could abort a build.Also, if an invariant is ever broken and
sarifOutputPathis null, theassertwon’t protect production runs (assertions are typically disabled).I recommend tightening this block to both validate the path and guard against unexpected runtime failures:
- if (parentChecker == null && sarifReportGenerator != null) { - assert sarifOutputPath != null; - try { - sarifReportGenerator.writeReport(sarifOutputPath); - message(Diagnostic.Kind.NOTE, "SARIF report written to: " + sarifOutputPath); - } catch (IOException e) { - message(Diagnostic.Kind.WARNING, "Failed to write SARIF report: " + e.getMessage()); - } - } + if (parentChecker == null && sarifReportGenerator != null) { + if (sarifOutputPath == null || sarifOutputPath.isEmpty()) { + message( + Diagnostic.Kind.WARNING, + "SARIF output was enabled but no valid -AsarifOutput path was provided; skipping report."); + } else { + try { + sarifReportGenerator.writeReport(sarifOutputPath); + message(Diagnostic.Kind.NOTE, "SARIF report written to: " + sarifOutputPath); + } catch (IOException | RuntimeException e) { + message( + Diagnostic.Kind.WARNING, + "Failed to write SARIF report (compilation will continue): " + e.getMessage()); + } + } + }This keeps SARIF truly best‑effort while making the invariant about
sarifOutputPathexplicit.framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
189-215: Make SARIF file writes more robust (temp file + atomic move)Writing directly to
outputPathrisks leaving a partially written SARIF file ifwriteValuefails mid‑stream (disk full, process killed, etc.). You can improve robustness by writing to a temporary file in the same directory and then moving it into place.For example:
+import java.nio.file.Files; +import java.nio.file.StandardCopyOption; @@ public void writeReport(String outputPath) throws IOException { @@ - // Write SARIF log to JSON file - ObjectMapper mapper = new ObjectMapper(); - Path path = Paths.get(outputPath); - mapper.writerWithDefaultPrettyPrinter().writeValue(path.toFile(), sarifLog); + ObjectMapper mapper = new ObjectMapper(); + Path path = Paths.get(outputPath); + Path parent = path.getParent(); + if (parent == null) { + parent = Paths.get(System.getProperty("java.io.tmpdir")); + } + Path tempFile = Files.createTempFile(parent, "sarif-", ".tmp"); + try { + mapper.writerWithDefaultPrettyPrinter().writeValue(tempFile.toFile(), sarifLog); + Files.move( + tempFile, + path, + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { + Files.deleteIfExists(tempFile); + throw e; + } }This ensures consumers see either the old complete file or the new one, but never a truncated file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java(1 hunks)framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (1)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
SarifReportGenerator(44-216)
🔇 Additional comments (1)
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java (1)
1452-1524: Validate messaging API changes and update documentation for messageKeyThe new
messageKeyplumbing throughreport,printOrStoreMessage, andCheckerMessagelooks reasonable for SARIF, but there are a few follow‑ups to avoid regressions:
printOrStoreMessage's signature changed (additionalmessageKeyparameter). Any subclasses (notablyBaseTypeCheckerand external checkers that overrideprintOrStoreMessage) must be updated to override the new signature; otherwise those overrides will silently stop intercepting diagnostics. Please confirm all such overrides were updated.- The Javadocs for both
printOrStoreMessageoverloads, and forCheckerMessage, don't document the newmessageKeyparameter. Updating them will make the contract clearer, especially now that the field is used for SARIFruleId.CheckerMessage.equals/hashCodedeliberately ignoremessageKey, so equality semantics remain "same kind/message/source/checker". If you intend two messages that only differ in messageKey to be treated as distinct in the store,messageKeyshould be folded into equals/hashCode; if not, a brief comment explaining why it's excluded would help future maintainers.Functionally, forwarding to the root checker's
sarifReportGeneratorfromprintOrStoreMessagelooks correct and ensures compound checkers share a single SARIF run.Also applies to: 1602-1621, 2317-2323, 3532-3578
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
164-184: Consider atomic write to avoid partially written SARIF files
writeReportcurrently writes directly to the target path. If the JVM crashes or anIOExceptionoccurs mid‑write, callers may end up with a truncated SARIF file.As previously suggested in an earlier review, you could write to a temporary file in the same directory and then atomically move it into place:
+ java.nio.file.Path path = Paths.get(outputPath); + java.nio.file.Path tempFile = + java.nio.file.Files.createTempFile(path.getParent(), "sarif", ".tmp"); @@ - ObjectMapper mapper = new ObjectMapper(); - Path path = Paths.get(outputPath); - mapper.writerWithDefaultPrettyPrinter().writeValue(path.toFile(), sarifLog); + ObjectMapper mapper = new ObjectMapper(); + try { + mapper.writerWithDefaultPrettyPrinter().writeValue(tempFile.toFile(), sarifLog); + java.nio.file.Files.move( + tempFile, + path, + java.nio.file.StandardCopyOption.REPLACE_EXISTING, + java.nio.file.StandardCopyOption.ATOMIC_MOVE); + } catch (IOException e) { + java.nio.file.Files.deleteIfExists(tempFile); + throw e; + }You can pull the additional
java.nio.filetypes into imports if you prefer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java(1 hunks)
🔇 Additional comments (1)
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java (1)
143-157: Version lookup fallback is reasonableThe
git.propertieslookup with try‑with‑resources and a quiet fallback to"Unknown"is straightforward and safe; this is fine for a reporting-only feature.
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/report/SarifReportGenerator.java
Show resolved
Hide resolved
|
@Gaoyan1999 The title includes "proof of concept". Do you want feedback on this pull request, or not yet? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
framework/build.gradle(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
- GitHub Check: typetools.checker-framework (inference_part1_jdk25)
- GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
- GitHub Check: typetools.checker-framework (misc_jdk25)
- GitHub Check: typetools.checker-framework (junit_jdk25)
- GitHub Check: typetools.checker-framework (inference_part2_jdk25)
- GitHub Check: typetools.checker-framework (nonjunit_jdk25)
@mernst Yes, I’d appreciate feedback. This is just a simple PoC for now — no tests and only the happy path. At this stage, I’m mainly looking for feedback on the overall idea and whether it’s worth continuing and building this out further. |
2 participants
Summary
This PR introduces a proof-of-concept implementation of SARIF report generation for the Checker Framework. It enables checkers to produce SARIF 2.1.0–compliant JSON report files containing diagnostic messages and their corresponding source locations.
Unit tests are not included at this stage. I’d like to first align with contributors on the overall design before adding tests
Motivation
Related issue: #5666
How to Use SARIF
This generates a
report.sariffile containing all diagnostics in SARIF 2.1.0 format.The
report.sariffile would be like that:{ "version" : "2.1.0", "runs" : [ { "tool" : { "driver" : { "name" : "Checker Framework", "version" : "3.52.1-SNAPSHOT" } }, "results" : [ { "ruleId" : "assignment", "level" : "error", "message" : { "text" : "[assignment] incompatible types in assignment.\nfound : Set<@KeyFor(\"sharedCounts1\") String>\nrequired: Set<@KeyFor({\"sharedBooks\", \"sharedCounts1\"}) String>" }, "locations" : [ { "physicalLocation" : { "artifactLocation" : { "uri" : "file:///Users/gaoyan/workspace/gsoc/fork/checker-framework/checker/tests/nullness/KeyForMultiple.java" }, "region" : { "startLine" : 25, "startColumn" : 73, "endLine" : 25, "endColumn" : 95 } } } ] }, { "ruleId" : "assignment", "level" : "error", "message" : { "text" : "[assignment] incompatible types in assignment.\nfound : Set<@KeyFor(\"sharedCounts2\") String>\nrequired: Set<@KeyFor({\"sharedBooks\", \"sharedCounts2\"}) String>" }, "locations" : [ { "physicalLocation" : { "artifactLocation" : { "uri" : "file:///Users/gaoyan/workspace/gsoc/fork/checker-framework/checker/tests/nullness/KeyForMultiple.java" }, "region" : { "startLine" : 41, "startColumn" : 73, "endLine" : 41, "endColumn" : 95 } } } ] } ] } ] }Implementation Details
Message Flow
The SARIF generator is initialized in the root checker’s initChecker() method to ensure a single instance per compilation.
printOrStoreMessage()retrieves the root checker’s generator and forwards diagnostic messages to it.At the end of processing,
typeProcessingOver()writes the collected results to the SARIF file.Compound Checker Support
Only the root checker creates and owns the SARIF generator.
Subcheckers access the same instance via getRootChecker().
This ensures all diagnostics from compound checkers are included in a single unified SARIF report.
Future Work
This POC lays the foundation, and several enhancements can be implemented later:
messages.properties(currently only ruleId is included)Backward Compatibility
The feature is fully backward compatible:
SARIF generation is opt-in via the -AsarifOutput option
Existing console output remains unchanged
No breaking API changes
Failures in SARIF generation do not impact compilation