Skip to content

Commit 4b4160e

Browse files
charliermarshcharliermarsh
authored
Allow removal of typing from exempt-modules (#9214)
## Summary If you remove `typing` from `exempt-modules`, we tend to panic, since we try to add `TYPE_CHECKING` to `from typing import ...` statements while concurrently attempting to remove other members from that import. This PR adds special-casing for typing imports to avoid such panics. Closes #5331 Closes #9196. Closes #9197.
1 parent 29846f5 commit 4b4160e

10 files changed

Lines changed: 235 additions & 35 deletions

‎crates/ruff_linter/resources/test/fixtures/flake8_type_checking/exempt_type_checking_1.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"""Add `TYPE_CHECKING` to an existing `typing` import. Another member is moved."""
2+
3+
from __future__ import annotations
4+
5+
from typing import Final
6+
7+
Const: Final[dict] = {}

‎crates/ruff_linter/resources/test/fixtures/flake8_type_checking/exempt_type_checking_2.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"""Using `TYPE_CHECKING` from an existing `typing` import. Another member is moved."""
2+
3+
from __future__ import annotations
4+
5+
from typing import Final, TYPE_CHECKING
6+
7+
Const: Final[dict] = {}

‎crates/ruff_linter/resources/test/fixtures/flake8_type_checking/exempt_type_checking_3.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
"""Using `TYPE_CHECKING` from an existing `typing` import. Another member is moved."""
2+
3+
from __future__ import annotations
4+
5+
from typing import Final, Mapping
6+
7+
Const: Final[dict] = {}

‎crates/ruff_linter/src/importer/mod.rs‎

Lines changed: 80 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use ruff_text_size::{Ranged, TextSize};
1313
use ruff_diagnostics::Edit;
1414
use ruff_python_ast::imports::{AnyImport, Import, ImportFrom};
1515
use ruff_python_codegen::Stylist;
16-
use ruff_python_semantic::SemanticModel;
16+
use ruff_python_semantic::{ImportedName, SemanticModel};
1717
use ruff_python_trivia::textwrap::indent;
1818
use ruff_source_file::Locator;
1919

@@ -132,7 +132,48 @@ impl<'a> Importer<'a> {
132132
)?;
133133

134134
// Import the `TYPE_CHECKING` symbol from the typing module.
135-
let (type_checking_edit, type_checking) = self.get_or_import_type_checking(at, semantic)?;
135+
let (type_checking_edit, type_checking) =
136+
if let Some(type_checking) = Self::find_type_checking(at, semantic)? {
137+
// Special-case: if the `TYPE_CHECKING` symbol is imported as part of the same
138+
// statement that we're modifying, avoid adding a no-op edit. For example, here,
139+
// the `TYPE_CHECKING` no-op edit would overlap with the edit to remove `Final`
140+
// from the import:
141+
// ```python
142+
// from __future__ import annotations
143+
//
144+
// from typing import Final, TYPE_CHECKING
145+
//
146+
// Const: Final[dict] = {}
147+
// ```
148+
let edit = if type_checking.statement(semantic) == import.statement {
149+
None
150+
} else {
151+
Some(Edit::range_replacement(
152+
self.locator.slice(type_checking.range()).to_string(),
153+
type_checking.range(),
154+
))
155+
};
156+
(edit, type_checking.into_name())
157+
} else {
158+
// Special-case: if the `TYPE_CHECKING` symbol would be added to the same import
159+
// we're modifying, import it as a separate import statement. For example, here,
160+
// we're concurrently removing `Final` and adding `TYPE_CHECKING`, so it's easier to
161+
// use a separate import statement:
162+
// ```python
163+
// from __future__ import annotations
164+
//
165+
// from typing import Final
166+
//
167+
// Const: Final[dict] = {}
168+
// ```
169+
let (edit, name) = self.import_symbol(
170+
&ImportRequest::import_from("typing", "TYPE_CHECKING"),
171+
at,
172+
Some(import.statement),
173+
semantic,
174+
)?;
175+
(Some(edit), name)
176+
};
136177

137178
// Add the import to a `TYPE_CHECKING` block.
138179
let add_import_edit = if let Some(block) = self.preceding_type_checking_block(at) {
@@ -157,28 +198,21 @@ impl<'a> Importer<'a> {
157198
})
158199
}
159200

