Skip to content

Commit 7b0abac

Browse files
committed
[mypyc] Fix AttributeError in async try/finally with mixed return paths
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
1 parent 5e9d657 commit 7b0abac

File tree

10 files changed

+401
-2
lines changed

10 files changed

+401
-2
lines changed

mypyc/analysis/dataflow.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
FloatNeg,
2525
FloatOp,
2626
GetAttr,
27+
GetAttrNullable,
2728
GetElementPtr,
2829
Goto,
2930
IncRef,
@@ -209,6 +210,9 @@ def visit_load_literal(self, op: LoadLiteral) -> GenAndKill[T]:
209210
def visit_get_attr(self, op: GetAttr) -> GenAndKill[T]:
210211
return self.visit_register_op(op)
211212

213+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> GenAndKill[T]:
214+
return self.visit_register_op(op)
215+
212216
def visit_set_attr(self, op: SetAttr) -> GenAndKill[T]:
213217
return self.visit_register_op(op)
214218

mypyc/analysis/ircheck.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
FloatNeg,
2222
FloatOp,
2323
GetAttr,
24+
GetAttrNullable,
2425
GetElementPtr,
2526
Goto,
2627
IncRef,
@@ -319,6 +320,10 @@ def visit_get_attr(self, op: GetAttr) -> None:
319320
# Nothing to do.
320321
pass
321322

323+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> None:
324+
# Nothing to do.
325+
pass
326+
322327
def visit_set_attr(self, op: SetAttr) -> None:
323328
# Nothing to do.
324329
pass

mypyc/analysis/selfleaks.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
FloatNeg,
1717
FloatOp,
1818
GetAttr,
19+
GetAttrNullable,
1920
GetElementPtr,
2021
Goto,
2122
InitStatic,
@@ -114,6 +115,13 @@ def visit_get_attr(self, op: GetAttr) -> GenAndKill:
114115
return self.check_register_op(op)
115116
return CLEAN
116117

118+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> GenAndKill:
119+
cl = op.class_type.class_ir
120+
if cl.get_method(op.attr):
121+
# Property -- calls a function
122+
return self.check_register_op(op)
123+
return CLEAN
124+
117125
def visit_set_attr(self, op: SetAttr) -> GenAndKill:
118126
cl = op.class_type.class_ir
119127
if cl.get_method(op.attr):

mypyc/codegen/emitfunc.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
FloatNeg,
4141
FloatOp,
4242
GetAttr,
43+
GetAttrNullable,
4344
GetElementPtr,
4445
Goto,
4546
IncRef,
@@ -426,6 +427,24 @@ def visit_get_attr(self, op: GetAttr) -> None:
426427
elif not always_defined:
427428
self.emitter.emit_line("}")
428429

430+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> None:
431+
"""Handle GetAttrNullable which allows NULL without raising AttributeError."""
432+
dest = self.reg(op)
433+
obj = self.reg(op.obj)
434+
rtype = op.class_type
435+
cl = rtype.class_ir
436+
attr_rtype, decl_cl = cl.attr_details(op.attr)
437+
438+
# Direct struct access without NULL check
439+
attr_expr = self.get_attr_expr(obj, op, decl_cl)
440+
self.emitter.emit_line(f"{dest} = {attr_expr};")
441+
442+
# Only emit inc_ref if not NULL
443+
if attr_rtype.is_refcounted and not op.is_borrowed:
444+
self.emitter.emit_line(f"if ({dest} != NULL) {{")
445+
self.emitter.emit_inc_ref(dest, attr_rtype)
446+
self.emitter.emit_line("}")
447+
429448
def next_branch(self) -> Branch | None:
430449
if self.op_index + 1 < len(self.ops):
431450
next_op = self.ops[self.op_index + 1]

mypyc/ir/ops.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,35 @@ def accept(self, visitor: OpVisitor[T]) -> T:
799799
return visitor.visit_get_attr(self)
800800

801801

