[pyflakes] Report duplicate imports in typing.TYPE_CHECKING block F811#22560
Open
chirizxc wants to merge 37 commits intoastral-sh:mainfrom
Open
[pyflakes] Report duplicate imports in typing.TYPE_CHECKING block F811#22560chirizxc wants to merge 37 commits intoastral-sh:mainfrom
pyflakes] Report duplicate imports in typing.TYPE_CHECKING block F811#22560chirizxc wants to merge 37 commits intoastral-sh:mainfrom
Conversation
Contributor
Author
|
This behavior makes it closer to the original |
pyflakes] Report more diagnostic for F811pyflakes] Report duplicate imports in typing.TYPE_CHECKING block F811
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| F811 | 51 | 51 | 0 | 0 | 0 |
Contributor
Author
|
I'm not sure how much this can be considered a false positive, but there is such code in mypy: |
Contributor
Author
|
Should the fix report mention that one of the imports is a duplicate and should be removed based on the code context? (instead of |
amyreese
reviewed
Feb 11, 2026
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
…used.rs Co-authored-by: Amethyst Reese <amethyst@n7.gg>
…used.rs Co-authored-by: Amethyst Reese <amethyst@n7.gg>
ntBre
reviewed
Feb 12, 2026
Contributor
ntBre
left a comment
There was a problem hiding this comment.
Thank you! And sorry for the delay in review. Will this close #17555 and #12270 too?
This looks reasonable to me overall, I just had some smaller suggestions. And I think we should make this a preview change even though it's arguably a bug fix. It's still a fairly large expansion of an existing rule in practice.
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
Contributor
Author
I think it should, but I also saw a false positive in mypy and some other project, but there it already said noqa: F401, so I think this is about the expected behavior. |
ntBre
requested changes
Feb 23, 2026
Contributor
ntBre
left a comment
There was a problem hiding this comment.
Thanks again. I took a deeper look here today, and it seems that there is some extraneous, or at least untested, code here. I'm also not sure we should be treating TYPE_CHECKING duplicates as so different from the other cases the rule covers.
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
ntBre
added a commit
to ntBre/mypy
that referenced
this pull request
Feb 23, 2026
Summary -- While reviewing astral-sh/ruff#22560, which expands our `redefined-while-unused` rule to catch re-imports in `TYPE_CHECKING` blocks, we noticed a new diagnostic on this code. When I checked the blame and noticed that the comment was 8 years old, I thought it might be worth trying to remove the import. I'm very happy to close this if I'm missing something, but the tests seem to be passing locally, so I figured I'd open a PR! Test Plan -- I set up my environment based on the contributing guide and then ran: ```shell python runtests.py ```
hauntsaninja
pushed a commit
to python/mypy
that referenced
this pull request
Feb 24, 2026
While reviewing astral-sh/ruff#22560, which expands our `redefined-while-unused` rule to catch re-imports in `TYPE_CHECKING` blocks, we noticed a new diagnostic on this code. When I checked the blame and noticed that the comment was 8 years old, I thought it might be worth trying to remove the import. I'm very happy to close this if I'm missing something, but the tests seem to be passing locally, so I figured I'd open a PR!
Contributor
Author
|
Is it worth adding code with |
ntBre
reviewed
Feb 27, 2026
Contributor
ntBre
left a comment
There was a problem hiding this comment.
Thank you! All but one of my comments are resolved, I just had two more very small nits. I am still curious about the second preview check, though.
I also noticed one potential issue in the ecosystem check.
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
I still have this question, and I think adding an example to the documentation would be useful when people see that the ruff update has started detecting new cases F811. |
Contributor
|
Sure, I think it's fine to add another example. |
ntBre
reviewed
Mar 2, 2026
ntBre
reviewed
Mar 3, 2026
Contributor
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me overall. I just had one more question about ecosystem results.
I was looking at the pandas case you mentioned here again today, and I think we may be able to handle this by using the SemanticModel::dominates method. I asked Claude to look into this and it came up with this patch:
diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F811_34.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F811_34.py
index 7755851b7a..432539d675 100644
--- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F811_34.py
+++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F811_34.py
@@ -13,4 +13,15 @@ if TYPE_CHECKING:
import pyarrow_hotfix
def foo():
- import pyarrow_hotfix
\ No newline at end of file
+ import pyarrow_hotfix
+
+# Should NOT detect: the runtime import is itself conditional, so the
+# TYPE_CHECKING import is not redundant (e.g., needed for type checkers
+# when the condition is False).
+HAS_THING = True
+
+if HAS_THING:
+ from foo import Bar
+
+if TYPE_CHECKING:
+ from foo import Bar
\ No newline at end of file
diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
index fdba0a65ba..f33e0bef4e 100644
--- a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
+++ b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs
@@ -303,7 +303,12 @@ fn bindings_in_different_forks(
(left_binding.as_any_import(), right_binding.as_any_import())
&& left_import.qualified_name() == right_import.qualified_name()
{
- return false;
+ // Only report if the non-`TYPE_CHECKING` import unconditionally
+ // precedes the `TYPE_CHECKING` import. If the runtime import is
+ // itself inside a conditional block (e.g., `if HAS_PYARROW:`),
+ // the `TYPE_CHECKING` import is not redundant.
+ let (non_tc, tc) = if right_ { (left, right) } else { (right, left) };
+ return !checker.semantic().dominates(non_tc, tc);
}
}The comment is rather verbose and the non_tc and tc names aren't the best, but it seems to resolve the false positive in a fairly neat way. What do you think?
I was kind of hoping that dominates would simplify bindings_in_different_forks in general, but I think that's only the case if we want to treat other conditional branches like we're treating TYPE_CHECKING here.
Contributor
Author
Yes, I think that would be more logical. I will change the names to make them more readable. |
Contributor
|
Nice, that looks like an ecosystem improvement. I am still seeing one more potentially wrong case (well two instances of the same issue):
I feel like we may have talked about this earlier, so sorry for the duplication if so, but these are cases like: if TYPE_CHECKING:
import foo
import foo # F811where the runtime import is after the type checking import. I think we probably shouldn't emit a diagnostic in this case, or at least we should add a test case verifying that we suggest removing the I went through all of the other ecosystem checks, though, and they look good to me. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
See #22554
Test Plan
cargo nextest run pyflake