Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-105340: include hidden fast-locals in locals()#105715
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* 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)
Uh oh!
There was an error while loading.Please reload this page.
* main: (57 commits)pythongh-105831: Fix NEWS blurb frompythongh-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 commentedJun 16, 2023
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You also need to changedir() (implemented in_dir_locals inObjects/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 darwinType "help", "copyright", "credits" or "license" for more information.>>> [dir() for i in range(1)][['__annotations__', '__builtins__', '__doc__', '__loader__', '__name__', '__package__', '__spec__']]>>> ^D% python3.11Python 3.11.1 (main, Dec 21 2022, 16:19:04) [Clang 14.0.0 (clang-1400.0.29.202)] on darwinType "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 commentedJun 21, 2023
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.
I'd be happy to change the PEP to always return a proxy. I don't have an opinion on what is the best approach for 3.12. |
JelleZijlstra left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Include/cpython/ceval.h Outdated
| PyAPI_FUNC(int)_PyEval_SliceIndex(PyObject*,Py_ssize_t*); | ||
| PyAPI_FUNC(int)_PyEval_SliceIndexNotNone(PyObject*,Py_ssize_t*); | ||
| PyAPI_FUNC(PyObject*)PyEval_GetFrameLocals(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 callPyEval_GetLocals()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If there are C extensions callingPyEval_GetLocals() while within a class or module-scoped comprehension and running into the equivalent of the OP case, then they would want to switch toPyEval_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 commentedJun 21, 2023
A new dictionary is only returned when
As I see it, we have three options for this issue:
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?
👍 |
Yhg1s commentedJul 3, 2023
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 isprobably good enough, and the complexity of the change is acceptable. Either is acceptable for 3.12. |
carljm commentedJul 3, 2023
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 |
* 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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedJul 3, 2023
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. |
Include/internal/pycore_ceval.h Outdated
| externint_Py_HandlePending(PyThreadState*tstate); | ||
| PyAPI_FUNC(PyObject*)_PyEval_GetLocals(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this still need to be a PyAPI_FUNC? Since it's only used in the core, seems like we could leave it as simplyextern like the function immediately above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wasn't sure what was preferred here; there are also lots of examples of functions declared inpycore_*.h files (including this header) that usePyAPI_FUNC. But the definition ofPyAPI_FUNC says it is for "public API functions," and I think you're right that it's not needed, so I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://devguide.python.org/developer-workflow/c-api/#with-the-extern-keyword says to use extern when in doubt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't thinkextern 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 aPyEval_GetLocals() that does a subtly different thing. I'm not fond of using the same name with just a_ difference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 oflocals().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Currently there's no use of the function in any standard library extension modules (PyEval_GetLocals was used only once inobject.c and several times inbltinmodule.c), and I don't expect it to be needed in any, so I don't think we need to usePyAPI_FUNC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 introducePyEval_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) ...
miss-islington commentedJul 5, 2023
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 commit104d7b7)Co-authored-by: Carl Meyer <carl@oddbird.net>
bedevere-bot commentedJul 5, 2023
GH-106470 is a backport of this pull request to the3.12 branch. |
Uh oh!
There was an error while loading.Please reload this page.
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()(orvars()) 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 inlocals()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 tolocals()(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_GetLocalsand the frame functions it calls, except for the unfortunate detail thatPyEval_GetLocalscurrently 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 introducesPyEval_GetFrameLocals, which is instead defined to return a new reference, and uses it inlocals(),vars(),dir(),exec()andeval()(all the locations wherePyEval_GetLocalswas 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_FastToLocalswould disappear entirely, and this fix would just naturally fall out of the behavior oflocals()as returning a mapping object that is a consistent view on the current locals namespace. PEP 667 also explicitly deprecates the borrowed-reference-returningPyEval_GetLocalset al and replaces them with new-reference-returning forms. I chose the namePyEval_GetFrameLocalsin this PR so as to maximize compatibility with a possible PEP 667 future.