Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-builtins] Make strict module name comparison optional (A005) #15951

Merged
merged 15 commits into from
Feb 10, 2025
100 changes: 67 additions & 33 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2272,38 +2272,36 @@ class Foo[_T, __T]:
);
}

#[test]
fn a005_module_shadowing_strict() -> Result<()> {
/// 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
fn create_a005_module_structure(tempdir: &TempDir) -> 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] {
Expand All @@ -2317,11 +2315,22 @@ fn a005_module_shadowing_strict() -> Result<()> {
// also create a ruff.toml to mark the project root
fs::File::create(tempdir.path().join("ruff.toml"))?;

Ok(())
}

/// Test A005 with `builtins-strict-checking = true`
#[test]
fn a005_module_shadowing_strict() -> Result<()> {
let tempdir = TempDir::new()?;
create_a005_module_structure(&tempdir)?;

insta::with_settings!({
filters => vec![(r"\\", "/")]
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.arg("--config")
.arg(r#"lint.flake8-builtins.builtins-strict-checking = true"#)
.args(["--select", "A005"])
.current_dir(tempdir.path()),
@r"
Expand All @@ -2338,9 +2347,24 @@ fn a005_module_shadowing_strict() -> Result<()> {

----- stderr -----
");
});

Ok(())
}

/// Test A005 with `builtins-strict-checking = false`
#[test]
fn a005_module_shadowing_non_strict() -> Result<()> {
let tempdir = TempDir::new()?;
create_a005_module_structure(&tempdir)?;

insta::with_settings!({
filters => vec![(r"\\", "/")]
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.arg("--config")
.arg(r#"lint.flake8-builtins.builtins-strict-checking = false"#)
.args(["--select", "A005"])
.current_dir(tempdir.path()),
@r"
Expand All @@ -2349,17 +2373,27 @@ fn a005_module_shadowing_strict() -> Result<()> {
----- 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.
Found 2 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
});

Ok(())
}

/// Test A005 with `builtins-strict-checking` unset
/// TODO(brent) This should currently match the strict version, but after the next minor
/// release it will match the non-strict version directly above
#[test]
fn a005_module_shadowing_strict_default() -> Result<()> {
let tempdir = TempDir::new()?;
create_a005_module_structure(&tempdir)?;

insta::with_settings!({
filters => vec![(r"\\", "/")]
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--select", "A005"])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ linter.flake8_bandit.check_typed_exception = false
linter.flake8_bugbear.extend_immutable_calls = []
linter.flake8_builtins.builtins_allowed_modules = []
linter.flake8_builtins.builtins_ignorelist = []
linter.flake8_builtins.builtins_strict_checking = true
linter.flake8_comprehensions.allow_dict_calls_with_keyword_arguments = false
linter.flake8_copyright.notice_rgx = (?i)Copyright\s+((?:\(C\)|©)\s+)?\d{4}((-|,\s)\d{4})*
linter.flake8_copyright.author = none
Expand Down
26 changes: 24 additions & 2 deletions crates/ruff_linter/src/rules/flake8_builtins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ mod tests {

use crate::assert_messages;
use crate::registry::Rule;
use crate::rules::flake8_builtins;
use crate::settings::types::PythonVersion;
use crate::settings::LinterSettings;
use crate::test::{test_path, test_resource_path};
Expand Down Expand Up @@ -50,7 +51,13 @@ mod tests {
let snapshot = format!("{}_{}", 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),
&LinterSettings {
flake8_builtins: flake8_builtins::settings::Settings {
builtins_strict_checking: true,
..Default::default()
},
..LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
Expand All @@ -74,7 +81,13 @@ mod tests {
);
let diagnostics = test_path(
Path::new("flake8_builtins").join(path).as_path(),
&LinterSettings::for_rule(rule_code),
&LinterSettings {
flake8_builtins: flake8_builtins::settings::Settings {
builtins_strict_checking: strict,
..Default::default()
},
..LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
Expand All @@ -92,6 +105,10 @@ mod tests {
Path::new("flake8_builtins").join(path).as_path(),
&LinterSettings {
src: vec![test_resource_path(src.join(path.parent().unwrap()))],
flake8_builtins: flake8_builtins::settings::Settings {
builtins_strict_checking: false,
..Default::default()
},
..LinterSettings::for_rule(rule_code)
},
)?;
Expand All @@ -112,6 +129,10 @@ mod tests {
Path::new("flake8_builtins").join(path).as_path(),
&LinterSettings {
project_root: test_resource_path(src.join(path.parent().unwrap())),
flake8_builtins: flake8_builtins::settings::Settings {
builtins_strict_checking: false,
..Default::default()
},
..LinterSettings::for_rule(rule_code)
},
)?;
Expand Down Expand Up @@ -179,6 +200,7 @@ mod tests {
&LinterSettings {
flake8_builtins: super::settings::Settings {
builtins_allowed_modules: vec!["xml".to_string(), "logging".to_string()],
builtins_strict_checking: true,
..Default::default()
},
..LinterSettings::for_rules(vec![rule_code])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ pub(crate) fn stdlib_module_shadowing(
return None;
}

// not allowed generally, but check for a parent in non-strict mode
if !settings.flake8_builtins.builtins_strict_checking && components.next().is_some() {
return None;
}

Some(Diagnostic::new(
StdlibModuleShadowing {
name: module_name.to_string(),
Expand Down
14 changes: 13 additions & 1 deletion crates/ruff_linter/src/rules/flake8_builtins/settings.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
//! Settings for the `flake8-builtins` plugin.

use crate::display_settings;
use crate::{display_settings, settings::types::PreviewMode};
use ruff_macros::CacheKey;
use std::fmt::{Display, Formatter};

#[derive(Debug, Clone, Default, CacheKey)]
pub struct Settings {
pub builtins_ignorelist: Vec<String>,
pub builtins_allowed_modules: Vec<String>,
pub builtins_strict_checking: bool,
}

impl Settings {
pub fn new(preview: PreviewMode) -> Self {
Self {
builtins_ignorelist: Vec::new(),
builtins_allowed_modules: Vec::new(),
builtins_strict_checking: preview.is_disabled(),
}
}
}

impl Display for Settings {
Expand All @@ -18,6 +29,7 @@ impl Display for Settings {
fields = [
self.builtins_allowed_modules | array,
self.builtins_ignorelist | array,
self.builtins_strict_checking,
]
}
Ok(())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +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

12 changes: 8 additions & 4 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,13 @@ impl Configuration {

let rules = lint.as_rule_table(lint_preview)?;

let flake8_builtins = lint
.flake8_builtins
.map(|builtins| builtins.into_settings(lint_preview))
.unwrap_or_else(|| {
ruff_linter::rules::flake8_builtins::settings::Settings::new(lint_preview)
});

// LinterSettings validation
let isort = lint
.isort
Expand Down Expand Up @@ -335,10 +342,7 @@ impl Configuration {
.flake8_bugbear
.map(Flake8BugbearOptions::into_settings)
.unwrap_or_default(),
flake8_builtins: lint
.flake8_builtins
.map(Flake8BuiltinsOptions::into_settings)
.unwrap_or_default(),
flake8_builtins,
flake8_comprehensions: lint
.flake8_comprehensions
.map(Flake8ComprehensionsOptions::into_settings)
Expand Down
18 changes: 16 additions & 2 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use ruff_linter::rules::{
pycodestyle, pydoclint, pydocstyle, pyflakes, pylint, pyupgrade, ruff,
};
use ruff_linter::settings::types::{
IdentifierPattern, OutputFormat, PythonVersion, RequiredVersion,
IdentifierPattern, OutputFormat, PreviewMode, PythonVersion, RequiredVersion,
};
use ruff_linter::{warn_user_once, RuleSelector};
use ruff_macros::{CombineOptions, OptionsMetadata};
Expand Down Expand Up @@ -1143,13 +1143,27 @@ pub struct Flake8BuiltinsOptions {
)]
/// List of builtin module names to allow.
pub builtins_allowed_modules: Option<Vec<String>>,
#[option(
default = r#"true"#,
value_type = "bool",
example = "builtins-strict-checking = false"
)]
/// Compare module names instead of full module paths.
pub builtins_strict_checking: Option<bool>,
}

impl Flake8BuiltinsOptions {
pub fn into_settings(self) -> ruff_linter::rules::flake8_builtins::settings::Settings {
pub fn into_settings(
self,
preview: PreviewMode,
) -> ruff_linter::rules::flake8_builtins::settings::Settings {
ruff_linter::rules::flake8_builtins::settings::Settings {
builtins_ignorelist: self.builtins_ignorelist.unwrap_or_default(),
builtins_allowed_modules: self.builtins_allowed_modules.unwrap_or_default(),
builtins_strict_checking: self
.builtins_strict_checking
// use the old default of true on non-preview
.unwrap_or(preview.is_disabled()),
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading