python-goto
python-goto copied to clipboard
Various minor crash/etc fixes
This includes pull request #21 and #24 and the following fixes (with tests):
- Fixes for jumping out of try/with/except/finally statements (esp. in pypy and now-supported Python 3.8)
It also includes the following non-fix enhancements:
- Cache patched code blocks to avoid unnecessary patching when @with_goto is applied on a nested function.
- Add warnings if something goes wrong for easier debugging. (If you get a warning when applying with_goto, there's a very good chance the resulting function will misbehave or crash)
All locally tested on Python 2.6/2.7/3.4/3.8 & pypy2.7&3.6
Thanks, very much appreciated! But reviewing this huge PR is difficult. Let's land #21 separately, and if you don't mind can you move the other changes that are unrelated of supporting Python 3.8 to separate pull requests as well?
Also can you update tox.ini, .travis.yml and README.md in order to cover Python 3.8?
I've split this PR into py38 and non-py38. (This is the non-py38 PR and should be merged AFTER the py38 PR)
Splitting the PR further would be too much work, though, and it contains mostly related fixes (+ one unrelated minor change that should hopefully be easy to review together with the rest - just look at lines containing _patched_code_cache)
Let me know if you're unsure about why something's been added or changed, I'll gladly explain.
I didn't do a full review yet (as this pull request is difficult to review until rebased/merged against master) but I already have a couple remarks:
- I'm not a fan of the caching. It seems like a premature optimization and a potential memory leak. But I also fail to see a common scenario where the same code is passed through the
gotodecorator more than once. - Specifically detecting
__pypy__seems wrong to me. I rather stick to feature detection (e.g. compile some code, and inspect the resulting bytecode, like I did in order to detect Python 3.6+ wordcode, see the_Bytecodeclass and how its properties are initialized).
I've updated the changes per flake8 & your notes in the other PR.
To respond to your questions above:
- The common scenario that this handles is as follows:
def outer_func():
@with_goto
def inner_func():
<do something func with gotos>
return inner_func
Since the decorator runs every time outer_func is called, we would needlessly patch the function's bytecode every time (E.g. imagine outer_func is called in a heavy loop). This results not only in needless speed loss but also in needless creation of duplicate code objects. This is why caching is required and is not a premature optimization. As for a memory leak - that's what using a weak-keyed dictionary should resolve. (Supported in py2.7+). Plus, code objects usually have infinite scope, so it's not a real concern even in py2.6 (compared with the alternative).
- Hmm, that could work - I'll give it a try.
Additional notes:
-
I added two "noqa"s - one for the pypy import (will probably be removed per (2)) and one for a test checking the "except:" case (I moved most tests to check the "except Exception:" case, but keeping one good test for the "except:" case is important too.
-
Due to changing the "except" to "except Exception" in the testcases, I ended up finding a new issue. To resolve it, I made some additional code changes, including "backporting" a relatively major change from the next PR - code no longer relies on POP_BLOCK but instead looks at the jump argument supplied in the SETUP_* opcodes, which seems more reliable in several cases. (To be clear, the "backported" changes are relevant to this PR - the fixing of various crashes/etc in existing supported python-goto functionality)
-
Let me know if you want me to make the following improvement to the code: Use namedtuple instead of the brittle ,,x=y and y[0] approaches. (Supported in py26+ so should be good)
EDIT: I'll also do a refactor of _find_labels_and_gotos to make it use a class for the block_stack management, as it's currently quite brittle (especially due to the lack of python3.x's nonlocal) EDIT EDIT: Done.
About (2), while it's not possible to (normally?) detect this at runtime (since the generated opcodes are the same - their semantic is different), I realize I could largely remove the feature check altogether and always write the END_FINALLY when exiting finally/with blocks (previously - was only written for pypy).
In fact, one could well say that not calling END_FINALLY in such cases is a bug, just one that non-pypy pythons don't care about (of course, one could say the same about adding goto to python in general, but let's not go there :P)
Let me know if you'd rather have the pypy feature check over writing 2 extra opcodes (BEGIN_FINALLY/equivalent & END_FINALLY) in all relevant cases. The extra 2 opcodes shouldn't be a problem now that we can write how much we want.
Also, I went ahead a bit further and made some changes to fully handle the _BYTECODE.has_begin_finally flag by translating SETUP_FINALLY to SETUP_EXECPT once detected as well. This allowed me to remove the pypy feature check entirely (as opposed to under the !has_begin_finally feature check). It's not strictly required, but fits better with the "you should call END_FINALLY after finally/with blocks" narrative and will be helpful if we ever want to add other changes that rely on SETUP_EXCEPT.
This PR is now ready from my perspective. Let me know if you want any changes.