8000 gh-58749: Removes the erroneous explanation regarding the restrictions of the global statement by bombs-kim · Pull Request #126523 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-58749: Removes the erroneous explanation regarding the restrictions of the global statement #126523

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.

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
merged 4 commits into from
Nov 12, 2024

Conversation

bombs-kim
Copy link
Contributor
@bombs-kim bombs-kim commented Nov 7, 2024

As pointed out by @ncoghlan , CPython does not restrict a name in a global statement from reappearing in a for target list, class definition, function definition, or import statement

Additionally, CPython does not impose the same restrictions on with statements or except clauses, or or variable annotations.

The only real restriction is that a variable cannot be used or assigned to prior to its global declaration in the scope.

A few more notes on the changes

  • I’ve even omitted the part about restrictions on formal parameters, although it's a real restriction. This is because 'a variable cannot be used or assigned to prior to its global declaration in the scope' already implies that restriction.
  • I’ve revised the sentence to make it more similar to the nonlocal section. Both explain a similar concept, and I believe the nonlocal explanation is clearer and more concise.

The nonlocal statement applies to the entire scope of a function or
class body. A :exc:SyntaxError is raised if a variable is used or
assigned to prior to its nonlocal declaration in the scope.

  • I removed the sentence, “The :keyword:global statement is a declaration which holds for the entire current code block,” as it is redundant with the following explanation: “The global statement applies to the entire scope of a function or class body.”

📚 Documentation preview 📚: https://cpython-previews--126523.org.readthedocs.build/

… a name declared as global can appear later as a target in :keyword:with statements, :keyword:except clauses, :keyword:for target lists, :keyword:class definitions, function definitions, :keyword:import statements.
@bombs-kim bombs-kim requested a review from willingc as a code owner November 7, 2024 05:59
@ghost
Copy link
ghost commented Nov 7, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bombs-kim
Copy link
Contributor Author

As a side note, in #13232, @andresdelfino addressed the same section. However, it did not correct the erroneous explanation noted in #58749; it merely added cases to the existing list of name usages, which was already errorneous.

@andresdelfino
Copy link
Contributor
andresdelfino commented Nov 7, 2024

As a side note, in #13232, @andresdelfino addressed the same section. However, it did not correct the erroneous explanation noted in #58749; it merely added cases to the existing list of name usages, which was already errorneous.

Have you noticed the tests I added in the PR? Because I ran them today, with Python 3.13.0, and they still fail.

@bombs-kim
Copy link
Contributor Author

@andresdelfino Yes! I've seen all the cases

Since this works, it's not accurate to describe that names listed in a :keyword:global statement must not be defined in a :keyword:for

def for_global_before():
    global bar
    for bar in []:
        ...

Could you let me know if i'm missing something here?

@ncoghlan
Copy link
Contributor
ncoghlan commented Nov 8, 2024

Something that has changed since I wrote that post back in 2012 is the rise of type hints and static type checking. Even if CPython isn't enforcing the name binding restrictions in the paragraph being modified here, it may be that mypy and friends are doing so (since the language spec says they should be).

@bombs-kim
Copy link
Contributor Author
bombs-kim commented Nov 8, 2024

@ncoghlan thanks for noting an important point. I've checked if mypy prevents certain usages of the same names after a global declaration. I've found no mention of such things in the documentation. Also, the following code passed the latest stable mypy (1.13.0)

def fn():
    global name

    with name:
        ...

    try:
        ...
    except name:
        ...

    class name:
        ...

    def name():
        ...

    var: name = 1

   # import name  # this will raise an error, but not because of the global declaration above

If you wish me to conduct more research on another type hints and static type checking tool, I would be happy to do that. However, I personally think, even if type checking tools prevent using the same names after a global declaration, it may not be a huge problem, since they are supposed to impose restrictions that the language spec does not have anyway.

The main problem this PR deals with is that there is a discrepancy btw the actual CPython implementation and the language spec document. I think it's better to get rid of the discrepancy either by changing the docs, or changing the compiler.

@ncoghlan
Copy link
Contributor
ncoghlan commented Nov 10, 2024

Thanks for running that check, @bombs-kim. However, several of those checks aren't using their respective as name clauses, so they're not checking the name binding parts of those operations.

