[mypyc] Fix AttributeError in async try/finally with mixed return paths#19361
[mypyc] Fix AttributeError in async try/finally with mixed return paths#19361JukkaL merged 3 commits intopython:masterfrom
Conversation
Async functions with try/finally blocks were raising AttributeError when: - Some paths in the try block return while others don't - The non-return path is executed at runtime - No further await calls are needed This occurred because mypyc's IR requires all control flow paths to assign to spill targets (temporary variables stored as generator attributes). The non-return path assigns NULL to maintain this invariant, but reading NULL attributes raises AttributeError in Python. Created a new IR operation `GetAttrNullable` that can read NULL attributes without raising AttributeError. This operation is used specifically in try/finally resolution when reading spill targets. - Added `GetAttrNullable` class to mypyc/ir/ops.py with error_kind=ERR_NEVER - Added `read_nullable_attr` method to IRBuilder for creating these operations - Modified `try_finally_resolve_control` in statement.py to use GetAttrNullable only for spill targets (attributes starting with '__mypyc_temp__') - Implemented C code generation in emitfunc.py that reads attributes without NULL checks and only increments reference count if not NULL - Added visitor implementations to all required files: - ir/pprint.py (pretty printing) - analysis/dataflow.py (dataflow analysis) - analysis/ircheck.py (IR validation) - analysis/selfleaks.py (self leak analysis) - transform/ir_transform.py (IR transformation) 1. **Separate operation vs flag**: Created a new operation instead of adding a flag to GetAttr for better performance - avoids runtime flag checks on every attribute access. 2. **Targeted fix**: Only applied to spill targets in try/finally resolution, not a general replacement for GetAttr. This minimizes risk and maintains existing behavior for all other attribute access. 3. **No initialization changes**: Initially tried initializing spill targets to Py_None instead of NULL, but this would incorrectly make try/finally blocks return None instead of falling through to subsequent code. Added two test cases to mypyc/test-data/run-async.test: 1. **testAsyncTryFinallyMixedReturn**: Tests the basic issue with async try/finally blocks containing mixed return/non-return paths. 2. **testAsyncWithMixedReturn**: Tests async with statements (which use try/finally under the hood) to ensure the fix works for this common pattern as well. Both tests verify that the AttributeError no longer occurs when taking the non-return path through the try block. See mypyc/mypyc#1115
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a comment about a style issue, overall looks good.
mypyc/ir/ops.py
Outdated
| return visitor.visit_get_attr(self) | ||
|
|
||
|
|
||
| class GetAttrNullable(GetAttr): |
There was a problem hiding this comment.
It would be more consistent with other ops to add a new flag to GetAttr instead of adding a subclass. For example, add allow_null: bool = False as a keyword-only argument to GetAttr.__init__, and set the error kind accordingly in __init__ when the flag is true, and a boolean attribute to GetAttr. I expect that this change would also reduce the the size of the PR. Can you make this change?
(Your approach clearly has some benefits over adding a flag, as it's arguably easy to forget to handle the flag in a visitor, but since we already have a bunch of flags that impact the semantics in Op subclasses, this is something that already needs to be remembered when writing a visitor.)
There was a problem hiding this comment.
I used the flag approach somewhere in my journey, my concern was that this might reduce performance as every GetAttr would be a tiny bit slower. That being said, if you prefer to have it that way, I guess I can make the change.
There was a problem hiding this comment.
I expect the performance impact to be quite small, since checking a boolean flag is quick (here branch prediction success rate should be high). I try to avoid subclasses of concrete classes such as GetAttr as a style convention, especially in new code, so I'd prefer to have this as a flag.
mypyc/codegen/emitfunc.py
Outdated
| elif not always_defined: | ||
| self.emitter.emit_line("}") | ||
|
|
||
| def visit_get_attr_nullable(self, op: GetAttrNullable) -> None: |
There was a problem hiding this comment.
If you implement the suggestion to not use a subclass, it still makes sense to keep this code in a separate method that is called from the visit_get_attr handler if the flag is set.
There was a problem hiding this comment.
Not sure I understand what you mean here
There was a problem hiding this comment.
Even if the functionality is merged from GetAttrNullable into the GetAttr class, and there is no longer a visit_get_attr_nullable method in OpVisitor, the code here doesn't need to be merged into the visit_get_attr method, but this method can be renamed and called from visit_get_attr method to handle the nullable case. I.e. visit_get_attr could look like this (some details omitted):
def visit_get_attr(self, op: GetAttr) -> None:
if op.allow_null:
self.get_attr_with_allow_null(op)
return
... <existing code of visit_get_attr>
def get_attr_with_allow_null(self, op: GetAttr) -> None:
... <similar to visit_get_attr_nullable in original version of this PR>
|
I've made the requested changes. Revised message (not sure how/if you actually use these for final commit squash, but information has changed, so) [mypyc] Fix AttributeError in async try/finally with mixed return paths Async functions with try/finally blocks were raising AttributeError when:
This occurred because mypyc's IR requires all control flow paths to assign Modified the GetAttr IR operation to support reading NULL attributes without raising AttributeError through a new
Design decisions:
Added two test cases to mypyc/test-data/run-async.test:
Both tests verify that the AttributeError no longer occurs when taking See mypyc/mypyc#1115 |
It's fine to edit the top-level PR summary if you make changes, and maybe leave a comment saying something like "updated summary to reflect the current state of the PR". I can do this for you now. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Thanks for the updates, looks good!
…hs (#19361) Async functions with try/finally blocks were raising AttributeError when: * Some paths in the try block return while others don't * The non-return path is executed at runtime * No further await calls are needed This occurred because mypyc's IR requires all control flow paths to assign to spill targets. The non-return path assigns NULL to maintain this invariant, but reading NULL attributes raises AttributeError in Python. Modified the GetAttr IR operation to support reading NULL attributes without raising AttributeError through a new allow_null parameter. This parameter is used specifically in try/finally resolution when reading spill targets. * Added allow_null: bool = False parameter to GetAttr.init in mypyc/ir/ops.py * When allow_null=True, sets error_kind=ERR_NEVER to prevent AttributeError * Modified read_nullable_attr in IRBuilder to create GetAttr with allow_null=True * Modified try_finally_resolve_control in statement.py to use read_nullable_attr only for spill targets (attributes starting with 'mypyc_temp') * Updated C code generation in emitfunc.py: * visit_get_attr checks for allow_null and delegates to get_attr_with_allow_null * get_attr_with_allow_null reads attributes without NULL checks and only increments reference count if not NULL Design decisions: * Targeted fix: Only applied to spill targets in try/finally resolution, not a general replacement for GetAttr. This minimizes risk and maintains existing behavior for all other attribute access. * No initialization changes: Initially tried initializing spill targets to Py_None instead of NULL, but this would incorrectly make try/finally blocks return None instead of falling through to subsequent code. Added two test cases to mypyc/test-data/run-async.test: * testAsyncTryFinallyMixedReturn: Tests the basic issue with async try/finally blocks containing mixed return/non-return paths. * testAsyncWithMixedReturn: Tests async with statements (which use try/finally under the hood) to ensure the fix works for this common pattern as well. Both tests verify that the AttributeError no longer occurs when taking the non-return path through the try block. See mypyc/mypyc#1115
2 participants
Async functions with try/finally blocks were raising AttributeError when:
This occurred because mypyc's IR requires all control flow paths to assign
to spill targets. The non-return path assigns NULL to maintain this invariant, but reading NULL attributes raises AttributeError in Python.
Modified the GetAttr IR operation to support reading NULL attributes without raising AttributeError through a new allow_null parameter. This parameter is used specifically in try/finally resolution when reading spill targets.
only for spill targets (attributes starting with 'mypyc_temp')
increments reference count if not NULL
Design decisions:
Targeted fix: Only applied to spill targets in try/finally resolution,
not a general replacement for GetAttr. This minimizes risk and maintains
existing behavior for all other attribute access.
No initialization changes: Initially tried initializing spill targets
to Py_None instead of NULL, but this would incorrectly make try/finally
blocks return None instead of falling through to subsequent code.
Added two test cases to mypyc/test-data/run-async.test:
testAsyncTryFinallyMixedReturn: Tests the basic issue with async
try/finally blocks containing mixed return/non-return paths.
testAsyncWithMixedReturn: Tests async with statements (which use
try/finally under the hood) to ensure the fix works for this common
pattern as well.
Both tests verify that the AttributeError no longer occurs when taking
the non-return path through the try block.
See mypyc/mypyc#1115