-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add fixes to output-format=sarif
#20300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an Url
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two issues with how we map edits:
inserted_content
should beNone
ifedit.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
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
Sorry for the noisy update, I accidentally pushed extra commits when merging. I'm going to fix this to remove the noise. |
5482e8a
to
d359d0b
Compare
end_line: end_location.line, | ||
end_column: end_location.column, | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I would add error handling if there is a problem with the source file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
SarifEmitter
to include fixes
in the SARIF ouputoutput-format=sarif
output-format=sarif
output-format=sarif
output-format=sarif
output-format=sarif
Summary
Addresses #19962
This PR extends
SarifEmitter
to includefixes
in the SARIF ouput, if an auto fix is available. Before SARIF output did not include thefixes
field. This makes Ruff's SARIF output more compliant with the spec.Key changes:
SarifFix
,SarifArtifactChange
, and related structs to represent fixes in SARIF.fixes
is outputted only if an auto fix is available.Test Plan
Check for SARIF spec compliance and regressions.