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 <[email protected]>
astral-sh · Nov 14, 2024 · 8095ff0 · 8095ff0
1 parent 24cd592
commit 8095ff0
Show file tree

Hide file tree

Showing 8 changed files with 173 additions and 12 deletions.
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
@@ -0,0 +1 @@
+import this as this
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
@@ -0,0 +1 @@
+from module import this as this
diff --git 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
@@ -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
@@ -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<String>) -> bool {
+        self.required_imports
+            .contains(&NameImport::Import(ModuleNameImport {
+                name: Alias { name, as_name },
+            }))
+    }
+    pub fn requires_member_import(
+        &self,
+        module: Option<String>,
+        name: String,
+        as_name: Option<String>,
+        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/...ts/ruff_linter__rules__isort__tests__required_import_with_useless_alias_this_this.py.snap b/...ts/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/...inter__rules__isort__tests__required_importfrom_with_useless_alias_this_this_from.py.snap b/...inter__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
@@ -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<String> {
+        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);
 }