Re-reading the comments on the original issue, this change should also include a test suite addition (in https://github.com/python/cpython/blob/main/Lib/test/test_scope.py so the global and nonlocal test cases can go in the same file) that ensures these name bindings are all permitted.

Something like the following (using https://docs.python.org/3/reference/executionmodel.html#naming-and-binding to build the list of name binding operations to be checked):

import contextlib
from types import SimpleNamespace

class TestGlobalNameBinding:
    # Ensure binding global names works as expected.
    # Genuine scope conflicts (such as names being both local and
    # nonlocal, or local and global) are covered in the compiler tests.
    # Note: does not check `async def`, `async for`, or `async with`.

    def test_assignment_statement(self):
        global _global_assignment_statement
        value = object()
        _global_assignment_statement = value
        self.assertIs(globals()["_global_assignment_statement"], value)
        del _global_assignment_expression

    def test_unpacking_assignment(self):
        global _global_unpacking_assignment
        value = object()
        _, _global_unpacking_assignment = [None, value]
        self.assertIs(globals()["_global_unpacking_assignment"], value)
        del _global_unpacking_assignment

    def test_assignment_expression(self):
        global _global_assignment_expression
        value = object()
        if (_global_assignment_expression := value):
            pass
        self.assertIs(globals()["_global_assignment_expression"], value)
        del _global_assignment_expression

    def test_iteration_variable(self):
        global _global_iteration_variable
        value = object()
        for _global_iteration_variable in [value]:
            pass
        self.assertIs(globals()["_global_iteration_variable"], value)
        del _global_iteration_variable

    def test_func_def(self):
        global _global_func_def
        def _global_func_def():
            pass
        value = _global_func_def
        self.assertIs(globals()["_global_func_def"], value)
        del _global_func_def

    def test_class_def(self):
        global _global_class_def
        class _global_class_def():
            pass
        value = _global_class_def
        self.assertIs(globals()["_global_class_def"], value)
        del _global_class_def

    def test_type_alias(self):
        global _global_type_alias
        type _global_type_alias = tuple[int, int]
        value = _global_type_alias
        self.assertIs(globals()["_global_type_statement"], value)
        del _global_type_alias

    def test_caught_exception(self):
        global _global_caught_exc
        try:
            1/0
        except ZeroDivisionError as _global_caught_exc:
            value = _global_caught_exc
        self.assertIs(globals()["_global_caught_exc"], value)
        del _global_caught_exc

    def test_caught_exception_group(self):
        global _global_caught_exc_group
        try:
            try:
                1/0
            except ZeroDivisionError as exc:
                raise ExceptionGroup("eg", [exc])
        except* ZeroDivisionError as _global_caught_exc_group:
            value = _global_caught_exc_group
        self.assertIs(globals()["_global_caught_exc_group"], value)
        del _global_caught_exc

    def test_enter_result(self):
        global _global_enter_result
        value = object()
        with contextlib.nullcontext(value) as _global_enter_result:
            pass
        self.assertIs(globals()["_global_enter_result"], value)
        del
8000
 _global_enter_result

    def test_import_result(self):
        global _global_import_result
        value = contextlib
        import contextlib as _global_import_result
        self.assertIs(globals()["_global_import_result"], value)
        del _global_import_result

    def test_match(self):
        global _global_match
        value = object()
        match value:
            case _global_match:
                pass
        self.assertIs(globals()["_global_match"], value)
        del _global_match

    def test_match_as(self):
        global _global_match_as
        value = object()
        match value:
            case _ as _global_match_as:
                pass
        self.assertIs(globals()["_global_match_as"], value)
        del _global_match_as

    def test_match_seq(self):
        global _global_match_seq
        value = object()
        match (None, value):
            case (_, _global_match_seq):
                pass
        self.assertIs(globals()["_global_match_seq"], value)
        del _global_match_seq

    def test_match_map(self):
        global _global_match_map
        value = object()
        match {"key": value}:
            case {"key": _global_match_map}:
                pass
        self.assertIs(globals()["_global_match_map"], value)
        del _global_match_map

    def test_match_attr(self):
        global _global_match_attr
        value = object()
        match SimpleNamespace(key=value):
            case SimpleNamespace(key=_global_match_attr):
                pass
        self.assertIs(globals()["_global_match_attr"], value)
        del _global_match_attr

Copy link
Contributor
@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Blocking comment from the original issue discussion: the spec wording change should be accompanied by a test suite update that ensures the revised wording remains accurate.

For closing the original issue, the nonlocal wording also needs to be updated, but that can be done in a separate PR after the global change is resolved.

There's also a process question that I need to follow up before this can be merged (the level of review needed for language spec changes is generally higher than it was back when I first filed the linked issue, even when the spec change is fixing an error in the spec)

@bedevere-app
Copy link
bedevere-app bot commented Nov 10, 2024

@ncoghlan
Copy link
Contributor
ncoghlan commented Nov 10, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

In relation to the "global after use" error reporting behaviour, that's covered in https://github.com/python/cpython/blob/main/Lib/test/test_global.py .

There is currently no test_nonlocal.py equivalent for nonlocal statements, though.

So while I suggested using test_scope.py above, another plausible way to arrange the tests for this behaviour would be:

  • add the draft test case above to test_global.py (rewording the test module description accordingly)
  • add a new test_nonlocal.py that covered the declaration-after-use equivalent checks for nonlocal, in addition to the expected declaration-before-use behaviour

The biggest advantage of this would be test discoverability, since the tests for these statements would be in files specifically named for them. The headers of each statement-specific test module could then refer off to test_scope.py as covering additional related behavioural tests.

Edit to clarify: The reason statement order matters in the "global after use" and "nonlocal after use" cases is because the initial usage already implicitly declared the symbol as being local, making the later declaration as a global (or nonlocal) invalid for the same reason you can't declare function arguments as global (or nonlocal) - every symbol in an optimised scope has to be local, nonlocal, or global, it can't be in multiple categories at once. You also get an error if you try to declare something as both global and nonlocal, regardless of the statement order:

>>> def outer():
...     x = 1
...     def inner():
...         nonlocal x
...         global x
...
  File "<stdin>", line 4
SyntaxError: name 'x' is nonlocal and global
>>> def outer():
...     x = 1
...     def inner():
...         global x
...         nonlocal x
...
  File "<stdin>", line 4
SyntaxError: name 'x' is nonlocal and global

By contrast, when the explicit declaration comes first, it turns off the implicit local declarations for that symbol, so there's no conflict with the later name binding statements (they respect the explicit target scope declaration).

Refer to test_scope.py in the description for additional behavioural tests
Verify that syntax errors are correctly raised for improper `global`
statements
Test various name-binding scenarios for global
variables to ensure correct behavior
@bombs-kim
Copy link
Contributor Author
bombs-kim commented Nov 11, 2024

Hi @ncoghlan, I’m updating you on the changes based on your feedback:

  • I’ve chosen to update test_global.py rather than test_scope.py for the new tests.
  • I've added the tests cases based on the code you provided. I've adjusted the existing tests to be consistent with the test
    • Tests for test_caught_exception and test_caught_exception_group have been slighted changed, as the original tests are not supposed to pass
  • The module description has been updated to reflect the changes in tests and includes a reference to test_scope.py for related behaviors.

Also, the previous implementation details section has been removed. With the recent documentation update, the prior warning no longer applies.

Please let me know if you have additional suggestions.

@ncoghlan
Copy link
Contributor

Core dev consensus was that this is still a clear bug in the spec, so there's no need to get the SC directly involved.

@ncoghlan ncoghlan merged commit 494360a into python:main Nov 12, 2024
39 of 40 checks passed
The :keyword:`global` statement is a declaration which holds for the entire
current code block. It means that the listed identifiers are to be interpreted
as globals. It would be impossible to assign to a global variable without
The :keyword:`global` causes the listed identifiers to be interpreted
Copy link
Member

Choose a reason for hiding this comment

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

This reads The global, so is missing a noun: The global keyword

Copy link
Contributor Author
@bombs-kim bombs-kim Nov 12, 2024

Choose a reason for hiding this comment

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

Hey @merwok , thanks for the correction. I opened #126720 that reflects this feedback and handles some more mishaps.

picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
…l statement (pythonGH-126523)

* Removes erroneous explanation of the `global` statement restrictions; a name declared as global can be subsequently bound using any kind of name binding operation.
* Updates `test_global.py` to also test various name-binding scenarios for global
variables to ensure correct behavior
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
…l statement (pythonGH-126523)

* Removes erroneous explanation of the `global` statement restrictions; a name declared as global can be subsequently bound using any kind of name binding operation.
* Updates `test_global.py` to also test various name-binding scenarios for global
variables to ensure correct behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc 5177 dir skip news
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants
0