From 693f957b90a8ae395d5c898870aa190b9d0acaac Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 19 Oct 2023 00:48:27 -0400 Subject: [PATCH] [pylint] - implement `global-at-module-level` (`W0604`) (#8058) ## Summary Implements [`global-at-module-level`/`W0604`](https://pylint.pycqa.org/en/latest/user_guide/messages/warning/global-at-module-level.html) See #970 ## Test Plan `cargo test` and manually --- .../fixtures/pylint/global_at_module_level.py | 10 ++++++ .../src/checkers/ast/analyze/statement.rs | 3 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../pylint/rules/global_at_module_level.rs | 34 +++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 ++ ...ts__PLW0604_global_at_module_level.py.snap | 21 ++++++++++++ ruff.schema.json | 1 + 8 files changed, 73 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/global_at_module_level.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/global_at_module_level.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0604_global_at_module_level.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/global_at_module_level.py b/crates/ruff_linter/resources/test/fixtures/pylint/global_at_module_level.py new file mode 100644 index 0000000000000..83ca8a93911fb --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/global_at_module_level.py @@ -0,0 +1,10 @@ +global price # W0604 + +price = 25 + +if True: + global X # W0604 + +def no_error(): + global price + price = 30 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index bf5313b981e00..b51301de44e70 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -21,6 +21,9 @@ use crate::settings::types::PythonVersion; pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { match stmt { Stmt::Global(ast::StmtGlobal { names, range: _ }) => { + if checker.enabled(Rule::GlobalAtModuleLevel) { + pylint::rules::global_at_module_level(checker, stmt); + } if checker.enabled(Rule::AmbiguousVariableName) { checker.diagnostics.extend(names.iter().filter_map(|name| { pycodestyle::rules::ambiguous_variable_name(name, name.range()) diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 6835e7e725a98..e5360cabfae85 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -264,6 +264,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0131") => (RuleGroup::Stable, rules::pylint::rules::NamedExprWithoutContext), (Pylint, "W0406") => (RuleGroup::Stable, rules::pylint::rules::ImportSelf), (Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned), + (Pylint, "W0604") => (RuleGroup::Preview, rules::pylint::rules::GlobalAtModuleLevel), (Pylint, "W0603") => (RuleGroup::Stable, rules::pylint::rules::GlobalStatement), (Pylint, "W0711") => (RuleGroup::Stable, rules::pylint::rules::BinaryOpException), (Pylint, "W1508") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarDefault), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index d17a662cbdb4f..d1807b2695d11 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -138,6 +138,7 @@ mod tests { #[test_case(Rule::NoSelfUse, Path::new("no_self_use.py"))] #[test_case(Rule::MisplacedBareRaise, Path::new("misplaced_bare_raise.py"))] #[test_case(Rule::LiteralMembership, Path::new("literal_membership.py"))] + #[test_case(Rule::GlobalAtModuleLevel, Path::new("global_at_module_level.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/global_at_module_level.rs b/crates/ruff_linter/src/rules/pylint/rules/global_at_module_level.rs new file mode 100644 index 0000000000000..89d7cca946c45 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/global_at_module_level.rs @@ -0,0 +1,34 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::Stmt; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of the `global` keyword at the module level. +/// +/// ## Why is this bad? +/// The `global` keyword is used within functions to indicate that a name +/// refers to a global variable, rather than a local variable. +/// +/// At the module level, all names are global by default, so the `global` +/// keyword is redundant. +#[violation] +pub struct GlobalAtModuleLevel; + +impl Violation for GlobalAtModuleLevel { + #[derive_message_formats] + fn message(&self) -> String { + format!("`global` at module level is redundant") + } +} + +/// PLW0604 +pub(crate) fn global_at_module_level(checker: &mut Checker, stmt: &Stmt) { + if checker.semantic().current_scope().kind.is_module() { + checker + .diagnostics + .push(Diagnostic::new(GlobalAtModuleLevel, stmt.range())); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index c4d7bd831b8ed..30ede8337a5ab 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -14,6 +14,7 @@ pub(crate) use comparison_with_itself::*; pub(crate) use continue_in_finally::*; pub(crate) use duplicate_bases::*; pub(crate) use eq_without_hash::*; +pub(crate) use global_at_module_level::*; pub(crate) use global_statement::*; pub(crate) use global_variable_not_assigned::*; pub(crate) use import_self::*; @@ -78,6 +79,7 @@ mod comparison_with_itself; mod continue_in_finally; mod duplicate_bases; mod eq_without_hash; +mod global_at_module_level; mod global_statement; mod global_variable_not_assigned; mod import_self; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0604_global_at_module_level.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0604_global_at_module_level.py.snap new file mode 100644 index 0000000000000..75c9b9db2aa9a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0604_global_at_module_level.py.snap @@ -0,0 +1,21 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +global_at_module_level.py:1:1: PLW0604 `global` at module level is redundant + | +1 | global price # W0604 + | ^^^^^^^^^^^^ PLW0604 +2 | +3 | price = 25 + | + +global_at_module_level.py:6:5: PLW0604 `global` at module level is redundant + | +5 | if True: +6 | global X # W0604 + | ^^^^^^^^ PLW0604 +7 | +8 | def no_error(): + | + + diff --git a/ruff.schema.json b/ruff.schema.json index e4ad0ae65cd86..e7e12d3756298 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3071,6 +3071,7 @@ "PLW060", "PLW0602", "PLW0603", + "PLW0604", "PLW07", "PLW071", "PLW0711",