Show fixes by default #19919

ntBre · 2025-08-14T18:52:50Z

Summary

This PR fixes #7352 by exposing the show_fix_diff option used in our snapshot tests in the CLI. As the issue suggests, we plan to make this the default output format in the future, so this is added to the full output format in preview for now.

This turned out to be pretty straightforward. I just used our existing Applicability settings to determine whether or not to print the diff.

The snapshot differences are because we now set Applicability::DisplayOnly for our snapshot tests. This Applicability is also used to determine whether or not the fix icon ([*]) is rendered, so this is now shown for display-only fixes in our snapshots. This was already the case previously, but we were only setting Applicability::Unsafe in these tests and ignoring the Applicability when rendering fix diffs. CLI users can't enable display-only fixes, so this is only a test change for now, but this should work smoothly if we decide to expose a --display-only-fixes flag or similar in the future.

I also deleted the PrinterFlags::SHOW_FIX_DIFF flag. This was completely unused before, and it seemed less confusing just to delete it than to enable it in the right place and check it along with the OutputFormat and preview.

Test Plan

I only added one CLI test for now. I'm kind of assuming that we have decent coverage of the cases where this shouldn't be firing, especially the output_format CLI test, which shows that this definitely doesn't affect non-preview full output. I'm happy to add more tests with different combinations of options, if we're worried about any in particular. I did try --diff and --preview and a few other combinations manually.

And here's a screenshot using our trusty UP049 example from the design discussion confirming that all the colors and other formatting still look as expected:

And one with an unsafe fix to see the footer:

Related issues and PR

I think we could probably close all of these issues now. I think we've either resolved or avoided most of them, and if we encounter them again with the new output format, it would probably make sense to open new ones anyway.

github-actions · 2025-08-14T19:04:10Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Summary -- This is a preparatory PR in support of #19919. It moves our `Diff` rendering code from `ruff_linter` to `ruff_db`, where we have direct access to the `DiagnosticStylesheet` used by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests. The `Diff` implementation is mostly unchanged. I just switched from a Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an `expect_ruff_source_file` call) and updated the `LineStyle` struct and other styling calls to use `fmt_styled` and our existing stylesheet. In support of these changes, I added three styles to our stylesheet: `insertion` and `deletion` for the corresponding diff operations, and `underline`, which apparently we _can_ use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me. I moved the `calculate_print_width` function from the now-deleted `diff.rs` to a method on `OneIndexed`, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (using `ilog10` instead of a loop). This does make it `const` and slightly simplifies things in my opinion, but I'm happy to revert it if preferred. I also inlined a version of `show_nonprinting` from the `ShowNonprinting` trait in `ruff_linter`: https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_linter/src/text_helpers.rs#L3-L5 This trait is now only used in `source_kind.rs`, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them. https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_db/src/diagnostic/render.rs#L990-L998 We could also move the trait to another crate where we can use it in `ruff_db` instead of inlining here, of course. Finally, this PR makes `TextEmitter` a very thin wrapper around a `DisplayDiagnosticsConfig`. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to a `DisplayDiagnostics`. Test Plan -- Existing snapshot tests with diffs

