From 46197021bc2234ae69c44114b9a905e02b8cbddb Mon Sep 17 00:00:00 2001 From: mishamsk Date: Mon, 20 Jan 2025 23:03:43 -0500 Subject: [PATCH] honor banned top level imports by TID253 in PLC0415. Fixes #12803 --- .../import_outside_top_level_with_banned.py | 7 +++ .../src/checkers/ast/analyze/statement.rs | 47 ++++++++++++++++++- crates/ruff_linter/src/rules/pylint/mod.rs | 25 +++++++++- .../pylint/rules/import_outside_top_level.rs | 38 +++++++++++++-- ..._import_outside_top_level_with_banned.snap | 9 ++++ 5 files changed, 120 insertions(+), 6 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level_with_banned.py create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__import_outside_top_level_with_banned.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level_with_banned.py b/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level_with_banned.py new file mode 100644 index 00000000000000..6c6e41e7bb0dcc --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/import_outside_top_level_with_banned.py @@ -0,0 +1,7 @@ +def f(): + # this should be allowed due to TID253 top-level ban + import foo_banned + from pkg import bar_banned + + # this should still trigger an error due to multiple imports + from pkg import foo_allowed, bar_banned diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 5611a307f44df6..87b76f3e16b492 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -563,7 +563,21 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pycodestyle::rules::module_import_not_at_top_of_file(checker, stmt); } if checker.enabled(Rule::ImportOutsideTopLevel) { - pylint::rules::import_outside_top_level(checker, stmt); + pylint::rules::import_outside_top_level( + checker, + stmt, + None, + &names + .iter() + .map(|alias| { + flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( + flake8_tidy_imports::matchers::MatchNameOrParent { + module: &alias.name, + }, + ) + }) + .collect(), + ); } if checker.enabled(Rule::GlobalStatement) { for name in names { @@ -752,7 +766,36 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pycodestyle::rules::module_import_not_at_top_of_file(checker, stmt); } if checker.enabled(Rule::ImportOutsideTopLevel) { - pylint::rules::import_outside_top_level(checker, stmt); + if let Some(module) = helpers::resolve_imported_module_path( + level, + module, + checker.module.qualified_name(), + ) { + pylint::rules::import_outside_top_level( + checker, + stmt, + Some( + flake8_tidy_imports::matchers::NameMatchPolicy::MatchNameOrParent( + flake8_tidy_imports::matchers::MatchNameOrParent { + module: &module, + }, + ), + ), + &names + .iter() + .map(|alias| { + flake8_tidy_imports::matchers::NameMatchPolicy::MatchName( + flake8_tidy_imports::matchers::MatchName { + module: &module, + member: &alias.name, + }, + ) + }) + .collect(), + ); + } else { + pylint::rules::import_outside_top_level(checker, stmt, None, &Vec::new()); + } } if checker.enabled(Rule::GlobalStatement) { for name in names { diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index fa3a69a75ecf6b..d6d62720fdfb68 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -13,7 +13,7 @@ mod tests { use test_case::test_case; use crate::registry::Rule; - use crate::rules::pylint; + use crate::rules::{flake8_tidy_imports, pylint}; use crate::settings::types::{PreviewMode, PythonVersion}; use crate::settings::LinterSettings; @@ -412,6 +412,29 @@ mod tests { Ok(()) } + #[test] + fn import_outside_top_level_with_banned() -> Result<()> { + let diagnostics = test_path( + Path::new("pylint/import_outside_top_level_with_banned.py"), + &LinterSettings { + preview: PreviewMode::Enabled, + flake8_tidy_imports: flake8_tidy_imports::settings::Settings { + banned_module_level_imports: vec![ + "foo_banned".to_string(), + "pkg.bar_banned".to_string(), + ], + ..Default::default() + }, + ..LinterSettings::for_rules(vec![ + Rule::BannedModuleLevelImports, + Rule::ImportOutsideTopLevel, + ]) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } + #[test_case( Rule::RepeatedEqualityComparison, Path::new("repeated_equality_comparison.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs b/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs index 91a1a81bc96b5b..9fb17000fbc640 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs @@ -3,7 +3,9 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::Stmt; use ruff_text_size::Ranged; -use crate::checkers::ast::Checker; +use crate::{ + checkers::ast::Checker, codes::Rule, rules::flake8_tidy_imports::matchers::NameMatchPolicy, +}; /// ## What it does /// Checks for `import` statements outside of a module's top-level scope, such @@ -53,8 +55,38 @@ impl Violation for ImportOutsideTopLevel { } /// C0415 -pub(crate) fn import_outside_top_level(checker: &mut Checker, stmt: &Stmt) { - if !checker.semantic().current_scope().kind.is_module() { +pub(crate) fn import_outside_top_level( + checker: &mut Checker, + stmt: &Stmt, + pkg: Option, + modules: &Vec, +) { + if !checker.semantic().current_scope().kind.is_module() + && (!checker.enabled(Rule::BannedModuleLevelImports) + || (pkg.is_some_and(|policy| { + policy + .find( + checker + .settings + .flake8_tidy_imports + .banned_module_level_imports + .iter() + .map(AsRef::as_ref), + ) + .is_none() + }) && modules.iter().any(|policy| { + policy + .find( + checker + .settings + .flake8_tidy_imports + .banned_module_level_imports + .iter() + .map(AsRef::as_ref), + ) + .is_none() + }))) + { checker .diagnostics .push(Diagnostic::new(ImportOutsideTopLevel, stmt.range())); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__import_outside_top_level_with_banned.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__import_outside_top_level_with_banned.snap new file mode 100644 index 00000000000000..7940549c8abfa7 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__import_outside_top_level_with_banned.snap @@ -0,0 +1,9 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +import_outside_top_level_with_banned.py:7:5: PLC0415 `import` should be at the top-level of a file + | +6 | # this should still trigger an error due to multiple imports +7 | from pkg import foo_allowed, bar_banned + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0415 + |