Generate one fix per statement for flake8-type-checking rules #4915
Conversation
| checker.stylist, | ||
| )?; | ||
| Ok(Fix::automatic(edit).isolate(checker.isolation(parent))) | ||
| } |
There was a problem hiding this comment.
I cleaned the implementation here up a bit, to use more of our modern conveniences (like binding.trimmed_range).
| @@ -61,72 +67,168 @@ impl Violation for RuntimeImportInTypeCheckingBlock { | |||
| /// TCH004 | |||
| pub(crate) fn runtime_import_in_type_checking_block( | |||
There was a problem hiding this comment.
This, typing_only_runtime_import, and unused_import are structurally very similar. They all follow the same pattern:
- Iterate over individual imports, and group them by statement of origin.
- Iterate over the statements, and generate a single fix for all imports (taking into account suppression comments).
- Generate a diagnostic for each import in the statement, using that single fix.
- (Generate a diagnostic for each suppressed violation, so that the suppression comments aren't marked as unused.)
A lot of the complexity comes from handling suppression comments, since we need the unified fix to omit those members that are suppressed, and we rarely do that kind of thing in lint rules (we typically leave suppression to the next phase).
Also, while these three files are structurally similar, there are a bunch of nuanced differences between them that make me not want to bother DRYing them up much further. For example, in the typing-only import rule, we have to not only group by statement, but also by import type, since a single import statement can contain imports of multiple types (e.g., import os, pandas), which means we need a fix for each import type.
charliermarsh
force-pushed
the
charlie/consolidated-typing-errors
branch
from
562bc03
to
e3dcfee
Compare
PR Check ResultsEcosystemℹ️ ecosystem check detected changes. (+5, -0, 0 error(s)) disnake (+5, -0)
+ disnake/ext/commands/common_bot_base.py:35:12: TCH004 Move import `importlib.machinery` out of type-checking block. Import is used for more than type hinting.
+ tests/ext/commands/test_core.py:9:35: TCH004 Move import `typing_extensions.assert_type` out of type-checking block. Import is used for more than type hinting.
+ tests/ui/test_action_row.py:13:35: TCH004 Move import `typing_extensions.assert_type` out of type-checking block. Import is used for more than type hinting.
+ tests/ui/test_action_row.py:15:28: TCH004 Move import `disnake.ui.MessageUIComponent` out of type-checking block. Import is used for more than type hinting.
+ tests/ui/test_action_row.py:15:48: TCH004 Move import `disnake.ui.ModalUIComponent` out of type-checking block. Import is used for more than type hinting.
BenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/consolidated-typing-errors
branch
from
e3dcfee
to
98854d1
Compare
| qualified_name: &'a str, | ||
| /// The first reference to the imported symbol. | ||
| reference_id: ReferenceId, | ||
| /// The trimmed range of the import. |
There was a problem hiding this comment.
Is this like List in from typing import List, Sequence?
There was a problem hiding this comment.
Yeah, I'll add a note.
| 2 3 | | ||
| 3 4 | if TYPE_CHECKING: | ||
| 4 |- from typing import List, Sequence, Set | ||
| 5 |+ from typing import Sequence, Set | ||
| 5 |+ pass |
There was a problem hiding this comment.
The is an unnecessary pass state; i'd expected it to be cleaned up by another rule but it seems there is no rule currently that detects extra pass statements? not sure if this is actionable for this PR tough
There was a problem hiding this comment.
We do have a rule that detects empty type-checking blocks, so this would be picked up from that at least.
|
I think the ecosystem CI checks must be stale -- I've checked multiple times locally, and I get those same results on main as on this branch. Perhaps the repo was updated between runs? |
charliermarsh
force-pushed
the
charlie/consolidated-typing-errors
branch
from
98854d1
to
0c84877
Compare
|
I think maybe this PR did it (?), but the following code, setting up a type alias: if typing.TYPE_CHECKING:
import numpy.typing
FloatArray = numpy.typing.NDArray[numpy.float64]
else:
FloatArray = numpy.ndarrayIs now being flagged in 0.272, and wasn't in 0.270. |
|
Thanks, I'll take a look. |
|
Are you able to include a snippet to reproduce whatever you're seeing? Or link me to the file? |
|
Sure, it's:
I'm not sure how the check is setup, but I could make the TypeAlias explicit if needed. |
|
It has to do with how we treat submodule imports. Generally, if you
|
|
I don't quite know the right language to describe it, but here's the same issue in |
|
It's somewhat common to have some typing-only code in a submodule (somewhat often called What strict setting? I usually like strict settings. :) |
|
Hah, I was thinking of https://beta.ruff.rs/docs/settings/#flake8-type-checking-strict, but it doesn't actually fix your issue, let me look at it again. |
|
This does fix the specific issue here: if typing.TYPE_CHECKING:
import numpy.typing
FloatArray = numpy.typing.NDArray[numpy.float64]
else:
import numpy
FloatArray = numpy.ndarrayIn the end, I think it's actually about the lack of branch analysis. Once we see the |
|
I applied that method to |
Summary
We have special-case handling for the unused imports diagnostic, whereby we generate one diagnostic per import member, but apply a unified fix on a per-statement basis. This strikes the balance between providing granular feedback in the LSP (i.e., being able to highlight individual members as unused), but allowing Code Actions to fix the entire statement in one pass.
This PR applies the same treatment to the flake8-type-checking rules. So, e.g., if you have
from pandas import Series, DataFrameand bothSeriesandDataFrameare only used in typing contexts, we'll generate two diagnostics (one for each member), but those diagnostics will share a fix, and that fix will move both members.Test Plan
Added some additional snapshots.