Add fixes to output-format=sarif
#20300
Conversation
|
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you, this is great. I've a few suggestions on how to improve this
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifArtifactLocation { | ||
| uri: String, |
There was a problem hiding this comment.
I think the spec says uri. https://docs.oasis-open.org/sarif/sarif/v2.1.0/csprd01/sarif-v2.1.0-csprd01.html#_Toc10540865
| .map(|edit| SarifReplacement { | ||
| deleted_region, | ||
| inserted_content: Some(InsertedContent { | ||
| text: edit.content().unwrap_or("").to_string(), | ||
| }), |
There was a problem hiding this comment.
I think there are two issues with how we map edits:
inserted_contentshould beNoneifedit.content()isNone.- We need to use the range from the edit for the deleted range and not the diagnostic range. You can get the range by calling
edit.range(). The range is guaranteed to be empty if it is an insertion. It's non empty for replacements or deletions
njhearp
requested review from
AlexWaygood,
carljm,
dcreager,
dhruvmanila and
sharkdp
as code owners
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
MichaReiser
removed request for
AlexWaygood,
carljm,
dcreager,
dhruvmanila and
sharkdp
|
Sorry for the noisy update, I accidentally pushed extra commits when merging. I'm going to fix this to remove the noise. |
njhearp
force-pushed
the
sarif-fixes
branch
from
5482e8a to
d359d0b
Compare
| end_line: end_location.line, | ||
| end_column: end_location.column, | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I thought I would add error handling if there is a problem with the source file.
There was a problem hiding this comment.
That makes sense. I changed the error handling to omit the fix when the source file is missing. Emitting a fix with made-up locations seems problematic, as it most likely will result in invalid code when applied.
Don't worry. It happened to all of us :) |
MichaReiser
changed the title
SarifEmitter to include fixes in the SARIF ouputoutput-format=sarif
MichaReiser
changed the title
output-format=sarifoutput-format=sarif
MichaReiser
changed the title
output-format=sarifoutput-format=sarif
2 participants
Summary
Addresses #19962
This PR extends
SarifEmitterto includefixesin the SARIF ouput, if an auto fix is available. Before SARIF output did not include thefixesfield. This makes Ruff's SARIF output more compliant with the spec.Key changes:
SarifFix,SarifArtifactChange, and related structs to represent fixes in SARIF.fixesis outputted only if an auto fix is available.Test Plan
Check for SARIF spec compliance and regressions.