From 48b134c551657cca957621af2deb0ef0eeeaaa2d Mon Sep 17 00:00:00 2001 From: Presley Graham Date: Sun, 6 Aug 2023 15:06:46 -0400 Subject: [PATCH 1/5] add TID253: ban module level imports~ --- .../fixtures/flake8_tidy_imports/TID253.py | 28 +++++ .../src/checkers/ast/analyze/statement.rs | 37 ++++++ crates/ruff/src/codes.rs | 1 + .../ruff/src/rules/flake8_tidy_imports/mod.rs | 19 +++ .../src/rules/flake8_tidy_imports/options.rs | 14 +++ .../rules/banned_module_level_imports.rs | 112 ++++++++++++++++++ .../rules/flake8_tidy_imports/rules/mod.rs | 2 + .../src/rules/flake8_tidy_imports/settings.rs | 1 + ...s__tests__banned_module_level_imports.snap | 72 +++++++++++ ruff.schema.json | 11 ++ 10 files changed, 297 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py create mode 100644 crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs create mode 100644 crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py b/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py new file mode 100644 index 0000000000000..e5775cc8a046e --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py @@ -0,0 +1,28 @@ +## Banned modules ## +import torch + +from torch import * + +from tensorflow import a, b, c + +import torch as torch_wearing_a_trenchcoat + +# banning a module also bans any submodules +import torch.foo.bar + +from tensorflow.foo import bar + +from torch.foo.bar import * + +# unlike TID251, inline imports are *not* banned +def my_cool_function(): + import tensorflow.foo.bar + +def another_cool_function(): + from torch.foo import bar + +def import_alias(): + from torch.foo import bar + +if TYPE_CHECKING: + import torch diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index cbdbff9f5ae54..174a69f39f8cc 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -571,6 +571,17 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { alias, ); } + + if checker.enabled(Rule::BannedModuleLevelImports) { + flake8_tidy_imports::rules::name_or_parent_is_banned_at_module_level( + checker, + &alias.name, + stmt, + alias, + checker.locator, + ); + } + if !checker.source_type.is_stub() { if checker.enabled(Rule::UselessImportAlias) { pylint::rules::useless_import_alias(checker, alias); @@ -739,6 +750,32 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } } + if checker.enabled(Rule::BannedModuleLevelImports) { + if let Some(module) = + helpers::resolve_imported_module_path(level, module, checker.module_path) + { + flake8_tidy_imports::rules::name_or_parent_is_banned_at_module_level( + checker, + &module, + stmt, + stmt, + checker.locator, + ); + + for alias in names { + if &alias.name == "*" { + continue; + } + flake8_tidy_imports::rules::name_is_banned_at_module_level( + checker, + &format!("{module}.{}", alias.name), + stmt, + alias, + checker.locator, + ); + } + } + } if checker.enabled(Rule::PytestIncorrectPytestImport) { if let Some(diagnostic) = flake8_pytest_style::rules::import_from(stmt, module, level) diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5e3cfe478a7f5..ef407cec8b3e5 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -309,6 +309,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // flake8-tidy-imports (Flake8TidyImports, "251") => (RuleGroup::Unspecified, rules::flake8_tidy_imports::rules::BannedApi), (Flake8TidyImports, "252") => (RuleGroup::Unspecified, rules::flake8_tidy_imports::rules::RelativeImports), + (Flake8TidyImports, "253") => (RuleGroup::Unspecified, rules::flake8_tidy_imports::rules::BannedModuleLevelImports), // flake8-return (Flake8Return, "501") => (RuleGroup::Unspecified, rules::flake8_return::rules::UnnecessaryReturnNone), diff --git a/crates/ruff/src/rules/flake8_tidy_imports/mod.rs b/crates/ruff/src/rules/flake8_tidy_imports/mod.rs index 8686f3971c06c..d136abbb1dd2e 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/mod.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/mod.rs @@ -124,4 +124,23 @@ mod tests { assert_messages!(diagnostics); Ok(()) } + + #[test] + fn banned_module_level_imports() -> Result<()> { + let diagnostics = test_path( + Path::new("flake8_tidy_imports/TID253.py"), + &Settings { + flake8_tidy_imports: flake8_tidy_imports::settings::Settings { + banned_module_level_imports: vec![ + "torch".to_string(), + "tensorflow".to_string(), + ], + ..Default::default() + }, + ..Settings::for_rules(vec![Rule::BannedModuleLevelImports]) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/options.rs b/crates/ruff/src/rules/flake8_tidy_imports/options.rs index 6f3eb99fcb631..f2a90be5d85f6 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/options.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/options.rs @@ -41,6 +41,18 @@ pub struct Options { /// Note that this rule is only meant to flag accidental uses, /// and can be circumvented via `eval` or `importlib`. pub banned_api: Option>, + #[option( + default = r#"[]"#, + value_type = r#"list[str]"#, + example = r#" + # Ban certain modules from being imported at module level. + # This does not ban these modules from being imported inline. + banned-module-level-imports = ["torch", "tensorflow"] + "# + )] + /// List of specific modules that can't be imported at module level. This does not ban these + /// modules from being imported inline. + pub banned_module_level_imports: Option>, } impl From for Settings { @@ -48,6 +60,7 @@ impl From for Settings { Self { ban_relative_imports: options.ban_relative_imports.unwrap_or(Strictness::Parents), banned_api: options.banned_api.unwrap_or_default(), + banned_module_level_imports: options.banned_module_level_imports.unwrap_or_default(), } } } @@ -57,6 +70,7 @@ impl From for Options { Self { ban_relative_imports: Some(settings.ban_relative_imports), banned_api: Some(settings.banned_api), + banned_module_level_imports: Some(settings.banned_module_level_imports), } } } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs new file mode 100644 index 0000000000000..2d9e2642dcc0e --- /dev/null +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs @@ -0,0 +1,112 @@ +use ruff_python_ast::{Ranged, Stmt}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_source_file::Locator; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for banned imports at module level. The banned imports are allowed inline, such as +/// within a function definition or an `if TYPE_CHECKING:` block. +/// +/// ## Why is this bad? +/// Some modules take a long time to import. Library authors might want to ensure that you only pay +/// the import cost for these modules if you directly use them, rather than if you import a module +/// that happens to use an expensive module in one of its functions. +/// +/// ## Options +/// - `flake8-tidy-imports.banned-module-level-imports` +#[violation] +pub struct BannedModuleLevelImports { + name: String, +} + +impl Violation for BannedModuleLevelImports { + #[derive_message_formats] + fn message(&self) -> String { + let BannedModuleLevelImports { name } = self; + format!("`{name}` is banned at module level. Please move the import inline.") + } +} + +enum NameMatchPolicy { + ExactOnly, + ExactOrParents, +} + +fn banned_at_module_level_with_policy( + checker: &mut Checker, + name: &str, + stmt: &Stmt, + located: &T, + locator: &Locator, + policy: &NameMatchPolicy, +) where + T: Ranged, +{ + if !locator.is_at_start_of_line(stmt.start()) { + return; + } + let banned_module_level_imports = &checker + .settings + .flake8_tidy_imports + .banned_module_level_imports; + for banned_module_name in banned_module_level_imports { + let name_is_banned = match policy { + NameMatchPolicy::ExactOnly => name == banned_module_name, + NameMatchPolicy::ExactOrParents => { + name == banned_module_name || name.starts_with(&format!("{banned_module_name}.")) + } + }; + if name_is_banned { + checker.diagnostics.push(Diagnostic::new( + BannedModuleLevelImports { + name: banned_module_name.to_string(), + }, + located.range(), + )); + return; + } + } +} + +/// TID253 +pub(crate) fn name_is_banned_at_module_level( + checker: &mut Checker, + name: &str, + stmt: &Stmt, + located: &T, + locator: &Locator, +) where + T: Ranged, +{ + banned_at_module_level_with_policy( + checker, + name, + stmt, + located, + locator, + &NameMatchPolicy::ExactOnly, + ); +} + +/// TID253 +pub(crate) fn name_or_parent_is_banned_at_module_level( + checker: &mut Checker, + name: &str, + stmt: &Stmt, + located: &T, + locator: &Locator, +) where + T: Ranged, +{ + banned_at_module_level_with_policy( + checker, + name, + stmt, + located, + locator, + &NameMatchPolicy::ExactOrParents, + ); +} diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/mod.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/mod.rs index 660116d718a84..a9c8e631d9e8e 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/mod.rs @@ -1,5 +1,7 @@ pub(crate) use banned_api::*; +pub(crate) use banned_module_level_imports::*; pub(crate) use relative_imports::*; mod banned_api; +mod banned_module_level_imports; mod relative_imports; diff --git a/crates/ruff/src/rules/flake8_tidy_imports/settings.rs b/crates/ruff/src/rules/flake8_tidy_imports/settings.rs index 90b2843280f27..a1267fbc9b8a8 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/settings.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/settings.rs @@ -26,4 +26,5 @@ pub enum Strictness { pub struct Settings { pub ban_relative_imports: Strictness, pub banned_api: FxHashMap, + pub banned_module_level_imports: Vec, } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap new file mode 100644 index 0000000000000..143f5d2dc714f --- /dev/null +++ b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap @@ -0,0 +1,72 @@ +--- +source: crates/ruff/src/rules/flake8_tidy_imports/mod.rs +--- +TID253.py:2:8: TID253 `torch` is banned at module level. Please move the import inline. + | +1 | ## Banned modules ## +2 | import torch + | ^^^^^ TID253 +3 | +4 | from torch import * + | + +TID253.py:4:1: TID253 `torch` is banned at module level. Please move the import inline. + | +2 | import torch +3 | +4 | from torch import * + | ^^^^^^^^^^^^^^^^^^^ TID253 +5 | +6 | from tensorflow import a, b, c + | + +TID253.py:6:1: TID253 `tensorflow` is banned at module level. Please move the import inline. + | +4 | from torch import * +5 | +6 | from tensorflow import a, b, c + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 +7 | +8 | import torch as torch_wearing_a_trenchcoat + | + +TID253.py:8:8: TID253 `torch` is banned at module level. Please move the import inline. + | + 6 | from tensorflow import a, b, c + 7 | + 8 | import torch as torch_wearing_a_trenchcoat + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 + 9 | +10 | # banning a module also bans any submodules + | + +TID253.py:11:8: TID253 `torch` is banned at module level. Please move the import inline. + | +10 | # banning a module also bans any submodules +11 | import torch.foo.bar + | ^^^^^^^^^^^^^ TID253 +12 | +13 | from tensorflow.foo import bar + | + +TID253.py:13:1: TID253 `tensorflow` is banned at module level. Please move the import inline. + | +11 | import torch.foo.bar +12 | +13 | from tensorflow.foo import bar + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 +14 | +15 | from torch.foo.bar import * + | + +TID253.py:15:1: TID253 `torch` is banned at module level. Please move the import inline. + | +13 | from tensorflow.foo import bar +14 | +15 | from torch.foo.bar import * + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 +16 | +17 | # unlike TID251, inline imports are *not* banned + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 2c5bfc309006c..64a134867315c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1062,6 +1062,16 @@ "additionalProperties": { "$ref": "#/definitions/ApiBan" } + }, + "banned-module-level-imports": { + "description": "List of specific modules that can't be imported at module level. This does not ban these modules from being imported inline.", + "type": [ + "array", + "null" + ], + "items": { + "type": "string" + } } }, "additionalProperties": false @@ -2607,6 +2617,7 @@ "TID25", "TID251", "TID252", + "TID253", "TRY", "TRY0", "TRY00", From 04fa4e824d1293bc38ffa43a0b59a47ea33546ef Mon Sep 17 00:00:00 2001 From: Presley Graham Date: Fri, 11 Aug 2023 20:36:49 -0400 Subject: [PATCH 2/5] Implement konstin's suggested improvements --- .../src/checkers/ast/analyze/statement.rs | 6 +-- .../rules/banned_module_level_imports.rs | 38 +++++++++---------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 174a69f39f8cc..1354c1547de20 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -577,7 +577,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &alias.name, stmt, - alias, + &alias.range(), checker.locator, ); } @@ -758,7 +758,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &module, stmt, - stmt, + &stmt.range(), checker.locator, ); @@ -770,7 +770,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &format!("{module}.{}", alias.name), stmt, - alias, + &alias.range(), checker.locator, ); } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs index 2d9e2642dcc0e..7f33f0dd18214 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs @@ -3,6 +3,7 @@ use ruff_python_ast::{Ranged, Stmt}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_source_file::Locator; +use ruff_text_size::TextRange; use crate::checkers::ast::Checker; @@ -11,9 +12,10 @@ use crate::checkers::ast::Checker; /// within a function definition or an `if TYPE_CHECKING:` block. /// /// ## Why is this bad? -/// Some modules take a long time to import. Library authors might want to ensure that you only pay -/// the import cost for these modules if you directly use them, rather than if you import a module -/// that happens to use an expensive module in one of its functions. +/// Some modules take a relatively long time to import, such as `torch` or `tensorflow`. Library +/// authors might want to ensure that you only pay the import cost for these modules if you +/// directly use them, rather than if you import a module that happens to use an expensive module +/// in one of its functions. /// /// ## Options /// - `flake8-tidy-imports.banned-module-level-imports` @@ -35,16 +37,14 @@ enum NameMatchPolicy { ExactOrParents, } -fn banned_at_module_level_with_policy( +fn banned_at_module_level_with_policy( checker: &mut Checker, name: &str, stmt: &Stmt, - located: &T, + text_range: &TextRange, locator: &Locator, policy: &NameMatchPolicy, -) where - T: Ranged, -{ +) { if !locator.is_at_start_of_line(stmt.start()) { return; } @@ -64,7 +64,7 @@ fn banned_at_module_level_with_policy( BannedModuleLevelImports { name: banned_module_name.to_string(), }, - located.range(), + *text_range, )); return; } @@ -72,40 +72,36 @@ fn banned_at_module_level_with_policy( } /// TID253 -pub(crate) fn name_is_banned_at_module_level( +pub(crate) fn name_is_banned_at_module_level( checker: &mut Checker, name: &str, stmt: &Stmt, - located: &T, + text_range: &TextRange, locator: &Locator, -) where - T: Ranged, -{ +) { banned_at_module_level_with_policy( checker, name, stmt, - located, + text_range, locator, &NameMatchPolicy::ExactOnly, ); } /// TID253 -pub(crate) fn name_or_parent_is_banned_at_module_level( +pub(crate) fn name_or_parent_is_banned_at_module_level( checker: &mut Checker, name: &str, stmt: &Stmt, - located: &T, + text_range: &TextRange, locator: &Locator, -) where - T: Ranged, -{ +) { banned_at_module_level_with_policy( checker, name, stmt, - located, + text_range, locator, &NameMatchPolicy::ExactOrParents, ); From 51f2af32424da4abfeee3e33bed519f1a15b3cb6 Mon Sep 17 00:00:00 2001 From: Presley Graham Date: Fri, 11 Aug 2023 20:54:26 -0400 Subject: [PATCH 3/5] appease clippy --- crates/ruff/src/checkers/ast/analyze/statement.rs | 6 +++--- .../rules/banned_module_level_imports.rs | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 1354c1547de20..6c225c279544b 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -577,7 +577,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &alias.name, stmt, - &alias.range(), + alias.range(), checker.locator, ); } @@ -758,7 +758,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &module, stmt, - &stmt.range(), + stmt.range(), checker.locator, ); @@ -770,7 +770,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { checker, &format!("{module}.{}", alias.name), stmt, - &alias.range(), + alias.range(), checker.locator, ); } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs index 7f33f0dd18214..76a0176e32085 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs @@ -41,7 +41,7 @@ fn banned_at_module_level_with_policy( checker: &mut Checker, name: &str, stmt: &Stmt, - text_range: &TextRange, + text_range: TextRange, locator: &Locator, policy: &NameMatchPolicy, ) { @@ -64,7 +64,7 @@ fn banned_at_module_level_with_policy( BannedModuleLevelImports { name: banned_module_name.to_string(), }, - *text_range, + text_range, )); return; } @@ -76,7 +76,7 @@ pub(crate) fn name_is_banned_at_module_level( checker: &mut Checker, name: &str, stmt: &Stmt, - text_range: &TextRange, + text_range: TextRange, locator: &Locator, ) { banned_at_module_level_with_policy( @@ -94,7 +94,7 @@ pub(crate) fn name_or_parent_is_banned_at_module_level( checker: &mut Checker, name: &str, stmt: &Stmt, - text_range: &TextRange, + text_range: TextRange, locator: &Locator, ) { banned_at_module_level_with_policy( From 07a26e3dffbe6b8d57bc998e8bbcf6a139730657 Mon Sep 17 00:00:00 2001 From: Presley Graham Date: Sat, 12 Aug 2023 09:00:06 -0400 Subject: [PATCH 4/5] handle module level imports that aren't at the start of the line --- .../fixtures/flake8_tidy_imports/TID253.py | 3 ++ .../src/checkers/ast/analyze/statement.rs | 6 --- .../rules/banned_module_level_imports.rs | 29 ++---------- ...s__tests__banned_module_level_imports.snap | 45 +++++++++++-------- 4 files changed, 33 insertions(+), 50 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py b/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py index e5775cc8a046e..d0d3d14845f9a 100644 --- a/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py +++ b/crates/ruff/resources/test/fixtures/flake8_tidy_imports/TID253.py @@ -7,6 +7,9 @@ import torch as torch_wearing_a_trenchcoat +# this should count as module level +x = 1; import tensorflow + # banning a module also bans any submodules import torch.foo.bar diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 6c225c279544b..940eb78e8a578 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -576,9 +576,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_tidy_imports::rules::name_or_parent_is_banned_at_module_level( checker, &alias.name, - stmt, alias.range(), - checker.locator, ); } @@ -757,9 +755,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_tidy_imports::rules::name_or_parent_is_banned_at_module_level( checker, &module, - stmt, stmt.range(), - checker.locator, ); for alias in names { @@ -769,9 +765,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_tidy_imports::rules::name_is_banned_at_module_level( checker, &format!("{module}.{}", alias.name), - stmt, alias.range(), - checker.locator, ); } } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs index 76a0176e32085..1c04fb39d9255 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs @@ -1,8 +1,5 @@ -use ruff_python_ast::{Ranged, Stmt}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_source_file::Locator; use ruff_text_size::TextRange; use crate::checkers::ast::Checker; @@ -40,12 +37,10 @@ enum NameMatchPolicy { fn banned_at_module_level_with_policy( checker: &mut Checker, name: &str, - stmt: &Stmt, text_range: TextRange, - locator: &Locator, policy: &NameMatchPolicy, ) { - if !locator.is_at_start_of_line(stmt.start()) { + if !checker.semantic().at_top_level() { return; } let banned_module_level_imports = &checker @@ -75,34 +70,16 @@ fn banned_at_module_level_with_policy( pub(crate) fn name_is_banned_at_module_level( checker: &mut Checker, name: &str, - stmt: &Stmt, text_range: TextRange, - locator: &Locator, ) { - banned_at_module_level_with_policy( - checker, - name, - stmt, - text_range, - locator, - &NameMatchPolicy::ExactOnly, - ); + banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOnly); } /// TID253 pub(crate) fn name_or_parent_is_banned_at_module_level( checker: &mut Checker, name: &str, - stmt: &Stmt, text_range: TextRange, - locator: &Locator, ) { - banned_at_module_level_with_policy( - checker, - name, - stmt, - text_range, - locator, - &NameMatchPolicy::ExactOrParents, - ); + banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOrParents); } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap index 143f5d2dc714f..0c36701ed42e5 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap +++ b/crates/ruff/src/rules/flake8_tidy_imports/snapshots/ruff__rules__flake8_tidy_imports__tests__banned_module_level_imports.snap @@ -37,36 +37,45 @@ TID253.py:8:8: TID253 `torch` is banned at module level. Please move the import 8 | import torch as torch_wearing_a_trenchcoat | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 9 | -10 | # banning a module also bans any submodules +10 | # this should count as module level | -TID253.py:11:8: TID253 `torch` is banned at module level. Please move the import inline. +TID253.py:11:15: TID253 `tensorflow` is banned at module level. Please move the import inline. | -10 | # banning a module also bans any submodules -11 | import torch.foo.bar - | ^^^^^^^^^^^^^ TID253 +10 | # this should count as module level +11 | x = 1; import tensorflow + | ^^^^^^^^^^ TID253 12 | -13 | from tensorflow.foo import bar +13 | # banning a module also bans any submodules | -TID253.py:13:1: TID253 `tensorflow` is banned at module level. Please move the import inline. +TID253.py:14:8: TID253 `torch` is banned at module level. Please move the import inline. | -11 | import torch.foo.bar -12 | -13 | from tensorflow.foo import bar +13 | # banning a module also bans any submodules +14 | import torch.foo.bar + | ^^^^^^^^^^^^^ TID253 +15 | +16 | from tensorflow.foo import bar + | + +TID253.py:16:1: TID253 `tensorflow` is banned at module level. Please move the import inline. + | +14 | import torch.foo.bar +15 | +16 | from tensorflow.foo import bar | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 -14 | -15 | from torch.foo.bar import * +17 | +18 | from torch.foo.bar import * | -TID253.py:15:1: TID253 `torch` is banned at module level. Please move the import inline. +TID253.py:18:1: TID253 `torch` is banned at module level. Please move the import inline. | -13 | from tensorflow.foo import bar -14 | -15 | from torch.foo.bar import * +16 | from tensorflow.foo import bar +17 | +18 | from torch.foo.bar import * | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ TID253 -16 | -17 | # unlike TID251, inline imports are *not* banned +19 | +20 | # unlike TID251, inline imports are *not* banned | From 17d5c9a3aaa30b139b6de4fcea7fc14a28d58a29 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 12 Aug 2023 14:23:51 -0400 Subject: [PATCH 5/5] Tweak docs --- .../src/rules/flake8_tidy_imports/options.rs | 10 ++-- .../rules/banned_module_level_imports.rs | 59 +++++++++++-------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/crates/ruff/src/rules/flake8_tidy_imports/options.rs b/crates/ruff/src/rules/flake8_tidy_imports/options.rs index f2a90be5d85f6..56ac642ecdac5 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/options.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/options.rs @@ -45,13 +45,15 @@ pub struct Options { default = r#"[]"#, value_type = r#"list[str]"#, example = r#" - # Ban certain modules from being imported at module level. - # This does not ban these modules from being imported inline. + # Ban certain modules from being imported at module level, + # as opposed to importing them lazily (e.g., within a function + # definition). banned-module-level-imports = ["torch", "tensorflow"] "# )] - /// List of specific modules that can't be imported at module level. This does not ban these - /// modules from being imported inline. + /// List of specific modules that may not be imported at module level, and should instead be + /// imported conditionally (e.g., within a function definition, or an `if TYPE_CHECKING:` + /// block). pub banned_module_level_imports: Option>, } diff --git a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs index 1c04fb39d9255..0145a0ac9a9bb 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/rules/banned_module_level_imports.rs @@ -5,14 +5,19 @@ use ruff_text_size::TextRange; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for banned imports at module level. The banned imports are allowed inline, such as -/// within a function definition or an `if TYPE_CHECKING:` block. +/// Checks for module-level imports that should instead be imported within +/// a nested block (e.g., within a function definition). /// /// ## Why is this bad? -/// Some modules take a relatively long time to import, such as `torch` or `tensorflow`. Library -/// authors might want to ensure that you only pay the import cost for these modules if you -/// directly use them, rather than if you import a module that happens to use an expensive module -/// in one of its functions. +/// Some modules are expensive to import. For example, importing `torch` or +/// `tensorflow` can introduce a noticeable delay in the startup time of a +/// Python program. +/// +/// In some cases, you may want to import a module only if it is used in a +/// specific function, rather than importing it unconditionally. In this case, +/// you can import the module within a function definition or conditional +/// block, such as an `if TYPE_CHECKING` block, such that the module is only +/// imported if it is used.. /// /// ## Options /// - `flake8-tidy-imports.banned-module-level-imports` @@ -25,12 +30,34 @@ impl Violation for BannedModuleLevelImports { #[derive_message_formats] fn message(&self) -> String { let BannedModuleLevelImports { name } = self; - format!("`{name}` is banned at module level. Please move the import inline.") + format!("`{name}` is banned at the module level") } } +/// TID253 +pub(crate) fn name_is_banned_at_module_level( + checker: &mut Checker, + name: &str, + text_range: TextRange, +) { + banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOnly); +} + +/// TID253 +pub(crate) fn name_or_parent_is_banned_at_module_level( + checker: &mut Checker, + name: &str, + text_range: TextRange, +) { + banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOrParents); +} + +#[derive(Debug)] enum NameMatchPolicy { + /// Only match an exact module name (e.g., given `import foo.bar`, only match `foo.bar`). ExactOnly, + /// Match an exact module name or any of its parents (e.g., given `import foo.bar`, match + /// `foo.bar` or `foo`). ExactOrParents, } @@ -65,21 +92,3 @@ fn banned_at_module_level_with_policy( } } } - -/// TID253 -pub(crate) fn name_is_banned_at_module_level( - checker: &mut Checker, - name: &str, - text_range: TextRange, -) { - banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOnly); -} - -/// TID253 -pub(crate) fn name_or_parent_is_banned_at_module_level( - checker: &mut Checker, - name: &str, - text_range: TextRange, -) { - banned_at_module_level_with_policy(checker, name, text_range, &NameMatchPolicy::ExactOrParents); -}