From 8095ff0e55beb55ddca8f853d04aa4154ef2cf93 Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:39:38 -0600 Subject: [PATCH] enforce required imports even with useless alias (#14287) This PR handles a panic that occurs when applying unsafe fixes if a user inserts a required import (I002) that has a "useless alias" in it, like `import numpy as numpy`, and also selects PLC0414 (useless-import-alias) In this case, the fixes alternate between adding the required import statement, then removing the alias, until the recursion limit is reached. See linked issue for an example. Closes #14283 --------- Co-authored-by: Charlie Marsh --- .../isort/required_imports/this_this.py | 1 + .../isort/required_imports/this_this_from.py | 1 + .../src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/rules/isort/mod.rs | 51 +++++++++++ .../ruff_linter/src/rules/isort/settings.rs | 25 +++++- ...mport_with_useless_alias_this_this.py.snap | 9 ++ ..._with_useless_alias_this_this_from.py.snap | 9 ++ .../pylint/rules/useless_import_alias.rs | 87 ++++++++++++++++--- 8 files changed, 173 insertions(+), 12 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this.py create mode 100644 crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this_from.py create mode 100644 crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap create mode 100644 crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this.py b/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this.py new file mode 100644 index 00000000000000..3cbee92baf8a50 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this.py @@ -0,0 +1 @@ +import this as this \ No newline at end of file diff --git a/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this_from.py b/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this_from.py new file mode 100644 index 00000000000000..f79c88288fe219 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/isort/required_imports/this_this_from.py @@ -0,0 +1 @@ +from module import this as this \ No newline at end of file diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 1883fab5f4ada3..9bdc28a1c5863d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1028,7 +1028,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } if !checker.source_type.is_stub() { if checker.enabled(Rule::UselessImportAlias) { - pylint::rules::useless_import_alias(checker, alias); + pylint::rules::useless_import_from_alias(checker, alias, module, level); } } } diff --git a/crates/ruff_linter/src/rules/isort/mod.rs b/crates/ruff_linter/src/rules/isort/mod.rs index afd3bc6fab0acb..2db901583f847d 100644 --- a/crates/ruff_linter/src/rules/isort/mod.rs +++ b/crates/ruff_linter/src/rules/isort/mod.rs @@ -855,6 +855,57 @@ mod tests { Ok(()) } + #[test_case(Path::new("this_this_from.py"))] + fn required_importfrom_with_useless_alias(path: &Path) -> Result<()> { + let snapshot = format!( + "required_importfrom_with_useless_alias_{}", + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("isort/required_imports").join(path).as_path(), + &LinterSettings { + src: vec![test_resource_path("fixtures/isort")], + isort: super::settings::Settings { + required_imports: BTreeSet::from_iter([NameImport::ImportFrom( + MemberNameImport::alias( + "module".to_string(), + "this".to_string(), + "this".to_string(), + ), + )]), + ..super::settings::Settings::default() + }, + ..LinterSettings::for_rules([Rule::MissingRequiredImport, Rule::UselessImportAlias]) + }, + )?; + + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + #[test_case(Path::new("this_this.py"))] + fn required_import_with_useless_alias(path: &Path) -> Result<()> { + let snapshot = format!( + "required_import_with_useless_alias_{}", + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("isort/required_imports").join(path).as_path(), + &LinterSettings { + src: vec![test_resource_path("fixtures/isort")], + isort: super::settings::Settings { + required_imports: BTreeSet::from_iter([NameImport::Import( + ModuleNameImport::alias("this".to_string(), "this".to_string()), + )]), + ..super::settings::Settings::default() + }, + ..LinterSettings::for_rules([Rule::MissingRequiredImport, Rule::UselessImportAlias]) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Path::new("docstring.py"))] #[test_case(Path::new("docstring.pyi"))] #[test_case(Path::new("docstring_only.py"))] diff --git a/crates/ruff_linter/src/rules/isort/settings.rs b/crates/ruff_linter/src/rules/isort/settings.rs index 7324be1f8da664..efed45bce84bbb 100644 --- a/crates/ruff_linter/src/rules/isort/settings.rs +++ b/crates/ruff_linter/src/rules/isort/settings.rs @@ -13,7 +13,7 @@ use crate::display_settings; use crate::rules::isort::categorize::KnownModules; use crate::rules::isort::ImportType; use ruff_macros::CacheKey; -use ruff_python_semantic::NameImport; +use ruff_python_semantic::{Alias, MemberNameImport, ModuleNameImport, NameImport}; use super::categorize::ImportSection; @@ -75,6 +75,29 @@ pub struct Settings { pub length_sort_straight: bool, } +impl Settings { + pub fn requires_module_import(&self, name: String, as_name: Option) -> bool { + self.required_imports + .contains(&NameImport::Import(ModuleNameImport { + name: Alias { name, as_name }, + })) + } + pub fn requires_member_import( + &self, + module: Option, + name: String, + as_name: Option, + level: u32, + ) -> bool { + self.required_imports + .contains(&NameImport::ImportFrom(MemberNameImport { + module, + name: Alias { name, as_name }, + level, + })) + } +} + impl Default for Settings { fn default() -> Self { Self { diff --git a/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap new file mode 100644 index 00000000000000..ae68eebe4cb218 --- /dev/null +++ b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap @@ -0,0 +1,9 @@ +--- +source: crates/ruff_linter/src/rules/isort/mod.rs +--- +this_this.py:1:8: PLC0414 Required import does not rename original package. + | +1 | import this as this + | ^^^^^^^^^^^^ PLC0414 + | + = help: Change required import or disable rule. diff --git a/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap new file mode 100644 index 00000000000000..49767099e0faa1 --- /dev/null +++ b/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap @@ -0,0 +1,9 @@ +--- +source: crates/ruff_linter/src/rules/isort/mod.rs +--- +this_this_from.py:1:20: PLC0414 Required import does not rename original package. + | +1 | from module import this as this + | ^^^^^^^^^^^^ PLC0414 + | + = help: Change required import or disable rule. diff --git a/crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs b/crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs index 8b50c72922ae0d..0230ca802739b2 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/useless_import_alias.rs @@ -1,6 +1,6 @@ use ruff_python_ast::Alias; -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_text_size::Ranged; @@ -28,16 +28,29 @@ use crate::checkers::ast::Checker; /// import numpy /// ``` #[violation] -pub struct UselessImportAlias; +pub struct UselessImportAlias { + required_import_conflict: bool, +} + +impl Violation for UselessImportAlias { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; -impl AlwaysFixableViolation for UselessImportAlias { #[derive_message_formats] fn message(&self) -> String { - "Import alias does not rename original package".to_string() + #[allow(clippy::if_not_else)] + if !self.required_import_conflict { + "Import alias does not rename original package".to_string() + } else { + "Required import does not rename original package.".to_string() + } } - fn fix_title(&self) -> String { - "Remove import alias".to_string() + fn fix_title(&self) -> Option { + if self.required_import_conflict { + Some("Change required import or disable rule.".to_string()) + } else { + Some("Remove import alias".to_string()) + } } } @@ -52,11 +65,65 @@ pub(crate) fn useless_import_alias(checker: &mut Checker, alias: &Alias) { if alias.name.as_str() != asname.as_str() { return; } + // A required import with a useless alias causes an infinite loop. + // See https://github.com/astral-sh/ruff/issues/14283 + let required_import_conflict = checker + .settings + .isort + .requires_module_import(alias.name.to_string(), Some(asname.to_string())); + let mut diagnostic = Diagnostic::new( + UselessImportAlias { + required_import_conflict, + }, + alias.range(), + ); + if !required_import_conflict { + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + asname.to_string(), + alias.range(), + ))); + } + + checker.diagnostics.push(diagnostic); +} - let mut diagnostic = Diagnostic::new(UselessImportAlias, alias.range()); - diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( - asname.to_string(), +/// PLC0414 +pub(crate) fn useless_import_from_alias( + checker: &mut Checker, + alias: &Alias, + module: Option<&str>, + level: u32, +) { + let Some(asname) = &alias.asname else { + return; + }; + if alias.name.contains('.') { + return; + } + if alias.name.as_str() != asname.as_str() { + return; + } + // A required import with a useless alias causes an infinite loop. + // See https://github.com/astral-sh/ruff/issues/14283 + let required_import_conflict = checker.settings.isort.requires_member_import( + module.map(str::to_string), + alias.name.to_string(), + Some(asname.to_string()), + level, + ); + let mut diagnostic = Diagnostic::new( + UselessImportAlias { + required_import_conflict, + }, alias.range(), - ))); + ); + + if !required_import_conflict { + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + asname.to_string(), + alias.range(), + ))); + } + checker.diagnostics.push(diagnostic); }