160-
/// Generate an [`Edit`] to reference `typing.TYPE_CHECKING`. Returns the [`Edit`] necessary to
161-
/// make the symbol available in the current scope along with the bound name of the symbol.
162-
fn get_or_import_type_checking(
163-
&self,
201+
/// Find a reference to `typing.TYPE_CHECKING`.
202+
fn find_type_checking(
164203
at: TextSize,
165204
semantic: &SemanticModel,
166-
) -> Result<(Edit, String), ResolutionError> {
205+
) -> Result<Option<ImportedName>, ResolutionError> {
167206
for module in semantic.typing_modules() {
168-
if let Some((edit, name)) = self.get_symbol(
207+
if let Some(imported_name) = Self::find_symbol(
169208
&ImportRequest::import_from(module, "TYPE_CHECKING"),
170209
at,
171210
semantic,
172211
)? {
173-
return Ok((edit, name));
212+
return Ok(Some(imported_name));
174213
}
175214
}
176-
177-
self.import_symbol(
178-
&ImportRequest::import_from("typing", "TYPE_CHECKING"),
179-
at,
180-
semantic,
181-
)
215+
Ok(None)
182216
}
183217

184218
/// Generate an [`Edit`] to reference the given symbol. Returns the [`Edit`] necessary to make
@@ -192,16 +226,15 @@ impl<'a> Importer<'a> {
192226
semantic: &SemanticModel,
193227
) -> Result<(Edit, String), ResolutionError> {
194228
self.get_symbol(symbol, at, semantic)?
195-
.map_or_else(|| self.import_symbol(symbol, at, semantic), Ok)
229+
.map_or_else(|| self.import_symbol(symbol, at, None, semantic), Ok)
196230
}
197231

198-
/// Return an [`Edit`] to reference an existing symbol, if it's present in the given [`SemanticModel`].
199-
fn get_symbol(
200-
&self,
232+
/// Return the [`ImportedName`] to for existing symbol, if it's present in the given [`SemanticModel`].
233+
fn find_symbol(
201234
symbol: &ImportRequest,
202235
at: TextSize,
203236
semantic: &SemanticModel,
204-
) -> Result<Option<(Edit, String)>, ResolutionError> {
237+
) -> Result<Option<ImportedName>, ResolutionError> {
205238
// If the symbol is already available in the current scope, use it.
206239
let Some(imported_name) =
207240
semantic.resolve_qualified_import_name(symbol.module, symbol.member)
@@ -226,6 +259,21 @@ impl<'a> Importer<'a> {
226259
return Err(ResolutionError::IncompatibleContext);
227260
}
228261

262+
Ok(Some(imported_name))
263+
}
264+
265+
/// Return an [`Edit`] to reference an existing symbol, if it's present in the given [`SemanticModel`].
266+
fn get_symbol(
267+
&self,
268+
symbol: &ImportRequest,
269+
at: TextSize,
270+
semantic: &SemanticModel,
271+
) -> Result<Option<(Edit, String)>, ResolutionError> {
272+
// Find the symbol in the current scope.
273+
let Some(imported_name) = Self::find_symbol(symbol, at, semantic)? else {
274+
return Ok(None);
275+
};
276+
229277
// We also add a no-op edit to force conflicts with any other fixes that might try to
230278
// remove the import. Consider:
231279
//
@@ -259,9 +307,13 @@ impl<'a> Importer<'a> {
259307
&self,
260308
symbol: &ImportRequest,
261309
at: TextSize,
310+
except: Option<&Stmt>,
262311
semantic: &SemanticModel,
263312
) -> Result<(Edit, String), ResolutionError> {
264-
if let Some(stmt) = self.find_import_from(symbol.module, at) {
313+
if let Some(stmt) = self
314+
.find_import_from(symbol.module, at)
315+
.filter(|stmt| except != Some(stmt))
316+
{
265317
// Case 1: `from functools import lru_cache` is in scope, and we're trying to reference
266318
// `functools.cache`; thus, we add `cache` to the import, and return `"cache"` as the
267319
// bound name.
@@ -423,14 +475,18 @@ impl RuntimeImportEdit {
423475
#[derive(Debug)]
424476
pub(crate) struct TypingImportEdit {
425477
/// The edit to add the `TYPE_CHECKING` symbol to the module.
426-
type_checking_edit: Edit,
478+
type_checking_edit: Option<Edit>,
427479
/// The edit to add the import to a `TYPE_CHECKING` block.
428480
add_import_edit: Edit,
429481
}
430482

431483
impl TypingImportEdit {
432-
pub(crate) fn into_edits(self) -> Vec<Edit> {
433-
vec![self.type_checking_edit, self.add_import_edit]
484+
pub(crate) fn into_edits(self) -> (Edit, Option<Edit>) {
485+
if let Some(type_checking_edit) = self.type_checking_edit {
486+
(type_checking_edit, Some(self.add_import_edit))
487+
} else {
488+
(self.add_import_edit, None)
489+
}
434490
}
435491
}
436492

‎crates/ruff_linter/src/rules/flake8_type_checking/mod.rs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,35 @@ mod tests {
106106
Ok(())
107107
}
108108

109+
#[test_case(
110+
Rule::TypingOnlyStandardLibraryImport,
111+
Path::new("exempt_type_checking_1.py")
112+
)]
113+
#[test_case(
114+
Rule::TypingOnlyStandardLibraryImport,
115+
Path::new("exempt_type_checking_2.py")
116+
)]
117+
#[test_case(
118+
Rule::TypingOnlyStandardLibraryImport,
119+
Path::new("exempt_type_checking_3.py")
120+
)]
121+
fn exempt_type_checking(rule_code: Rule, path: &Path) -> Result<()> {
122+
let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
123+
let diagnostics = test_path(
124+
Path::new("flake8_type_checking").join(path).as_path(),
125+
&settings::LinterSettings {
126+
flake8_type_checking: super::settings::Settings {
127+
exempt_modules: vec![],
128+
strict: true,
129+
..Default::default()
130+
},
131+
..settings::LinterSettings::for_rule(rule_code)
132+
},
133+
)?;
134+
assert_messages!(snapshot, diagnostics);
135+
Ok(())
136+
}
137+
109138
#[test_case(
110139
Rule::RuntimeImportInTypeCheckingBlock,
111140
Path::new("runtime_evaluated_base_classes_1.py")

‎crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -473,15 +473,18 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
473473
)?;
474474

475475
// Step 2) Add the import to a `TYPE_CHECKING` block.
476-
let add_import_edit = checker.importer().typing_import_edit(
477-
&ImportedMembers {
478-
statement,
479-
names: member_names.iter().map(AsRef::as_ref).collect(),
480-
},
481-
at,
482-
checker.semantic(),
483-
checker.source_type,
484-
)?;
476+
let (type_checking_edit, add_import_edit) = checker
477+
.importer()
478+
.typing_import_edit(
479+
&ImportedMembers {
480+
statement,
481+
names: member_names.iter().map(AsRef::as_ref).collect(),
482+
},
483+
at,
484+
checker.semantic(),
485+
checker.source_type,
486+
)?
487+
.into_edits();
485488

486489
// Step 3) Quote any runtime usages of the referenced symbol.
487490
let quote_reference_edits = filter_contained(
@@ -507,10 +510,10 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
507510
);
508511

509512
Ok(Fix::unsafe_edits(
510-
remove_import_edit,
513+
type_checking_edit,
511514
add_import_edit
512-
.into_edits()
513515
.into_iter()
516+
.chain(std::iter::once(remove_import_edit))
514517
.chain(quote_reference_edits),
515518
)
516519
.isolate(Checker::isolation(

‎crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-standard-library-import_exempt_type_checking_1.py.snap‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
3+
---
4+
exempt_type_checking_1.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
5+
|
6+
3 | from __future__ import annotations
7+
4 |
8+
5 | from typing import Final
9+
| ^^^^^ TCH003
10+
6 |
11+
7 | Const: Final[dict] = {}
12+
|
13+
= help: Move into type-checking block
14+
15+
ℹ Unsafe fix
16+
2 2 |
17+
3 3 | from __future__ import annotations
18+
4 4 |
19+
5 |-from typing import Final
20+
5 |+from typing import TYPE_CHECKING
21+
6 |+
22+
7 |+if TYPE_CHECKING:
23+
8 |+ from typing import Final
24+
6 9 |
25+
7 10 | Const: Final[dict] = {}
26+
27+

‎crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-standard-library-import_exempt_type_checking_2.py.snap‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
3+
---
4+
exempt_type_checking_2.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
5+
|
6+
3 | from __future__ import annotations
7+
4 |
8+
5 | from typing import Final, TYPE_CHECKING
9+
| ^^^^^ TCH003
10+
6 |
11+
7 | Const: Final[dict] = {}
12+
|
13+
= help: Move into type-checking block
14+
15+
ℹ Unsafe fix
16+
2 2 |
17+
3 3 | from __future__ import annotations
18+
4 4 |
19+
5 |-from typing import Final, TYPE_CHECKING
20+
5 |+from typing import TYPE_CHECKING
21+
6 |+
22+
7 |+if TYPE_CHECKING:
23+
8 |+ from typing import Final
24+
6 9 |
25+
7 10 | Const: Final[dict] = {}
26+
27+

‎crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-standard-library-import_exempt_type_checking_3.py.snap‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
3+
---
4+
exempt_type_checking_3.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
5+
|
6+
3 | from __future__ import annotations
7+
4 |
8+
5 | from typing import Final, Mapping
9+
| ^^^^^ TCH003
10+
6 |
11+
7 | Const: Final[dict] = {}
12+
|
13+
= help: Move into type-checking block
14+
15+
ℹ Unsafe fix
16+
2 2 |
17+
3 3 | from __future__ import annotations
18+
4 4 |
19+
5 |-from typing import Final, Mapping
20+
5 |+from typing import Mapping
21+
6 |+from typing import TYPE_CHECKING
22+
7 |+
23+
8 |+if TYPE_CHECKING:
24+
9 |+ from typing import Final
25+
6 10 |
26+
7 11 | Const: Final[dict] = {}
27+
28+

0 commit comments

Comments
 (0)