Skip to content
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

locals() in a module or class scope comprehension does not include the iteration var(s) since 3.12b1 #105340

Closed
jaraco opened this issue Jun 6, 2023 · 8 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@jaraco
Copy link
Member

jaraco commented Jun 6, 2023

In Python 3.11, inside a comprehension, locals() was bound to the scope of the comprehension:

 $ py -3.11 -c "print([locals() for name in 'abc'])"
[{'.0': <str_ascii_iterator object at 0x10472dc30>, 'name': 'c'}, {'.0': <str_ascii_iterator object at 0x10472dc30>, 'name': 'c'}, {'.0': <str_ascii_iterator object at 0x10472dc30>, 'name': 'c'}]

But since 3.12b1, locals() is globals():

 $ py -3.12 -c "print([locals() for name in 'abc'])"
[{'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}, {'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}, {'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}]
 $ py -3.11 -c "print([globals() for name in 'abc'])"
[{'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}, {'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}, {'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>}]
 $ py -3.12 -c "print([locals() is globals() for name in 'abc'])"
[True, True, True]

May be related to #105256.

Discovered in this job, triggered by this code (which I intend to update not to use locals()).

Is this change intentional?

Linked PRs

@jaraco jaraco added the type-bug An unexpected behavior, bug, or error label Jun 6, 2023
@JelleZijlstra
Copy link
Member

Yes, this is intentional and called out in PEP-709. cc @carljm

@sunmy2019
Copy link
Member

>>> def foo():
...     print([locals() is globals() for name in 'abc'])
... 
>>> foo()
[False, False, False]

With PEP-709, list comprehensions do not have their own stack. This affects top-level sentences.

@jaraco
Copy link
Member Author

jaraco commented Jun 6, 2023

Thanks for the reference. I do think the current behavior violates the PEP, which indicates that "locals() includes outer variables", suggesting that locals() would continue to include inner variables, and confirmed by the example where "under this PEP", x is found in locals().

In my example above, name does not appear in locals(), which is a departure from the prior behavior and of the PEP.

@sunmy2019
Copy link
Member

The correct thing seems to be something like

locals() == {**globals(), "name": ... }

@Yhg1s
Copy link
Member

Yhg1s commented Jun 6, 2023

name doesn't appear in locals() at the global level because the generated bytecode uses fast locals for the name even at the global scope:

>>> dis.disco(compile("name = 1; [locals() for name in 'abc']", 'f', 'exec'))
  0           0 RESUME                   0

  1           2 LOAD_CONST               0 (1)
              4 STORE_NAME               0 (name)
              6 LOAD_CONST               1 ('abc')
              8 GET_ITER
             10 LOAD_FAST_AND_CLEAR      0 (name)
             12 SWAP                     2
             14 BUILD_LIST               0
             16 SWAP                     2
        >>   18 FOR_ITER                 9 (to 40)
             22 STORE_FAST               0 (name)
             24 PUSH_NULL
             26 LOAD_NAME                1 (locals)
             28 CALL                     0
             36 LIST_APPEND              2
             38 JUMP_BACKWARD           11 (to 18)
        >>   40 END_FOR
             42 SWAP                     2
             44 STORE_FAST               0 (name)
             46 POP_TOP
             48 RETURN_CONST             2 (None)

This seems to me to be more correct than the alternative (replacing the existing name for the duration of the list comprehension) in particular because globals are visible to outside code. Is there a practical problem with not including the loop variable in the locals() at the global level?

@jaraco
Copy link
Member Author

jaraco commented Jun 6, 2023

Is there a practical problem with not including the loop variable in the locals() at the global level?

I'm not sure I understand the question or if it was even directed at me.

The problem I see is that it's no longer possible to reference locals()['name'] inside the loop.

>>> [locals()['name'] for name in 'x']
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'name'

Any code that depends on locals actually representing the local variables will fail. I linked to the code that started failing as a result of this change... but as I mentioned, that code used a legacy construct that's superseded by f-strings. I'm not yet aware of any other practical uses. In general, this change feels like a breaking change deserving of a deprecation period.

@JelleZijlstra
Copy link
Member

I do agree that this is a behavior change that wasn't called out in PEP 709 and that we didn't realize would be an issue before. PEP 709 says that locals() now includes the variables from the outer frame. That's true in functions, where locals() now includes both outer and comprehension-local variables, but in module and class scopes, it now includes only the outer variables.

>>> class F:
...     print([locals() for i in range(1)])
... 
[{'__module__': '__main__', '__qualname__': 'F'}]
>>> [locals() for i in range(1)]
[{'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>, 'F': <class '__main__.F'>}]
>>> def f(arg):
...     return [locals() for i in range(1)]
... 
>>> f(1)
[{'arg': 1, 'i': 0}]

And that can indeed break code like the code @jaraco linked above.

To fix it, I wonder if inside the implementation of locals() we can detect that we're within a class- or module-scoped comprehension, and if so, add the comprehension locals to the returned dictionary.

@carljm
Copy link
Member

carljm commented Jun 6, 2023

Yes, the lack of name key in locals() in the given example is a problem. Thanks @jaraco for the report.

I'm traveling this week but will be able to look into possible fixes here more closely next week. It's tricky because we can't actually add the key to the module globals (or class "locals") dictionary without potentially stomping on an existing outer-scoped variable, which would be much worse. There may be potential band-aids where locals() returns a different dictionary within a comprehension, with the extra key added. The long term solution to this is something like PEP 667, which would allow fixing this much more cleanly.

@carljm carljm changed the title locals() bound to globals() inside comprehension since 3.12b1 locals() in a module or class scope comprehension does not include the iteration var(s) since 3.12b1 Jun 6, 2023
@carljm carljm self-assigned this Jun 12, 2023
carljm added a commit to carljm/cpython that referenced this issue Jun 12, 2023
carljm added a commit to carljm/cpython that referenced this issue Jun 12, 2023
carljm added a commit that referenced this issue Jul 5, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 5, 2023
)

* pythongh-105340: include hidden fast-locals in locals()
(cherry picked from commit 104d7b7)

Co-authored-by: Carl Meyer <carl@oddbird.net>
carljm added a commit that referenced this issue Jul 5, 2023
…106470)

gh-105340: include hidden fast-locals in locals() (GH-105715)

* gh-105340: include hidden fast-locals in locals()
(cherry picked from commit 104d7b7)

Co-authored-by: Carl Meyer <carl@oddbird.net>
@carljm carljm closed this as completed Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants