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

[ntBre]

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:

# try.py
x = None
global x

> 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.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Can you tell me a bit more about the motivation for adding this error? I'm not quiet convinced that we should emit the extra error at the module level (especially considering that it results in a 1% perf regression)

What's not quiet clear to me is how many errors will emit in the case where there's an assignment to a variable before its global declaration at the module level. What I understand from the PR summary is that will emit two errors:

1. That the globals statement is invalid on the module level
2. That x is used before the globals declaration

If that's the case, then I'd prefer not to make this change because the only relevant error for users to fix is to not use the globals statement and everything will then work as expected (at least for assignments, uses are a different story).

[ntBre]

Can you tell me a bit more about the motivation for adding this error? I'm not quiet convinced that we should emit the extra error at the module level (especially considering that it results in a 1% perf regression)

The main motivation was just that it came up in the fuzzing results from #7633, and I noticed that we didn't raise any error for it.

What's not quiet clear to me is how many errors will emit in the case where there's an assignment to a variable before its global declaration at the module level. What I understand from the PR summary is that will emit two errors:

1. That the globals statement is invalid on the module level
2. That x is used before the globals declaration

If that's the case, then I'd prefer not to make this change because the only relevant error for users to fix is to not use the globals statement and everything will then work as expected (at least for assignments, uses are a different story).

I think the summary (and title) was a bit poorly worded. global is valid at the module level as far as CPython is concerned, only accessing the variable before the global statement is an error. This snippet runs fine in Python:

global x
x = 1

What I was trying to say in the title/description was more like "extend PLE0118 to detect load-before-global-declaration in the module scope," but I see now how it sounds very different. PLE0118 currently only applies in function and class scopes but should also apply in module scope to match CPython.

Ideally this would have been a rule-specific change, but instead it looked like I needed to modify the Checker/SemanticModel like I did here.

I don't feel strongly about this, though, especially with the perf hit, and am happy to close the PR. I only opened it for completeness of handling #7633. I think actual code like this would be pretty rare.

[MichaReiser]

Thanks. So there's nothing special about the module level. It's just that Ruff didn't emit the diagnostic

[MichaReiser]

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)

[ntBre]

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.

[ntBre]

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.

[ntBre]

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.