Address review comments

Author: p-sawicki

mypyc/ir/ops.py

@@ -1221,6 +1221,7 @@ def __init__(
         var_arg_idx: int = -1,
         *,
         is_pure: bool = False,
+        returns_null: bool = False,
     ) -> None:
         self.error_kind = error_kind
         super().__init__(line)
@@ -1235,7 +1236,10 @@ def __init__(
         # and all the arguments are immutable. Pure functions support
         # additional optimizations. Pure functions never fail.
         self.is_pure = is_pure
-        if is_pure:
+        # The function might return a null value that does not indicate
+        # an error.
+        self.returns_null = returns_null
+        if is_pure or returns_null:
             assert error_kind == ERR_NEVER
 
     def sources(self) -> list[Value]:

mypyc/irbuild/function.py

@@ -375,10 +375,10 @@ def generate_getattr_wrapper(builder: IRBuilder, cdef: ClassDef, getattr: FuncDe
     __getattr__ is not supported in classes that allow interpreted subclasses because the
     tp_getattro slot is inherited by subclasses and if the subclass overrides __getattr__,
     the override would be ignored in our wrapper. TODO: To support this, the wrapper would
-    have to resolve "__getattr__" against the type at runtime and call the returned method,
-    like _Py_slot_tp_getattr_hook in cpython.
+    have to check type of self and if it's not the compiled class, resolve "__getattr__" against
+    the type at runtime and call the returned method, like _Py_slot_tp_getattr_hook in cpython.
 
-    __getattr__ is not supported in classes which inherit from python classes because those
+    __getattr__ is not supported in classes which inherit from non-native classes because those
     have __dict__ which currently has some strange interactions when class attributes and
     variables are assigned through __dict__ vs. through regular attribute access. Allowing
     __getattr__ on top of that could be problematic.
@@ -391,7 +391,7 @@ def generate_getattr_wrapper(builder: IRBuilder, cdef: ClassDef, getattr: FuncDe
     if ir.allow_interpreted_subclasses:
         builder.error(error_base + "it allows interpreted subclasses", line)
     if ir.inherits_python:
-        builder.error(error_base + "it inherits from an interpreted class", line)
+        builder.error(error_base + "it inherits from a non-native class", line)
 
     with builder.enter_method(ir, name, object_rprimitive, internal=True):
         attr_arg = builder.add_argument("attr", object_rprimitive)

mypyc/irbuild/ll_builder.py

@@ -2048,6 +2048,7 @@ def call_c(
                 line,
                 var_arg_idx,
                 is_pure=desc.is_pure,
+                returns_null=desc.returns_null,
             )
         )
         if desc.is_borrowed:
@@ -2131,6 +2132,7 @@ def primitive_op(
                 desc.extra_int_constants,
                 desc.priority,
                 is_pure=desc.is_pure,
+                returns_null=False,
             )
             return self.call_c(c_desc, args, line, result_type=result_type)
 

mypyc/primitives/generic_ops.py

@@ -408,5 +408,5 @@
     return_type=object_rprimitive,
     c_function_name="CPyObject_GenericGetAttr",
     error_kind=ERR_NEVER,
-    is_borrowed=True,
+    returns_null=True,
 )

mypyc/primitives/registry.py

@@ -61,6 +61,7 @@ class CFunctionDescription(NamedTuple):
     extra_int_constants: list[tuple[int, RType]]
     priority: int
     is_pure: bool
+    returns_null: bool
 
 
 # A description for C load operations including LoadGlobal and LoadAddress
@@ -253,6 +254,7 @@ def custom_op(
     is_borrowed: bool = False,
     *,
     is_pure: bool = False,
+    returns_null: bool = False,
 ) -> CFunctionDescription:
     """Create a one-off CallC op that can't be automatically generated from the AST.
 
@@ -274,6 +276,7 @@ def custom_op(
         extra_int_constants,
         0,
         is_pure=is_pure,
+        returns_null=returns_null,
     )
 
 

mypyc/test-data/irbuild-classes.test

@@ -2091,35 +2091,48 @@ class InterpSub:
 
 [case testUnsupportedGetAttr]
 from mypy_extensions import mypyc_attr
-from typing import Optional
 
 @mypyc_attr(allow_interpreted_subclasses=True)
 class AllowsInterpreted:
-    def __getattr__(self, attr: str) -> Optional[object]:  # E: "__getattr__" not supported in class "AllowsInterpreted" because it allows interpreted subclasses
+    def __getattr__(self, attr: str) -> object:  # E: "__getattr__" not supported in class "AllowsInterpreted" because it allows interpreted subclasses
         return 0
 
 class InheritsInterpreted(dict):
-    def __getattr__(self, attr: str) -> Optional[object]:  # E: "__getattr__" not supported in class "InheritsInterpreted" because it inherits from an interpreted class
+    def __getattr__(self, attr: str) -> object:  # E: "__getattr__" not supported in class "InheritsInterpreted" because it inherits from a non-native class
+        return 0
+
+@mypyc_attr(native_class=False)
+class NonNative:
+    pass
+
+class InheritsNonNative(NonNative):
+    def __getattr__(self, attr: str) -> object:  # E: "__getattr__" not supported in class "InheritsNonNative" because it inherits from a non-native class
         return 0
 
 [case testGetAttr]
-from typing import Optional, Tuple
+from typing import ClassVar
 
 class GetAttr:
     class_var = "x"
+    class_var_annotated: ClassVar[int] = 99
 
     def __init__(self, regular_attr: int):
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> object:
         return attr
 
-def test_getattr() -> Tuple[object, object, object]:
+    def method(self) -> int:
+        return 0
+
+def test_getattr() -> list[object]:
     i = GetAttr(42)
     one = i.one
     two = i.regular_attr
     three = i.class_var
-    return (one, two, three)
+    four = i.class_var_annotated
+    five = i.method()
+    return [one, two, three, four, five]
 
 [typing fixtures/typing-full.pyi]
 [out]
@@ -2139,7 +2152,7 @@ def GetAttr.__getattr____wrapper(__mypyc_self__, attr):
     attr, r0 :: object
     r1 :: bit
     r2 :: str
-    r3 :: union[object, None]
+    r3 :: object
 L0:
     r0 = CPyObject_GenericGetAttr(__mypyc_self__, attr)
     r1 = r0 != 0
@@ -2150,6 +2163,10 @@ L2:
     r2 = cast(str, attr)
     r3 = __mypyc_self__.__getattr__(r2)
     return r3
+def GetAttr.method(self):
+    self :: __main__.GetAttr
+L0:
+    return 0
 def GetAttr.__mypyc_defaults_setup(__mypyc_self__):
     __mypyc_self__ :: __main__.GetAttr
     r0 :: str
@@ -2160,16 +2177,14 @@ L0:
 def test_getattr():
     r0, i :: __main__.GetAttr
     r1 :: str
-    r2 :: object
-    one :: union[object, None]
+    r2, one :: object
     r3, two :: int
-    r4, three :: str
-    r5 :: tuple[union[object, None], int, str]
-    r6 :: union[object, None]
-    r7 :: int
-    r8 :: str
-    r9 :: object
-    r10 :: tuple[union[object, None], object, str]
+    r4, three, r5 :: str
+    r6 :: object
+    r7, four, r8, five :: int
+    r9 :: list
+    r10, r11, r12 :: object
+    r13 :: ptr
 L0:
     r0 = GetAttr(84)
     i = r0
@@ -2180,10 +2195,21 @@ L0:
     two = r3
     r4 = i.class_var
     three = r4
-    r5 = (one, two, three)
-    r6 = r5[0]
-    r7 = r5[1]
-    r8 = r5[2]
-    r9 = box(int, r7)
-    r10 = (r6, r9, r8)
-    return r10
+    r5 = 'class_var_annotated'
+    r6 = CPyObject_GetAttr(i, r5)
+    r7 = unbox(int, r6)
+    four = r7
+    r8 = i.method()
+    five = r8
+    r9 = PyList_New(5)
+    r10 = box(int, two)
+    r11 = box(int, four)
+    r12 = box(int, five)
+    r13 = list_items r9
+    buf_init_item r13, 0, one
+    buf_init_item r13, 1, r10
+    buf_init_item r13, 2, three
+    buf_init_item r13, 3, r11
+    buf_init_item r13, 4, r12
+    keep_alive r9
+    return r9

mypyc/test-data/run-classes.test

@@ -4091,54 +4091,54 @@ def test_inheritance_2() -> None:
 
 [case testDunderGetAttr]
 from mypy_extensions import mypyc_attr
-from typing import Dict, Optional
+from typing import ClassVar
 
 class GetAttr:
     class_var = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> object:
         return self.extra_attrs.get(attr)
 
 class GetAttrDefault:
-    class_var = "x"
+    class_var: ClassVar[str] = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str, default: int = 8, mult: int = 1) -> Optional[object]:
+    def __getattr__(self, attr: str, default: int = 8, mult: int = 1) -> object:
         return self.extra_attrs.get(attr, default * mult)
 
 class GetAttrInherited(GetAttr):
     subclass_var = "y"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int, sub_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int, sub_attr: int):
         super().__init__(extra_attrs, regular_attr)
         self.sub_attr = sub_attr
 
 class GetAttrOverridden(GetAttr):
-    subclass_var = "y"
+    subclass_var: ClassVar[str] = "y"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int, sub_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int, sub_attr: int):
         super().__init__(extra_attrs, regular_attr)
         self.sub_attr = sub_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> str:
         return attr
 
 @mypyc_attr(native_class=False)
 class GetAttrNonNative:
     class_var = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> object:
         return self.extra_attrs.get(attr)
 
 def test_getattr() -> None:
@@ -4302,54 +4302,54 @@ def test_getattr_nonnative() -> None:
 
 [case testDunderGetAttrInterpreted]
 from mypy_extensions import mypyc_attr
-from typing import Dict, Optional
+from typing import ClassVar
 
 class GetAttr:
     class_var = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> object:
         return self.extra_attrs.get(attr)
 
 class GetAttrDefault:
-    class_var = "x"
+    class_var: ClassVar[str] = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str, default: int = 8, mult: int = 1) -> Optional[object]:
+    def __getattr__(self, attr: str, default: int = 8, mult: int = 1) -> object:
         return self.extra_attrs.get(attr, default * mult)
 
 class GetAttrInherited(GetAttr):
     subclass_var = "y"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int, sub_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int, sub_attr: int):
         super().__init__(extra_attrs, regular_attr)
         self.sub_attr = sub_attr
 
 class GetAttrOverridden(GetAttr):
-    subclass_var = "y"
+    subclass_var: ClassVar[str] = "y"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int, sub_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int, sub_attr: int):
         super().__init__(extra_attrs, regular_attr)
         self.sub_attr = sub_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> str:
         return attr
 
 @mypyc_attr(native_class=False)
 class GetAttrNonNative:
     class_var = "x"
 
-    def __init__(self, extra_attrs: Dict[str, object], regular_attr: int):
+    def __init__(self, extra_attrs: dict[str, object], regular_attr: int):
         self.extra_attrs = extra_attrs
         self.regular_attr = regular_attr
 
-    def __getattr__(self, attr: str) -> Optional[object]:
+    def __getattr__(self, attr: str) -> object:
         return self.extra_attrs.get(attr)
 
 [file driver.py]

mypyc/transform/refcount.py

@@ -33,6 +33,7 @@
     Assign,
     BasicBlock,
     Branch,
+    CallC,
     ControlOp,
     DecRef,
     Goto,
@@ -89,7 +90,9 @@ def insert_ref_count_opcodes(ir: FuncIR) -> None:
 
 
 def is_maybe_undefined(post_must_defined: set[Value], src: Value) -> bool:
-    return isinstance(src, Register) and src not in post_must_defined
+    return (isinstance(src, Register) and src not in post_must_defined) or (
+        isinstance(src, CallC) and src.returns_null
+    )
 
 
 def maybe_append_dec_ref(