bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError #949
Conversation
|
@svelankar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ncoghlan, @berkerpeksag and @Yhg1s to be potential reviewers. |
Lib/contextlib.py
Outdated
| raise | ||
| if sys.exc_info()[1] is value: | ||
| return False | ||
| raise | ||
| else: |
There was a problem hiding this comment.
@ncoghlan suggested to drop this else: clause.
Lib/test/test_contextlib.py
Outdated
| @@ -152,6 +152,28 @@ def woohoo(): | |||
| else: | |||
| self.fail('StopIteration was suppressed') | |||
|
|
|||
| def test_contextmanager_do_not_unchain_non_stopiteration_exceptions(self): | |||
| code = """\ | |||
| from __future__ import generator_stop | |||
Lib/test/test_contextlib.py
Outdated
| try: | ||
| yield | ||
| except Exception as exc: | ||
| raise RuntimeError from exc |
There was a problem hiding this comment.
Add some argument to the raised RuntimeError for checking that the correct RuntimeError is caught.
| with test_issue29692(): | ||
| raise ZeroDivisionError | ||
| except Exception as ex: | ||
| self.assertIs(type(ex), RuntimeError) |
There was a problem hiding this comment.
Check that this is a RuntimeError raised by test_issue29692, not some other RuntimeError. Check it's __cause__ (it should be ZeroDivisionError, right?).
Lib/test/test_contextlib.py
Outdated
|
|
||
| try: | ||
| with test_issue29692(): | ||
| raise ZeroDivisionError |
There was a problem hiding this comment.
What if raise StopIteration? I think this case should be tested too.
|
Please add an entry in |
soolabettu
force-pushed
the
bpo-29692
branch
2 times, most recently
from
ecf5deb to
9538527
Compare
|
Implemented all changes, please review, thanks. |
Lib/test/test_contextlib.py
Outdated
| raise RuntimeError('issue29692:Chained') from exc | ||
| """ | ||
| locals = {} | ||
| exec(code, locals, locals) |
There was a problem hiding this comment.
Since from __future__ import generator_stop no longer used, the trick with exec() no longer needed.
Lib/test/test_contextlib.py
Outdated
| except Exception as ex: | ||
| self.assertIs(type(ex), StopIteration) | ||
| self.assertEqual(ex.args[0], 'issue29692:Unchained') | ||
| self.assertIs(ex.__cause__, None) |
Misc/NEWS
Outdated
| @@ -303,6 +303,10 @@ Extension Modules | |||
| Library | |||
| ------- | |||
|
|
|||
| - bpo-29692: Fixed arbitrary unchaining of RuntimeError exceptions in | |||
| contextlib.contextmanager | |||
There was a problem hiding this comment.
Add periods after sentence ends.
|
@serhiy-storchaka , thanks for the review. |
|
@ncoghlan could you please review the changes? Thanks |
|
Thanks for the fix, @svelankar! |
…ntimeError (pythonGH-949) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (GH-949) (#1105) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (pythonGH-949) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
…ntimeError (GH-949) (#1107) contextlib._GeneratorContextManager.__exit__ includes a special case to deal with PEP 479 RuntimeErrors created when `StopIteration` is thrown into the context manager body. Previously this check was too permissive, and undid one level of chaining on *all* RuntimeError instances, not just those that wrapped a StopIteration instance. (cherry picked from commit 00c75e9)
@ncoghlan @serhiy-storchaka @JelleZijlstra @brettcannon
I had to close PR 891 and open a new PR due to an incorrect rebase on my part.
All review comments made in PR 891 have been implemented and a test case added, please review, thanks.