Feature: Add SARIF output support by C-Hipple · Pull Request #9078 · astral-sh/ruff

C-Hipple · 2023-12-10T02:03:22Z

Summary

Adds support for sarif v2.1.0 output to cli, usable via the output-format paramter.

ruff . --output-format=sarif

Includes a few changes I wasn't sure of, namely:

Adds a few derives for Clone & Copy, which I think could be removed with a little extra work as well.

Test Plan

I built and ran this against several large open source projects and verified that the output sarif was valid, using Microsoft's SARIF validator tool

I've also attached an output of the sarif generated by this version of ruff on the main branch of django at commit: b287af5dc9
django_main_b287af5dc9_sarif.json

Note: this needs to be regenerated with the latest changes and confirmed.

Open Points

[ ] Convert to just using all Rules all the time
[ ] Fix the issue with getting the file URI when compiling for web assembly

charliermarsh · 2023-12-10T02:11:32Z

crates/ruff_cli/src/lib.rs

            printer.write_statistics(&diagnostics, &mut summary_writer)?;
        } else {
-            printer.write_once(&diagnostics, &mut summary_writer)?;
+            printer.write_once(&diagnostics, &mut summary_writer, &pyproject_config)?;


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. The pyproject_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?)

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.

Alternatively, could we just include all rules?

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.

Let's try all-rules for now. You can iterate over them with Rule::iter().

crates/ruff_linter/src/message/sarif.rs

charliermarsh · 2023-12-10T02:14:04Z

crates/ruff_linter/src/message/sarif.rs

+            rule: message.kind.rule(),
+            level: "error".to_string(),
+            message: message.kind.name.to_owned(),
+            uri: Url::from_file_path(message.filename()).unwrap().to_string(),


If filename is relative here, would this convert it to absolute? I do think filename will always be absolute here... we pass around absolute paths, and expect callers to call relativize_path when displaying user-facing relative paths... But we may want to be defensive (call .canonicalize() or normalize_path or similar).

This is what I was referring to in the PR body. It'll error if the path is relative, which is why I changed the path in the snapshot files. I'll update to safely handle relative paths, should be straight forward.

👍 I did see that, but wasn't sure how Url::from_file_path would behave if the path wasn't absolute.

yeah, normalize_path seems like the right call here, as .canonicalize() will fail if the file doesn't actually exist (a la in test).

github-actions · 2023-12-10T02:21:55Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

crates/ruff_linter/src/message/sarif.rs

charliermarsh

I think this is probably good to merge once we change to enumerate all-rules. @C-Hipple do you plan to make that change, or would you like a hand?

C-Hipple · 2023-12-13T01:53:28Z

I was planning on making this change tonight/tomorrow, but if that falls through I'll let you know as I'll be leaving on a trip. Seems simple enough and also fix the issue to build for wasm seems straightforward.

…

On Tue, Dec 12, 2023, 5:38 PM Charlie Marsh ***@***.***> wrote: ***@***.**** commented on this pull request. I think this is probably good to merge once we change to enumerate all-rules. @C-Hipple <https://github.com/C-Hipple> do you plan to make that change, or would you like a hand? — Reply to this email directly, view it on GitHub <#9078 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACBKSYUSPPC5AYNRJ5PRJ33YJDMIFAVCNFSM6AAAAABAOGQWJWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZYGU2TENJUHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

charliermarsh · 2023-12-13T03:52:42Z

Nice, thanks @C-Hipple!

C-Hipple · 2023-12-13T04:24:55Z

please don't merge it yet--I'm adding a few more sarif fields

charliermarsh · 2023-12-13T04:25:42Z

@C-Hipple -- Okay, sounds good. I just removed some of the copy and clone changes since the code seems to compile without them now. I'll wait for you to mark as ready for review.

C-Hipple · 2023-12-13T04:26:23Z

Thanks--your changes look good. Appreciate your input there as I"m relatively new to rust and was kinda using this issue as a way of getting a bit more familiar.

charliermarsh · 2023-12-13T04:28:11Z

Any questions let me know. Sorry if I stepped on your toes, didn't realize you were planning more changes!

C-Hipple · 2023-12-13T04:39:55Z

