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

carljm · 2023-06-12T23:33:41Z

With comprehension inlining (PEP 709), comprehension iteration variables in class- and module-scoped comprehensions are now stored as "hidden" fast-locals (in scopes that otherwise use dictionary locals, not fast locals.) This maintains good isolation, but it also has the side effect that calling locals() (or vars()) inside a class- or module-scoped comprehension does not include the comprehension iteration var. It can't in the naive way (just updating the frame's locals dict, as done in _PyFrame_FastToLocals) because that could overwrite a key of the same name from the outer class/module scope, breaking isolation. But there are existing use cases where having visibility of the name in locals() matters, e.g. prefixed_names = ["foo{name}".format(**locals()) for name in names].

This PR makes the comprehension iteration var visible in locals() within class- and module-scope comprehensions again, by creating and returning a new dictionary for calls to locals() (when there are hidden fast-locals) which is a copy of the outer locals, plus the extra hidden local(s) added.

This would be a very localized change encapsulated within PyEval_GetLocals and the frame functions it calls, except for the unfortunate detail that PyEval_GetLocals currently returns a borrowed reference to the singular locals dict cached on the frame, and so it simply can't ever return a new locals dictionary. And it's public API, so it has to keep returning a borrowed reference. So this PR introduces PyEval_GetFrameLocals, which is instead defined to return a new reference, and uses it in locals(), vars(), dir(), exec() and eval() (all the locations where PyEval_GetLocals was used internally.) Long term, returning a new reference is definitely preferable (as is almost always the case): it means the caller doesn't have to worry about the frame's lifetime, it doesn't require caching locals dicts on frames (leading to cyclic references), and it gives more internal implementation flexibility.

In the 3.13 cycle I'd like to move PEP 667 forward, which would bring a more comprehensive consistency to the behavior of locals() in all kinds of namespaces. With PEP 667, _PyFrame_FastToLocals would disappear entirely, and this fix would just naturally fall out of the behavior of locals() as returning a mapping object that is a consistent view on the current locals namespace. PEP 667 also explicitly deprecates the borrowed-reference-returning PyEval_GetLocals et al and replaces them with new-reference-returning forms. I chose the name PyEval_GetFrameLocals in this PR so as to maximize compatibility with a possible PEP 667 future.

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

* main: pythongh-105540: Fix code generator tests (python#105707) pythongh-105375: Explicitly initialise all {Pickler,Unpickler}Object fields (python#105686) pythongh-105331: Change `asyncio.sleep` to raise ``ValueError` for nan (python#105641) Remove support for legacy bytecode instructions (python#105705)

Python/bltinmodule.c

* main: (57 commits) pythongh-105831: Fix NEWS blurb from pythongh-105828 (python#105833) pythongh-105820: Fix tok_mode expression buffer in file & readline tokenizer (python#105828) pythongh-105751, test_ctypes: Remove disabled tests (python#105826) pythongh-105821: Use a raw f-string in test_httpservers.py (python#105822) pythongh-105751: Remove platform usage in test_ctypes (python#105819) pythongh-105751: Reenable disable test_ctypes tests (python#105818) pythongh-105751: Remove dead code in test_ctypes (python#105817) More reorganisation of the typing docs (python#105787) Improve docs for `typing.dataclass_transform` (python#105792) pythonGH-89812: Churn `pathlib.Path` test methods (python#105807) pythongh-105800: Issue SyntaxWarning in f-strings for invalid escape sequences (python#105801) pythongh-105751: Cleanup test_ctypes imports (python#105803) pythongh-105481: add HAS_JUMP flag to opcode metadata (python#105791) pythongh-105751: test_ctypes avoids the operator module (pythonGH-105797) pythongh-105751: test_ctypes: Remove @need_symbol decorator (pythonGH-105798) pythongh-104909: Implement conditional stack effects for macros (python#105748) pythongh-75905: Remove test_xmlrpc_net: skipped since 2017 (python#105796) pythongh-105481: Fix types and a bug for pseudos (python#105788) Update DSL docs for cases generator (python#105753) pythonGH-77273: Better bytecodes for f-strings (pythonGH-6132) ...

carljm · 2023-06-16T00:28:02Z

Requesting review from @Yhg1s as 3.12 RM, since this fix is intended for backport to 3.12. By 3.13 I hope to have the cleaner fix via PEP 667.

JelleZijlstra

You also need to change dir() (implemented in _dir_locals in Objects/object.c).

% ./python.exe 
Python 3.13.0a0 (heads/unifyunion:9f58421b61, Jun 15 2023, 19:53:15) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> [dir() for i in range(1)]
[['__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__']]
>>> ^D
% python3.11
Python 3.11.1 (main, Dec 21 2022, 16:19:04) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> [dir() for i in range(1)]
[['.0', 'i']]
>>>

Otherwise, looks good.

* main: pythongh-104799: PEP 695 backward compatibility for ast.unparse (python#105846) pythongh-105834: Add tests for calling `issubclass()` between two protocols (python#105835) CI: Remove docs build from Azure Pipelines (python#105823) pythongh-105844: Consistently use 'minor version' for X.Y versions (python#105851) Fix inaccuracies in "Assorted Topics" section of "Defining Extension Types" tutorial (python#104969) pythongh-105433: Add `pickle` tests for PEP695 (python#105443) bpo-44530: Document the change in MAKE_FUNCTION behavior (python#93189) pythonGH-103124: Multiline statement support for pdb (pythonGH-103125) pythonGH-105588: Add missing error checks to some obj2ast_* converters (pythonGH-105589)

markshannon · 2023-06-21T09:11:23Z

Returning a new dictionary is a significant change. It should be OK for 3.13. I suspect it may not be OK to backport, but that's not my decision.

PEP 667, as currently written, won't help, but we can change it.

The locals() function will act the same as it does now for class and modules scopes.

I'd be happy to change the PEP to always return a proxy.
It is more consistent that way, although it is a slightly bigger change.

I don't have an opinion on what is the best approach for 3.12.
For 3.13 I think we should push on with PEP 667.

JelleZijlstra

Looks good, left a question about the public status of the new function.

I do think this is the right solution for 3.12. For 3.13 we can consider more general changes to how locals() and co work.

JelleZijlstra · 2023-06-21T14:19:58Z

Include/cpython/ceval.h

@@ -33,3 +33,5 @@ _PyEval_RequestCodeExtraIndex(freefunc f) {

 PyAPI_FUNC(int) _PyEval_SliceIndex(PyObject *, Py_ssize_t *);
 PyAPI_FUNC(int) _PyEval_SliceIndexNotNone(PyObject *, Py_ssize_t *);
+
+PyAPI_FUNC(PyObject *) PyEval_GetFrameLocals(void);


At least in 3.12, should we name this with a leading underscore and keep it in the internal API? It's a bit late to add a public API function, and it doesn't seem necessary to fix the bug.

Or do you think this is needed for C extensions that call PyEval_GetLocals()?

If there are C extensions calling PyEval_GetLocals() while within a class or module-scoped comprehension and running into the equivalent of the OP case, then they would want to switch to PyEval_GetFrameLocals() to fix that. I'm skeptical that any such cases will exist, but that's why I left it public. Open to whatever @Yhg1s prefers here.

carljm · 2023-06-21T17:36:21Z

Returning a new dictionary is a significant change.

A new dictionary is only returned when locals() is called inside a comprehension at class or module scope. In previous versions, locals() called inside the comprehension would have returned a different dictionary from locals() called outside the comprehension anyway, so I think it's very unlikely that this is a difference that will impact anyone. One way it would make an observable difference if someone does [locals() for x in range(3)] -- previously that would result in a list of three references to the same locals dictionary, now it would result in a list of three different locals dictionaries. I don't think this detail of the behavior of locals() is defined or guaranteed anywhere, though, and there are other things about the current behavior of locals() that are already much weirder than that. And we can clean this up for 3.13.

I suspect it may not be OK to backport

As I see it, we have three options for this issue:

Let the OP example break in 3.12.
Backport this fix to 3.12.
Remove inlining of class- and module-scoped comprehensions entirely in 3.12 (and maybe bring it back along with something like PEP 667 in 3.13?)

I think (1) is not a good option, and I think (2) is less invasive than (3).

@Yhg1s what do you prefer to do for 3.12?

PEP 667, as currently written, won't help, but we can change it... I'd be happy to change the PEP to always return a proxy.

👍

Yhg1s · 2023-07-03T17:05:17Z

As a matter of principle I'd rather see the optimisation not enabled at the global scope, given the subtlies involved... But I do agree that it's a very obscure corner case and the workaround is probably good enough, and the complexity of the change is acceptable. Either is acceptable for 3.12.

carljm · 2023-07-03T17:57:01Z

Thanks @Yhg1s! I discussed with @JelleZijlstra, and I think I still feel that it's better to merge this fix for 3.12, since you're OK with it. I think rolling back the inlining at module and class scope is a more disruptive change to users at this point in the 3.12 beta than this change will be, and it's more confusing if comprehensions work differently in different places. And I'm confident we can clean up the subtleties around behavior of locals() (along with many oddities of its behavior that long predate PEP 709) in the 3.13 cycle, with PEP 667.

* main: (167 commits) pythongh-91053: make func watcher tests resilient to other func watchers (python#106286) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106354) pythongh-106359: Fix corner case bugs in Argument Clinic converter parser (python#106361) pythongh-104146: Remove unused attr 'parameter_indent' from clinic.DLParser (python#106358) pythongh-106320: Remove private _PyErr C API functions (python#106356) pythongh-104050: Annotate Argument Clinic DSLParser attributes (python#106357) pythongh-106320: Create pycore_modsupport.h header file (python#106355) pythongh-106320: Move _PyUnicodeWriter to the internal C API (python#106342) pythongh-61215: New mock to wait for multi-threaded events to happen (python#16094) Document PYTHONSAFEPATH along side -P (python#106122) Replace the esoteric term 'datum' when describing dict comprehensions (python#106119) pythongh-104050: Add more type hints to Argument Clinic DSLParser() (python#106343) pythongh-106320: _testcapi avoids private _PyUnicode_EqualToASCIIString() (python#106341) pythongh-106320: Add pycore_complexobject.h header file (python#106339) pythongh-106078: Move DecimalException to _decimal state (python#106301) pythongh-106320: Use _PyInterpreterState_GET() (python#106336) pythongh-106320: Remove private _PyInterpreterState functions (python#106335) pythongh-104922: Doc: add note about PY_SSIZE_T_CLEAN (python#106314) pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106329) pythongh-104922: remove PY_SSIZE_T_CLEAN (python#106315) ...

JelleZijlstra

The code looks good. My preference is to make the new API private (or unstable?) at least initially and make it public only if someone asks for it.

carljm · 2023-07-03T18:16:21Z

I'll rename it and make it private. I realized that I chose the name for better continuity with a possible future PEP 667, but I think this was wrong, and in fact this point argues the other direction: under PEP 667 it would return something quite different (not a dict), and so we shouldn't use the same name.

JelleZijlstra · 2023-07-03T18:33:21Z

Include/internal/pycore_ceval.h

@@ -154,6 +154,7 @@ extern PyObject* _Py_MakeCoro(PyFunctionObject *func);

 extern int _Py_HandlePending(PyThreadState *tstate);

+PyAPI_FUNC(PyObject *) _PyEval_GetLocals(void);


Does this still need to be a PyAPI_FUNC? Since it's only used in the core, seems like we could leave it as simply extern like the function immediately above.

I wasn't sure what was preferred here; there are also lots of examples of functions declared in pycore_*.h files (including this header) that use PyAPI_FUNC. But the definition of PyAPI_FUNC says it is for "public API functions," and I think you're right that it's not needed, so I'll remove it.

https://devguide.python.org/developer-workflow/c-api/#with-the-extern-keyword says to use extern when in doubt

I don't think extern does anything here, but some people prefer it for style reasons, and it seems like most (not all) non-static function declarations in this file use it, so I'll use it.

If the function is to be used by extension modules that are part of the standard library (or say they are, by defining Py_BUILD_CORE), which can be built as .so files, you need to use PyAPI_FUNC. Otherwise the symbol may not be exported on Windows (and I believe a few other platforms, like Android) or when using -fvisibility=hidden.

FWIW, I hadn't realised you're adding a _PyEval_GetLocals() when there's already a PyEval_GetLocals() that does a subtly different thing. I'm not fond of using the same name with just a _ difference here.

I could make it _PyEval_GetLocalsRef (c.f. python/steering-council#201) since the one notable difference is that it returns a new reference.

The other notable difference is the behavior inside an inlined comprehension in module or class scope, but I don't think there is any way to summarize that difference in a name.

This function will go away in 3.13 if we do PEP 667, or something like it, to standardize the behavior of locals().

Currently there's no use of the function in any standard library extension modules (PyEval_GetLocals was used only once in object.c and several times in bltinmodule.c), and I don't expect it to be needed in any, so I don't think we need to use PyAPI_FUNC.

Or, I could go back to using _PyEval_GetFrameLocals like I did originally in this PR (but this time with a leading underscore). If we get PEP 667, it would introduce PyEval_GetFrameLocals as public API, and the private _PyEval_GetFrameLocals wouldn't be needed anymore and could be removed.

* main: (39 commits) pythongh-102542 Remove unused bytes object and bytes slicing (python#106433) Clarify state of CancelledError in doc (python#106453) pythongh-64595: Fix regression in file write logic in Argument Clinic (python#106449) pythongh-104683: Rename Lib/test/clinic.test as Lib/test/clinic.test.c (python#106443) tp_flags docs: fix indentation (python#106420) pythongh-104050: Partially annotate Argument Clinic CLanguage class (python#106437) pythongh-106368: Add tests for formatting helpers in Argument Clinic (python#106415) pythongh-104050: Annotate Argument Clinic parameter permutation helpers (python#106431) pythongh-104050: Annotate toplevel functions in clinic.py (python#106435) pythongh-106320: Fix specialize.c compilation by including pycore_pylifecycle.h (python#106434) Add some codeowners for `Tools/clinic/` (python#106430) pythongh-106217: Truncate the issue body size of `new-bugs-announce-notifier` (python#106423) pythongh-61215: Rename `wait_until_any_call` to `wait_until_any_call_with` (python#106414) pythongh-106162: array: suppress warning in test_array (python#106404) pythongh-106320: Remove _PyInterpreterState_HasFeature() (python#106425) pythonGH-106360: Support very basic superblock introspection (python#106422) pythongh-106406: Fix _Py_IsInterpreterFinalizing() in _winapi.c (python#106408) pythongh-106396: Special-case empty format spec to gen empty JoinedStr node (python#106401) pythongh-106368: Add tests for permutation helpers in Argument Clinic (python#106407) pythonGH-106008: Fix refleak when peepholing `None` comparisons (python#106367) ...

…cals

miss-islington · 2023-07-05T23:05:06Z

Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

) * pythongh-105340: include hidden fast-locals in locals() (cherry picked from commit 104d7b7) Co-authored-by: Carl Meyer <carl@oddbird.net>

bedevere-bot · 2023-07-05T23:05:16Z

GH-106470 is a backport of this pull request to the 3.12 branch.

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

bedevere-bot mentioned this pull request Jun 12, 2023

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

Closed

pythongh-105340: include hidden fast-locals in locals()

5609f64

carljm force-pushed the modulelocals branch from 344c9af to 5609f64 Compare June 12, 2023 23:42

Eclips4 reviewed Jun 13, 2023

View reviewed changes

Python/bltinmodule.c Outdated Show resolved Hide resolved

carljm added 2 commits June 15, 2023 13:23

fixes

bd0c357

carljm marked this pull request as ready for review June 15, 2023 21:02

carljm requested a review from markshannon as a code owner June 15, 2023 21:02

bedevere-bot added the awaiting core review label Jun 15, 2023

carljm requested a review from Yhg1s June 16, 2023 00:28

JelleZijlstra reviewed Jun 16, 2023

View reviewed changes

carljm added 2 commits June 16, 2023 10:20

also fix dir()

1e27721

carljm requested a review from JelleZijlstra June 20, 2023 16:11

JelleZijlstra reviewed Jun 21, 2023

View reviewed changes

carljm added the needs backport to 3.12 bug and security fixes label Jul 3, 2023

JelleZijlstra approved these changes Jul 3, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 3, 2023

rename PyEval_GetFrameLocals to _PyEval_GetLocals and make it private

ab95d4b

JelleZijlstra reviewed Jul 3, 2023

View reviewed changes

don't use PyAPI_FUNC for _PyEval_GetLocals

7c33113

carljm added 2 commits July 5, 2023 10:46

Rename to _PyEval_GetFrameLocals, so its less similar to PyEval_GetLo…

6ec94bf

…cals

carljm merged commit 104d7b7 into python:main Jul 5, 2023

bedevere-bot removed the awaiting merge label Jul 5, 2023

carljm deleted the modulelocals branch July 5, 2023 23:05

bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jul 5, 2023

carljm mentioned this pull request Jul 11, 2023

PEP 667: add 709 impact, other tweaks python/peps#3196

Merged

carljm mentioned this pull request Sep 6, 2023

gh-108732: include comprehension locals in frame.f_locals #109026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

carljm commented Jun 12, 2023 •

edited

Loading

carljm commented Jun 16, 2023

JelleZijlstra left a comment

markshannon commented Jun 21, 2023

JelleZijlstra left a comment

JelleZijlstra Jun 21, 2023

carljm Jun 21, 2023 •

edited

Loading

carljm commented Jun 21, 2023

Yhg1s commented Jul 3, 2023

carljm commented Jul 3, 2023

JelleZijlstra left a comment

carljm commented Jul 3, 2023

JelleZijlstra Jul 3, 2023

carljm Jul 3, 2023

JelleZijlstra Jul 3, 2023

carljm Jul 3, 2023

Yhg1s Jul 3, 2023

carljm Jul 3, 2023 •

edited

Loading

carljm Jul 3, 2023

carljm Jul 3, 2023

miss-islington commented Jul 5, 2023

bedevere-bot commented Jul 5, 2023

		@@ -154,6 +154,7 @@ extern PyObject* _Py_MakeCoro(PyFunctionObject *func);

		extern int _Py_HandlePending(PyThreadState *tstate);

		PyAPI_FUNC(PyObject *) _PyEval_GetLocals(void);

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

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

Conversation

carljm commented Jun 12, 2023 • edited Loading

carljm commented Jun 16, 2023

JelleZijlstra left a comment

Choose a reason for hiding this comment

markshannon commented Jun 21, 2023

JelleZijlstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

carljm commented Jun 21, 2023

Yhg1s commented Jul 3, 2023

carljm commented Jul 3, 2023

JelleZijlstra left a comment

Choose a reason for hiding this comment

carljm commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Jul 5, 2023

bedevere-bot commented Jul 5, 2023

carljm commented Jun 12, 2023 •

edited

Loading

carljm Jun 21, 2023 •

edited

Loading

carljm Jul 3, 2023 •

edited

Loading