Skip to content

Commit 6d3b1d1

Browse files
authored
[pylint] Detect global declarations in module scope (PLE0118) (#17411)
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 # try.py 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 ``` I'm not sure this is the best or most elegant solution, but it was a quick fix that passed all of our tests. Test Plan -- New PLE0118 test case. [this comment]: #7633 (comment) [load-before-global-declaration (PLE0118)]: https://docs.astral.sh/ruff/rules/load-before-global-declaration/#load-before-global-declaration-ple0118
1 parent 3f84e75 commit 6d3b1d1

4 files changed

Lines changed: 60 additions & 19 deletions

File tree

crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,3 +156,8 @@ def f():
156156
def f():
157157
global x
158158
print(f"{x=}")
159+
160+
161+
# surprisingly still an error, global in module scope
162+
x = None
163+
global x

crates/ruff_linter/src/checkers/ast/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2067,8 +2067,12 @@ impl<'a> Visitor<'a> for Checker<'a> {
20672067
}
20682068

20692069
impl<'a> Checker<'a> {
2070-
/// Visit a [`Module`]. Returns `true` if the module contains a module-level docstring.
2070+
/// Visit a [`Module`].
20712071
fn visit_module(&mut self, python_ast: &'a Suite) {
2072+
// Extract any global bindings from the module body.
2073+
if let Some(globals) = Globals::from_body(python_ast) {
2074+
self.semantic.set_globals(globals);
2075+
}
20722076
analyze::module(python_ast, self);
20732077
}
20742078

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,3 +123,11 @@ load_before_global_declaration.py:113:14: PLE0118 Name `x` is used prior to glob
123123
| ^ PLE0118
124124
114 | global x
125125
|
126+
127+
load_before_global_declaration.py:162:1: PLE0118 Name `x` is used prior to global declaration on line 163
128+
|
129+
161 | # surprisingly still an error, global in module scope
130+
162 | x = None
131+
| ^ PLE0118
132+
163 | global x
133+
|

crates/ruff_python_semantic/src/model.rs

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1503,24 +1503,48 @@ impl<'a> SemanticModel<'a> {
15031503

15041504
/// Set the [`Globals`] for the current [`Scope`].
15051505
pub fn set_globals(&mut self, globals: Globals<'a>) {
1506-
// If any global bindings don't already exist in the global scope, add them.
1507-
for (name, range) in globals.iter() {
1508-
if self
1509-
.global_scope()
1510-
.get(name)
1511-
.is_none_or(|binding_id| self.bindings[binding_id].is_unbound())
1512-
{
1513-
let id = self.bindings.push(Binding {
1514-
kind: BindingKind::Assignment,
1515-
range: *range,
1516-
references: Vec::new(),
1517-
scope: ScopeId::global(),
1518-
source: self.node_id,
1519-
context: self.execution_context(),
1520-
exceptions: self.exceptions(),
1521-
flags: BindingFlags::empty(),
1522-
});
1523-
self.global_scope_mut().add(name, id);
1506+
// If any global bindings don't already exist in the global scope, add them, unless we are
1507+
// also in the global scope, where we don't want these to count as definitions for rules
1508+
// like `undefined-name` (F821). For example, adding bindings in the top-level scope causes
1509+
// a false negative in cases like this:
1510+
//
1511+
// ```python
1512+
// global x
1513+
//
1514+
// def f():
1515+
// print(x) # F821 false negative
1516+
// ```
1517+
//
1518+
// On the other hand, failing to add bindings in non-top-level scopes causes false
1519+
// positives:
1520+
//
1521+
// ```python
1522+
// def f():
1523+
// global foo
1524+
// import foo
1525+
//
1526+
// def g():
1527+
// foo.is_used() # F821 false positive
1528+
// ```
1529+
if !self.at_top_level() {
1530+
for (name, range) in globals.iter() {
1531+
if self
1532+
.global_scope()
1533+
.get(name)
1534+
.is_none_or(|binding_id| self.bindings[binding_id].is_unbound())
1535+
{
1536+
let id = self.bindings.push(Binding {
1537+
kind: BindingKind::Assignment,
1538+
range: *range,
1539+
references: Vec::new(),
1540+
scope: ScopeId::global(),
1541+
source: self.node_id,
1542+
context: self.execution_context(),
1543+
exceptions: self.exceptions(),
1544+
flags: BindingFlags::empty(),
1545+
});
1546+
self.global_scope_mut().add(name, id);
1547+
}
15241548
}
15251549
}
15261550

0 commit comments

Comments
 (0)