diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined.py b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined.py new file mode 100644 index 0000000000000..5f7a59930d107 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined.py @@ -0,0 +1,68 @@ +# pylint: disable=invalid-name, import-outside-toplevel, too-few-public-methods, unused-import +# pylint: disable=missing-module-docstring, missing-function-docstring, missing-class-docstring +# pylint: disable=global-at-module-level, global-statement, global-variable-not-assigned +# pylint: diable=redefined-outer-name +import dataclasses +from os import getenv + +CONSTANT = 1 +UNDEFINED: int + + +def FUNC(): + pass + + +class CLASS: + pass + + +# BAD +def global_variable_undefined(): + global SOMEVAR # [global-variable-undefined] + SOMEVAR = 2 + + +# OK +def global_constant(): + global CONSTANT + print(CONSTANT) + global UNDEFINED + UNDEFINED = 1 + global CONSTANT_2 + print(CONSTANT_2) + global dataclasses + dataclasses = 3 + global getenv + getenv = 4 + + +def global_with_import(): + global sys + import sys + + +def global_with_import_from(): + global namedtuple + from collections import namedtuple + + +def override_func(): + global FUNC + + def FUNC(): + pass + + FUNC() + + +def override_class(): + global CLASS + + class CLASS(): + pass + + CLASS() + + +CONSTANT_2 = 2 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined_star_import.py b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined_star_import.py new file mode 100644 index 0000000000000..5f49650dca2bd --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/global_variable_undefined_star_import.py @@ -0,0 +1,10 @@ +# pylint: disable=missing-module-docstring, missing-function-docstring +# pylint: disable=redefined-builtin, unnecessary-lambda-assignment +# pylint: disable=global-statement, unused-wildcard-import, wildcard-import +from os import * + + +# OK +def global_star_import(): + global system + system = lambda _: None diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 6e6e572993f23..bbb5be5e91063 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -28,6 +28,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pycodestyle::rules::ambiguous_variable_name(checker, name, name.range()); } } + if checker.enabled(Rule::GlobalVariableUndefined) { + pylint::rules::global_variable_undefined(checker, stmt); + } } Stmt::Nonlocal(nonlocal @ ast::StmtNonlocal { names, range: _ }) => { if checker.enabled(Rule::AmbiguousVariableName) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index e463c338269ca..eff99fb8af1a7 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -276,6 +276,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0211") => (RuleGroup::Stable, rules::pylint::rules::BadStaticmethodArgument), (Pylint, "W0245") => (RuleGroup::Stable, rules::pylint::rules::SuperWithoutBrackets), (Pylint, "W0406") => (RuleGroup::Stable, rules::pylint::rules::ImportSelf), + (Pylint, "W0601") => (RuleGroup::Preview, rules::pylint::rules::GlobalVariableUndefined), (Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned), (Pylint, "W0603") => (RuleGroup::Stable, rules::pylint::rules::GlobalStatement), (Pylint, "W0604") => (RuleGroup::Stable, rules::pylint::rules::GlobalAtModuleLevel), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 8a61555b950c3..c31e808e62115 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -64,6 +64,14 @@ mod tests { Rule::GlobalVariableNotAssigned, Path::new("global_variable_not_assigned.py") )] + #[test_case( + Rule::GlobalVariableUndefined, + Path::new("global_variable_undefined.py") + )] + #[test_case( + Rule::GlobalVariableUndefined, + Path::new("global_variable_undefined_star_import.py") + )] #[test_case(Rule::ImportOutsideTopLevel, Path::new("import_outside_top_level.py"))] #[test_case( Rule::ImportPrivateName, diff --git a/crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs b/crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs new file mode 100644 index 0000000000000..d985f3146a4e4 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/global_variable_undefined.rs @@ -0,0 +1,152 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, Stmt, StmtGlobal}; +use ruff_python_semantic::{BindingId, BindingKind, Scope, ScopeKind}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks that all `global` variables are indeed defined on module level +/// +/// ## Why is this bad? +/// If the module level declaration is missing, then either if was +/// forgotten or the `global` can be omitted. +/// +/// ## Example +/// ```python +/// def foo(): +/// global var # [global-variable-undefined] +/// var = 10 +/// print(var) +/// ``` +/// +/// Use instead: +/// ```python +/// var = 1 +/// +/// +/// def foo(): +/// global var +/// var = 10 +/// print(var) +/// ``` +#[violation] +pub struct GlobalVariableUndefined { + name: String, +} + +impl Violation for GlobalVariableUndefined { + #[derive_message_formats] + fn message(&self) -> String { + let GlobalVariableUndefined { name } = self; + format!("Global variable `{name}` is undefined at the module") + } +} + +/// PLW0601 +pub(crate) fn global_variable_undefined(checker: &mut Checker, stmt: &Stmt) { + if checker.semantic().current_scope().kind.is_module() { + return; + } + + let Stmt::Global(StmtGlobal { names, range }) = stmt else { + return; + }; + + if checker + .semantic() + .current_scopes() + .any(Scope::uses_star_imports) + { + return; + } + + let module_scope = checker.semantic().global_scope(); + let imported_names = get_imports(checker); + let mut undefined_names = vec![]; + + for name in names { + // Skip if imported names + if imported_names.contains(&name.as_str()) { + continue; + } + // Test binding which has been already defined + let Some(binding_id) = module_scope.get(name) else { + continue; + }; + if is_global_binding(checker, binding_id) { + continue; + } + // Test binding which has been only declared + if let Some(shadowed_binding_id) = module_scope.shadowed_binding(binding_id) { + if is_global_binding(checker, shadowed_binding_id) { + continue; + } + } + + undefined_names.push(name); + } + + for name in undefined_names { + checker.diagnostics.push(Diagnostic::new( + GlobalVariableUndefined { + name: name.to_string(), + }, + *range, + )); + } +} + +fn get_imports<'a>(checker: &'a Checker) -> Vec<&'a str> { + // Get all names imported in the current scope + let Some(fs) = checker + .semantic() + .current_scopes() + .find(|scope| scope.kind.is_function()) + else { + return vec![]; + }; + let ScopeKind::Function(ast::StmtFunctionDef { body, .. }) = fs.kind else { + return vec![]; + }; + let mut import_names = vec![]; + for stmt in body { + match stmt { + Stmt::Import(ast::StmtImport { names, .. }) + | Stmt::ImportFrom(ast::StmtImportFrom { names, .. }) => { + for name in names { + import_names.push(name.name.as_str()); + } + } + _ => (), + } + } + import_names +} + +fn is_global_binding(checker: &Checker, binding_id: BindingId) -> bool { + let binding = checker.semantic().binding(binding_id); + // Skip if import + if matches!( + binding.kind, + BindingKind::FromImport(_) | BindingKind::Import(_) | BindingKind::FutureImport + ) { + return true; + } + // Skip if module level class or function definition + if matches!( + binding.kind, + BindingKind::ClassDefinition(_) | BindingKind::FunctionDefinition(_) + ) { + return true; + } + // Skip if module level definition + let Some(node_id) = binding.source else { + return true; + }; + let node = checker.semantic().node(node_id); + if let Some(Expr::Name(ast::ExprName { .. })) = node.as_expression() { + return true; + }; + false +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index bb14d868f71a0..e25dfc92fe2ad 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -22,6 +22,7 @@ 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 global_variable_undefined::*; pub(crate) use if_stmt_min_max::*; pub(crate) use import_outside_top_level::*; pub(crate) use import_private_name::*; @@ -125,6 +126,7 @@ mod eq_without_hash; mod global_at_module_level; mod global_statement; mod global_variable_not_assigned; +mod global_variable_undefined; mod if_stmt_min_max; mod import_outside_top_level; mod import_private_name; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined.py.snap new file mode 100644 index 0000000000000..1a568b517198a --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined.py.snap @@ -0,0 +1,11 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +global_variable_undefined.py:22:5: PLW0601 Global variable `SOMEVAR` is undefined at the module + | +20 | # BAD +21 | def global_variable_undefined(): +22 | global SOMEVAR # [global-variable-undefined] + | ^^^^^^^^^^^^^^ PLW0601 +23 | SOMEVAR = 2 + | diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined_star_import.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined_star_import.py.snap new file mode 100644 index 0000000000000..889e8380a5f63 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0601_global_variable_undefined_star_import.py.snap @@ -0,0 +1,3 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- diff --git a/ruff.schema.json b/ruff.schema.json index c4adb82957e41..dc68cbc5ffef9 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3589,6 +3589,7 @@ "PLW0406", "PLW06", "PLW060", + "PLW0601", "PLW0602", "PLW0603", "PLW0604",