@C-Hipple -- Okay, sounds good. I just removed some of the copy and clone changes since the code seems to compile without them now. I'll wait for you to mark as ready for review.

Yeah, those were needed at one point with how SarifResults were copying rules into them, was also planning on backing them out but you beat me to it.

Any questions let me know. Sorry if I stepped on your toes, didn't realize you were planning more changes!

no worries!

C-Hipple · 2023-12-13T05:03:02Z

@charliermarsh I'm happy with the PR now, but I'm really unsure of the last commit--the anyhow import was failing lint due to being unused in wasm and it was also failing lint for unnecessary wrapping, so I just ignored the errors, but open to learning if there's a more appropriate/idiomatic way

charliermarsh · 2023-12-13T05:22:59Z

@C-Hipple -- What you have there is reasonable! But I'll play around with it and see if it can be improved.

charliermarsh · 2023-12-13T05:31:50Z

@C-Hipple -- The only change I made was to use absolute references for the imports that were unused in the Wasm path. It's not a material difference, but it is nice to avoid the unused import ignore.

charliermarsh · 2023-12-13T05:32:52Z

Unfortunately, #[allow(clippy::unnecessary_wraps)] is hard to avoid since we need the function to have the same signature on all platforms -- so if any of them return a Result, they all need to return a Result. We run into this in other parts of Ruff too.

C-Hipple added 2 commits December 9, 2023 20:41

add sarif output--update snapshots

f53695d

format

63be34a

C-Hipple mentioned this pull request Dec 10, 2023

Add SARIF output support #2046

Closed

C-Hipple changed the title ~~add sarif output--update snapshots~~ Feature: Add SARIF output support Dec 10, 2023

charliermarsh reviewed Dec 10, 2023

View reviewed changes

crates/ruff_linter/src/message/sarif.rs Show resolved Hide resolved

charliermarsh reviewed Dec 10, 2023

View reviewed changes

C-Hipple added 4 commits December 9, 2023 22:22

handle relative paths, clippy fixes, derive debug on SarifResult

4883f4a

remove sarif snapshot test

27d9298

lint

5c16403

target_arch = wasm32

8d0684e

C-Hipple commented Dec 10, 2023

View reviewed changes

crates/ruff_linter/src/message/sarif.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Dec 12, 2023

View reviewed changes

iter all rules, resolve unused import for wasm

9a94fc1

charliermarsh added 2 commits December 12, 2023 23:12

Run cargo fmt

252afa8

Merge branch 'main' into feature-add-sarif-output

685e48c

Back out some changes

25e835e

C-Hipple added 2 commits December 12, 2023 23:34

add help, end_line, end_column

5b830ca

update test to no-longer use default

9286b54

C-Hipple added 2 commits December 12, 2023 23:40

cargo fmt

f12f0f5

fix lint on wasm

3dad7ad

C-Hipple marked this pull request as ready for review December 13, 2023 05:07

charliermarsh force-pushed the feature-add-sarif-output branch 3 times, most recently from 16d4bf0 to 3f5438b Compare December 13, 2023 05:26

Try removing clippy ignore

3f5438b

charliermarsh approved these changes Dec 13, 2023

View reviewed changes

charliermarsh added the linter Related to the linter label Dec 13, 2023

charliermarsh merged commit cb99815 into astral-sh:main Dec 13, 2023

krehel mentioned this pull request Dec 13, 2023

ruff 0.1.8 Homebrew/homebrew-core#157266

Merged

C-Hipple deleted the feature-add-sarif-output branch December 19, 2023 20:15

Conversation

C-Hipple commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Open Points

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charliermarsh Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

Uh oh!

charliermarsh left a comment

Choose a reason for hiding this comment

Uh oh!

C-Hipple commented Dec 13, 2023 via email

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

C-Hipple commented Dec 13, 2023

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

C-Hipple commented Dec 13, 2023

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

C-Hipple commented Dec 13, 2023

Uh oh!

C-Hipple commented Dec 13, 2023

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

charliermarsh commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

C-Hipple commented Dec 10, 2023 •

edited

Loading

charliermarsh Dec 10, 2023 •

edited

Loading

github-actions bot commented Dec 10, 2023 •

edited

Loading

`ruff-ecosystem` results