[mypyc] Fix AttributeError in async try/finally with mixed return paths #19361
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
| @@ -799,6 +799,24 @@ def accept(self, visitor: OpVisitor[T]) -> T: | |||
| 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.
| @@ -426,6 +427,24 @@ def visit_get_attr(self, op: GetAttr) -> None: | |||
| 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>
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 (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
GetAttrNullablethat can read NULL attributes without raising AttributeError. This operation is used specifically in try/finally resolution when reading spill targets.GetAttrNullableclass to mypyc/ir/ops.py with error_kind=ERR_NEVERread_nullable_attrmethod to IRBuilder for creating these operationstry_finally_resolve_controlin statement.py to use GetAttrNullable only for spill targets (attributes starting with 'mypyc_temp')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.
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