Enhancement #5666: enhancing report with adding Sarif Report(POC) - #7408
Gaoyan1999 wants to merge 12 commits into
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.
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
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.
|
@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. |
| */ | ||
| public class SarifReportGenerator { | ||
|
|
||
| private final ProcessingEnvironment processingEnv; |
There was a problem hiding this comment.
Checker Framework style requires a Javadoc comment on every field and method.
| * | ||
| * @param source the tree node | ||
| * @param root the compilation unit containing the tree | ||
| * @return a Region with start and end line/column numbers, or default values if position cannot |
There was a problem hiding this comment.
Please say what the default values are (1 and 1).
| /** | ||
| * Add a diagnostic result to the report. | ||
| * | ||
| * <p>Only ERROR and MANDATORY_WARNING diagnostics are collected. Other diagnostic kinds (NOTE, |
There was a problem hiding this comment.
What is the rationale for this? I think all should be collected.
There was a problem hiding this comment.
Result.Level supports NOTE, so the reason is not that the Sarif library cannot support notes.
| * | ||
| * @return the Checker Framework version string | ||
| */ | ||
| private String getCheckerVersion() { |
There was a problem hiding this comment.
Please use SourceChecker.getCheckerVersion() instead of defining a new method.
| } | ||
|
|
||
| /** | ||
| * Write the SARIF report to file. |
| // argument inference crashes to crash the type checker. | ||
| "convertTypeArgInferenceCrashToWarning" | ||
| "convertTypeArgInferenceCrashToWarning", | ||
| "sarifOutput" |
There was a problem hiding this comment.
This requires a comment, and it should go in the appropriate section above ("Format of messages") rather than randomly at the end of the list.
| // argument inference crashes to crash the type checker. | ||
| "convertTypeArgInferenceCrashToWarning" | ||
| "convertTypeArgInferenceCrashToWarning", | ||
| "sarifOutput" |
There was a problem hiding this comment.
This must be documented in the manual, as explained on line 113 above.
| // argument inference crashes to crash the type checker. | ||
| "convertTypeArgInferenceCrashToWarning" | ||
| "convertTypeArgInferenceCrashToWarning", | ||
| "sarifOutput" |
There was a problem hiding this comment.
Both of the other comments on this line are about weaknesses that should have been obvious. Is this an AI-created pull request that you didn't bother to read yourself?
| private boolean sarifOutputEnabled = false; | ||
|
|
||
| /** Path to SARIF output file. */ | ||
| private @Nullable String sarifOutputPath = null; |
There was a problem hiding this comment.
These two variables should be @MonotonicNonNull, not @Nullable.
| private @Nullable String sarifOutputPath = null; | ||
|
|
||
| /** SARIF report generator, if enabled. */ | ||
| private @Nullable SarifReportGenerator sarifReportGenerator = null; |
There was a problem hiding this comment.
Document the relationships among the variables. For example, are both of the nullable variables non-null exactly when sarifOutputEnabled is true?
There was a problem hiding this comment.
Can sarifReportGenerator be non-null if parentChecker is non-null? If "yes", why?
| requirePrefixInWarningSuppressions = hasOption("requirePrefixInWarningSuppressions"); | ||
| showPrefixInWarningMessages = hasOption("showPrefixInWarningMessages"); | ||
| warnUnneededSuppressions = hasOption("warnUnneededSuppressions"); | ||
| sarifOutputEnabled = hasOption("sarifOutput"); |
There was a problem hiding this comment.
Rather than having two variables -- enabled and filename -- why not have only one? If both exist, then a default file name should be used.
| String message, | ||
| Tree source, | ||
| CompilationUnitTree root, | ||
| String messageKey) { |
There was a problem hiding this comment.
You must add Javadoc for each newly-added formal parameter.
This is also indicated by the CI failure. Did you examine the CI failure? Please always make CI pass before marking a pull request as ready for review.
| new CheckerMessage(kind, message, source, this, trace, messageKey); | ||
| messageStore.add(checkerMessage); | ||
| } | ||
| // Use root checker's sarifReportGenerator so all subcheckers share the same instance |
There was a problem hiding this comment.
Add a period at the end of a sentence to make it proper English.
|
Thank you for starting on this feature. I like the feature a lot. Documentation such as the user manual is missing. That is more important than the code, because it explains what the code is supposed to do. It is very difficult to review code, try to infer what it is supposed to do, and also find mistakes in it. I see no reason not to support all the warning types such as NOTE. That would be just a line or two of code, so it is odd that you left it out. Tests must pass before you mark a pull request as ready for review. Did you examine the test failures? |
| implementation(libs.plume.util) | ||
| implementation(libs.reflection.util) | ||
| implementation(libs.classgraph) | ||
| implementation "com.fasterxml.jackson.core:jackson-databind:2.15.0" |
There was a problem hiding this comment.
These need to be shadowed in ../build.gradle. See comment above.
| // argument inference crashes to crash the type checker. | ||
| "convertTypeArgInferenceCrashToWarning" | ||
| "convertTypeArgInferenceCrashToWarning", | ||
| "sarifOutput" |
|
I think the overall design is fine. Please add tests and make sure the PR passes all required tests. |
3 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