-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
PyEval_GetLocals()
leaks locals
#118934
Comments
This is tricky, because after PEP 669, the reference So we either
Which is the lesser of two evils? |
i.e. a proxy should be created on first acccess, and then returned. Is that not an option any more? |
I had a fix in #119769. It's not the prettiest solution, but it should work. We don't have a plan to deprecate this API yet, but it should be discouraged to use |
It turns out there's an internal inconsistency in PEP 667 here. When writing the docs updates in #119893 I was going off https://peps.python.org/pep-0667/#pyeval-getlocals stating that the "proxy mapping" would be cached on the frame, and https://peps.python.org/pep-0667/#changes-to-existing-apis saying "The semantics of I missed the subsequent definition in terms of Caching a snapshot (as suggested in the sample code) rather than a proxy instance (as suggested in the prose) would mean that updates made via the return value of That interoperability issue is avoided if |
I think it's better to keep the behavior of This is consistent because in that way:
are borrowed version of
to their python equivalent
Changing the semantics of |
We'll need to get confirmation from the SC then, since that's definitely not what the prose in the accepted PEP 667 says.
As written, aside from the reference to This approach makes sense, since using shared mutable storage is only beneficial if other consumers of that storage can see the changes that you make. If Ensuring that |
After further reflection, I've come around to the point of view that reverting The cases that were concerning me were mainly tracing functions implemented in C, but those are going to be calling I'll post new PRs bringing the docs and PEP into line with |
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed).
…yeval_getlocals-docs
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for gh-74929 didn't match either the current behaviour or the intended behaviour once gh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once gh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
See Implementation Notes in the updated PEP for details of the correction/clarification and the rationale for it. The discrepancy was detected when writing the documentation for python/cpython#74929 and when reviewing python/cpython#118934
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added) (cherry picked from commit fd6cd62) Co-authored-by: Alyssa Coghlan <[email protected]>
PEP text clarification PR posted here: python/peps#3809 SC ratification requested here: python/steering-council#245 (I'll be genuinely surprised if they object, but I figure it's better to be fully transparent about it) I've already updated the docs to reflect the way |
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for gh-74929 didn't match either the current behaviour or the intended behaviour once gh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once gh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added) (cherry picked from commit fd6cd62) Co-authored-by: Alyssa Coghlan <[email protected]>
I really think we should implement It seems that all the complication stems from wanting to return a |
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
This proved complex enough that it became a question for the Steering Council in python/steering-council#245 (the PEP text was internally inconsistent, with different sections suggesting different behaviour for @Yhg1s I added the deferred blocker label (if we don't get this resolved for the final beta, it should definitely be resolved before the first release candidate) |
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
Co-authored-by: Alyssa Coghlan <[email protected]>
…honGH-119769) (cherry picked from commit e65cb4c) Co-authored-by: Tian Gao <[email protected]> Co-authored-by: Alyssa Coghlan <[email protected]>
PEP 667's description of the planned changes to PyEval_GetLocals was internally inconsistent when accepted, so the docs added for pythongh-74929 didn't match either the current behaviour or the intended behaviour once pythongh-118934 is fixed. This PR updates the documentation and 3.13 What's New to match the intended behaviour (once pythongh-118934 is fixed). It also tidies up lingering references to `f_locals` always being a dictionary (this hasn't been true since at least when custom namespace support for class statement execution was added)
…-119769) (#121869) gh-118934: Make PyEval_GetLocals return borrowed reference (GH-119769) (cherry picked from commit e65cb4c) Co-authored-by: Tian Gao <[email protected]> Co-authored-by: Alyssa Coghlan <[email protected]>
|
I think something went wrong with the behavior revert since the linked commit is now causing assertion errors in |
Bug report
PyEval_GetLocals()
is documented as returning a borrowed reference. It now returns a new reference, which causes callers to leak the local variables:cpython/Python/ceval.c
Lines 2478 to 2479 in 35c4361
cc @gaogaotiantian @markshannon
Linked PRs
The text was updated successfully, but these errors were encountered: