Skip to content

Commit

Permalink
Allow removal of typing from exempt-modules (#9214)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
charliermarsh authored Dec 20, 2023
1 parent 29846f5 commit 4b4160e
Show file tree
Hide file tree
Showing 10 changed files with 235 additions and 35 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
"""Add `TYPE_CHECKING` to an existing `typing` import. Another member is moved."""

from __future__ import annotations

from typing import Final

Const: Final[dict] = {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
"""Using `TYPE_CHECKING` from an existing `typing` import. Another member is moved."""

from __future__ import annotations

from typing import Final, TYPE_CHECKING

Const: Final[dict] = {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
"""Using `TYPE_CHECKING` from an existing `typing` import. Another member is moved."""

from __future__ import annotations

from typing import Final, Mapping

Const: Final[dict] = {}
104 changes: 80 additions & 24 deletions crates/ruff_linter/src/importer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use ruff_text_size::{Ranged, TextSize};
use ruff_diagnostics::Edit;
use ruff_python_ast::imports::{AnyImport, Import, ImportFrom};
use ruff_python_codegen::Stylist;
use ruff_python_semantic::SemanticModel;
use ruff_python_semantic::{ImportedName, SemanticModel};
use ruff_python_trivia::textwrap::indent;
use ruff_source_file::Locator;

Expand Down Expand Up @@ -132,7 +132,48 @@ impl<'a> Importer<'a> {
)?;

// Import the `TYPE_CHECKING` symbol from the typing module.
let (type_checking_edit, type_checking) = self.get_or_import_type_checking(at, semantic)?;
let (type_checking_edit, type_checking) =
if let Some(type_checking) = Self::find_type_checking(at, semantic)? {
// Special-case: if the `TYPE_CHECKING` symbol is imported as part of the same
// statement that we're modifying, avoid adding a no-op edit. For example, here,
// the `TYPE_CHECKING` no-op edit would overlap with the edit to remove `Final`
// from the import:
// ```python
// from __future__ import annotations
//
// from typing import Final, TYPE_CHECKING
//
// Const: Final[dict] = {}
// ```
let edit = if type_checking.statement(semantic) == import.statement {
None
} else {
Some(Edit::range_replacement(
self.locator.slice(type_checking.range()).to_string(),
type_checking.range(),
))
};
(edit, type_checking.into_name())
} else {
// Special-case: if the `TYPE_CHECKING` symbol would be added to the same import
// we're modifying, import it as a separate import statement. For example, here,
// we're concurrently removing `Final` and adding `TYPE_CHECKING`, so it's easier to
// use a separate import statement:
// ```python
// from __future__ import annotations
//
// from typing import Final
//
// Const: Final[dict] = {}
// ```
let (edit, name) = self.import_symbol(
&ImportRequest::import_from("typing", "TYPE_CHECKING"),
at,
Some(import.statement),
semantic,
)?;
(Some(edit), name)
};

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

/// Generate an [`Edit`] to reference `typing.TYPE_CHECKING`. Returns the [`Edit`] necessary to
/// make the symbol available in the current scope along with the bound name of the symbol.
fn get_or_import_type_checking(
&self,
/// Find a reference to `typing.TYPE_CHECKING`.
fn find_type_checking(
at: TextSize,
semantic: &SemanticModel,
) -> Result<(Edit, String), ResolutionError> {
) -> Result<Option<ImportedName>, ResolutionError> {
for module in semantic.typing_modules() {
if let Some((edit, name)) = self.get_symbol(
if let Some(imported_name) = Self::find_symbol(
&ImportRequest::import_from(module, "TYPE_CHECKING"),
at,
semantic,
)? {
return Ok((edit, name));
return Ok(Some(imported_name));
}
}

self.import_symbol(
&ImportRequest::import_from("typing", "TYPE_CHECKING"),
at,
semantic,
)
Ok(None)
}

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

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

Ok(Some(imported_name))
}

/// Return an [`Edit`] to reference an existing symbol, if it's present in the given [`SemanticModel`].
fn get_symbol(
&self,
symbol: &ImportRequest,
at: TextSize,
semantic: &SemanticModel,
) -> Result<Option<(Edit, String)>, ResolutionError> {
// Find the symbol in the current scope.
let Some(imported_name) = Self::find_symbol(symbol, at, semantic)? else {
return Ok(None);
};

// We also add a no-op edit to force conflicts with any other fixes that might try to
// remove the import. Consider:
//
Expand Down Expand Up @@ -259,9 +307,13 @@ impl<'a> Importer<'a> {
&self,
symbol: &ImportRequest,
at: TextSize,
except: Option<&Stmt>,
semantic: &SemanticModel,
) -> Result<(Edit, String), ResolutionError> {
if let Some(stmt) = self.find_import_from(symbol.module, at) {
if let Some(stmt) = self
.find_import_from(symbol.module, at)
.filter(|stmt| except != Some(stmt))
{
// Case 1: `from functools import lru_cache` is in scope, and we're trying to reference
// `functools.cache`; thus, we add `cache` to the import, and return `"cache"` as the
// bound name.
Expand Down Expand Up @@ -423,14 +475,18 @@ impl RuntimeImportEdit {
#[derive(Debug)]
pub(crate) struct TypingImportEdit {
/// The edit to add the `TYPE_CHECKING` symbol to the module.
type_checking_edit: Edit,
type_checking_edit: Option<Edit>,
/// The edit to add the import to a `TYPE_CHECKING` block.
add_import_edit: Edit,
}

impl TypingImportEdit {
pub(crate) fn into_edits(self) -> Vec<Edit> {
vec![self.type_checking_edit, self.add_import_edit]
pub(crate) fn into_edits(self) -> (Edit, Option<Edit>) {
if let Some(type_checking_edit) = self.type_checking_edit {
(type_checking_edit, Some(self.add_import_edit))
} else {
(self.add_import_edit, None)
}
}
}

Expand Down
29 changes: 29 additions & 0 deletions crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,35 @@ mod tests {
Ok(())
}

#[test_case(
Rule::TypingOnlyStandardLibraryImport,
Path::new("exempt_type_checking_1.py")
)]
#[test_case(
Rule::TypingOnlyStandardLibraryImport,
Path::new("exempt_type_checking_2.py")
)]
#[test_case(
Rule::TypingOnlyStandardLibraryImport,
Path::new("exempt_type_checking_3.py")
)]
fn exempt_type_checking(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
let diagnostics = test_path(
Path::new("flake8_type_checking").join(path).as_path(),
&settings::LinterSettings {
flake8_type_checking: super::settings::Settings {
exempt_modules: vec![],
strict: true,
..Default::default()
},
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(
Rule::RuntimeImportInTypeCheckingBlock,
Path::new("runtime_evaluated_base_classes_1.py")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,15 +473,18 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
)?;

// Step 2) Add the import to a `TYPE_CHECKING` block.
let add_import_edit = checker.importer().typing_import_edit(
&ImportedMembers {
statement,
names: member_names.iter().map(AsRef::as_ref).collect(),
},
at,
checker.semantic(),
checker.source_type,
)?;
let (type_checking_edit, add_import_edit) = checker
.importer()
.typing_import_edit(
&ImportedMembers {
statement,
names: member_names.iter().map(AsRef::as_ref).collect(),
},
at,
checker.semantic(),
checker.source_type,
)?
.into_edits();

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

Ok(Fix::unsafe_edits(
remove_import_edit,
type_checking_edit,
add_import_edit
.into_edits()
.into_iter()
.chain(std::iter::once(remove_import_edit))
.chain(quote_reference_edits),
)
.isolate(Checker::isolation(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
---
exempt_type_checking_1.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
|
3 | from __future__ import annotations
4 |
5 | from typing import Final
| ^^^^^ TCH003
6 |
7 | Const: Final[dict] = {}
|
= help: Move into type-checking block

Unsafe fix
2 2 |
3 3 | from __future__ import annotations
4 4 |
5 |-from typing import Final
5 |+from typing import TYPE_CHECKING
6 |+
7 |+if TYPE_CHECKING:
8 |+ from typing import Final
6 9 |
7 10 | Const: Final[dict] = {}


Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
---
exempt_type_checking_2.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
|
3 | from __future__ import annotations
4 |
5 | from typing import Final, TYPE_CHECKING
| ^^^^^ TCH003
6 |
7 | Const: Final[dict] = {}
|
= help: Move into type-checking block

Unsafe fix
2 2 |
3 3 | from __future__ import annotations
4 4 |
5 |-from typing import Final, TYPE_CHECKING
5 |+from typing import TYPE_CHECKING
6 |+
7 |+if TYPE_CHECKING:
8 |+ from typing import Final
6 9 |
7 10 | Const: Final[dict] = {}


Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
---
source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs
---
exempt_type_checking_3.py:5:20: TCH003 [*] Move standard library import `typing.Final` into a type-checking block
|
3 | from __future__ import annotations
4 |
5 | from typing import Final, Mapping
| ^^^^^ TCH003
6 |
7 | Const: Final[dict] = {}
|
= help: Move into type-checking block

Unsafe fix
2 2 |
3 3 | from __future__ import annotations
4 4 |
5 |-from typing import Final, Mapping
5 |+from typing import Mapping
6 |+from typing import TYPE_CHECKING
7 |+
8 |+if TYPE_CHECKING:
9 |+ from typing import Final
6 10 |
7 11 | Const: Final[dict] = {}


Loading

0 comments on commit 4b4160e

Please sign in to comment.