## Summary I spun this off from #19919 to separate the rendering code change and snapshot updates from the (much smaller) changes to expose this in the CLI. I grouped all of the `ruff_linter` snapshot changes in the final commit in an effort to make this easier to review. The code changes are in [this range](https://github.com/astral-sh/ruff/pull/20101/files/619395eb417d2030e31fbb0e487a72dac58902db). I went through all of the snapshots, albeit fairly quickly, and they all looked correct to me. In the last few commits I was trying to resolve an existing issue in the alignment of the line number separator: https://github.com/astral-sh/ruff/blob/73720c73be981df6f71ce837a67ca1167da0265e/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap#L87-L89 In the snapshot above on `main`, you can see that a double-digit line number at the end of the context lines for a snippet was causing a misalignment with the other separators. That's now resolved. The one downside is that this can lead to a mismatch with the diagnostic above: ``` C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as a tuple literal) --> C409.py:4:6 | 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) 4 | t4 = tuple([ | ______^ 5 | | 1, 6 | | 2 7 | | ]) | |__^ 8 | t5 = tuple( 9 | (1, 2) | help: Rewrite as a tuple literal 1 | t1 = tuple([]) 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) - t4 = tuple([ 4 + t4 = ( 5 | 1, 6 | 2 - ]) 7 + ) 8 | t5 = tuple( 9 | (1, 2) 10 | ) note: This is an unsafe fix and may remove comments or change runtime behavior ``` But I don't think we can avoid that without really reworking this rendering to make the diagnostic and diff rendering aware of each other. Anyway, this should only happen in relatively rare cases where the diagnostic is near a digit boundary and also near a context boundary. Most of our diagnostics line up nicely. Another potential downside of the new rendering format is its handling of long stretches of `+` or `-` lines: ``` help: Replace with `Literal[...] | None` 21 | ... 22 | 23 | - def func6(arg1: Literal[ - "hello", - None # Comment 1 - , "world" - ]): 24 + def func6(arg1: Literal["hello", "world"] | None): 25 | ... 26 | 27 | note: This is an unsafe fix and may remove comments or change runtime behavior ``` To me it just seems a little hard to tell what's going on with just a long streak of `-`-prefixed lines. I saw an even more exaggerated example at some point, but I think this is also fairly rare. Most of the snapshots seem more like the examples we looked at on Discord with plenty of `|` lines and pairs of `+` and `-` lines. ## Test Plan Existing tests plus one new test in `ruff_db` to isolate a line separator alignment issue

passing an Applicability here doesn't change the results for the CLI, which still converts from an UnsafeFixes, but will allow us to pass DisplayOnly for our snapshot tests

accept display-only fix changes we previously had separate logic for displaying these fix indicators and displaying fixes. we were only setting UnsafeFixes::Enabled, which translated to an Applicability of Unsafe, which is what was checked for this fixability icon. Then, we were unconditionally showing fixes, which included display-only fixes. I think using the actual applicability for both of these makes sense but makes these small changes to our display-only fix snapshots. since users still can't enable display-only fixes in the CLI, this will not be a user-facing change

remove unnecessary SHOW_FIX_DIFF flag

dhruvmanila · 2025-08-29T08:21:17Z

crates/ruff_db/src/diagnostic/mod.rs


+    /// Returns `true` if the diagnostic is [`fixable`](Diagnostic::fixable) and applies at the
+    /// configured applicability level.
+    pub fn fix_applies(&self, config: &DisplayDiagnosticConfig) -> bool {


nit: fix_applicable / is_fix_applicable

I was also thinking about this but landed on not saying something :) I think my best idea was has_applicable_fix

(I think the is_applicable verbiage makes more sense on the fix type)

