Display fixes by default (#7352) #12595

chriskrycho · 2024-07-31T15:28:01Z

Summary

Use the existing printer::Flags::SHOW_FIX_DIFF to enable printing fixable diffs in check mode automatically. Enable it whenever the output format is Full, but keep a distinct flag for it (a) because output format and whether to print diffs are really separate concerns, (b) because it makes it easy to make changes to what sets the flag to true if/as that is needful, and (c) because it minimally perturbs the rest of the code.

Fixes #7352.

Details

This changes the result of running ruff check by adding the diff for possible fixes, and adds calls to action for those messages.

Note that this currently displays only safe fixes to users. There are a number of design questions around when and how to display them; see #12598.

a file with three unused imports

Input:

import b
import a
import c

Output:

/Users/chris/Desktop/lol.py:1:8: F401 [*] `b` imported but unused
  |
1 | import b
  |        ^ F401
2 | import a
3 | import c
  |
  = help: Remove unused import: `b`

Suggested fix:
1   |-import b
2 1 | import a
3 2 | import c
4 3 |

    Run `ruff check --fix` to apply this fix.

/Users/chris/Desktop/lol.py:2:8: F401 [*] `a` imported but unused
  |
1 | import b
2 | import a
  |        ^ F401
3 | import c
  |
  = help: Remove unused import: `a`

Suggested fix:
1 1 | import b
2   |-import a
3 2 | import c
4 3 |

    Run `ruff check --fix` to apply this fix.

/Users/chris/Desktop/lol.py:3:8: F401 [*] `c` imported but unused
  |
1 | import b
2 | import a
3 | import c
  |        ^ F401
  |
  = help: Remove unused import: `c`

Suggested fix:
1 1 | import b
2 2 | import a
3   |-import c
4 3 |

    Run `ruff check --fix` to apply this fix.

Found 3 errors.
[*] 3 fixable with the `--fix` option.

The test file crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi

New output:

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:2:40: F401 [*] `collections.abc.Set` imported but unused
  |
1 | def f():
2 |     from collections.abc import Set as AbstractSet  # Ok
  |                                        ^^^^^^^^^^^ F401
3 | 
4 | def f():
  |
  = help: Remove unused import: `collections.abc.Set`

Suggested fix:
1 1 | def f():
2   |-    from collections.abc import Set as AbstractSet  # Ok
  2 |+    pass  # Ok
3 3 | 
4 4 | def f():
5 5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:4:5: F811 Redefinition of unused `f` from line 1
  |
2 |     from collections.abc import Set as AbstractSet  # Ok
3 | 
4 | def f():
  |     ^ F811
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  |
  = help: Remove definition: `f`

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:5:33: F401 [*] `collections.abc.Container` imported but unused
  |
4 | def f():
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  |                                 ^^^^^^^^^ F401
6 | 
7 | def f():
  |
  = help: Remove unused import

Suggested fix:
2 2 |     from collections.abc import Set as AbstractSet  # Ok
3 3 | 
4 4 | def f():
5   |-    from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  5 |+    pass  # Ok
6 6 | 
7 7 | def f():
8 8 |     from collections.abc import Set  # PYI025

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:5:44: F401 [*] `collections.abc.Sized` imported but unused
  |
4 | def f():
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  |                                            ^^^^^ F401
6 | 
7 | def f():
  |
  = help: Remove unused import

Suggested fix:
2 2 |     from collections.abc import Set as AbstractSet  # Ok
3 3 | 
4 4 | def f():
5   |-    from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  5 |+    pass  # Ok
6 6 | 
7 7 | def f():
8 8 |     from collections.abc import Set  # PYI025

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:5:58: F401 [*] `collections.abc.Set` imported but unused
  |
4 | def f():
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  |                                                          ^^^^^^^^^^^ F401
6 | 
7 | def f():
  |
  = help: Remove unused import

Suggested fix:
2 2 |     from collections.abc import Set as AbstractSet  # Ok
3 3 | 
4 4 | def f():
5   |-    from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  5 |+    pass  # Ok
6 6 | 
7 7 | def f():
8 8 |     from collections.abc import Set  # PYI025

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:5:71: F401 [*] `collections.abc.ValuesView` imported but unused
  |
4 | def f():
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  |                                                                       ^^^^^^^^^^ F401
6 | 
7 | def f():
  |
  = help: Remove unused import

Suggested fix:
2 2 |     from collections.abc import Set as AbstractSet  # Ok
3 3 | 
4 4 | def f():
5   |-    from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
  5 |+    pass  # Ok
6 6 | 
7 7 | def f():
8 8 |     from collections.abc import Set  # PYI025

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:7:5: F811 Redefinition of unused `f` from line 4
  |
5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
6 | 
7 | def f():
  |     ^ F811
8 |     from collections.abc import Set  # PYI025
  |
  = help: Remove definition: `f`

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:8:33: F401 [*] `collections.abc.Set` imported but unused
   |
 7 | def f():
 8 |     from collections.abc import Set  # PYI025
   |                                 ^^^ F401
 9 | 
10 | def f():
   |
   = help: Remove unused import: `collections.abc.Set`

Suggested fix:
5 5 |     from collections.abc import Container, Sized, Set as AbstractSet, ValuesView  # Ok
6 6 | 
7 7 | def f():
8   |-    from collections.abc import Set  # PYI025
  8 |+    pass  # PYI025
9 9 | 
10 10 | def f():
11 11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:10:5: F811 Redefinition of unused `f` from line 7
   |
 8 |     from collections.abc import Set  # PYI025
 9 | 
10 | def f():
   |     ^ F811
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   |
   = help: Remove definition: `f`

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:11:33: F401 [*] `collections.abc.Container` imported but unused
   |
10 | def f():
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   |                                 ^^^^^^^^^ F401
12 | 
13 | def f():
   |
   = help: Remove unused import

Suggested fix:
8  8  |     from collections.abc import Set  # PYI025
9  9  | 
10 10 | def f():
11    |-    from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   11 |+    pass  # PYI025
12 12 | 
13 13 | def f():
14 14 |     """Test: local symbol renaming."""

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:11:44: F401 [*] `collections.abc.Sized` imported but unused
   |
10 | def f():
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   |                                            ^^^^^ F401
12 | 
13 | def f():
   |
   = help: Remove unused import

Suggested fix:
8  8  |     from collections.abc import Set  # PYI025
9  9  | 
10 10 | def f():
11    |-    from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   11 |+    pass  # PYI025
12 12 | 
13 13 | def f():
14 14 |     """Test: local symbol renaming."""

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:11:51: F401 [*] `collections.abc.Set` imported but unused
   |
10 | def f():
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   |                                                   ^^^ F401
12 | 
13 | def f():
   |
   = help: Remove unused import

Suggested fix:
8  8  |     from collections.abc import Set  # PYI025
9  9  | 
10 10 | def f():
11    |-    from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   11 |+    pass  # PYI025
12 12 | 
13 13 | def f():
14 14 |     """Test: local symbol renaming."""

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:11:56: F401 [*] `collections.abc.ValuesView` imported but unused
   |
10 | def f():
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   |                                                        ^^^^^^^^^^ F401
12 | 
13 | def f():
   |
   = help: Remove unused import

Suggested fix:
8  8  |     from collections.abc import Set  # PYI025
9  9  | 
10 10 | def f():
11    |-    from collections.abc import Container, Sized, Set, ValuesView  # PYI025
   11 |+    pass  # PYI025
12 12 | 
13 13 | def f():
14 14 |     """Test: local symbol renaming."""

    Run `ruff check --fix` to apply this fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:13:5: F811 Redefinition of unused `f` from line 10
   |
11 |     from collections.abc import Container, Sized, Set, ValuesView  # PYI025
12 | 
13 | def f():
   |     ^ F811
14 |     """Test: local symbol renaming."""
15 |     if True:
   |
   = help: Remove definition: `f`

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:20:5: F841 Local variable `x` is assigned to but never used
   |
18 |         Set = 1
19 | 
20 |     x: Set = set()
   |     ^ F841
21 | 
22 |     x: Set
   |
   = help: Remove assignment to unused variable `x`

Suggested fix:
17 17 |     else:
18 18 |         Set = 1
19 19 | 
20    |-    x: Set = set()
21 20 | 
22 21 |     x: Set
23 22 | 

    Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:27:15: F821 Undefined name `Set`
   |
26 |     def f():
27 |         print(Set)
   |               ^^^ F821
28 | 
29 |         def Set():
   |

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:27:15: F823 Local variable `Set` referenced before assignment
   |
26 |     def f():
27 |         print(Set)
   |               ^^^ F823
28 | 
29 |         def Set():
   |

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:33:1: E402 Module level import not at top of file
   |
31 |         print(Set)
32 | 
33 | from collections.abc import Set
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E402
34 | 
35 | def f():
   |

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:35:5: F811 Redefinition of unused `f` from line 13
   |
33 | from collections.abc import Set
34 | 
35 | def f():
   |     ^ F811
36 |     """Test: global symbol renaming."""
37 |     global Set
   |
   = help: Remove definition: `f`

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi:42:5: F811 Redefinition of unused `f` from line 35
   |
40 |     print(Set)
41 | 
42 | def f():
   |     ^ F811
43 |     """Test: nonlocal symbol renaming."""
44 |     from collections.abc import Set
   |
   = help: Remove definition: `f`

Found 20 errors.
[*] 10 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

Test Plan

This does not introduce new tests, but does update the existing tests to account for the new default output. Of particular interest among the many snapshots, see the updated snapshot for PYI025_1.pyi]snap, which does some truncation of diffs.

