Skip to content

Commit cbe28b2

Browse files
authored
[mypyc] Fix AttributeError in async try/finally with mixed return paths (#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
1 parent 3df3d79 commit cbe28b2

File tree

5 files changed

+348
-4
lines changed

5 files changed

+348
-4
lines changed

mypyc/codegen/emitfunc.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,9 @@ def get_attr_expr(self, obj: str, op: GetAttr | SetAttr, decl_cl: ClassIR) -> st
358358
return f"({cast}{obj})->{self.emitter.attr(op.attr)}"
359359

360360
def visit_get_attr(self, op: GetAttr) -> None:
361+
if op.allow_null:
362+
self.get_attr_with_allow_null(op)
363+
return
361364
dest = self.reg(op)
362365
obj = self.reg(op.obj)
363366
rtype = op.class_type
@@ -426,6 +429,24 @@ def visit_get_attr(self, op: GetAttr) -> None:
426429
elif not always_defined:
427430
self.emitter.emit_line("}")
428431

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

mypyc/ir/ops.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -777,15 +777,20 @@ class GetAttr(RegisterOp):
777777

778778
error_kind = ERR_MAGIC
779779

780-
def __init__(self, obj: Value, attr: str, line: int, *, borrow: bool = False) -> None:
780+
def __init__(
781+
self, obj: Value, attr: str, line: int, *, borrow: bool = False, allow_null: bool = False
782+
) -> None:
781783
super().__init__(line)
782784
self.obj = obj
783785
self.attr = attr
786+
self.allow_null = allow_null
784787
assert isinstance(obj.type, RInstance), "Attribute access not supported: %s" % obj.type
785788
self.class_type = obj.type
786789
attr_type = obj.type.attr_type(attr)
787790
self.type = attr_type
788-
if attr_type.error_overlap:
791+
if allow_null:
792+
self.error_kind = ERR_NEVER
793+
elif attr_type.error_overlap:
789794
self.error_kind = ERR_MAGIC_OVERLAPPING
790795
self.is_borrowed = borrow and attr_type.is_refcounted
791796

mypyc/irbuild/builder.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,15 @@ def read(
708708

709709
assert False, "Unsupported lvalue: %r" % target
710710

711+
def read_nullable_attr(self, obj: Value, attr: str, line: int = -1) -> Value:
712+
"""Read an attribute that might be NULL without raising AttributeError.
713+
714+
This is used for reading spill targets in try/finally blocks where NULL
715+
indicates the non-return path was taken.
716+
"""
717+
assert isinstance(obj.type, RInstance) and obj.type.class_ir.is_ext_class
718+
return self.add(GetAttr(obj, attr, line, allow_null=True))
719+
711720
def assign(self, target: Register | AssignmentTarget, rvalue_reg: Value, line: int) -> None:
712721
if isinstance(target, Register):
713722
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)