Skip to content
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

[flake8-type-checking] Avoid TC004 false positive where the runtime definition is provided by __getattr__ #16052

Merged

Conversation

Daverball
Copy link
Contributor

Fixes #16045

Summary

Module-level __getattr__ is too dynamic for us to understand, so if our only runtime reference to a typing only import happens inside __all__, we should assume a module-level __getattr__ will provide a runtime accessible fallback, since we can't prove that it doesn't.

Test Plan

cargo nextest run

Module-level `__getattr__` is too dynamic for us to understand, so if
our only runtime reference to a typing only import happens inside
`__all__`, we should assume a module-level `__getattr__` will provide
a runtime accessible fallback, since we can't prove that it doesn't.
Copy link
Contributor

github-actions bot commented Feb 9, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member

Just to double-check I understand the current behaviour of this rule correctly. This kind of thing currently triggers TC004, because the only definition of Any is in a TYPE_CHECKING block, but Any is used at runtime:

import typing

if typing.TYPE_CHECKING:
    from typing import Any

X: typing.TypeAlias = Any

But this kind of thing does not trigger TC004, because it is also defined in a non-TYPE_CHECKING branch:

import typing

if typing.TYPE_CHECKING:
    from typing import Any
else:
    from typing import Any

X: typing.TypeAlias = Any

Is that correct? If so, it might be good to adjust the rule's documentation to clarify this. The behaviour makes sense to me, but I assumed from the docs that all definitions in TYPE_CHECKING blocks were prohibited if the definition was used at runtime; it wasn't clear to me that conditional declarations like this (where there were also runtime definitions alongside the TYPE_CHECKING definitions) were also permitted.

Something like this might do the trick:

diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
index 3fe243cdc..d5fea43cb 100644
--- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
+++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
@@ -16,11 +16,12 @@ use crate::rules::flake8_type_checking::helpers::{filter_contained, quote_annota
 use crate::rules::flake8_type_checking::imports::ImportBinding;
 
 /// ## What it does
-/// Checks for runtime imports defined in a type-checking block.
+/// Checks for imports that are required at runtime but are only defined in
+/// type-checking blocks.
 ///
 /// ## Why is this bad?
-/// The type-checking block is not executed at runtime, so the import will not
-/// be available at runtime.
+/// The type-checking block is not executed at runtime, so if the only definition
+/// of a symbol is in a type-checking block, it will not be available at runtime.
 ///
 /// If [`lint.flake8-type-checking.quote-annotations`] is set to `true`,
 /// annotations will be wrapped in quotes if doing so would enable the

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than my suggested improvement to the docs

@Daverball
Copy link
Contributor Author

@AlexWaygood Correct, I updated the docs accordingly

@AlexWaygood
Copy link
Member

Thank you!

@AlexWaygood AlexWaygood added the bug Something isn't working label Feb 9, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) February 9, 2025 16:24
@AlexWaygood AlexWaygood changed the title [flake8-type-checking] Avoid TC004 false positive with __getattr__ [flake8-type-checking] Avoid TC004 false positive where the runtime definition is provided by __getattr__ Feb 9, 2025
@AlexWaygood AlexWaygood merged commit 9ae98d4 into astral-sh:main Feb 9, 2025
20 checks passed
@Daverball Daverball deleted the bugfix/tc004-module-level-dunder-getattr branch February 9, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TC004 probably shouldn't trigger on typing-modules
2 participants