github-actions · 2024-07-31T15:41:15Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@zanieb

## Summary @zanieb noticed while we were discussing #12595 that this flag is now unnecessary, so remove it and the flags which reference it. ## Test Plan Question for maintainers: is there a test to add *or* remove here? (I’ve opened this as a draft PR with that in view!)

Use the existing `printer::Flags::SHOW_FIX_DIFF` to enable printing fixable diffs in `check` mode automatically. Enable it whenever the output format is `Full`, but keep a distinct flag for it (a) because output format and whether to print diffs are really separate concerns, (b) because it makes it easy to make changes to what sets the flag to true if/as that is needful, and (c) because it minimally perturbs the rest of the code. This does not introduce *new* tests, but does update the existing tests to account for the new default output.

crates/ruff/tests/integration_test.rs

zanieb · 2024-07-31T17:16:46Z

crates/ruff/tests/integration_test.rs

@@ -1523,7 +1581,17 @@ fn check_hints_hidden_unsafe_fixes() {
    exit_code: 1
    ----- stdout -----
    -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
+    Suggested fix:


It looks like we need to do something special for violations without help messages? We want a dedicated test case for that and a test case for violations without source context.

Ah, I missed that one. I’ll see about adding a test for those. What’s a scenario where you would end up without source context?

Well, for example, there's no source context for this test rule right? I think if the violation is attached to an empty range we elide the source context. This behavior was recently modified in #12304

I'm going to open a new issue tracking what to do in each of those cases, with the existing baseline implementation here as a useful starting point.

For missing help: Require rules with fixes to include help message #12599

For missing source: Design display of fixes in check when there is no source #12600

zanieb · 2024-07-31T17:18:08Z

crates/ruff/tests/integration_test.rs

+    Suggested fix:
+      1 |+# fix from stable-test-rule-safe-fix
+
+        Run `ruff check --fix` to apply this fix.


Looking at this more holistically, I wonder if this message is necessary since we have a call to action at the end:

[*] 1 fixable with the --fix option (1 hidden fix can be enabled with the --unsafe-fixes option).

I saw that as well, but my intuition was that it is helpful to have both the details and the summary.

zanieb · 2024-07-31T17:20:21Z

crates/ruff/tests/integration_test.rs

+    Suggested fix:
+      1 |+# fix from stable-test-rule-unsafe-fix
+
+        Run `ruff check --fix --unsafe-fixes` to apply this unsafe fix.


Given the message below this ("1 hidden fix..."), I wonder if we should be hiding unsafe fixes unless the opt-in is provided. I think this would solve the "how do we display applicability" problem, to an extent?

If so, I think we could figure out what to do with "display-only" fixes later and just never show them to start (to limit the scope of this pull request)

This would require a bit of futzing to retain the existing coverage in our test suite, maybe just using --unsafe-fixes by default in some places?

Hmm, yeah—I can definitely back out those parts of the change if that’s preferable!

It feels like the current design (prior to this pull request) hides unsafe fixes and allowing opt-in to their display, at which point we don't need to indicate that they're unsafe? It seems easiest to follow that pattern for now and consider if and how we want to display unsafe fixes by default separately, i.e., I think more discussion in an issue would be prudent.

In terms of handling applicability, we could do that in a couple of ways:

The Diff itself could take a configuration level and its Display implementation could use that to decide what to print.

The caller could decide whether to print based on message applicability at a higher level, keeping it out of the Diff.

The latter is a slightly smaller scope and IMO better isolation of concerns; it keeps it properly in the TextEmitter, whereas the Diff can be concerned entirely with the presentation of a given diff.

If we go that way, we still have the choice whether to keep the applicability message in the diff. I think I am still mildly inclined toward showing that CTA, because of the differences when you do include --unsafe-fixes; it seems like it would still be nice to know which changes are which.

It seems reasonable to decide whether or not the Diff should be displayed outside its display implementation (the latter option).

Regarding the CTA, I have a preference for splitting it out of this pull request so we can merge this sooner and have focused discussion on each objective, like:

Display available fixes by default

Disambiguate safe and unsafe fixes when displayed (i.e., via a CTA or other label)

Display unsafe fixes by default (needs discussion)

Determine how to make display-only fixes user-facing

What do you think?

That makes good sense to me. I will split out issues (Edit: #12598) for further discussion on those points and update this to simply display fixes only for the Applicability::Safe level, in a slightly clunky way that is amenable to cleaning up by way of broader refactors once some of those other design questions are fleshed out.

There are some open design questions about how the calls to action should work for unsafe and display-only fixes, so leave them aside for now. This hard-codes that behavior into the `Display` implementation, but with a tracking issue for moving that up to the `TextEmitter`, and also includes a TODO with a tracking issue for the design of the CTAs.

MichaReiser

I'm concerned about the fact that this change removes all test coverage for unsafe fixes. I don't think we should land this before recovering the test coverage.

MichaReiser · 2024-07-31T20:03:59Z

crates/ruff/src/lib.rs

@@ -287,6 +287,9 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
    if show_fixes {
        printer_flags |= PrinterFlags::SHOW_FIX_SUMMARY;
    }
+    if output_format == OutputFormat::Full {


Should this be gated behind preview mode to get some user feedback before enabling it for everyone?

I don't think it's strictly necessary, but we may want to do so if it's going to change significantly (i.e. by being split into follow-ups)

MichaReiser · 2024-07-31T20:05:17Z

crates/ruff_linter/src/message/diff.rs

+        // TODO: Instead of hard-coding this in, the `TextEmitter` should be
+        // handling it. See https://github.com/astral-sh/ruff/issues/12597 for
+        // further discussion.
+        if matches!(
+            self.fix.applicability(),
+            Applicability::Unsafe | Applicability::DisplayOnly
+        ) {
+            return Ok(());
+        }


Does this mean that we won't see unsafe fixes in snapshot tests anymore?

I would find this concerning because it means we loose all test coverage for unsafe fixes (of which there are plenty)

Yes, we need to address this before merging.

Yeah, we realized this late in our time working on it yesterday. It may warrant going ahead and doing the bit I highlighted in #12597, etc. I think this was a case where trying to keep the change minimal actually makes it harder to land—this particularly bit of coupling wasn’t obvious to me until the very end when we changed from including all the CTAs to including none of them, and turning off.

I think it probably needs the aforementioned refactor (where the TextEmitter handles what to print or not) plus setting the relevant flags based on both --unsafe-fixes and whether it is testing.

chriskrycho mentioned this pull request Jul 31, 2024

Remove ecosystem_ci flag from Ruff CLI #12596

Merged

chriskrycho requested a review from AlexWaygood as a code owner July 31, 2024 16:52

zanieb reviewed Jul 31, 2024

View reviewed changes

crates/ruff/tests/integration_test.rs Outdated Show resolved Hide resolved

zanieb reviewed Jul 31, 2024

View reviewed changes

AlexWaygood removed their request for review July 31, 2024 17:18

zanieb reviewed Jul 31, 2024

View reviewed changes

Remove 'Suggested fix:' header from check diff output

1f07f5f

chriskrycho mentioned this pull request Jul 31, 2024

Refactor applicability handling in Display impl for message::diff::Diff #12597

Open

This was referenced Jul 31, 2024

Require rules with fixes to include help message #12599

Open

Design display of fixes in check when there is no source #12600

Open

Design call to action messages for fixes displayed in check #12598

Open

Show only diffs, not CTAs, in check

fde34bc

MichaReiser added the cli Related to the command-line interface label Jul 31, 2024

MichaReiser reviewed Jul 31, 2024

View reviewed changes

zanieb self-assigned this Jul 31, 2024

chriskrycho closed this by deleting the head repository Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display fixes by default (#7352) #12595

Display fixes by default (#7352) #12595

chriskrycho commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

zanieb Jul 31, 2024

chriskrycho Jul 31, 2024

zanieb Jul 31, 2024

chriskrycho Jul 31, 2024 •

edited

Loading

zanieb Jul 31, 2024 •

edited

Loading

chriskrycho Jul 31, 2024

zanieb Jul 31, 2024

zanieb Jul 31, 2024

chriskrycho Jul 31, 2024

zanieb Jul 31, 2024

chriskrycho Jul 31, 2024 •

edited

Loading

zanieb Jul 31, 2024

chriskrycho Jul 31, 2024 •

edited

Loading

MichaReiser left a comment •

edited

Loading

MichaReiser Jul 31, 2024

zanieb Jul 31, 2024

MichaReiser Jul 31, 2024

zanieb Jul 31, 2024

chriskrycho Aug 1, 2024

Display fixes by default (#7352) #12595

Display fixes by default (#7352) #12595

Conversation

chriskrycho commented Jul 31, 2024 • edited Loading

Summary

Details

Test Plan

github-actions bot commented Jul 31, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

zanieb Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

`ruff-ecosystem` results

chriskrycho Jul 31, 2024 •

edited

Loading

zanieb Jul 31, 2024 •

edited

Loading

chriskrycho Jul 31, 2024 •

edited

Loading

chriskrycho Jul 31, 2024 •

edited

Loading

MichaReiser left a comment •

edited

Loading