Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 2, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b0abac
[mypyc] Fix AttributeError in async try/finally with mixed return paths
Chainfire Jun 30, 2025
db7d454
Make GetAttrNullable a subclass of GetAttr
Chainfire Jun 30, 2025
7601eeb
Remove GetAttrNullable, use allow_null flag on GetAttr instead
Chainfire Jul 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mypyc/analysis/dataflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
FloatNeg,
FloatOp,
GetAttr,
GetAttrNullable,
GetElementPtr,
Goto,
IncRef,
Expand Down Expand Up @@ -209,6 +210,9 @@ def visit_load_literal(self, op: LoadLiteral) -> GenAndKill[T]:
def visit_get_attr(self, op: GetAttr) -> GenAndKill[T]:
return self.visit_register_op(op)

def visit_get_attr_nullable(self, op: GetAttrNullable) -> GenAndKill[T]:
return self.visit_register_op(op)

def visit_set_attr(self, op: SetAttr) -> GenAndKill[T]:
return self.visit_register_op(op)

Expand Down
5 changes: 5 additions & 0 deletions mypyc/analysis/ircheck.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
FloatNeg,
FloatOp,
GetAttr,
GetAttrNullable,
GetElementPtr,
Goto,
IncRef,
Expand Down Expand Up @@ -319,6 +320,10 @@ def visit_get_attr(self, op: GetAttr) -> None:
# Nothing to do.
pass

def visit_get_attr_nullable(self, op: GetAttrNullable) -> None:
# Nothing to do.
pass

def visit_set_attr(self, op: SetAttr) -> None:
# Nothing to do.
pass
Expand Down
8 changes: 8 additions & 0 deletions mypyc/analysis/selfleaks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
FloatNeg,
FloatOp,
GetAttr,
GetAttrNullable,
GetElementPtr,
Goto,
InitStatic,
Expand Down Expand Up @@ -114,6 +115,13 @@ def visit_get_attr(self, op: GetAttr) -> GenAndKill:
return self.check_register_op(op)
return CLEAN

def visit_get_attr_nullable(self, op: GetAttrNullable) -> GenAndKill:
cl = op.class_type.class_ir
if cl.get_method(op.attr):
# Property -- calls a function
return self.check_register_op(op)
return CLEAN

def visit_set_attr(self, op: SetAttr) -> GenAndKill:
cl = op.class_type.class_ir
if cl.get_method(op.attr):
Expand Down
19 changes: 19 additions & 0 deletions mypyc/codegen/emitfunc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
FloatNeg,
FloatOp,
GetAttr,
GetAttrNullable,
GetElementPtr,
Goto,
IncRef,
Expand Down Expand Up @@ -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:
Copy link
Collaborator

@JukkaL JukkaL Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Chainfire Chainfire Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you mean here

Copy link
Collaborator

@JukkaL JukkaL Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

"""Handle GetAttrNullable 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]
Expand Down
22 changes: 22 additions & 0 deletions mypyc/ir/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,24 @@ def accept(self, visitor: OpVisitor[T]) -> T:
return visitor.visit_get_attr(self)


class GetAttrNullable(GetAttr):
Copy link
Collaborator

@JukkaL JukkaL Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@Chainfire Chainfire Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@JukkaL JukkaL Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""obj.attr (for a native object) - allows NULL without raising AttributeError

This is used for spill targets where NULL indicates the non-return path was taken.
Unlike GetAttr, this won't raise AttributeError when the attribute is NULL.
"""

error_kind = ERR_NEVER

def __init__(self, obj: Value, attr: str, line: int, *, borrow: bool = False) -> None:
super().__init__(obj, attr, line, borrow=borrow)
# Override error_kind since GetAttr sets it based on attr_type.error_overlap
self.error_kind = ERR_NEVER

def accept(self, visitor: OpVisitor[T]) -> T:
return visitor.visit_get_attr_nullable(self)


class SetAttr(RegisterOp):
"""obj.attr = src (for a native object)

Expand Down Expand Up @@ -1728,6 +1746,10 @@ def visit_load_literal(self, op: LoadLiteral) -> T:
def visit_get_attr(self, op: GetAttr) -> T:
raise NotImplementedError

@abstractmethod
def visit_get_attr_nullable(self, op: GetAttrNullable) -> T:
raise NotImplementedError

@abstractmethod
def visit_set_attr(self, op: SetAttr) -> T:
raise NotImplementedError
Expand Down
4 changes: 4 additions & 0 deletions mypyc/ir/pprint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
FloatNeg,
FloatOp,
GetAttr,
GetAttrNullable,
GetElementPtr,
Goto,
IncRef,
Expand Down Expand Up @@ -128,6 +129,9 @@ def visit_load_literal(self, op: LoadLiteral) -> str:
def visit_get_attr(self, op: GetAttr) -> str:
return self.format("%r = %s%r.%s", op, self.borrow_prefix(op), op.obj, op.attr)

def visit_get_attr_nullable(self, op: GetAttrNullable) -> str:
return self.format("%r = %s%r.%s?", op, self.borrow_prefix(op), op.obj, op.attr)

def borrow_prefix(self, op: Op) -> str:
if op.is_borrowed:
return "borrow "
Expand Down
10 changes: 10 additions & 0 deletions mypyc/irbuild/builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
Branch,
ComparisonOp,
GetAttr,
GetAttrNullable,
InitStatic,
Integer,
IntOp,
Expand Down Expand Up @@ -708,6 +709,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(GetAttrNullable(obj, attr, line))

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)))
Expand Down
10 changes: 8 additions & 2 deletions mypyc/irbuild/statement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
YieldExpr,
YieldFromExpr,
)
from mypyc.common import TEMP_ATTR_NAME
from mypyc.ir.ops import (
NAMESPACE_MODULE,
NO_TRACEBACK_LINE_NO,
Expand Down Expand Up @@ -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)
Expand Down
Loading