diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 8e41b59f30c52..ff33a98effafc 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -3,9 +3,9 @@ #![cfg(not(target_family = "wasm"))] use regex::escape; -use std::fs; use std::process::Command; use std::str; +use std::{fs, path::Path}; use anyhow::Result; use assert_fs::fixture::{ChildPath, FileTouch, PathChild}; @@ -2236,3 +2236,114 @@ def func(t: _T) -> _T: " ); } + +#[test] +fn a005_module_shadowing_strict() -> Result<()> { + fn create_module(path: &Path) -> Result<()> { + fs::create_dir(path)?; + fs::File::create(path.join("__init__.py"))?; + Ok(()) + } + // construct a directory tree with this structure: + // . + // ├── abc + // │   └── __init__.py + // ├── collections + // │   ├── __init__.py + // │   ├── abc + // │   │   └── __init__.py + // │   └── foobar + // │   └── __init__.py + // ├── foobar + // │   ├── __init__.py + // │   ├── abc + // │   │   └── __init__.py + // │   └── collections + // │   ├── __init__.py + // │   ├── abc + // │   │   └── __init__.py + // │   └── foobar + // │   └── __init__.py + // ├── ruff.toml + // └── urlparse + // └── __init__.py + + let tempdir = TempDir::new()?; + let foobar = tempdir.path().join("foobar"); + create_module(&foobar)?; + for base in [&tempdir.path().into(), &foobar] { + for dir in ["abc", "collections"] { + create_module(&base.join(dir))?; + } + create_module(&base.join("collections").join("abc"))?; + create_module(&base.join("collections").join("foobar"))?; + } + create_module(&tempdir.path().join("urlparse"))?; + // also create a ruff.toml to mark the project root + fs::File::create(tempdir.path().join("ruff.toml"))?; + + insta::with_settings!({ + filters => vec![(r"\\", "/")] + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(["--select", "A005"]) + .current_dir(tempdir.path()), + @r" + success: false + exit_code: 1 + ----- stdout ----- + abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + foobar/collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + Found 6 errors. + + ----- stderr ----- + "); + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(["--select", "A005"]) + .current_dir(tempdir.path()), + @r" + success: false + exit_code: 1 + ----- stdout ----- + abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + foobar/collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + Found 6 errors. + + ----- stderr ----- + "); + + // TODO(brent) Default should currently match the strict version, but after the next minor + // release it will match the non-strict version directly above + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(["--select", "A005"]) + .current_dir(tempdir.path()), + @r" + success: false + exit_code: 1 + ----- stdout ----- + abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + foobar/collections/__init__.py:1:1: A005 Module `collections` shadows a Python standard-library module + foobar/collections/abc/__init__.py:1:1: A005 Module `abc` shadows a Python standard-library module + Found 6 errors. + + ----- stderr ----- + "); + }); + + Ok(()) +} diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/__init__.py b/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/logging.py b/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/logging.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/crates/ruff_linter/src/checkers/filesystem.rs b/crates/ruff_linter/src/checkers/filesystem.rs index b9d2ecb3f4549..98ee3b80c3912 100644 --- a/crates/ruff_linter/src/checkers/filesystem.rs +++ b/crates/ruff_linter/src/checkers/filesystem.rs @@ -46,12 +46,7 @@ pub(crate) fn check_file_path( // flake8-builtins if settings.rules.enabled(Rule::StdlibModuleShadowing) { - if let Some(diagnostic) = stdlib_module_shadowing( - path, - package, - &settings.flake8_builtins.builtins_allowed_modules, - settings.target_version, - ) { + if let Some(diagnostic) = stdlib_module_shadowing(path, settings) { diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/rules/flake8_builtins/mod.rs b/crates/ruff_linter/src/rules/flake8_builtins/mod.rs index 08ffa9c1d7049..1aaa477f3000d 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/mod.rs @@ -14,7 +14,7 @@ mod tests { use crate::registry::Rule; use crate::settings::types::PythonVersion; use crate::settings::LinterSettings; - use crate::test::test_path; + use crate::test::{test_path, test_resource_path}; #[test_case(Rule::BuiltinVariableShadowing, Path::new("A001.py"))] #[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))] @@ -56,6 +56,69 @@ mod tests { Ok(()) } + #[test_case( + Rule::StdlibModuleShadowing, + Path::new("A005/modules/utils/logging.py"), + true + )] + #[test_case( + Rule::StdlibModuleShadowing, + Path::new("A005/modules/utils/logging.py"), + false + )] + fn non_strict_checking(rule_code: Rule, path: &Path, strict: bool) -> Result<()> { + let snapshot = format!( + "{}_{}_{strict}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("flake8_builtins").join(path).as_path(), + &LinterSettings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + /// Test that even with strict checking disabled, a module in `src` will trigger A005 + #[test_case( + Rule::StdlibModuleShadowing, + Path::new("A005/modules/utils/logging.py") + )] + fn non_strict_checking_src(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}_src", rule_code.noqa_code(), path.to_string_lossy()); + let src = Path::new("fixtures/flake8_builtins"); + let diagnostics = test_path( + Path::new("flake8_builtins").join(path).as_path(), + &LinterSettings { + src: vec![test_resource_path(src.join(path.parent().unwrap()))], + ..LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + + /// Test that even with strict checking disabled, a module in the `project_root` will trigger + /// A005 + #[test_case( + Rule::StdlibModuleShadowing, + Path::new("A005/modules/utils/logging.py") + )] + fn non_strict_checking_root(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}_root", rule_code.noqa_code(), path.to_string_lossy()); + let src = Path::new("fixtures/flake8_builtins"); + let diagnostics = test_path( + Path::new("flake8_builtins").join(path).as_path(), + &LinterSettings { + project_root: test_resource_path(src.join(path.parent().unwrap())), + ..LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Rule::BuiltinVariableShadowing, Path::new("A001.py"))] #[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))] #[test_case(Rule::BuiltinAttributeShadowing, Path::new("A003.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs b/crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs index 9225637e8d2d1..0be13af864385 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs @@ -1,4 +1,5 @@ -use std::path::Path; +use std::borrow::Cow; +use std::path::{Component, Path, PathBuf}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -7,8 +8,7 @@ use ruff_python_stdlib::path::is_module_file; use ruff_python_stdlib::sys::is_known_standard_library; use ruff_text_size::TextRange; -use crate::package::PackageRoot; -use crate::settings::types::PythonVersion; +use crate::settings::LinterSettings; /// ## What it does /// Checks for modules that use the same names as Python standard-library @@ -58,37 +58,38 @@ impl Violation for StdlibModuleShadowing { /// A005 pub(crate) fn stdlib_module_shadowing( - path: &Path, - package: Option>, - allowed_modules: &[String], - target_version: PythonVersion, + mut path: &Path, + settings: &LinterSettings, ) -> Option { if !PySourceType::try_from_path(path).is_some_and(PySourceType::is_py_file) { return None; } - let package = package?; + // strip src and root prefixes before converting to a fully-qualified module path + let prefix = get_prefix(settings, path); + if let Some(Ok(new_path)) = prefix.map(|p| path.strip_prefix(p)) { + path = new_path; + } - let module_name = if is_module_file(path) { - package.path().file_name().unwrap().to_string_lossy() + // for modules like `modname/__init__.py`, use the parent directory name, otherwise just trim + // the `.py` extension + let path = if is_module_file(path) { + Cow::from(path.parent()?) } else { - path.file_stem().unwrap().to_string_lossy() + Cow::from(path.with_extension("")) }; - if !is_known_standard_library(target_version.minor(), &module_name) { - return None; - } + // convert a filesystem path like `foobar/collections/abc` to a reversed sequence of modules + // like `["abc", "collections", "foobar"]`, stripping anything that's not a normal component + let mut components = path + .components() + .filter(|c| matches!(c, Component::Normal(_))) + .map(|c| c.as_os_str().to_string_lossy()) + .rev(); - // Shadowing private stdlib modules is okay. - // https://github.com/astral-sh/ruff/issues/12949 - if module_name.starts_with('_') && !module_name.starts_with("__") { - return None; - } + let module_name = components.next()?; - if allowed_modules - .iter() - .any(|allowed_module| allowed_module == &module_name) - { + if is_allowed_module(settings, &module_name) { return None; } @@ -99,3 +100,36 @@ pub(crate) fn stdlib_module_shadowing( TextRange::default(), )) } + +/// Return the longest prefix of `path` between `settings.src` and `settings.project_root`. +fn get_prefix<'a>(settings: &'a LinterSettings, path: &Path) -> Option<&'a PathBuf> { + let mut prefix = None; + for dir in settings.src.iter().chain([&settings.project_root]) { + if path.starts_with(dir) + // TODO `is_none_or` when MSRV >= 1.82 + && (prefix.is_none() || prefix.is_some_and(|existing| existing < dir)) + { + prefix = Some(dir); + } + } + prefix +} + +fn is_allowed_module(settings: &LinterSettings, module: &str) -> bool { + // Shadowing private stdlib modules is okay. + // https://github.com/astral-sh/ruff/issues/12949 + if module.starts_with('_') && !module.starts_with("__") { + return true; + } + + if settings + .flake8_builtins + .builtins_allowed_modules + .iter() + .any(|allowed_module| allowed_module == module) + { + return true; + } + + !is_known_standard_library(settings.target_version.minor(), module) +} diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_false.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_false.snap new file mode 100644 index 0000000000000..2154e514668f1 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_false.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs +--- +logging.py:1:1: A005 Module `logging` shadows a Python standard-library module diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_root.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_root.snap new file mode 100644 index 0000000000000..2154e514668f1 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_root.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs +--- +logging.py:1:1: A005 Module `logging` shadows a Python standard-library module diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_src.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_src.snap new file mode 100644 index 0000000000000..2154e514668f1 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_src.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs +--- +logging.py:1:1: A005 Module `logging` shadows a Python standard-library module diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_true.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_true.snap new file mode 100644 index 0000000000000..2154e514668f1 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A005_A005__modules__utils__logging.py_true.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs +--- +logging.py:1:1: A005 Module `logging` shadows a Python standard-library module