(@ntBre un-resolving because I commented as you were resolving. Just for visibility — I don't have strong feelings)

Ah diagnostic.has_applicable_fix does read nicely, thanks!

Summary -- This is a preparatory PR in support of astral-sh#19919. It moves our `Diff` rendering code from `ruff_linter` to `ruff_db`, where we have direct access to the `DiagnosticStylesheet` used by our other diagnostic rendering code. As shown by the tests, this shouldn't cause any visible changes. The colors aren't exactly the same, as I note in a TODO comment, but I don't think there's any existing way to see those, even in tests. The `Diff` implementation is mostly unchanged. I just switched from a Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an `expect_ruff_source_file` call) and updated the `LineStyle` struct and other styling calls to use `fmt_styled` and our existing stylesheet. In support of these changes, I added three styles to our stylesheet: `insertion` and `deletion` for the corresponding diff operations, and `underline`, which apparently we _can_ use, as I hoped on Discord. This isn't supported in all terminals, though. It worked in ghostty but not in st for me. I moved the `calculate_print_width` function from the now-deleted `diff.rs` to a method on `OneIndexed`, where it was available everywhere we needed it. I'm not sure if that's desirable, or if my other changes to the function are either (using `ilog10` instead of a loop). This does make it `const` and slightly simplifies things in my opinion, but I'm happy to revert it if preferred. I also inlined a version of `show_nonprinting` from the `ShowNonprinting` trait in `ruff_linter`: https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_linter/src/text_helpers.rs#L3-L5 This trait is now only used in `source_kind.rs`, so I'm not sure it's worth having the trait or the macro-generated implementation (which is only called once). This is obviously closely related to our unprintable character handling in diagnostic rendering, but the usage seems different enough not to try to combine them. https://github.com/astral-sh/ruff/blob/f4be05a83be770c8c9781088508275170396b411/crates/ruff_db/src/diagnostic/render.rs#L990-L998 We could also move the trait to another crate where we can use it in `ruff_db` instead of inlining here, of course. Finally, this PR makes `TextEmitter` a very thin wrapper around a `DisplayDiagnosticsConfig`. It's still used in a few places, though, unlike the other emitters we've replaced, so I figured it was worth keeping around. It's a pretty nice API for setting all of the options on the config and then passing that along to a `DisplayDiagnostics`. Test Plan -- Existing snapshot tests with diffs

## Summary I spun this off from astral-sh#19919 to separate the rendering code change and snapshot updates from the (much smaller) changes to expose this in the CLI. I grouped all of the `ruff_linter` snapshot changes in the final commit in an effort to make this easier to review. The code changes are in [this range](https://github.com/astral-sh/ruff/pull/20101/files/619395eb417d2030e31fbb0e487a72dac58902db). I went through all of the snapshots, albeit fairly quickly, and they all looked correct to me. In the last few commits I was trying to resolve an existing issue in the alignment of the line number separator: https://github.com/astral-sh/ruff/blob/73720c73be981df6f71ce837a67ca1167da0265e/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap#L87-L89 In the snapshot above on `main`, you can see that a double-digit line number at the end of the context lines for a snippet was causing a misalignment with the other separators. That's now resolved. The one downside is that this can lead to a mismatch with the diagnostic above: ``` C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as a tuple literal) --> C409.py:4:6 | 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) 4 | t4 = tuple([ | ______^ 5 | | 1, 6 | | 2 7 | | ]) | |__^ 8 | t5 = tuple( 9 | (1, 2) | help: Rewrite as a tuple literal 1 | t1 = tuple([]) 2 | t2 = tuple([1, 2]) 3 | t3 = tuple((1, 2)) - t4 = tuple([ 4 + t4 = ( 5 | 1, 6 | 2 - ]) 7 + ) 8 | t5 = tuple( 9 | (1, 2) 10 | ) note: This is an unsafe fix and may remove comments or change runtime behavior ``` But I don't think we can avoid that without really reworking this rendering to make the diagnostic and diff rendering aware of each other. Anyway, this should only happen in relatively rare cases where the diagnostic is near a digit boundary and also near a context boundary. Most of our diagnostics line up nicely. Another potential downside of the new rendering format is its handling of long stretches of `+` or `-` lines: ``` help: Replace with `Literal[...] | None` 21 | ... 22 | 23 | - def func6(arg1: Literal[ - "hello", - None # Comment 1 - , "world" - ]): 24 + def func6(arg1: Literal["hello", "world"] | None): 25 | ... 26 | 27 | note: This is an unsafe fix and may remove comments or change runtime behavior ``` To me it just seems a little hard to tell what's going on with just a long streak of `-`-prefixed lines. I saw an even more exaggerated example at some point, but I think this is also fairly rare. Most of the snapshots seem more like the examples we looked at on Discord with plenty of `|` lines and pairs of `+` and `-` lines. ## Test Plan Existing tests plus one new test in `ruff_db` to isolate a line separator alignment issue

## Summary This PR fixes astral-sh#7352 by exposing the `show_fix_diff` option used in our snapshot tests in the CLI. As the issue suggests, we plan to make this the default output format in the future, so this is added to the `full` output format in preview for now. This turned out to be pretty straightforward. I just used our existing `Applicability` settings to determine whether or not to print the diff. The snapshot differences are because we now set `Applicability::DisplayOnly` for our snapshot tests. This `Applicability` is also used to determine whether or not the fix icon (`[*]`) is rendered, so this is now shown for display-only fixes in our snapshots. This was already the case previously, but we were only setting `Applicability::Unsafe` in these tests and ignoring the `Applicability` when rendering fix diffs. CLI users can't enable display-only fixes, so this is only a test change for now, but this should work smoothly if we decide to expose a `--display-only-fixes` flag or similar in the future. I also deleted the `PrinterFlags::SHOW_FIX_DIFF` flag. This was completely unused before, and it seemed less confusing just to delete it than to enable it in the right place and check it along with the `OutputFormat` and `preview`. ## Test Plan I only added one CLI test for now. I'm kind of assuming that we have decent coverage of the cases where this shouldn't be firing, especially the `output_format` CLI test, which shows that this definitely doesn't affect non-preview `full` output. I'm happy to add more tests with different combinations of options, if we're worried about any in particular. I did try `--diff` and `--preview` and a few other combinations manually. And here's a screenshot using our trusty UP049 example from the design discussion confirming that all the colors and other formatting still look as expected: <img width="786" height="629" alt="image" src="https://github.com/user-attachments/assets/94e408bc-af7b-4573-b546-a5ceac2620f2" /> And one with an unsafe fix to see the footer: <img width="782" height="367" alt="image" src="https://github.com/user-attachments/assets/bbb29e47-310b-4293-b2c2-cc7aee3baff4" /> ## Related issues and PR - astral-sh#7352 - astral-sh#12595 - astral-sh#12598 - astral-sh#12599 - astral-sh#12600 I think we could probably close all of these issues now. I think we've either resolved or avoided most of them, and if we encounter them again with the new output format, it would probably make sense to open new ones anyway.

ntBre added cli Related to the command-line interface preview Related to preview mode features labels Aug 14, 2025

ntBre mentioned this pull request Aug 20, 2025

Move diff rendering to ruff_db #20006

Merged

ntBre force-pushed the brent/show-fixes branch from 5aa988d to c623448 Compare August 21, 2025 16:03

This was referenced Aug 25, 2025

Diff rendering assumptions #20080

Open

Use new diff rendering format in tests #20101

Merged

TextEmitter::with_unsafe_fixes -> with_fix_applicability

0d35da9

passing an Applicability here doesn't change the results for the CLI, which still converts from an UnsafeFixes, but will allow us to pass DisplayOnly for our snapshot tests

ntBre changed the title ~~[prototype] Show fixes by default~~ Show fixes by default Aug 28, 2025

ntBre added 4 commits August 28, 2025 14:05

add Diagnostic::fix_applies, use it for fixability and diffs

804cc23

enable show_fix_diff when output format is full and preview enabled

5dba592

remove unnecessary SHOW_FIX_DIFF flag

add a CLI test

e42006e

ntBre force-pushed the brent/show-fixes branch from c623448 to e42006e Compare August 28, 2025 18:31

ntBre marked this pull request as ready for review August 28, 2025 19:35

ntBre requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners August 28, 2025 19:35

ntBre requested review from dhruvmanila and zanieb and removed request for AlexWaygood, carljm, dcreager and sharkdp August 28, 2025 19:36

zanieb approved these changes Aug 28, 2025

View reviewed changes

dhruvmanila approved these changes Aug 29, 2025

View reviewed changes

ntBre added 2 commits August 29, 2025 09:14

fix_applies -> is_fix_applicable

910f28c

is_fix_applicable -> has_applicable_fix

5c656ca

ntBre merged commit 8a6db4f into main Aug 29, 2025
35 checks passed

ntBre deleted the brent/show-fixes branch August 29, 2025 13:53

ntBre mentioned this pull request Sep 3, 2025

non-pep695-generic-function (UP047) weirdness #19155

Open

BrewTestBot mentioned this pull request Sep 4, 2025

ruff 0.12.12 Homebrew/homebrew-core#236182

Merged

ntBre mentioned this pull request Sep 30, 2025

Move diff rendering into annotate-snippets #20648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Show fixes by default #19919

Show fixes by default #19919

Uh oh!

ntBre commented Aug 14, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Aug 14, 2025 •

edited

Loading

Uh oh!

dhruvmanila Aug 29, 2025

Uh oh!

zanieb Aug 29, 2025

Uh oh!

zanieb Aug 29, 2025

Uh oh!

zanieb Aug 29, 2025

Uh oh!

ntBre Aug 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Show fixes by default #19919

Show fixes by default #19919

Uh oh!

Conversation

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

Uh oh!

Summary

Test Plan

Related issues and PR

Uh oh!

github-actions bot commented Aug 14, 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!

dhruvmanila Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

zanieb Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ntBre commented Aug 14, 2025 •

edited

Loading

github-actions bot commented Aug 14, 2025 •

edited

Loading

`ruff-ecosystem` results