From c944d23053fe84e63a7515c75a50ce46b63dc2e0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 17 Dec 2023 07:53:58 -0500 Subject: [PATCH] Avoid nested quotations in auto-quoting fix (#9168) ## Summary Given `Callable[[Callable[_P, _R]], Callable[_P, _R]]` from the originating issue, when quoting `Callable`, we quoted the inner `[Callable[_P, _R]]`, and then created a separate edit for the outer `Callable`. Since there's an extra level of nesting in the subscript, the edit for `[Callable[_P, _R]]` correctly did _not_ expand to the entire expression. However, in this case, we should discard the inner edit, since the expression is getting quoted by the outer edit anyway. Closes https://github.com/astral-sh/ruff/issues/9162. --- .../fixtures/flake8_type_checking/quote.py | 3 ++ .../src/rules/flake8_type_checking/helpers.rs | 14 ++++++ .../runtime_import_in_type_checking_block.rs | 43 +++++++++--------- .../rules/typing_only_runtime_import.rs | 45 ++++++++++--------- ...ping-only-third-party-import_quote.py.snap | 8 ++++ 5 files changed, 71 insertions(+), 42 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py index de3d609d07444..77075dae2f639 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py @@ -87,3 +87,6 @@ class C: x: DataFrame[ int ] = 1 + + def func() -> DataFrame[[DataFrame[_P, _R]], DataFrame[_P, _R]]: + ... diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index 1f548896057b9..47ce35214b0eb 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -235,3 +235,17 @@ pub(crate) fn quote_annotation( expr.range(), )) } + +/// Filter out any [`Edit`]s that are completely contained by any other [`Edit`]. +pub(crate) fn filter_contained(edits: Vec) -> Vec { + let mut filtered: Vec = Vec::with_capacity(edits.len()); + for edit in edits { + if filtered + .iter() + .all(|filtered_edit| !filtered_edit.range().contains_range(edit.range())) + { + filtered.push(edit); + } + } + filtered +} 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 7f43ebbfd548d..2c15f1ff49911 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 @@ -1,7 +1,6 @@ use std::borrow::Cow; use anyhow::Result; -use itertools::Itertools; use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; @@ -13,7 +12,7 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; -use crate::rules::flake8_type_checking::helpers::quote_annotation; +use crate::rules::flake8_type_checking::helpers::{filter_contained, quote_annotation}; use crate::rules::flake8_type_checking::imports::ImportBinding; /// ## What it does @@ -263,27 +262,29 @@ pub(crate) fn runtime_import_in_type_checking_block( /// Generate a [`Fix`] to quote runtime usages for imports in a type-checking block. fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result { - let quote_reference_edits = imports - .iter() - .flat_map(|ImportBinding { binding, .. }| { - binding.references.iter().filter_map(|reference_id| { - let reference = checker.semantic().reference(*reference_id); - if reference.context().is_runtime() { - Some(quote_annotation( - reference.expression_id()?, - checker.semantic(), - checker.locator(), - checker.stylist(), - checker.generator(), - )) - } else { - None - } + let quote_reference_edits = filter_contained( + imports + .iter() + .flat_map(|ImportBinding { binding, .. }| { + binding.references.iter().filter_map(|reference_id| { + let reference = checker.semantic().reference(*reference_id); + if reference.context().is_runtime() { + Some(quote_annotation( + reference.expression_id()?, + checker.semantic(), + checker.locator(), + checker.stylist(), + checker.generator(), + )) + } else { + None + } + }) }) - }) - .collect::>>()?; + .collect::>>()?, + ); - let mut rest = quote_reference_edits.into_iter().unique(); + let mut rest = quote_reference_edits.into_iter(); let head = rest.next().expect("Expected at least one reference"); Ok(Fix::unsafe_edits(head, rest).isolate(Checker::isolation( checker.semantic().parent_statement_id(node_id), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 2ad9753362d11..decb3c8287c7a 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -1,7 +1,6 @@ use std::borrow::Cow; use anyhow::Result; -use itertools::Itertools; use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix, FixAvailability, Violation}; @@ -13,7 +12,9 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; -use crate::rules::flake8_type_checking::helpers::{is_typing_reference, quote_annotation}; +use crate::rules::flake8_type_checking::helpers::{ + filter_contained, is_typing_reference, quote_annotation, +}; use crate::rules::flake8_type_checking::imports::ImportBinding; use crate::rules::isort::{categorize, ImportSection, ImportType}; @@ -483,32 +484,34 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> )?; // Step 3) Quote any runtime usages of the referenced symbol. - let quote_reference_edits = imports - .iter() - .flat_map(|ImportBinding { binding, .. }| { - binding.references.iter().filter_map(|reference_id| { - let reference = checker.semantic().reference(*reference_id); - if reference.context().is_runtime() { - Some(quote_annotation( - reference.expression_id()?, - checker.semantic(), - checker.locator(), - checker.stylist(), - checker.generator(), - )) - } else { - None - } + let quote_reference_edits = filter_contained( + imports + .iter() + .flat_map(|ImportBinding { binding, .. }| { + binding.references.iter().filter_map(|reference_id| { + let reference = checker.semantic().reference(*reference_id); + if reference.context().is_runtime() { + Some(quote_annotation( + reference.expression_id()?, + checker.semantic(), + checker.locator(), + checker.stylist(), + checker.generator(), + )) + } else { + None + } + }) }) - }) - .collect::>>()?; + .collect::>>()?, + ); Ok(Fix::unsafe_edits( remove_import_edit, add_import_edit .into_edits() .into_iter() - .chain(quote_reference_edits.into_iter().unique()), + .chain(quote_reference_edits), ) .isolate(Checker::isolation( checker.semantic().parent_statement_id(node_id), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap index 3a4417a661d87..9a0e6d8adb8f2 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap @@ -292,6 +292,10 @@ quote.py:78:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a typ 88 |- int 89 |- ] = 1 89 |+ x: "DataFrame[int]" = 1 +90 90 | +91 |- def func() -> DataFrame[[DataFrame[_P, _R]], DataFrame[_P, _R]]: + 91 |+ def func() -> "DataFrame[[DataFrame[_P, _R]], DataFrame[_P, _R]]": +92 92 | ... quote.py:78:35: TCH002 [*] Move third-party import `pandas.Series` into a type-checking block | @@ -329,5 +333,9 @@ quote.py:78:35: TCH002 [*] Move third-party import `pandas.Series` into a type-c 88 |- int 89 |- ] = 1 89 |+ x: "DataFrame[int]" = 1 +90 90 | +91 |- def func() -> DataFrame[[DataFrame[_P, _R]], DataFrame[_P, _R]]: + 91 |+ def func() -> "DataFrame[[DataFrame[_P, _R]], DataFrame[_P, _R]]": +92 92 | ...