Repository navigation
Conversation
craigbarratt
left a comment
There was a problem hiding this comment.
Thanks for this, and for the CPython comparison test, which is a great way to pin down the semantics. Exiting only the managers that were entered is the right fix for #881.
I found a few places where the new code still differs from CPython, mostly because the exits now run after the except block instead of inside it. I checked each case below by running it through CPython and through pyscript with this PR.
Suggestion: nest single-item with statements recursively
Instead of collecting the error and replaying it by hand, each item could be handled like its own single-item with that wraps the rest. Every exit then runs inside a real except handler, so Python handles the active exception, __context__ chaining and the re-raise for us:
async def ast_with(self, arg, async_attr=""):
"""Execute with statement; several items behave like nested with statements."""
return await self.with_items(arg, arg.items, async_attr)
async def with_items(self, arg, items, async_attr):
"""Enter the first manager, run the remaining items and body inside it, then exit it."""
if not items:
val = None
for arg1 in arg.body:
val = await self.aeval(arg1)
if isinstance(val, EvalStopFlow):
break
return val
item = items[0]
manager = await self.aeval(item.context_expr)
enter_attr = f"__{async_attr}enter__"
exit_attr = f"__{async_attr}exit__"
enter_func = getattr(type(manager), enter_attr)
exit_func = getattr(type(manager), exit_attr)
value = await self.call_func(enter_func, enter_attr, manager)
try:
if item.optional_vars:
await self.recurse_assign(item.optional_vars, value)
val = await self.with_items(arg, items[1:], async_attr)
except (asyncio.CancelledError, KeyboardInterrupt, SystemExit):
# unlike Python, the exit can't suppress these, so tasks can always be stopped
await self.call_func(exit_func, exit_attr, manager, *sys.exc_info())
raise
except Exception:
if not await self.call_func(exit_func, exit_attr, manager, *sys.exc_info()):
raise
return None
await self.call_func(exit_func, exit_attr, manager, None, None, None)
return valI tried this locally. It passes your test_with_matches_python unchanged and also matches CPython on the cases in points 1 and 2 below. It needs no error variable, so the reference cycle in point 4 goes away too. If you'd rather keep the current structure, points 1 and 2 still need fixing.
1. No exception is active while an exit runs
The exits now run after the except block has finished, so sys.exc_info() is (None, None, None) inside __exit__. A pyscript __exit__ that does a bare raise now fails with RuntimeError: No active exception to reraise instead of re-raising the body's error. logging.exception() / log.error(..., exc_info=True) inside an exit also loses the traceback. The old code called the exits from inside the except, so this is a regression.
2. The body's error can drop out of the __context__ chain
exc.__context__ is only set when it is None. If an exit raises from inside its own except block (e.g. except KeyError: raise IndexError), the new error's context is already the KeyError, so the body's original error is lost. CPython gives IndexError → KeyError → ZeroDivisionError, but this PR gives IndexError → KeyError.
3. Don't let exits suppress CancelledError, KeyboardInterrupt or SystemExit
CancelledError used to always propagate. With except BaseException, an exit that returns True (common for "ignore errors" managers) now swallows it. That stops task.cancel(), task.unique() and pyscript reloads from killing the task, which leaves it running. I confirmed this: with a suppressing manager around task.sleep(), cancelling the task runs the code after the with and the task keeps going.
Please still call the exits with these exceptions so resources are released, but always re-raise them (as in the sketch above). This is a deliberate difference from Python, so please add a bullet to the "Language Limitations" list in docs/reference.rst, something like:
- A context manager's
__exit__(or__aexit__) is called when a task is cancelled, but returning a true value doesn't suppressCancelledError,KeyboardInterruptorSystemExitthe way it does in Python. This ensures pyscript tasks can always be stopped.
Tests
Please extend the CPython comparison with cases that check what sys.exc_info() sees during __exit__, an exit with a bare raise, an exit that raises a chained exception, and return/break inside the body. Also add a pyscript-only test that a suppressing manager doesn't swallow cancellation. Points 1 and 2 pass the current test, so it doesn't catch them.
The pyscript subclass __init__ issue you mentioned is unrelated; please open a separate issue for it.
| if not hit_except: | ||
| for ctx in reversed(ctx_list): | ||
| await self.call_func(ctx["exit"], exit_attr, ctx["manager"], None, None, None) | ||
| except BaseException as exc: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
With BaseException, an exit returning True now suppresses CancelledError, so task.cancel()/task.unique() can no longer stop the task (see point 3 in the review). Please run the exits for CancelledError, KeyboardInterrupt and SystemExit but always re-raise them.
| try: | ||
| if error is None: | ||
| await self.call_func(exit_func, exit_attr, manager, None, None, None) | ||
| elif await self.call_func( |
There was a problem hiding this comment.
This runs after the except block has finished, so sys.exc_info() is empty inside __exit__. A bare raise in a pyscript __exit__ gives RuntimeError: No active exception to reraise (point 1).
| error = None | ||
| except BaseException as exc: # pylint: disable=broad-exception-caught | ||
| if error is not None and exc is not error and exc.__context__ is None: | ||
| exc.__context__ = error |
There was a problem hiding this comment.
When the exit's exception already has a __context__ (raised from inside an except in __exit__), the body's error is dropped from the chain. CPython keeps it at the end: IndexError → KeyError → ZeroDivisionError (point 2).
| exc.__context__ = error | ||
| error = exc | ||
| if error is not None: | ||
| raise error |
There was a problem hiding this comment.
Minor: re-raising a long-lived local creates a frame → exception → traceback → frame cycle, which keeps the managers alive until GC. If you keep this structure, use try: raise error / finally: error = None. The recursive version in the review avoids it.
| import ast as ast_module | ||
| import contextlib | ||
| from dataclasses import dataclass | ||
| import types |
There was a problem hiding this comment.
Nit: from types import ModuleType is already imported. Use from types import FunctionType, ModuleType instead of adding import types.
Fixes #881
ast_withevaluated all managers first, then entered them, and on an error called the exit of every manager in the list, including the one whose enter had just raised. With an@asynccontextmanager(e.g. httpxAsyncClient.stream()on a refused connection) that turned the real error intoRuntimeError: generator didn't stop after athrow().It now follows Python's semantics for several items, i.e. nested
withstatements:None(before, every exit had to return true)__context__CancelledErroris passed to the exits too (before they gotNone)This is mostly what @jfriend00 suggested in the issue, plus the last two points.
The new
test_with_matches_pythonruns the same source through CPython and through pyscript and compares the results: 7 cases forwithand the same 7 forasync with(enter fails, inner or outer exit suppresses, exits raise, no error), plus a generator based manager whose enter fails. It fails on master and passes with this change.I could only run the interpreter tests on Windows with a stand-in for
hass(Home Assistant needsfcntlthere), so I'd rely on CI for the full suite. In that setup theevalTestsandevalTestsExceptionscases that don't need a real hass give the same results before and after this change.One thing I ran into while writing the test: a pyscript class that subclasses another pyscript class doesn't inherit its
__init__(ACtx() takes no arguments), so the async manager in the test wrapsCtxinstead of subclassing it. That's separate from this fix.