[flake8-builtins] Match upstream module name comparison (A005)

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(())
+}

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);
         }
     }

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"))]

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<PackageRoot<'_>>,
-    allowed_modules: &[String],
-    target_version: PythonVersion,
+    mut path: &Path,
+    settings: &LinterSettings,
 ) -> Option<Diagnostic> {
     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)
+}

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

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

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

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