[mypyc] Fix AttributeError in async try/finally with mixed return paths (#19361)

Chainfire · esarp · commit 5c65e330b0e4 · 2025-07-10T11:08:46.000-05:00
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
diff --git a/mypyc/codegen/emitfunc.py b/mypyc/codegen/emitfunc.py
@@ -358,6 +358,9 @@ def get_attr_expr(self, obj: str, op: GetAttr | SetAttr, decl_cl: ClassIR) -> st
             return f"({cast}{obj})->{self.emitter.attr(op.attr)}"
 
     def visit_get_attr(self, op: GetAttr) -> None:
+        if op.allow_null:
+            self.get_attr_with_allow_null(op)
+            return
         dest = self.reg(op)
         obj = self.reg(op.obj)
         rtype = op.class_type
@@ -426,6 +429,24 @@ def visit_get_attr(self, op: GetAttr) -> None:
             elif not always_defined:
                 self.emitter.emit_line("}")
 
+    def get_attr_with_allow_null(self, op: GetAttr) -> None:
+        """Handle GetAttr with allow_null=True which allows NULL without raising AttributeError."""
+        dest = self.reg(op)
+        obj = self.reg(op.obj)
+        rtype = op.class_type
+        cl = rtype.class_ir
+        attr_rtype, decl_cl = cl.attr_details(op.attr)
+
+        # Direct struct access without NULL check
+        attr_expr = self.get_attr_expr(obj, op, decl_cl)
+        self.emitter.emit_line(f"{dest} = {attr_expr};")
+
+        # Only emit inc_ref if not NULL
+        if attr_rtype.is_refcounted and not op.is_borrowed:
+            self.emitter.emit_line(f"if ({dest} != NULL) {{")
+            self.emitter.emit_inc_ref(dest, attr_rtype)
+            self.emitter.emit_line("}")
+
     def next_branch(self) -> Branch | None:
         if self.op_index + 1 < len(self.ops):
             next_op = self.ops[self.op_index + 1]
diff --git a/mypyc/ir/ops.py b/mypyc/ir/ops.py
@@ -777,15 +777,20 @@ class GetAttr(RegisterOp):
 
     error_kind = ERR_MAGIC
 
-    def __init__(self, obj: Value, attr: str, line: int, *, borrow: bool = False) -> None:
+    def __init__(
+        self, obj: Value, attr: str, line: int, *, borrow: bool = False, allow_null: bool = False
+    ) -> None:
         super().__init__(line)
         self.obj = obj
         self.attr = attr
+        self.allow_null = allow_null
         assert isinstance(obj.type, RInstance), "Attribute access not supported: %s" % obj.type
         self.class_type = obj.type
         attr_type = obj.type.attr_type(attr)
         self.type = attr_type
-        if attr_type.error_overlap:
+        if allow_null:
+            self.error_kind = ERR_NEVER
+        elif attr_type.error_overlap:
             self.error_kind = ERR_MAGIC_OVERLAPPING
         self.is_borrowed = borrow and attr_type.is_refcounted
 
diff --git a/mypyc/irbuild/builder.py b/mypyc/irbuild/builder.py
@@ -708,6 +708,15 @@ def read(
 
         assert False, "Unsupported lvalue: %r" % target
 
+    def read_nullable_attr(self, obj: Value, attr: str, line: int = -1) -> Value:
+        """Read an attribute that might be NULL without raising AttributeError.
+
+        This is used for reading spill targets in try/finally blocks where NULL
+        indicates the non-return path was taken.
+        """
+        assert isinstance(obj.type, RInstance) and obj.type.class_ir.is_ext_class
+        return self.add(GetAttr(obj, attr, line, allow_null=True))
+
     def assign(self, target: Register | AssignmentTarget, rvalue_reg: Value, line: int) -> None:
         if isinstance(target, Register):
             self.add(Assign(target, self.coerce_rvalue(rvalue_reg, target.type, line)))
diff --git a/mypyc/irbuild/statement.py b/mypyc/irbuild/statement.py
@@ -46,6 +46,7 @@
     YieldExpr,
     YieldFromExpr,
 )
+from mypyc.common import TEMP_ATTR_NAME
 from mypyc.ir.ops import (
     NAMESPACE_MODULE,
     NO_TRACEBACK_LINE_NO,
@@ -653,10 +654,15 @@ def try_finally_resolve_control(
     if ret_reg:
         builder.activate_block(rest)
         return_block, rest = BasicBlock(), BasicBlock()
-        builder.add(Branch(builder.read(ret_reg), rest, return_block, Branch.IS_ERROR))
+        # For spill targets in try/finally, use nullable read to avoid AttributeError
+        if isinstance(ret_reg, AssignmentTargetAttr) and ret_reg.attr.startswith(TEMP_ATTR_NAME):
+            ret_val = builder.read_nullable_attr(ret_reg.obj, ret_reg.attr, -1)
+        else:
+            ret_val = builder.read(ret_reg)
+        builder.add(Branch(ret_val, rest, return_block, Branch.IS_ERROR))
 
         builder.activate_block(return_block)
-        builder.nonlocal_control[-1].gen_return(builder, builder.read(ret_reg), -1)
+        builder.nonlocal_control[-1].gen_return(builder, ret_val, -1)
 
     # TODO: handle break/continue
     builder.activate_block(rest)
diff --git a/mypyc/test-data/run-async.test b/mypyc/test-data/run-async.test
