Skip to content

Conversation

njhearp
Copy link
Contributor

@njhearp njhearp commented Sep 8, 2025

Summary

Addresses #19962

This PR extends SarifEmitter to include fixes in the SARIF ouput, if an auto fix is available. Before SARIF output did not include the fixes 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.

Copy link
Contributor

github-actions bot commented Sep 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added the cli Related to the command-line interface label Sep 16, 2025
Copy link
Member

@MichaReiser MichaReiser left a 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,
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 255 to 259
.map(|edit| SarifReplacement {
deleted_region,
inserted_content: Some(InsertedContent {
text: edit.content().unwrap_or("").to_string(),
}),
Copy link
Member

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:

  1. inserted_content should be None if edit.content() is None.
  2. 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

Copy link
Contributor

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@njhearp njhearp marked this pull request as draft September 17, 2025 16:51
Copy link
Contributor

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@njhearp
Copy link
Contributor Author

njhearp commented Sep 17, 2025

Sorry for the noisy update, I accidentally pushed extra commits when merging. I'm going to fix this to remove the noise.

end_line: end_location.line,
end_column: end_location.column,
}
} else {
Copy link
Contributor Author

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.

Copy link
Member

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.

@njhearp njhearp marked this pull request as ready for review September 18, 2025 04:43
@MichaReiser
Copy link
Member

Sorry for the noisy update, I accidentally pushed extra commits when merging. I'm going to fix this to remove the noise.

Don't worry. It happened to all of us :)

@MichaReiser MichaReiser changed the title Extended SarifEmitter to include fixes in the SARIF ouput Include fixes in output-format=sarif Sep 18, 2025
@MichaReiser MichaReiser changed the title Include fixes in output-format=sarif Add fixes in output-format=sarif Sep 18, 2025
@MichaReiser MichaReiser changed the title Add fixes in output-format=sarif Add fixes to output-format=sarif Sep 18, 2025
@MichaReiser MichaReiser merged commit c4d3593 into astral-sh:main Sep 18, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to the command-line interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants