From 88b543d73a3fb6adaeefcbdc7cf119905ebd2f82 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Sun, 9 Feb 2025 19:33:03 -0500 Subject: [PATCH] [`flake8-builtins`] Make strict module name comparison optional (`A005`) (#15951) ## Summary This PR adds the configuration option `lint.flake8-builtins.builtins-strict-checking`, which is used in A005 to determine whether the fully-qualified module name (relative to the project root or source directories) should be checked instead of just the final component as is currently the case. As discussed in https://github.com/astral-sh/ruff/issues/15399#issuecomment-2587017147, the default value of the new option is `false` on preview, so modules like `utils.logging` from the initial report are no longer flagged by default. For non-preview the default is still strict checking. ## Test Plan New A005 test module with the structure reported in #15399. Fixes #15399 --- crates/ruff/tests/lint.rs | 166 +++++++++++------- ...ow_settings__display_default_settings.snap | 1 + .../src/rules/flake8_builtins/mod.rs | 26 ++- .../rules/stdlib_module_shadowing.rs | 5 + .../src/rules/flake8_builtins/settings.rs | 14 +- ...005__modules__utils__logging.py_false.snap | 2 +- crates/ruff_workspace/src/configuration.rs | 12 +- crates/ruff_workspace/src/options.rs | 18 +- ruff.schema.json | 7 + 9 files changed, 175 insertions(+), 76 deletions(-) diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 1b9f231ffb..f9c819a5c4 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -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] { @@ -2317,6 +2315,82 @@ 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" + 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(()) +} + +/// 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" + 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 + Found 2 errors. + + ----- stderr ----- + "); + + }); + + 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"\\", "/")] }, { @@ -2338,46 +2412,6 @@ fn a005_module_shadowing_strict() -> Result<()> { ----- 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/tests/snapshots/show_settings__display_default_settings.snap b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap index 06ace66e18..b44e9e23c5 100644 --- a/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap +++ b/crates/ruff/tests/snapshots/show_settings__display_default_settings.snap @@ -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 diff --git a/crates/ruff_linter/src/rules/flake8_builtins/mod.rs b/crates/ruff_linter/src/rules/flake8_builtins/mod.rs index 1aaa477f30..983df23b47 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/mod.rs @@ -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}; @@ -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(()) @@ -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(()) @@ -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) }, )?; @@ -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) }, )?; @@ -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]) 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 0be13af864..64bf8691c7 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 @@ -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(), diff --git a/crates/ruff_linter/src/rules/flake8_builtins/settings.rs b/crates/ruff_linter/src/rules/flake8_builtins/settings.rs index cfb5573ee0..d22c671769 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/settings.rs @@ -1,6 +1,6 @@ //! 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}; @@ -8,6 +8,17 @@ use std::fmt::{Display, Formatter}; pub struct Settings { pub builtins_ignorelist: Vec, pub builtins_allowed_modules: Vec, + 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 { @@ -18,6 +29,7 @@ impl Display for Settings { fields = [ self.builtins_allowed_modules | array, self.builtins_ignorelist | array, + self.builtins_strict_checking, ] } Ok(()) 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 index 2154e51466..df35fcb66a 100644 --- 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 @@ -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 + diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 66e3015e6a..b27a877bae 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -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 @@ -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) diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 058e3dbbe9..95851353a2 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -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}; @@ -1143,13 +1143,27 @@ pub struct Flake8BuiltinsOptions { )] /// List of builtin module names to allow. pub builtins_allowed_modules: Option>, + #[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, } 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()), } } } diff --git a/ruff.schema.json b/ruff.schema.json index 92e5311a5f..6ee9cbc780 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1024,6 +1024,13 @@ "items": { "type": "string" } + }, + "builtins-strict-checking": { + "description": "Compare module names instead of full module paths.", + "type": [ + "boolean", + "null" + ] } }, "additionalProperties": false