Trio103: except Cancelled/BaseException must be re-raised #8
Conversation
…eException block with a code path that doesn't re-raise the error> - couple stuff WIP/TODO
|
Approach looks good overall! |
jakkdl
changed the title
…t re-raising by way of creating a new BaseException/Cancelled, handle nested try's
|
I think it's fully finished now, so review away! I got almost all the way to a fully thorough parsing of nested except BaseException as e:
try:
raise
except ValueError:
raise # error, but parent except is fully covered if replaced with `raise e`
except:
raise eBut I can probably fix it if you want. Would require the beautiful |
|
OK, I've merged the others and cherry-picked a few parts of this too; I'll let you merge or rebase as you prefer. |
|
Fixed all comments. Was sloppy and did that in the same commit as the merge, so hope that doesn't mess up your review habits too much. |
…d comments, new exceptions are not accepted, bugs and rewrites and stuff.
… that files have been changed. Not the first time that causes me to push bad code
|
Merged in #14 and was glad I didn't have any mismatches at first, but then I got to reorganizing and restructuring the tests and ooh boi that was some work. Also came up with and added test cases for avoiding re-raising by abusing |
tests/trio103_104.py
Outdated
| # don't bypass raise by raising from nested except | ||
| except trio.Cancelled as e: | ||
| try: | ||
| pass | ||
| except ValueError as g: | ||
| raise g # error | ||
| except BaseException as h: | ||
| raise h # error ? | ||
| raise e |
There was a problem hiding this comment.
Need help thinking on what is going on here. Is it always wrong to do except <critical> while inside a except <critical>?
There was a problem hiding this comment.
No, nesting like that can make sense depending on the contents of the inner try: block.
There was a problem hiding this comment.
so: nested raise are not okay, unless the nested except is a handler for Cancelled/BaseException?
But hmm, this will bypass the Cancelled:
try:
...
except trio.Cancelled:
try:
print(1/0)
except:
raiseThere was a problem hiding this comment.
I propose we just ignore this for now - get the simple cases done in this PR, and open an issue for any follow-up which we can do (much) later.
There was a problem hiding this comment.
sure - would just like an executive decision on whether to be silent about nested critical excepts vs put up a warning and expect users to #noqa. Could add a third message "TRIO107 Warning: nested BaseException/trio.Cancelled" (or increment current 105&106 so they're adjacent - maybe good to not reserve error codes for WIP features in the future).
Putting that in would just be a few lines in visit_ExceptHandler so no big complexity hit.
If you just don't care / prefer status quo, you can just merge it as is - the PR should be done otherwise.
|
hehe, good luck! 😁 |
First draft, think I like the approach - although not totally sure how nested
try's should be handled. And I haven't looked up howraise .. fromshould be handled.This should probably have two different error messages and numbers, one for "reached end of block without re-raising" and another for "raising different error or returning".