diff --git a/README.md b/README.md index ed53076758..c8e05f2aaf 100644 --- a/README.md +++ b/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) diff --git a/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID252.py b/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID252.py index 01716a0322..162b99c2d7 100644 --- a/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID252.py +++ b/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 diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index dc6541af96..82f398e492 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/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); diff --git a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs index 028e6c18d4..11a524754a 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs +++ b/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 = 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 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 { + 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 { 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( diff --git a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_all_imports.snap b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_all_imports.snap index 63661cd540..713e67ce1c 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_all_imports.snap +++ b/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: ~ + fix: + content: + - from resources import grandparent + location: + row: 11 + column: 0 + end_location: + row: 11 + column: 27 parent: ~ - kind: - RelativeImports: all + RelativeImports: + strictness: all location: - row: 8 + row: 12 column: 0 end_location: - row: 8 + 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: ~ diff --git a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_parent_imports.snap b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_parent_imports.snap index 424bef95cb..6360f81b4c 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__relative_imports__tests__ban_parent_imports.snap +++ b/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: ~ + fix: + content: + - from test import parent + location: + row: 9 + column: 0 + end_location: + row: 9 + column: 21 parent: ~ - kind: - RelativeImports: parents + RelativeImports: + strictness: parents location: - row: 5 + row: 10 column: 0 end_location: - row: 5 + row: 10 column: 28 - fix: ~ + fix: + content: + - from test.parent import example + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 28 parent: ~ - kind: - RelativeImports: parents + RelativeImports: + strictness: parents location: - row: 7 + row: 11 column: 0 end_location: - row: 7 + row: 11 column: 27 - fix: ~ + fix: + content: + - from resources import grandparent + location: + row: 11 + column: 0 + end_location: + row: 11 + column: 27 parent: ~ - kind: - RelativeImports: parents + RelativeImports: + strictness: parents location: - row: 8 + row: 12 column: 0 end_location: - row: 8 + 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: ~ diff --git a/crates/ruff_python/src/string.rs b/crates/ruff_python/src/string.rs index 16499f2182..5f02194005 100644 --- a/crates/ruff_python/src/string.rs +++ b/crates/ruff_python/src/string.rs @@ -3,6 +3,7 @@ use regex::Regex; pub static STRING_QUOTE_PREFIX_REGEX: Lazy = Lazy::new(|| Regex::new(r#"^(?i)[urb]*['"](?P.*)['"]$"#).unwrap()); +pub static LOWER_OR_UNDERSCORE: Lazy = 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")); diff --git a/docs/rules/relative-imports.md b/docs/rules/relative-imports.md new file mode 100644 index 0000000000..bdab5fea53 --- /dev/null +++ b/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 +``` \ No newline at end of file