Skip to content

[pylint] Detect global declarations in module scope (PLE0118) #17411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ntBre merged 4 commits into from
Apr 25, 2025
Merged

[pylint] Detect global declarations in module scope (PLE0118) #17411

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5140fa3
[`pylint`] Detect `global` declarations in module scope (PLE0118)
ntBre Apr 15, 2025
c34dca3
move globals to visit_module and fix comment
ntBre Apr 15, 2025
ff42008
delete unrelated but outdated comment
ntBre Apr 15, 2025
3f2f302
add examples in `set_globals`
ntBre Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/load_before_global_declaration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 5 additions & 1 deletion crates/ruff_linter/src/checkers/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2057,8 +2057,12 @@ 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) {
self.semantic.set_globals(globals);
}
analyze::module(python_ast, self);
}

Expand Down
8 changes: 8 additions & 0 deletions ...apshots/ruff_linter__rules__pylint__tests__PLE0118_load_before_global_declaration.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
40 changes: 22 additions & 18 deletions crates/ruff_python_semantic/src/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
MichaReiser marked this conversation as resolved.
// also in the global scope, where we don't want these to count as definitions for rules
// like `undefined-name` (F821)
Copy link
Copy Markdown
Member

@MichaReiser MichaReiser Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you maybe add an example here of what problem would arise if we did run this code at the global level too? It's not entirely clear to me and I'm wondering if not running it creates different problems at the root level (you could try to comment out this code and see what tests fail for the non-global scope and then decide if this is or isn't a problem for the global scope)

Copy link
Copy Markdown
Contributor Author

@ntBre ntBre Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I did try deleting this code entirely before because that actually seemed more natural to me, but I didn't write down the results. This is the tricky F821 (F821_6.py) case that fails if we comment out the binding loop entirely:

"""Test: annotated global."""


n: int


def f():
    print(n)  # F821 false positive here


def g():
    global n
    n = 1


g()
f()

This shorter case also gets a false positive with the whole loop commented out:

def f(): global foo; import foo
def g(): foo.is_used()  # false positive here

I don't think we can run into something similar if the global is in the module scope, but this was all quite tricky, so I'm glad to get your eyes on these examples too.

If we run the binding loop in the global scope too, this case, also for F821, fails:

global x
def foo():
    print(x)  # F821 false negative

The second and third cases here look like nice additions to the docs.

Copy link
Copy Markdown
Contributor Author

@ntBre ntBre Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the more I stare at these examples, the more I think I might have been right to consider deleting this code entirely. It's really the assignment and the import that should be adding the bindings, not the global statements. We can just coincidentally get the right answer by letting global handle it, but it's more obvious that a real binding is needed in the module scope.

Copy link
Copy Markdown
Contributor Author

@ntBre ntBre Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is easily actionable though. I poked around a bit in Checker::add_binding and my attempts at checking for globals didn't work out. I've added the examples for now.

if !self.at_top_level() {
Comment thread
MichaReiser marked this conversation as resolved.
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);
}
}
}

Expand Down
Loading