Skip to content

Comments

Allow hiding the diagnostic severity in ruff_db#19644

Merged
ntBre merged 23 commits intomainfrom
brent/hide-severity
Aug 5, 2025
Merged

Allow hiding the diagnostic severity in ruff_db#19644
ntBre merged 23 commits intomainfrom
brent/hide-severity

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Jul 30, 2025

Summary

This PR is a spin-off from #19415. It enables replacing the severity and lint name in a ty-style diagnostic:

error[unused-import]: `os` imported but unused

with the noqa code and optional fix availability icon for a Ruff diagnostic:

F401 [*] `os` imported but unused
F821 Undefined name `a`

or nothing at all for a Ruff syntax error, where the previous SyntaxError prefix is replaced with the invalid-syntax lint name:

invalid-syntax: Expected one or more symbol names after import

Ruff adds the SyntaxError prefix to these messages manually.

Initially (d912458), I just passed a hide_severity flag through a bunch of calls to get it into annotate-snippets, but after looking at it again today, I think reusing the None severity/level gave a nicer result. As I note in a lengthy code comment, I think all of this code should be temporary and reverted when Ruff gets real severities, so hopefully it's okay if it feels a little hacky.

I think the main visible downside of this approach is that we can't style the asterisk in the fix availabilty icon in cyan, as in Ruff's current output. It's part of the message in this PR and any styling gets overwritten in annotate-snippets.

image

Hmm, I guess reusing Level::None also means the F401 isn't red anymore. Maybe my initial approach was better after all. In any case, the rest of the PR should be basically the same, it just depends how we want to toggle the severity.

Test Plan

New ruff_db tests. These snapshots should be compared to the two tests just above them (hide_severity_output vs output and hide_severity_syntax_errors against syntax_errors).

@ntBre ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jul 30, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2025

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+29 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

openai/openai-cookbook (+29 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select A,E703,F704,B015,B018,D100

+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 12:1:8: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:11: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:19: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:50: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:11: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:16: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:22: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:24: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:27: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:34: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:48: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:49: invalid-syntax: Expected '}', found NonLogicalNewline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:12:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:3: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:4: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:5: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:6: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:1: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:2: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:4:7: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:5:14: invalid-syntax: Expected ':', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:25: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:46: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:47: invalid-syntax: Expected '}', found newline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:9: invalid-syntax: Expected ',', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:13: invalid-syntax: Expected ':', found 'break'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:8:28: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:9:18: invalid-syntax: Expected an expression

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
invalid-syntax: 29 29 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+29 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

openai/openai-cookbook (+29 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select A,E703,F704,B015,B018,D100

+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 12:1:8: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:11: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:19: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 21:1:50: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:11: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:16: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:22: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:10:24: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:27: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:34: invalid-syntax: Expected ',', found name
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:48: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:11:49: invalid-syntax: Expected '}', found NonLogicalNewline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:12:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:3: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:4: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:5: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:13:6: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:1: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:14:2: invalid-syntax: Expected a statement
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:4:7: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:5:14: invalid-syntax: Expected ':', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:25: invalid-syntax: Expected ',', found '='
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:46: invalid-syntax: Expected ',', found ';'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:47: invalid-syntax: Expected '}', found newline
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:6:9: invalid-syntax: Expected ',', found '{'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:13: invalid-syntax: Expected ':', found 'break'
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:7:1: invalid-syntax: Unexpected indentation
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:8:28: invalid-syntax: Simple statements must be separated by newlines or semicolons
+ examples/mcp/databricks_mcp_cookbook.ipynb:cell 30:9:18: invalid-syntax: Expected an expression

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
invalid-syntax: 29 29 0 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@ntBre ntBre marked this pull request as ready for review July 30, 2025 18:57
@MichaReiser
Copy link
Member

I'll take a closer look tomorrow but my preference for SyntaxError is to not special case them and render invalid-syntax: <message>

@ntBre
Copy link
Contributor Author

ntBre commented Jul 30, 2025

I thought it would be a bit repetitive to render invalid-syntax in front of SyntaxError. I think it would disrupt the concise output to strip SyntaxError: from the front of the actual message contents (unless we literally strip_prefix here, which I did consider in one early draft), but I agree with you otherwise.

@MichaReiser
Copy link
Member

Can't we remove the SyntaxError: prefix in whatever place we construct those diagnostics

@ntBre
Copy link
Contributor Author

ntBre commented Jul 30, 2025

Yes, but that will also change the concise diagnostics, where we wanted to preserve compatibility for now.

That's exactly what we should do as soon as we can break concise diagnostics for syntax errors, though.

@ntBre
Copy link
Contributor Author

ntBre commented Jul 30, 2025

I guess we could move the SyntaxError prefix into the concise rendering code for now.

Edit: after trying this, we'd actually have to move the prefix into every output format except full to avoid changing their snapshots. (And except gitlab, which already uses strip_prefix)

@MichaReiser
Copy link
Member

Edit: after trying this, we'd actually have to move the prefix into every output format except full to avoid changing their snapshots. (And except gitlab, which already uses strip_prefix)

I'm sort of fine if they change. They should all contain the lint id and the message, so that the prefix seems unnecessary

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.

I now took a closer look. I'd prefer to avoid the invalid syntax special casing. An empty id feels hacky. I'd simply make invalid syntax an error like every other one (except that we use the diagnostic.id instead of the secondary_code as we should for all errors without a secondary code)

@ntBre
Copy link
Contributor Author

ntBre commented Jul 31, 2025

That totally makes sense to me, I was just worried about compatibility. Is this like what you had in mind?

-SyntaxError: Expected one or more symbol names after import
+invalid-syntax SyntaxError: Expected one or more symbol names after import

That's after unwrapping to the diagnostic.id instead of String::default. I'm planning also to remove the SyntaxError: prefix to give this:

invalid-syntax Expected one or more symbol names after import

I'd prefer having a colon there (invalid-syntax:), but I don't think there's an easy way to do that without also having a colon before a rule code. I think that also looks nice for a normal rule:

F821: Undefined name `a`
 --> undef.py:1:4      

but not so much with the fix indicator:

F401: [*] `os` imported but unused
 --> fib.py:1:8                  

Eh, maybe that's not so bad. What do you think?

@MichaReiser
Copy link
Member

MichaReiser commented Jul 31, 2025

I do like the colon! But I don't think we should make the change now as it breaks the concise output format. I think it's fine without a colon and we can create an issue to improve this later

ntBre added 3 commits July 31, 2025 12:19
This one is complicated a bit by the `unique_rules` collection, which excludes
syntax errors. I added the new enum to avoid needing to change the definition of
`SarifRule::from` for `SecondaryCode`.

Initially it felt like a syntax error should also be a unique `SarifRule`, but
it doesn't have most of the other fields like `linter`, `summary`, or `url`, and
`name` and `code` would be the same.
@ntBre ntBre force-pushed the brent/hide-severity branch from 1e1bd62 to 1cdb26c Compare July 31, 2025 16:23
@ntBre
Copy link
Contributor Author

ntBre commented Jul 31, 2025

Okay I think I've implemented what we discussed here and on Discord for the full and concise formats. In short, we just replaced the manual SyntaxError: prefix with invalid-syntax:, which reuses the LintId. All of the snapshots in b8aa43f fall into this category.

The junit, pylint, and rdjson formats already included the LintId for syntax errors, so I similarly accepted those without further changes in 0dc0616.

The other output formats did not fall back on the LintId, so I went through each of them in separate commits.

The github format seems a bit redundant because we print invalid-syntax twice, but that matches what we do for noqa codes too:

 ::error title=Ruff (F821),file=[TMP]/input.py,line=2,col=5,endLine=2,endColumn=6::input.py:2:5: F821 Undefined name `y`
-::error title=Ruff,file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+::error title=Ruff (invalid-syntax),file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: invalid-syntax: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)

The sarif changes are slightly complicated by the unique_rules collection, which excludes syntax errors. I added the new enum to avoid needing to change the definition of SarifRule::from for SecondaryCode.

Initially it felt like a syntax error should also be a unique SarifRule, but it doesn't have most of the other fields like linter, summary, or url, and name and code would be the same.

@ntBre
Copy link
Contributor Author

ntBre commented Jul 31, 2025

I added a similar change for WASM as part of fixing that test failure. Instead of this:
image

syntax errors now render as this in the playground:

image

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.

The snapshot changes look good to me. I'll review the rendering changes tomorrow

::error title=Ruff (F401),file=[TMP]/input.py,line=1,col=8,endLine=1,endColumn=10::input.py:1:8: F401 `os` imported but unused
::error title=Ruff (F821),file=[TMP]/input.py,line=2,col=5,endLine=2,endColumn=6::input.py:2:5: F821 Undefined name `y`
::error title=Ruff,file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
::error title=Ruff (invalid-syntax),file=[TMP]/input.py,line=3,col=1,endLine=3,endColumn=6::input.py:3:1: invalid-syntax: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
Copy link
Member

Choose a reason for hiding this comment

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

I agree, this feels odd but it is consistent to how we handle the other rules. I'm fine leaving this as is (seems like a separate change)

@ntBre
Copy link
Contributor Author

ntBre commented Jul 31, 2025

I think the ecosystem check makes sense now. I could make the regex look for either SyntaxError or invalid-syntax temporarily to get +0/-0, but I think this will be fine after this PR is merged.

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!

@ntBre
Copy link
Contributor Author

ntBre commented Aug 1, 2025

Thanks for the review! I fixed your comments and also added a small commit (1ae8695) fixing the styling issue I pointed out in the summary (Severity::None meant the error code was unformatted). Not sure why I didn't think of it before, but this also fits into the TODO about severities, so I added an additional note to the comment to be sure we catch it.

With that, the diagnostics look quite similar in color to Ruff's current full output:

image

It's still a bit of a shame about the missing cyan in the fixability marker, but I don't think there's a good way to fix that unless we add an is_fixable flag to the Annotation type in annotate-snippets. I guess that's not such a big deal, though, if it sounds good to you?

(I temporarily switched --output-format concise to trigger the new full rendering in myruff, only locally!)

@MichaReiser
Copy link
Member

It's still a bit of a shame about the missing cyan in the fixability marker, but I don't think there's a good way to fix that unless we add an is_fixable flag to the Annotation type in annotate-snippets

At this point it will be very hard to every use the upstream crate again because I don't think we can convince them to add this as a feature. But who knows, maybe the have something much more flexible.

I think I'd be okay with adding it

@ntBre
Copy link
Contributor Author

ntBre commented Aug 1, 2025

I got something working, but it's a little more invasive than I hoped. I didn't realize there were at least two distinct Annotation types in annotate-snippets, so I had to pass it through snippet::Message instead of snippet::Annotation like I was expecting. snippet::Annotation never gets converted directly into a display_list::Annotation, as far as I can tell. The Message header is rendered separately from its snippets, which contain the snippet::Annotations, so the header rendering doesn't have access to the snippet::Annotations.

The asterisk is also bold, but I figured that was close enough, in lieu of plumbing another style through annotate-snippets.

The top output is again from this branch.

image

Comment on lines 256 to 260
if annotation.is_fixable {
buffer.append(line_offset, "[", stylesheet.none);
buffer.append(line_offset, "*", stylesheet.help);
buffer.append(line_offset, "] ", stylesheet.none);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to gate this behind hide_severity or could we always render it after the severity/and id (unconditionally) if is_fixable is true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point. I don't think we strictly need to gate it, but it would look something like this if we had a fixable ty-style diagnostic right now:

error[invalid-assignment][*]: Object of type `Literal[1]` is not assignable to `str`

I guess that looks pretty good, and it should be unreachable currently, in any case. I'll move it down!

@ntBre ntBre merged commit 78e5fe0 into main Aug 5, 2025
35 checks passed
@ntBre ntBre deleted the brent/hide-severity branch August 5, 2025 13:56
ntBre added a commit that referenced this pull request Aug 5, 2025
Summary
--

This is the other commit I wanted to spin off from #19415, currently
stacked on #19644.

This PR suppresses blank snippets for empty ranges at the very beginning
of a file, and for empty ranges in non-existent files. Ruff includes
empty ranges for IO errors, for example.


https://github.com/astral-sh/ruff/blob/f4e93b63351bfe035b1fc5a7bc605a52979e7841/crates/ruff_linter/src/message/text.rs#L100-L110

The diagnostics now look like this (new snapshot test):

```
error[test-diagnostic]: main diagnostic message
--> example.py:1:1                             
```

Instead of [^*]

```
error[test-diagnostic]: main diagnostic message
--> example.py:1:1
 |
 |
```

Test Plan
--

A new `ruff_db` test showing the expected output format

[^*]: This doesn't correspond precisely to the example in the PR because
of some details of the diagnostic builder helper methods in `ruff_db`,
but you can see another example in the current version of the summary in
#19415.
ntBre added a commit that referenced this pull request Aug 5, 2025
I thought this was unreachable when adding
it (#19644 (comment)), but
obviously it's tested here!

Without the spacing tweak, the snapshots were rendering like this:

```
error[unused-import][*] : `os` imported but unused
error[unused-import][*] : `math` imported but unused
```

with awkward extra spaces before the colons.
ntBre added a commit that referenced this pull request Aug 13, 2025
## Summary

Fixes #19881. While I was here, I also made a couple of related tweaks
to the output format. First, we don't need to strip the `SyntaxError: `
prefix anymore since that's not added directly to the diagnostic message
after #19644. Second, we can use `secondary_code_or_id` to fall back on
the lint ID for syntax errors, which changes the `check_name` from
`syntax-error` to `invalid-syntax`. And then the main change requested
in the issue, prepending the `check_name` to the description.

## Test Plan

Existing tests and a new screenshot from GitLab:

<img width="362" height="113" alt="image"
src="https://github.com/user-attachments/assets/97654ad4-a639-4489-8c90-8661c7355097"
/>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostics Related to reporting of diagnostics. internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants