Move fix suggestion to subdiagnostic #19464

ntBre · 2025-07-21T14:11:54Z

Summary

This PR tweaks Ruff's internal usage of the new diagnostic model to more closely
match the intended use, as I understand it. Specifically, it moves the fix/help
suggestion from the primary annotation's message to a subdiagnostic. In turn, it
adds the secondary/noqa code as the new primary annotation message. As shown in
the new ruff_db tests, this more closely mirrors Ruff's current diagnostic
output.

I also added Severity::Help to render the fix suggestion with a help: prefix
instead of info:.

These changes don't have any external impact now but should help a bit with #19415.

Test Plan

New full output format tests in ruff_db

Rendered Diagnostics

Full diagnostic output from annotate-snippets in this PR:

error[unused-import]: `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^
   |
 help: Remove unused import: `os`

Current Ruff output for the same code:

fib.py:1:8: F401 [*] `os` imported but unused
  |
1 | import os
  |        ^^ F401
  |
  = help: Remove unused import: `os`

Proposed final output after #19415:

F401 [*] `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^
   |
 help: Remove unused import: `os`

These are slightly updated from #19464 (comment) below to remove the extra noqa codes in the primary annotation messages for the first and third cases.

github-actions · 2025-07-21T14:16:21Z

`mypy_primer` results

No ecosystem changes detected ✅
No memory usage changes detected ✅

codspeed-hq · 2025-07-21T14:21:32Z

CodSpeed Instrumentation Performance Report

Merging #19464 will not alter performance

_{Comparing brent/new-diagnostic-suggestion (10676cb) with main (c82fa94)}

Summary

✅ 41 untouched benchmarks

github-actions · 2025-07-21T14:22:17Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

ntBre · 2025-07-21T15:43:58Z

I'm not coming up with much to do about the perf regression. At first I thought using a SmallVec for the subdiagnostics might help, as I think we discussed briefly before, but that made things slightly worse. Looking at the flamegraphs on codspeed, the difference is just from the extra IntoDiagnosticMessage/Display call rather than any Vec allocations.

AlexWaygood · 2025-07-21T15:49:36Z

I also added Severity::Help to render the fix suggestion with a help: prefix
instead of info:.

Nice -- I've wanted this for a while for ty's diagnostics, but never got round to writing up the issue! :-)

MichaReiser · 2025-07-21T15:54:52Z

Nice -- I've wanted this for a while for ty's diagnostics, but never got round to writing up the issue! :-)

Was it help or note? Trying to remember...

In turn, it adds the secondary/noqa code as the new primary annotation message.

It's not clear to me if this is indeed better. I find the code mostly redudant as it's already rendered in the diagnostic summary. But it's hard to judge from the changes in this PR. Can you show how the full diagnostic rendering is changing and what you propose to be the final rendering?

ntBre · 2025-07-21T15:56:32Z

It's not clear to me if this is indeed better. I find the code mostly redudant as it's already rendered in the diagnostic summary.

I kind of agree, and that would probably fix the benchmark regression too! I'll revert that part

AlexWaygood · 2025-07-21T15:58:08Z

Was it help or note? Trying to remember...

note might also be useful in some situations, but help would I think be perfect for things like this, where we're trying to steer the user directly towards a potential solution:

ruff/crates/ty_python_semantic/src/types/infer.rs

Lines 4749 to 4751 in 5cace28

    
           diag.info(format_args!( 
        
               "Consider adding a declaration to the global scope, e.g. `{name}: int`" 
        
           ));

MichaReiser

I don't think I've the full context to review this PR just yet. My main concern is that we over emphesize on not changing the output format vs picking the format that's most intuitive.

MichaReiser · 2025-07-21T15:57:48Z

crates/ruff_db/src/diagnostic/mod.rs

