mirror of https://github.com/astral-sh/ruff
[`flake8-builtins`] Match upstream module name comparison (`A005`) (#16006)
See #15951 for the original discussion and reviews. This is just the first half of that PR (reaching parity with `flake8-builtins` without adding any new configuration options) split out for nicer changelog entries. For posterity, here's a script for generating the module structure that was useful for interactive testing and creating the table [here](https://github.com/astral-sh/ruff/pull/15951#issuecomment-2640662041). The results for this branch are the same as the `Strict` column there, as expected. ```shell mkdir abc collections foobar urlparse for i in */ do touch $i/__init__.py done cp -r abc foobar collections/. cp -r abc collections foobar/. touch ruff.toml touch foobar/logging.py ``` --------- Co-authored-by: Micha Reiser <micha@reiser.io>
This commit is contained in:
parent
efa8a3ddcc
commit
d4a5772d96
|
|
@ -3,9 +3,9 @@
|
||||||
#![cfg(not(target_family = "wasm"))]
|
#![cfg(not(target_family = "wasm"))]
|
||||||
|
|
||||||
use regex::escape;
|
use regex::escape;
|
||||||
use std::fs;
|
|
||||||
use std::process::Command;
|
use std::process::Command;
|
||||||
use std::str;
|
use std::str;
|
||||||
|
use std::{fs, path::Path};
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
use assert_fs::fixture::{ChildPath, FileTouch, PathChild};
|
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(())
|
||||||
|
}
|
||||||
|
|
|
||||||
0
crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/__init__.py
vendored
Normal file
0
crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/__init__.py
vendored
Normal file
0
crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/logging.py
vendored
Normal file
0
crates/ruff_linter/resources/test/fixtures/flake8_builtins/A005/modules/utils/logging.py
vendored
Normal file
|
|
@ -46,12 +46,7 @@ pub(crate) fn check_file_path(
|
||||||
|
|
||||||
// flake8-builtins
|
// flake8-builtins
|
||||||
if settings.rules.enabled(Rule::StdlibModuleShadowing) {
|
if settings.rules.enabled(Rule::StdlibModuleShadowing) {
|
||||||
if let Some(diagnostic) = stdlib_module_shadowing(
|
if let Some(diagnostic) = stdlib_module_shadowing(path, settings) {
|
||||||
path,
|
|
||||||
package,
|
|
||||||
&settings.flake8_builtins.builtins_allowed_modules,
|
|
||||||
settings.target_version,
|
|
||||||
) {
|
|
||||||
diagnostics.push(diagnostic);
|
diagnostics.push(diagnostic);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -14,7 +14,7 @@ mod tests {
|
||||||
use crate::registry::Rule;
|
use crate::registry::Rule;
|
||||||
use crate::settings::types::PythonVersion;
|
use crate::settings::types::PythonVersion;
|
||||||
use crate::settings::LinterSettings;
|
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::BuiltinVariableShadowing, Path::new("A001.py"))]
|
||||||
#[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))]
|
#[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))]
|
||||||
|
|
@ -56,6 +56,69 @@ mod tests {
|
||||||
Ok(())
|
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::BuiltinVariableShadowing, Path::new("A001.py"))]
|
||||||
#[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))]
|
#[test_case(Rule::BuiltinArgumentShadowing, Path::new("A002.py"))]
|
||||||
#[test_case(Rule::BuiltinAttributeShadowing, Path::new("A003.py"))]
|
#[test_case(Rule::BuiltinAttributeShadowing, Path::new("A003.py"))]
|
||||||
|
|
|
||||||
|
|
@ -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_diagnostics::{Diagnostic, Violation};
|
||||||
use ruff_macros::{derive_message_formats, ViolationMetadata};
|
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_python_stdlib::sys::is_known_standard_library;
|
||||||
use ruff_text_size::TextRange;
|
use ruff_text_size::TextRange;
|
||||||
|
|
||||||
use crate::package::PackageRoot;
|
use crate::settings::LinterSettings;
|
||||||
use crate::settings::types::PythonVersion;
|
|
||||||
|
|
||||||
/// ## What it does
|
/// ## What it does
|
||||||
/// Checks for modules that use the same names as Python standard-library
|
/// Checks for modules that use the same names as Python standard-library
|
||||||
|
|
@ -58,37 +58,38 @@ impl Violation for StdlibModuleShadowing {
|
||||||
|
|
||||||
/// A005
|
/// A005
|
||||||
pub(crate) fn stdlib_module_shadowing(
|
pub(crate) fn stdlib_module_shadowing(
|
||||||
path: &Path,
|
mut path: &Path,
|
||||||
package: Option<PackageRoot<'_>>,
|
settings: &LinterSettings,
|
||||||
allowed_modules: &[String],
|
|
||||||
target_version: PythonVersion,
|
|
||||||
) -> Option<Diagnostic> {
|
) -> Option<Diagnostic> {
|
||||||
if !PySourceType::try_from_path(path).is_some_and(PySourceType::is_py_file) {
|
if !PySourceType::try_from_path(path).is_some_and(PySourceType::is_py_file) {
|
||||||
return None;
|
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) {
|
// for modules like `modname/__init__.py`, use the parent directory name, otherwise just trim
|
||||||
package.path().file_name().unwrap().to_string_lossy()
|
// the `.py` extension
|
||||||
|
let path = if is_module_file(path) {
|
||||||
|
Cow::from(path.parent()?)
|
||||||
} else {
|
} else {
|
||||||
path.file_stem().unwrap().to_string_lossy()
|
Cow::from(path.with_extension(""))
|
||||||
};
|
};
|
||||||
|
|
||||||
if !is_known_standard_library(target_version.minor(), &module_name) {
|
// convert a filesystem path like `foobar/collections/abc` to a reversed sequence of modules
|
||||||
return None;
|
// 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.
|
let module_name = components.next()?;
|
||||||
// https://github.com/astral-sh/ruff/issues/12949
|
|
||||||
if module_name.starts_with('_') && !module_name.starts_with("__") {
|
|
||||||
return None;
|
|
||||||
}
|
|
||||||
|
|
||||||
if allowed_modules
|
if is_allowed_module(settings, &module_name) {
|
||||||
.iter()
|
|
||||||
.any(|allowed_module| allowed_module == &module_name)
|
|
||||||
{
|
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -99,3 +100,36 @@ pub(crate) fn stdlib_module_shadowing(
|
||||||
TextRange::default(),
|
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)
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -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
|
||||||
|
|
@ -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
|
||||||
|
|
@ -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
|
||||||
|
|
@ -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
|
||||||
Loading…
Reference in New Issue