Sorry, something went wrong.
There was a problem hiding this comment.
rebase needed and build failures needed addressing.
Sorry, something went wrong.
|
@njsmith Aside from the TODO's in the PR itself, this is a pretty complete implementation of the current version of PEP 558 now. |
Sorry, something went wrong.
|
One other known issue: the header file updates still need to be modified to conform with the new conventions. |
Sorry, something went wrong.
|
I don't know if this is already addressed elsewhere, but I'm getting a strange error. Running the below script with python.exe built with this branch (I removed an unused parameter to make it compile): def foo():
import pdb; pdb.set_trace()
x=1
foo()
gives $ ./python.exe jump.py
> /Users/henry/pep558/cpython/jump.py(3)foo()
-> x=1
(Pdb) x=123
(Pdb) c
python.exe(69485,0x10961d5c0) malloc: *** error for object 0x1029ee790: pointer being freed was not allocated
python.exe(69485,0x10961d5c0) malloc: *** set a breakpoint in malloc_error_break to debug
Abort trap: 6
but this error is absent for certain values of x, say: $ ./python.exe jump.py
> /Users/henry/pep558/cpython/jump.py(3)foo()
-> x=1
(Pdb) x=1234
(Pdb) c
$
|
Sorry, something went wrong.
|
Hmm, looks like I may have a refcounting bug, and the small integer cache is able to provoke it into crashing. |
Sorry, something went wrong.
|
I can report that not all small numbers trigger this error. On my machine x = 11, say, is fine. Most perplexing! |
Sorry, something went wrong.
|
Running test_frame consistently triggers a crash in test_clear_locals for me, so I'm focusing on that initially. |
Sorry, something went wrong.
|
Last push resolves the test_frame segfault, but adds a TODO noting that the locals proxy may still misbehave after a frame has been cleared (or is in the process of being destroyed). There's also a threading test which checks reference cycles aren't being created through the frames, which is currently failing (quite possibly for a related reason) |
Sorry, something went wrong.
|
Ugh, the refcyle issue detected by test_threading is kinda inherent in the current implementation: once frame.f_locals gets created, there's now a cyclic reference between the frame and its own locals proxy. I'm going to try forcibly breaking the cycle when an optimized frame exits (before the eval loop drops its own reference). |
Sorry, something went wrong.
|
I've pushed an update that resolves almost all of my own "PEP 558 TODO" items in the code (and ticked them off in the checklist above as well):
I will be adding another todo item to the checklist, which is to explicitly test the handling of cleared frames. Implementing proxy.clear() exposed some inconsistencies between the established FastToLocals() logic and the proxy's behaviour when accessing cell variables via a cleared frame, but no unit tests failed when I changed the behaviour to be consistent with the FastToLocals() precedent. For the merge conflict, I'm hoping to get #27525 merged before I try to sync this branch with the main branch again. It was my first attempt at doing that which prompted me to file bpo-44800 in the first place, as it was quite unclear how the conflicts should be resolved given the currently checked in variable naming conventions. |
Sorry, something went wrong.
|
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
I have a concern about raising RuntimeError from PyFrame_LocalsToFast. Was it possible for PyFrame_LocalsToFast before this change to ever raise RuntimeError for any other reason? If not, then it would be nice to have that explicitly documented (at least in the PEP, maybe elsewhere if appropriate and not too noisy to include it). But RuntimeError is raised for internal problems, like if memory allocations fail, right? So it seems like the kind of error type that's liable to fly out of any internal function, with no real guarantees that it can't? So I think it might be a bit of a challenge for third parties to write code that
Because it seems like code released today:
For what it's worth, this isn't insurmountable - in my little toy library that mutates functions' variables through f_locals, I settled on trying to modify a variable in the frame of a test function call, checking if that worked, and only if it didn't, I try to import the locals-to-fast stuff. I think this is probably the best in terms of portability across all versions of all implementations of Python where mutating a frame's locals is possible, and elegantly avoids ever causing the RuntimeError that this PEP adds on any implementation+version of Python that implements this PEP. But it takes all this code, might be fragile in other ways or have other downsides I haven't realized yet, and wasn't trivial to think of. TL;DR: because change takes PyFrame_LocalsToFast from necessary for certain goals to unusable, the added error should be programmatically easy to distinguish for the code that was using it while it was necessary. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
Closing as per the deferral notice in https://github.com/python/peps/pull/3050/files |
Sorry, something went wrong.
(Declined as per the PEP deferral notice in https://github.com/python/peps/pull/3050/files - this reference implementation was useful at the time, but after the frame management changes in Python 3.11 any implementation of the updated semantics in PEP 558 or PEP 667 would be best off starting from scratch. The test cases from this branch might still be useful, but those can always be copied out to a new branch easily enough)
Previously, trace hooks running on a nested function
could incorrectly reset the state of a closure cell.
This avoids that by changing the semantics of the
f_locals namespace on function frames to use a
write-through proxy, such that changes are made
immediately rather than being written back some
arbitrary time later.
PEP 558 and bpo-17960 cover additional changes
to locals() and frame.f_locals semantics that are
part of this update.
Remaining TODO items before this PR could be considered complete:
Desirable code structure improvements (current plan for these is to put any duplicated into a sharable location in this PR, but file separate maintainability issues to migrate to using the shared code in the original locations, to avoid the diff size impact on this PR):
https://bugs.python.org/issue30744