New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
BenchmarkLinux
Windows
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this like List
in from typing import List, Sequence
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.ndarray Is 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.ndarray In 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, DataFrame
and bothSeries
andDataFrame
are 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.