-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature: Add SARIF output support #9078
Merged
charliermarsh
merged 15 commits into
astral-sh:main
from
C-Hipple:feature-add-sarif-output
Dec 13, 2023
Merged
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
f53695d
add sarif output--update snapshots
C-Hipple 63be34a
format
C-Hipple 4883f4a
handle relative paths, clippy fixes, derive debug on SarifResult
C-Hipple 27d9298
remove sarif snapshot test
C-Hipple 5c16403
lint
C-Hipple 8d0684e
target_arch = wasm32
C-Hipple 9a94fc1
iter all rules, resolve unused import for wasm
C-Hipple 252afa8
Run cargo fmt
charliermarsh 685e48c
Merge branch 'main' into feature-add-sarif-output
charliermarsh 25e835e
Back out some changes
charliermarsh 5b830ca
add help, end_line, end_column
C-Hipple 9286b54
update test to no-longer use default
C-Hipple f12f0f5
cargo fmt
C-Hipple 3dad7ad
fix lint on wasm
C-Hipple 3f5438b
Try removing clippy ignore
charliermarsh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,205 @@ | ||
use std::io::Write; | ||
use url::Url; | ||
|
||
use serde::{Serialize, Serializer}; | ||
use serde_json::json; | ||
|
||
use ruff_source_file::OneIndexed; | ||
|
||
use crate::fs::normalize_path; | ||
use crate::message::{Emitter, EmitterContext, Message}; | ||
use crate::registry::{AsRule, Linter, Rule, RuleNamespace}; | ||
use crate::settings::rule_table::RuleTable; | ||
use crate::VERSION; | ||
|
||
#[derive(Default)] | ||
pub struct SarifEmitter<'a> { | ||
applied_rules: Vec<SarifRule<'a>>, | ||
} | ||
|
||
impl SarifEmitter<'_> { | ||
#[must_use] | ||
pub fn with_applied_rules(mut self, rule_table: RuleTable) -> Self { | ||
let mut applied_rules = Vec::new(); | ||
|
||
for rule in rule_table.iter_enabled() { | ||
applied_rules.push(SarifRule::from_rule(rule)); | ||
} | ||
self.applied_rules = applied_rules; | ||
self | ||
} | ||
} | ||
|
||
impl Emitter for SarifEmitter<'_> { | ||
fn emit( | ||
&mut self, | ||
writer: &mut dyn Write, | ||
messages: &[Message], | ||
_context: &EmitterContext, | ||
) -> anyhow::Result<()> { | ||
let results = messages | ||
.iter() | ||
.map(SarifResult::from_message) | ||
.collect::<Vec<_>>(); | ||
|
||
let output = json!({ | ||
"$schema": "https://json.schemastore.org/sarif-2.1.0.json", | ||
"version": "2.1.0", | ||
"runs": [{ | ||
"tool": { | ||
"driver": { | ||
"name": "ruff", | ||
"informationUri": "https://github.com/astral-sh/ruff", | ||
"rules": self.applied_rules, | ||
"version": VERSION.to_string(), | ||
} | ||
}, | ||
"results": results, | ||
}], | ||
}); | ||
serde_json::to_writer_pretty(writer, &output)?; | ||
Ok(()) | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
struct SarifRule<'a> { | ||
name: &'a str, | ||
code: String, | ||
linter: &'a str, | ||
summary: &'a str, | ||
explanation: Option<&'a str>, | ||
url: Option<String>, | ||
} | ||
|
||
impl<'a> SarifRule<'a> { | ||
fn from_rule(rule: Rule) -> Self { | ||
let code = rule.noqa_code().to_string(); | ||
let (linter, _) = Linter::parse_code(&code).unwrap(); | ||
Self { | ||
name: rule.into(), | ||
code, | ||
linter: linter.name(), | ||
summary: rule.message_formats()[0], | ||
explanation: rule.explanation(), | ||
url: rule.url(), | ||
} | ||
} | ||
} | ||
|
||
impl Serialize for SarifRule<'_> { | ||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
where | ||
S: Serializer, | ||
{ | ||
json!({ | ||
"id": self.code, | ||
"shortDescription": { | ||
"text": self.summary, | ||
}, | ||
"fullDescription": { | ||
"text": self.explanation, | ||
}, | ||
"helpUri": self.url, | ||
"properties": { | ||
"id": self.code, | ||
"kind": self.linter, | ||
"name": self.name, | ||
"problem.severity": "error".to_string(), | ||
}, | ||
}) | ||
.serialize(serializer) | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
struct SarifResult { | ||
charliermarsh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
rule: Rule, | ||
level: String, | ||
message: String, | ||
uri: String, | ||
start_line: OneIndexed, | ||
start_column: OneIndexed, | ||
} | ||
|
||
impl SarifResult { | ||
#[cfg(not(target_arch = "wasm32"))] | ||
fn from_message(message: &Message) -> Self { | ||
let start_location = message.compute_start_location(); | ||
let abs_filepath = normalize_path(message.filename()); | ||
Self { | ||
rule: message.kind.rule(), | ||
level: "error".to_string(), | ||
message: message.kind.name.clone(), | ||
uri: Url::from_file_path(abs_filepath).unwrap().to_string(), | ||
start_line: start_location.row, | ||
start_column: start_location.column, | ||
} | ||
} | ||
#[cfg(target_arch = "wasm32")] | ||
fn from_message(message: &Message) -> Self { | ||
let start_location = message.compute_start_location(); | ||
let abs_filepath = normalize_path(message.filename()); | ||
Self { | ||
rule: message.kind.rule(), | ||
level: "error".to_string(), | ||
message: message.kind.name.clone(), | ||
uri: abs_filepath.display().to_string(), | ||
charliermarsh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
start_line: start_location.row, | ||
start_column: start_location.column, | ||
} | ||
} | ||
} | ||
|
||
impl Serialize for SarifResult { | ||
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
where | ||
S: Serializer, | ||
{ | ||
json!({ | ||
"level": self.level, | ||
"message": { | ||
"text": self.message, | ||
}, | ||
"locations": [{ | ||
"physicalLocation": { | ||
"artifactLocation": { | ||
"uri": self.uri, | ||
}, | ||
"region": { | ||
"startLine": self.start_line, | ||
"startColumn": self.start_column, | ||
} | ||
} | ||
}], | ||
"ruleId": self.rule.noqa_code().to_string(), | ||
}) | ||
.serialize(serializer) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
|
||
use crate::message::tests::{capture_emitter_output, create_messages}; | ||
use crate::message::SarifEmitter; | ||
|
||
fn get_output() -> String { | ||
let mut emitter = SarifEmitter::default(); | ||
capture_emitter_output(&mut emitter, &create_messages()) | ||
} | ||
|
||
#[test] | ||
fn valid_json() { | ||
let content = get_output(); | ||
serde_json::from_str::<serde_json::Value>(&content).unwrap(); | ||
} | ||
|
||
#[test] | ||
fn test_results() { | ||
let content = get_output(); | ||
let sarif = serde_json::from_str::<serde_json::Value>(content.as_str()).unwrap(); | ||
let results = sarif["runs"][0]["results"].as_array().unwrap(); | ||
assert_eq!(results.len(), 3); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
So one potential issue here is that
pyproject_config
isn't guaranteed to contain the correct set of enabled rules for all files. Ruff supports hierarchical configuration, so you can have different configuration files that apply to different subdirectories in your project. Thepyproject_config
here really just represents the settings in the current working directory.Could we infer the set of rules from the diagnostic messages? (What is this used for on the SARIF side?)
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.
Gotcha--thanks for that context. SARIF lists the rules applied as part of the tool description so that you can be sure you're getting an apples to apples comparison, rather than just saying both times you checked with a particular rust version.
While the field is not mandatory in the SARIF specification Section 3.19.23 rules property it is required by Github. Also see: Understand Rules and Results
I'm not exactly sure how github would handle having the
rules
be supplied as only the ones for which diagnostics could be found as that's not documented.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.
Alternatively, could we just include all rules?
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.
Including all rules is probably the safest bet. In my experience with code scanning on github, I've never looked at what checks were done, only the findings, so this is probably closest to the SARIF expectation. My gut is that most people have most rules applied, and only turn off a handful, but I could be wrong, especially for older/larger projects.
Only downsides are if someone complains and then we change it, if others are using it in their CI, the rules set will change, which again, not sure how bad that is and that the file size will be a bit larger.
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.
Let's try all-rules for now. You can iterate over them with
Rule::iter()
.