802+
class GetAttrNullable(RegisterOp):
803+
"""obj.attr (for a native object) - allows NULL without raising AttributeError
804+
805+
This is used for spill targets where NULL indicates the non-return path was taken.
806+
Unlike GetAttr, this won't raise AttributeError when the attribute is NULL.
807+
"""
808+
809+
error_kind = ERR_NEVER
810+
811+
def __init__(self, obj: Value, attr: str, line: int, *, borrow: bool = False) -> None:
812+
super().__init__(line)
813+
self.obj = obj
814+
self.attr = attr
815+
assert isinstance(obj.type, RInstance), "Attribute access not supported: %s" % obj.type
816+
self.class_type = obj.type
817+
attr_type = obj.type.attr_type(attr)
818+
self.type = attr_type
819+
self.is_borrowed = borrow and attr_type.is_refcounted
820+
821+
def sources(self) -> list[Value]:
822+
return [self.obj]
823+
824+
def set_sources(self, new: list[Value]) -> None:
825+
(self.obj,) = new
826+
827+
def accept(self, visitor: OpVisitor[T]) -> T:
828+
return visitor.visit_get_attr_nullable(self)
829+
830+
802831
class SetAttr(RegisterOp):
803832
"""obj.attr = src (for a native object)
804833
@@ -1728,6 +1757,10 @@ def visit_load_literal(self, op: LoadLiteral) -> T:
17281757
def visit_get_attr(self, op: GetAttr) -> T:
17291758
raise NotImplementedError
17301759

1760+
@abstractmethod
1761+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> T:
1762+
raise NotImplementedError
1763+
17311764
@abstractmethod
17321765
def visit_set_attr(self, op: SetAttr) -> T:
17331766
raise NotImplementedError

mypyc/ir/pprint.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
FloatNeg,
2929
FloatOp,
3030
GetAttr,
31+
GetAttrNullable,
3132
GetElementPtr,
3233
Goto,
3334
IncRef,
@@ -128,6 +129,9 @@ def visit_load_literal(self, op: LoadLiteral) -> str:
128129
def visit_get_attr(self, op: GetAttr) -> str:
129130
return self.format("%r = %s%r.%s", op, self.borrow_prefix(op), op.obj, op.attr)
130131

132+
def visit_get_attr_nullable(self, op: GetAttrNullable) -> str:
133+
return self.format("%r = %s%r.%s?", op, self.borrow_prefix(op), op.obj, op.attr)
134+
131135
def borrow_prefix(self, op: Op) -> str:
132136
if op.is_borrowed:
133137
return "borrow "

mypyc/irbuild/builder.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
Branch,
7373
ComparisonOp,
7474
GetAttr,
75+
GetAttrNullable,
7576
InitStatic,
7677
Integer,
7778
IntOp,
@@ -708,6 +709,15 @@ def read(
708709

709710
assert False, "Unsupported lvalue: %r" % target
710711

712+
def read_nullable_attr(self, obj: Value, attr: str, line: int = -1) -> Value:
713+
"""Read an attribute that might be NULL without raising AttributeError.
714+
715+
This is used for reading spill targets in try/finally blocks where NULL
716+
indicates the non-return path was taken.
717+
"""
718+
assert isinstance(obj.type, RInstance) and obj.type.class_ir.is_ext_class
719+
return self.add(GetAttrNullable(obj, attr, line))
720+
711721
def assign(self, target: Register | AssignmentTarget, rvalue_reg: Value, line: int) -> None:
712722
if isinstance(target, Register):
713723
self.add(Assign(target, self.coerce_rvalue(rvalue_reg, target.type, line)))

mypyc/irbuild/statement.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
YieldExpr,
4747
YieldFromExpr,
4848
)
49+
from mypyc.common import TEMP_ATTR_NAME
4950
from mypyc.ir.ops import (
5051
NAMESPACE_MODULE,
5152
NO_TRACEBACK_LINE_NO,
@@ -653,10 +654,15 @@ def try_finally_resolve_control(
653654
if ret_reg:
654655
builder.activate_block(rest)
655656
return_block, rest = BasicBlock(), BasicBlock()
656-
builder.add(Branch(builder.read(ret_reg), rest, return_block, Branch.IS_ERROR))
657+
# For spill targets in try/finally, use nullable read to avoid AttributeError
658+
if isinstance(ret_reg, AssignmentTargetAttr) and ret_reg.attr.startswith(TEMP_ATTR_NAME):
659+
ret_val = builder.read_nullable_attr(ret_reg.obj, ret_reg.attr, -1)
660+
else:
661+
ret_val = builder.read(ret_reg)
662+
builder.add(Branch(ret_val, rest, return_block, Branch.IS_ERROR))
657663

658664
builder.activate_block(return_block)
659-
builder.nonlocal_control[-1].gen_return(builder, builder.read(ret_reg), -1)
665+
builder.nonlocal_control[-1].gen_return(builder, ret_val, -1)
660666

661667
# TODO: handle break/continue
662668
builder.activate_block(rest)

0 commit comments

Comments
 (0)