From 5140fa3387c526f783747dee88682f1641e2a98a Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 15 Apr 2025 11:57:58 -0400 Subject: [PATCH 1/4] [`pylint`] Detect `global` declarations in module scope (PLE0118) Summary -- While going through the syntax errors in [this comment], I was surprised to see the error `name 'x' is assigned to before global declaration`, which corresponds to [load-before-global-declaration (PLE0118)] and has also been reimplemented as a syntax error (#17135). However, it looks like neither of the implementations consider `global` declarations in the top-level module scope, which is a syntax error in CPython: ```python x = None global x ``` ```shell > python -m compileall -f try.py Compiling 'try.py'... *** File "try.py", line 2 global x ^^^^^^^^ SyntaxError: name 'x' is assigned to before global declaration ``` Test Plan -- New PLE0118 test case. We should also check the codspeed results carefully here. `Globals::from_body` visits all of the statements in `body`, so this might be a really bad idea, but it looked like the simplest fix. [this comment]: https://github.com/astral-sh/ruff/issues/7633#issuecomment-1740424031 [load-before-global-declaration (PLE0118)]: https://docs.astral.sh/ruff/rules/load-before-global-declaration/#load-before-global-declaration-ple0118 --- .../pylint/load_before_global_declaration.py | 5 +++ crates/ruff_linter/src/checkers/ast/mod.rs | 4 ++ ...118_load_before_global_declaration.py.snap | 8 ++++ crates/ruff_python_semantic/src/model.rs | 40 ++++++++++--------- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py b/crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py index 5ca672e1a1a43..fdbe9cc9b8760 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py @@ -156,3 +156,8 @@ def f(): def f(): global x print(f"{x=}") + + +# surprisingly still an error, global in module scope +x = None +global x diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index ebff124383a04..03be172e7a6fa 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1971,6 +1971,10 @@ impl<'a> Visitor<'a> for Checker<'a> { // Step 4: Analysis analyze::suite(body, self); + if let Some(globals) = Globals::from_body(body) { + self.semantic.set_globals(globals); + } + // Step 2: Traversal for stmt in body { self.visit_stmt(stmt); diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap index 5d1994780df2a..adabada987380 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap @@ -123,3 +123,11 @@ load_before_global_declaration.py:113:14: PLE0118 Name `x` is used prior to glob | ^ PLE0118 114 | global x | + +load_before_global_declaration.py:162:1: PLE0118 Name `x` is used prior to global declaration on line 163 + | +161 | # surprisingly still an error, global in module scope +162 | x = None + | ^ PLE0118 +163 | global x + | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index d24060b7c41ca..2690b28cface8 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1503,24 +1503,28 @@ impl<'a> SemanticModel<'a> { /// Set the [`Globals`] for the current [`Scope`]. pub fn set_globals(&mut self, globals: Globals<'a>) { - // If any global bindings don't already exist in the global scope, add them. - for (name, range) in globals.iter() { - if self - .global_scope() - .get(name) - .is_none_or(|binding_id| self.bindings[binding_id].is_unbound()) - { - let id = self.bindings.push(Binding { - kind: BindingKind::Assignment, - range: *range, - references: Vec::new(), - scope: ScopeId::global(), - source: self.node_id, - context: self.execution_context(), - exceptions: self.exceptions(), - flags: BindingFlags::empty(), - }); - self.global_scope_mut().add(name, id); + // If any global bindings don't already exist in the global scope, add them, unless we are + // also in the global scope, in which case avoid adding them to flag + // `load-before-global-declaration` (PLE0118). + if !self.at_top_level() { + for (name, range) in globals.iter() { + if self + .global_scope() + .get(name) + .is_none_or(|binding_id| self.bindings[binding_id].is_unbound()) + { + let id = self.bindings.push(Binding { + kind: BindingKind::Assignment, + range: *range, + references: Vec::new(), + scope: ScopeId::global(), + source: self.node_id, + context: self.execution_context(), + exceptions: self.exceptions(), + flags: BindingFlags::empty(), + }); + self.global_scope_mut().add(name, id); + } } } From c34dca3b54f24c32c90d5f865eb0ac48e7919dcc Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 15 Apr 2025 13:10:42 -0400 Subject: [PATCH 2/4] move globals to visit_module and fix comment --- crates/ruff_linter/src/checkers/ast/mod.rs | 8 ++++---- crates/ruff_python_semantic/src/model.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 03be172e7a6fa..d58085d5c5ebd 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -1971,10 +1971,6 @@ impl<'a> Visitor<'a> for Checker<'a> { // Step 4: Analysis analyze::suite(body, self); - if let Some(globals) = Globals::from_body(body) { - self.semantic.set_globals(globals); - } - // Step 2: Traversal for stmt in body { self.visit_stmt(stmt); @@ -2063,6 +2059,10 @@ impl<'a> Visitor<'a> for Checker<'a> { impl<'a> Checker<'a> { /// Visit a [`Module`]. Returns `true` if the module contains a module-level docstring. fn visit_module(&mut self, python_ast: &'a Suite) { + // Extract any global bindings from the module body. + if let Some(globals) = Globals::from_body(python_ast) { + self.semantic.set_globals(globals); + } analyze::module(python_ast, self); } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 2690b28cface8..fe3bee92318ca 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1504,8 +1504,8 @@ impl<'a> SemanticModel<'a> { /// Set the [`Globals`] for the current [`Scope`]. pub fn set_globals(&mut self, globals: Globals<'a>) { // If any global bindings don't already exist in the global scope, add them, unless we are - // also in the global scope, in which case avoid adding them to flag - // `load-before-global-declaration` (PLE0118). + // also in the global scope, where we don't want these to count as definitions for rules + // like `undefined-name` (F821) if !self.at_top_level() { for (name, range) in globals.iter() { if self From ff420082e4517963ca7740873b7068c1619daf69 Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Tue, 15 Apr 2025 13:11:06 -0400 Subject: [PATCH 3/4] delete unrelated but outdated comment --- crates/ruff_linter/src/checkers/ast/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index d58085d5c5ebd..cb327d974e382 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -2057,7 +2057,7 @@ impl<'a> Visitor<'a> for Checker<'a> { } impl<'a> Checker<'a> { - /// Visit a [`Module`]. Returns `true` if the module contains a module-level docstring. + /// Visit a [`Module`]. fn visit_module(&mut self, python_ast: &'a Suite) { // Extract any global bindings from the module body. if let Some(globals) = Globals::from_body(python_ast) { From 3f2f302cfea989b0949267d168e10ee28370404e Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Thu, 24 Apr 2025 15:50:33 -0400 Subject: [PATCH 4/4] add examples in `set_globals` --- crates/ruff_python_semantic/src/model.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index fe3bee92318ca..70c26cd615898 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1505,7 +1505,27 @@ impl<'a> SemanticModel<'a> { pub fn set_globals(&mut self, globals: Globals<'a>) { // If any global bindings don't already exist in the global scope, add them, unless we are // also in the global scope, where we don't want these to count as definitions for rules - // like `undefined-name` (F821) + // like `undefined-name` (F821). For example, adding bindings in the top-level scope causes + // a false negative in cases like this: + // + // ```python + // global x + // + // def f(): + // print(x) # F821 false negative + // ``` + // + // On the other hand, failing to add bindings in non-top-level scopes causes false + // positives: + // + // ```python + // def f(): + // global foo + // import foo + // + // def g(): + // foo.is_used() # F821 false positive + // ``` if !self.at_top_level() { for (name, range) in globals.iter() { if self