@@ -379,7 +386,9 @@ impl Diagnostic {

    /// Returns the fix suggestion for the violation.


I find it very hard to judge which methods are Ruff specific and which aren't. Can we update the doc comments and ideally prefix the ruff specific methods?

Hmm, I see what you mean, but this isn't necessarily Ruff-specific, unless we think we would store the fix suggestion in a different part of the Diagnostic for ty (or not have "fix suggestions" in the same way). I can definitely add a note about this only currently being used by Ruff, if that would help, or rename it anyway.

The only other ambiguous method that I saw was Diagnostic::body, which is just an alias for Diagnostic::primary_message. It might help to delete that one now.

I think the method is misleading because the first sub diagnostic isn't guaranteed to always be the fix suggestion. It can also be a second diagnostic that shows a second code frame (e.g. to highlight the definition side of a missing argument).

Overall, Diagnostic is intentionally unstructured to give rules a very flexible framework on how they want to structure their diagnostics and I suspect that we'll move Ruff into a similar direction.

This does raise the question on how we'll handle suggestions in the output formats that currently emit them as a separate field. We may need a different way to mark those.

This discussion makes me realize that we'll need to rework the cache before Ruff can make use of the new Diagnostic features. The cache only stores exactly the fields required by ViolationMetadata. This means, any extra field will be lost after deserializing the message again. Instead, what the cache should do is serialize/deserialize the entire Diagnostic as is.

We also need to review all other places where suggestion is used to ensure they don't make any assumption about how a diagnostic is structured.

I would probably rename the method to first_help_text and clearly document that it is the message of the first sub-diagnostic with a Help severity and that this is used as the fix text in some of Ruff's output formats.

Making the Caching so that it can restore any diagnostic is a follow up task.

Thanks, that makes a lot of sense. I'll rename and document it as you suggested and follow up on the caching separately.

I looked at the references to suggestion, and they all assume that this will be the fix suggestion, but I don't think they make any other assumption about the diagnostic's structure.

crates/ruff/src/cache.rs 457: suggestion: msg.suggestion().map(ToString::to_string), crates/ruff_db/src/diagnostic/mod.rs 388: pub fn suggestion(&self) -> Option<&str> { crates/ruff_db/src/diagnostic/render/json.rs 90: message: diagnostic.suggestion(), crates/ruff_linter/src/message/text.rs 189: let suggestion = self.message.suggestion(); crates/ruff_linter/src/test.rs 275: !(fixable && diagnostic.suggestion().is_none()), crates/ruff_server/src/lint.rs 241: let suggestion = diagnostic.suggestion(); crates/ruff_wasm/src/lib.rs 237: message: msg.suggestion().map(ToString::to_string),

Fortunately there aren't as many as I expected, and the ones in the cache and text.rs should go away soon, it sounds like.

MichaReiser · 2025-07-21T16:00:28Z

crates/ruff_db/src/diagnostic/mod.rs

 pub enum Severity {
+    Help,


I'm a bit torn if we want Help as a Diagnostic severity or only allow it at the sub-diagnostic level/annotations. Curious what @BurntSushi thinks

Ah sorry, just catching up to this now. I would have been fine adding this to Severity. :-) But the extra type gives us a bit more precision, so that's nice.

ntBre · 2025-07-21T16:09:06Z

Can you show how the full diagnostic rendering is changing and what you propose to be the final rendering?

Full diagnostic output from annotate-snippets in this PR:

error[unused-import]: `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^ F401
   |
 help: Remove unused import: `os`

Current Ruff output for the same code:

fib.py:1:8: F401 [*] `os` imported but unused
  |
1 | import os
  |        ^^ F401
  |
  = help: Remove unused import: `os`

Proposed final output after #19415:

F401 [*] `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^ F401
   |
 help: Remove unused import: `os`

The main difference from the current Ruff diagnostic is that the filename is on the second line, and the main/only difference from current ty diagnostics is F401 [*] replacing error[unused-import].

Maybe it was a bad idea to split this out from #19415 since the final product is a combination of the two. I just thought it might be a little easier to do this mostly-internal rearrangement separately.

MichaReiser · 2025-07-21T16:14:57Z

Thanks. I do appreciate this being a separate PR. It just requires a few more details in the PR description to review it.

F401 [*] `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^ F401
   |
 help: Remove unused import: `os`

I think the repeated F401 makes ltitle sense. It also prevents any lint to set a more specific message for the primary annotation (which is a very prominent and useful message!).

Keeping suggestion as a sub diagnostic does make sense to me

MichaReiser

Thanks. This looks good to me now.

The only thing I suggest changing is to add a SubDiagnosticSeverity (or similar) which is the same as Severity but it also has a Help variant.

This is a bit annoying but it allows us to skip the decision if we neeed/want a Help severity at the diagnostic level, which we don't need now.

Summary -- This PR tweaks Ruff's internal usage of the new diagnostic model to more closely match the intended use, as I understand it. Specifically, it moves the fix/help suggestion from the primary annotation's message to a subdiagnostic. In turn, it adds the secondary/noqa code as the new primary annotation message. As shown in the new `ruff_db` tests, this more closely mirrors Ruff's current diagnostic output. I also added `Severity::Help` to render the fix suggestion with a `help:` prefix instead of `info:`. These changes don't have any external impact now but should help a bit with Test Plan -- New full output format tests in `ruff_db`

This reverts commit 7c0e0ff.

the idea here is that NoqaCode::display is the expensive step and this basically reduces it to cloning a string instead of reformatting the noqa code

BurntSushi · 2025-07-22T16:29:07Z

crates/ruff_db/src/diagnostic/mod.rs

+    Warning,
+    Error,
+    Fatal,
+}


It might be helpful to add a comment here explaining that this type was added to represent the additional Help variant that isn't present on Severity. And that, in particular, if we wanted to add Severity::Help, it would be fine to delete this type and move back to one type.

I say this because it clarifies that Help is the only reason for this type's existence, and that there isn't some other need for it. So hopefully it's easier for someone down the line to remove it if we do add Severity::Help.

Oh good idea. I'll sneak this into #19398 since I haven't merged it yet. Thanks for the reviews!

Let me know if you find anything else. I'll be working in the same area on full output next.

* main: (76 commits) Move fix suggestion to subdiagnostic (#19464) [ty] Implement non-stdlib stub mapping for classes and functions (#19471) [ty] Disallow illegal uses of `ClassVar` (#19483) [ty] Disallow `Final` in function parameter/return-type annotations (#19480) [ty] Extend `Final` test suite (#19476) [ty] Minor change to diagnostic message for invalid Literal uses (#19482) [ty] Detect illegal non-enum attribute accesses in Literal annotation (#19477) [ty] Reduce size of `TypeInference` (#19435) Run MD tests for Markdown-only changes (#19479) Revert "[ty] Detect illegal non-enum attribute accesses in Literal annotation" [ty] Detect illegal non-enum attribute accesses in Literal annotation [ty] Added semantic token support for more identifiers (#19473) [ty] Make tuple subclass constructors sound (#19469) [ty] Pass down specialization to generic dataclass bases (#19472) [ty] Garbage-collect reachability constraints (#19414) [ty] Implicit instance attributes declared `Final` (#19462) [ty] Expansion of enums into unions of literals (#19382) [ty] Avoid rechecking the entire project when changing the opened files (#19463) [ty] Add warning for unknown `TY_MEMORY_REPORT` value (#19465) [ty] Sync vendored typeshed stubs (#19461) ...

* main: [ty] Use `ThinVec` for sub segments in `PlaceExpr` (#19470) [ty] Splat variadic arguments into parameter list (#18996) [`flake8-pyi`] Skip fix if all `Union` members are `None` (`PYI016`) (#19416) Skip notebook with errors in ecosystem check (#19491) [ty] Consistent use of American english (in rules) (#19488) [ty] Support iterating over enums (#19486) Fix panic for illegal `Literal[…]` annotations with inner subscript expressions (#19489) Move fix suggestion to subdiagnostic (#19464) [ty] Implement non-stdlib stub mapping for classes and functions (#19471) [ty] Disallow illegal uses of `ClassVar` (#19483) [ty] Disallow `Final` in function parameter/return-type annotations (#19480) [ty] Extend `Final` test suite (#19476) [ty] Minor change to diagnostic message for invalid Literal uses (#19482) [ty] Detect illegal non-enum attribute accesses in Literal annotation (#19477) [ty] Reduce size of `TypeInference` (#19435) Run MD tests for Markdown-only changes (#19479) Revert "[ty] Detect illegal non-enum attribute accesses in Literal annotation" [ty] Detect illegal non-enum attribute accesses in Literal annotation [ty] Added semantic token support for more identifiers (#19473) [ty] Make tuple subclass constructors sound (#19469)

* main: (28 commits) [ty] highlight the argument in `static_assert` error messages (astral-sh#19426) [ty] Infer single-valuedness for enums based on `int`/`str` (astral-sh#19510) [ty] Restructure submodule query around `File` dependency [ty] Make `Module` a Salsa ingredient [ty] Reachability analysis for `isinstance(…)` branches (astral-sh#19503) [ty] Normalize single-member enums to their instance type (astral-sh#19502) [ty] Invert `ty_ide` and `ty_project` dependency (astral-sh#19501) [ty] Implement mock language server for testing (astral-sh#19391) [ty] Detect enums if metaclass is a subtype of EnumType/EnumMeta (astral-sh#19481) [ty] perform type narrowing for places marked `global` too (astral-sh#19381) [ty] Use `ThinVec` for sub segments in `PlaceExpr` (astral-sh#19470) [ty] Splat variadic arguments into parameter list (astral-sh#18996) [`flake8-pyi`] Skip fix if all `Union` members are `None` (`PYI016`) (astral-sh#19416) Skip notebook with errors in ecosystem check (astral-sh#19491) [ty] Consistent use of American english (in rules) (astral-sh#19488) [ty] Support iterating over enums (astral-sh#19486) Fix panic for illegal `Literal[…]` annotations with inner subscript expressions (astral-sh#19489) Move fix suggestion to subdiagnostic (astral-sh#19464) [ty] Implement non-stdlib stub mapping for classes and functions (astral-sh#19471) [ty] Disallow illegal uses of `ClassVar` (astral-sh#19483) ... # Conflicts: # crates/ty_ide/src/goto.rs

ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jul 21, 2025

ntBre marked this pull request as ready for review July 21, 2025 15:44

ntBre requested review from carljm, MichaReiser, AlexWaygood, sharkdp and dcreager as code owners July 21, 2025 15:44

ntBre requested review from BurntSushi and removed request for dcreager, carljm, sharkdp and AlexWaygood July 21, 2025 15:44

MichaReiser reviewed Jul 21, 2025

View reviewed changes

MichaReiser approved these changes Jul 22, 2025

View reviewed changes

ntBre added 5 commits July 22, 2025 08:50

add and use Severity::Help

04c820d

try using SmallVec for subdiagnostics

745b4ce

Revert "try using SmallVec for subdiagnostics"

7618525

This reverts commit 7c0e0ff.

try converting to string earlier

a4521ac

the idea here is that NoqaCode::display is the expensive step and this basically reduces it to cloning a string instead of reformatting the noqa code

ntBre added 3 commits July 22, 2025 08:50

avoid duplicate noqa code in primary message

a167803

Diagnostic::suggestion -> first_help_text, update docs

12226e3

add SubDiagnosticSeverity

90497de

ntBre force-pushed the brent/new-diagnostic-suggestion branch from c72c80d to 90497de Compare July 22, 2025 12:51

delete leftover Help variant in ty_wasm

10676cb

ntBre merged commit fd335eb into main Jul 22, 2025
37 checks passed

ntBre deleted the brent/new-diagnostic-suggestion branch July 22, 2025 14:03

ntBre added a commit that referenced this pull request Jul 22, 2025

use concise_message again after #19464

273dcd4

BurntSushi reviewed Jul 22, 2025

View reviewed changes

		@@ -379,7 +386,9 @@ impl Diagnostic {

		/// Returns the fix suggestion for the violation.

Move fix suggestion to subdiagnostic #19464

Move fix suggestion to subdiagnostic #19464

Uh oh!

Conversation

ntBre commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Rendered Diagnostics

Uh oh!

github-actions bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

codspeed-hq bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #19464 will not alter performance

Summary

Uh oh!

github-actions bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

ntBre commented Jul 21, 2025

Uh oh!

AlexWaygood commented Jul 21, 2025

Uh oh!

MichaReiser commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented Jul 21, 2025

Uh oh!

AlexWaygood commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser left a comment

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!

MichaReiser Jul 21, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Jul 21, 2025

Uh oh!

MichaReiser commented Jul 21, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntBre commented Jul 21, 2025 •

edited

Loading

github-actions bot commented Jul 21, 2025 •

edited

Loading

`mypy_primer` results

codspeed-hq bot commented Jul 21, 2025 •

edited

Loading

github-actions bot commented Jul 21, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser commented Jul 21, 2025 •

edited

Loading

AlexWaygood commented Jul 21, 2025 •

edited

Loading

MichaReiser Jul 21, 2025 •

edited

Loading