Implement autofix for relative imports (TID252) (#2739)

README.md

@@ -1249,7 +1249,7 @@ For more, see [flake8-tidy-imports](https://pypi.org/project/flake8-tidy-imports
 | Code | Name | Message | Fix |
 | ---- | ---- | ------- | --- |
 | TID251 | banned-api | `{name}` is banned: {message} |  |
-| TID252 | relative-imports | Relative imports from parent modules are banned |  |
+| TID252 | [relative-imports](https://github.com/charliermarsh/ruff/blob/main/docs/rules/relative-imports.md) | Relative imports from parent modules are banned | 🛠 |
 
 ### flake8-type-checking (TCH)
 

crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID252.py

@@ -1,12 +1,30 @@
-from . import sibling
-from .sibling import example
-
-from .. import parent
-from ..parent import example
-
-from ... import grandparent
-from ...grandparent import example
-
+# OK
 import other
 import other.example
 from other import example
+
+# TID252
+from . import sibling
+from .sibling import example
+from .. import parent
+from ..parent import example
+from ... import grandparent
+from ...grandparent import example
+from  .parent import hello
+from .\
+    parent import \
+        hello_world
+from \
+    ..parent\
+    import \
+    world_hello
+
+# TID252 (without autofix; too many levels up)
+from ..... import ultragrantparent
+from ...... import ultragrantparent
+from ....... import ultragrantparent
+from ......... import ultragrantparent
+from ........................... import ultragrantparent
+from .....parent import ultragrantparent
+from .........parent import ultragrantparent
+from ...........................parent import ultragrantparent

crates/ruff/src/checkers/ast.rs

@@ -1291,9 +1291,12 @@ where
                     if self.settings.rules.enabled(&Rule::RelativeImports) {
                         if let Some(diagnostic) =
                             flake8_tidy_imports::relative_imports::banned_relative_import(
+                                self,
                                 stmt,
                                 level.as_ref(),
+                                module.as_deref(),
                                 &self.settings.flake8_tidy_imports.ban_relative_imports,
+                                self.path,
                             )
                         {
                             self.diagnostics.push(diagnostic);

crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs

@@ -1,11 +1,19 @@
-use ruff_macros::{define_violation, derive_message_formats};
-use rustpython_parser::ast::Stmt;
+use std::path::Path;
+
+use rustpython_parser::ast::{Stmt, StmtKind};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
+use ruff_macros::{define_violation, derive_message_formats};
+use ruff_python::string::is_lower_with_underscore;
+
+use crate::ast::helpers::{create_stmt, unparse_stmt};
 use crate::ast::types::Range;
+use crate::checkers::ast::Checker;
+use crate::fix::Fix;
 use crate::registry::Diagnostic;
-use crate::violation::Violation;
+use crate::source_code::Stylist;
+use crate::violation::{AutofixKind, Availability, Violation};
 
 pub type Settings = Strictness;
 
@@ -20,34 +28,144 @@ pub enum Strictness {
 }
 
 define_violation!(
-    pub struct RelativeImports(pub Strictness);
+    /// ## What it does
+    /// Checks for relative imports.
+    ///
+    /// ## Why is this bad?
+    /// Absolute imports, or relative imports from siblings, are recommended by [PEP 8](https://peps.python.org/pep-0008/#imports):
+    ///
+    /// > Absolute imports are recommended, as they are usually more readable and tend to be better behaved...
+    /// > ```python
+    /// > import mypkg.sibling
+    /// > from mypkg import sibling
+    /// > from mypkg.sibling import example
+    /// > ```
+    /// > However, explicit relative imports are an acceptable alternative to absolute imports,
+    /// > especially when dealing with complex package layouts where using absolute imports would be
+    /// > unnecessarily verbose:
+    /// > ```python
+    /// > from . import sibling
+    /// > from .sibling import example
+    /// > ```
+    ///
+    /// Note that degree of strictness packages can be specified via the
+    /// [`strictness`](https://github.com/charliermarsh/ruff#strictness)
+    /// configuration option, which allows banning all relative imports (`strictness = "all"`)
+    /// or only those that extend into the parent module or beyond (`strictness = "parents"`).
+    ///
+    /// ## Example
+    /// ```python
+    /// from .. import foo
+    /// ```
+    ///
+    /// Use instead:
+    /// ```python
+    /// from mypkg import foo
+    /// ```
+    pub struct RelativeImports {
+        pub strictness: Strictness,
+    }
 );
 impl Violation for RelativeImports {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
-        let RelativeImports(strictness) = self;
-        match strictness {
+        match self.strictness {
             Strictness::Parents => format!("Relative imports from parent modules are banned"),
             Strictness::All => format!("Relative imports are banned"),
         }
     }
+
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        Some(|RelativeImports { strictness }| match strictness {
+            Strictness::Parents => {
+                format!("Replace relative imports from parent modules with absolute imports")
+            }
+            Strictness::All => format!("Replace relative imports with absolute imports"),
+        })
+    }
+}
+
+fn fix_banned_relative_import(
+    stmt: &Stmt,
+    level: Option<&usize>,
+    module: Option<&str>,
+    path: &Path,
+    stylist: &Stylist,
+) -> Option<Fix> {
+    let base = if let Some(module) = module {
+        module.to_string()
+    } else {
+        String::new()
+    };
+
+    let mut parent = path.parent()?;
+    for _ in 0..*level? {
+        parent = parent.parent()?;
+    }
+
+    let module_name = parent.file_name()?.to_string_lossy().to_string();
+
+    // Require import to be a valid PEP 8 module:
+    // https://python.org/dev/peps/pep-0008/#package-and-module-names
+    if !is_lower_with_underscore(module_name.as_str()) {
+        return None;
+    }
+
+    let new_import = if base.is_empty() {
+        module_name
+    } else {
+        format!("{}.{}", module_name, base)
+    };
+
+    let content = match &stmt.node {
+        StmtKind::ImportFrom { names, .. } => unparse_stmt(
+            &create_stmt(StmtKind::ImportFrom {
+                module: Some(new_import),
+                names: names.clone(),
+                level: Some(0),
+            }),
+            stylist,
+        ),
+        _ => return None,
+    };
+
+    Some(Fix::replacement(
+        content,
+        stmt.location,
+        stmt.end_location.unwrap(),
+    ))
 }
 
 /// TID252
 pub fn banned_relative_import(
+    checker: &Checker,
     stmt: &Stmt,
     level: Option<&usize>,
+    module: Option<&str>,
     strictness: &Strictness,
+    path: &Path,
 ) -> Option<Diagnostic> {
     let strictness_level = match strictness {
         Strictness::All => 0,
         Strictness::Parents => 1,
     };
     if level? > &strictness_level {
-        Some(Diagnostic::new(
-            RelativeImports(strictness.clone()),
+        let mut diagnostic = Diagnostic::new(
+            RelativeImports {
+                strictness: strictness.clone(),
+            },
             Range::from_located(stmt),
-        ))
+        );
+        if checker.patch(diagnostic.kind.rule()) {
+            if let Some(fix) =
+                fix_banned_relative_import(stmt, level, module, path, checker.stylist)
+            {
+                diagnostic.amend(fix);
+            };
+        }
+        Some(diagnostic)
     } else {
         None
     }
@@ -59,12 +177,13 @@ mod tests {
 
     use anyhow::Result;
 
-    use super::Strictness;
     use crate::assert_yaml_snapshot;
     use crate::registry::Rule;
     use crate::settings::Settings;
     use crate::test::test_path;
 
+    use super::Strictness;
+
     #[test]
     fn ban_parent_imports() -> Result<()> {
         let diagnostics = test_path(

crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_all_imports.snap

@@ -1,65 +1,264 @@
 ---
-source: src/rules/flake8_tidy_imports/relative_imports.rs
+source: crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs
 expression: diagnostics
 ---
 - kind:
-    RelativeImports: all
-  location:
-    row: 1
-    column: 0
-  end_location:
-    row: 1
-    column: 21
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: all
-  location:
-    row: 2
-    column: 0
-  end_location:
-    row: 2
-    column: 28
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: all
-  location:
-    row: 4
-    column: 0
-  end_location:
-    row: 4
-    column: 21
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: all
-  location:
-    row: 5
-    column: 0
-  end_location:
-    row: 5
-    column: 28
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: all
+    RelativeImports:
+      strictness: all
   location:
     row: 7
     column: 0
   end_location:
     row: 7
+    column: 21
+  fix:
+    content:
+      - from fixtures import sibling
+    location:
+      row: 7
+      column: 0
+    end_location:
+      row: 7
+      column: 21
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 8
+    column: 0
+  end_location:
+    row: 8
+    column: 28
+  fix:
+    content:
+      - from fixtures.sibling import example
+    location:
+      row: 8
+      column: 0
+    end_location:
+      row: 8
+      column: 28
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 9
+    column: 0
+  end_location:
+    row: 9
+    column: 21
+  fix:
+    content:
+      - from test import parent
+    location:
+      row: 9
+      column: 0
+    end_location:
+      row: 9
+      column: 21
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 10
+    column: 0
+  end_location:
+    row: 10
+    column: 28
+  fix:
+    content:
+      - from test.parent import example
+    location:
+      row: 10
+      column: 0
+    end_location:
+      row: 10
+      column: 28
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 11
+    column: 0
+  end_location:
+    row: 11
     column: 27
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: all
+  fix:
+    content:
+      - from resources import grandparent
     location:
-    row: 8
+      row: 11
       column: 0
     end_location:
-    row: 8
+      row: 11
+      column: 27
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 12
+    column: 0
+  end_location:
+    row: 12
+    column: 34
+  fix:
+    content:
+      - from resources.grandparent import example
+    location:
+      row: 12
+      column: 0
+    end_location:
+      row: 12
+      column: 34
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 13
+    column: 0
+  end_location:
+    row: 13
+    column: 26
+  fix:
+    content:
+      - from fixtures.parent import hello
+    location:
+      row: 13
+      column: 0
+    end_location:
+      row: 13
+      column: 26
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 14
+    column: 0
+  end_location:
+    row: 16
+    column: 19
+  fix:
+    content:
+      - from fixtures.parent import hello_world
+    location:
+      row: 14
+      column: 0
+    end_location:
+      row: 16
+      column: 19
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 17
+    column: 0
+  end_location:
+    row: 20
+    column: 15
+  fix:
+    content:
+      - from test.parent import world_hello
+    location:
+      row: 17
+      column: 0
+    end_location:
+      row: 20
+      column: 15
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 23
+    column: 0
+  end_location:
+    row: 23
     column: 34
   fix: ~
   parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 24
+    column: 0
+  end_location:
+    row: 24
+    column: 35
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 25
+    column: 0
+  end_location:
+    row: 25
+    column: 36
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 26
+    column: 0
+  end_location:
+    row: 26
+    column: 38
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 27
+    column: 0
+  end_location:
+    row: 27
+    column: 56
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 28
+    column: 0
+  end_location:
+    row: 28
+    column: 40
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 29
+    column: 0
+  end_location:
+    row: 29
+    column: 44
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: all
+  location:
+    row: 30
+    column: 0
+  end_location:
+    row: 30
+    column: 62
+  fix: ~
+  parent: ~
 

crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_parent_imports.snap

@@ -1,45 +1,188 @@
 ---
-source: src/rules/flake8_tidy_imports/relative_imports.rs
+source: crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs
 expression: diagnostics
 ---
 - kind:
-    RelativeImports: parents
+    RelativeImports:
+      strictness: parents
   location:
-    row: 4
+    row: 9
     column: 0
   end_location:
-    row: 4
+    row: 9
     column: 21
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: parents
+  fix:
+    content:
+      - from test import parent
     location:
-    row: 5
+      row: 9
       column: 0
     end_location:
-    row: 5
+      row: 9
+      column: 21
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 10
+    column: 0
+  end_location:
+    row: 10
     column: 28
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: parents
+  fix:
+    content:
+      - from test.parent import example
     location:
-    row: 7
+      row: 10
       column: 0
     end_location:
-    row: 7
+      row: 10
+      column: 28
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 11
+    column: 0
+  end_location:
+    row: 11
     column: 27
-  fix: ~
-  parent: ~
-- kind:
-    RelativeImports: parents
+  fix:
+    content:
+      - from resources import grandparent
     location:
-    row: 8
+      row: 11
       column: 0
     end_location:
-    row: 8
+      row: 11
+      column: 27
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 12
+    column: 0
+  end_location:
+    row: 12
+    column: 34
+  fix:
+    content:
+      - from resources.grandparent import example
+    location:
+      row: 12
+      column: 0
+    end_location:
+      row: 12
+      column: 34
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 17
+    column: 0
+  end_location:
+    row: 20
+    column: 15
+  fix:
+    content:
+      - from test.parent import world_hello
+    location:
+      row: 17
+      column: 0
+    end_location:
+      row: 20
+      column: 15
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 23
+    column: 0
+  end_location:
+    row: 23
     column: 34
   fix: ~
   parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 24
+    column: 0
+  end_location:
+    row: 24
+    column: 35
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 25
+    column: 0
+  end_location:
+    row: 25
+    column: 36
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 26
+    column: 0
+  end_location:
+    row: 26
+    column: 38
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 27
+    column: 0
+  end_location:
+    row: 27
+    column: 56
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 28
+    column: 0
+  end_location:
+    row: 28
+    column: 40
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 29
+    column: 0
+  end_location:
+    row: 29
+    column: 44
+  fix: ~
+  parent: ~
+- kind:
+    RelativeImports:
+      strictness: parents
+  location:
+    row: 30
+    column: 0
+  end_location:
+    row: 30
+    column: 62
+  fix: ~
+  parent: ~
 

crates/ruff_python/src/string.rs

@@ -3,6 +3,7 @@ use regex::Regex;
 
 pub static STRING_QUOTE_PREFIX_REGEX: Lazy<Regex> =
     Lazy::new(|| Regex::new(r#"^(?i)[urb]*['"](?P<raw>.*)['"]$"#).unwrap());
+pub static LOWER_OR_UNDERSCORE: Lazy<Regex> = Lazy::new(|| Regex::new(r"^[a-z_]+$").unwrap());
 
 pub fn is_lower(s: &str) -> bool {
     let mut cased = false;
@@ -28,6 +29,11 @@ pub fn is_upper(s: &str) -> bool {
     cased
 }
 
+// Module names should be lowercase, and may contain underscore
+pub fn is_lower_with_underscore(s: &str) -> bool {
+    LOWER_OR_UNDERSCORE.is_match(s)
+}
+
 /// Remove prefixes (u, r, b) and quotes around a string. This expects the given
 /// string to be a valid Python string representation, it doesn't do any
 /// validation.
@@ -43,7 +49,7 @@ pub fn strip_quotes_and_prefixes(s: &str) -> &str {
 
 #[cfg(test)]
 mod tests {
-    use crate::string::{is_lower, is_upper, strip_quotes_and_prefixes};
+    use crate::string::{is_lower, is_lower_with_underscore, is_upper, strip_quotes_and_prefixes};
 
     #[test]
     fn test_is_lower() {
@@ -56,6 +62,14 @@ mod tests {
         assert!(!is_lower("_"));
     }
 
+    #[test]
+    fn test_is_lower_underscore() {
+        assert!(is_lower_with_underscore("abc"));
+        assert!(is_lower_with_underscore("a_b_c"));
+        assert!(!is_lower_with_underscore("a-b-c"));
+        assert!(!is_lower_with_underscore("a_B_c"));
+    }
+
     #[test]
     fn test_is_upper() {
         assert!(is_upper("ABC"));

docs/rules/relative-imports.md

@@ -0,0 +1,40 @@
+# relative-imports (TID252)
+
+Derived from the **flake8-tidy-imports** linter.
+
+Autofix is sometimes available.
+
+## What it does
+Checks for relative imports.
+
+## Why is this bad?
+Absolute imports, or relative imports from siblings, are recommended by [PEP 8](https://peps.python.org/pep-0008/#imports):
+
+> Absolute imports are recommended, as they are usually more readable and tend to be better behaved...
+> ```python
+> import mypkg.sibling
+> from mypkg import sibling
+> from mypkg.sibling import example
+> ```
+> However, explicit relative imports are an acceptable alternative to absolute imports,
+> especially when dealing with complex package layouts where using absolute imports would be
+> unnecessarily verbose:
+> ```python
+> from . import sibling
+> from .sibling import example
+> ```
+
+Note that degree of strictness packages can be specified via the
+[`strictness`](https://github.com/charliermarsh/ruff#strictness)
+configuration option, which allows banning all relative imports (`strictness = "all"`)
+or only those that extend into the parent module or beyond (`strictness = "parents"`).
+
+## Example
+```python
+from .. import foo
+```
+
+Use instead:
+```python
+from mypkg import foo
+```