Repository: cython/cython
Pull request: Preserve default arguments values on methods for introspection
Fixes https://github.com/cython/cython/issues/4061.

Code is adjusted per discussion in issue to always add value to `default_args` or `default_kwargs`.

Problem statement:
Default arguments in methods are not preserved for introspection
Tried on the latest master:
```
# test.pyx
def run(a, b=1):
    return a + b

cdef class A:
    def run(self, a, b=1):
        return a + b

# app.py
from test import A, run
import inspect

a = A()
print(inspect.signature(run)) # ok - prints (a, b=1)
print(inspect.signature(a.run)) # not ok - prints (a, b)
print(inspect.signature(A.run)) # not ok - prints (self, a, b)
```

Hints from issue discussion:
I think the underlying cause of this is a known limitation

https://github.com/cython/cython/blob/8609e0fa7d361f1392823ff6e1a618720cd62df3/Cython/Compiler/ExprNodes.py#L9375-L9380

It might be possible to change it to make the introspection work without using the `__defaults__` attribute in the actual function though.
~I'm not completely convinced that code comment above is true (at least for def methods of cdef classes). So it might not need a huge change to allow introspection there. Or it could be that I'm missing something when skim-reading the code. But it looks like they are passed a suitable CyFunction object to read the defaults from.~ Actually it probably is true
Sorry, can you please explain why `def` in `cdef` class is special so rules for global functions cannot be applied for it?
```
def f(a=[]):
    return a

cdef class C:
    def f(self, a=[]):
        return a
```

For the global `f` Cython converts it to:

```
static PyObject *__pyx_pw_3xxx_1f(PyObject *__pyx_self, 
#if CYTHON_METH_FASTCALL
PyObject *const *__pyx_args, Py_ssize_t __pyx_nargs, PyObject *__pyx_kwds
#else
PyObject *__pyx_args, PyObject *__pyx_kwds
#endif
);
```

The `CyFunction` object is passed as `self`, allowing Cython to do:

```
__pyx_defaults *__pyx_dynamic_args = __Pyx_CyFunction_Defaults(__pyx_defaults, __pyx_self);
```

i.e. extract the defaults from the introspectable variable stored on the CyFunction.

--------------------

For the `cdef class` case, the signature looks the same. However, this time the `self` argument is interpreted as a pointer to the `cdef class` struct:

```
__pyx_r = __pyx_pf_3xxx_1C_f(((struct __pyx_obj_3xxx_C *)__pyx_v_self), __pyx_v_a);
```

Thus it does not have access the the defaults field stored in the `CyFunction` object and so it just retrieves the keyword value as a module-level global (which isn't introspectable)

```
values[0] = __pyx_k_;
```

----------------------------------------------

None of this is insurmountable, but it's defined like that for historic reasons (and to keep the implementation looking basically the same whether the is generated with `binding=True` or not) and would require fairly large changes to fix it.
out of curiosity: can Cython generate some equivalent of this as a workaround (with preserving identity of default values):
```
#main .pyx
def f(a=[]):
    return a

cdef class C:
    def f(self, a=[]):
        return a
    f.__defaults__ = ([],)

#.py
from main import C, f
import inspect

a = C()
print(inspect.signature(a.f))  # prints (a=[])
print(inspect.signature(C.f))  # prints (self, a=[])
print(inspect.signature(f))    # prints (a=[])
```

> out of curiosity: can Cython generate some equivalent of this as a workaround

Yes - having read-only default values that only exist for introspection (i.e. aren't read during the function call) is probably the easiest fix in the short-term.
> > out of curiosity: can Cython generate some equivalent of this as a workaround
> 
> Yes - having read-only default values that only exist for introspection (i.e. aren't read during the function call) is probably the easiest fix in the short-term.

It looks like the [versions we make make available for introspection are different from the values used in the implementation anyway](https://github.com/cython/cython/issues/2650) so there's really very little reason not to populate the attributes.
I can try to fix this if somebody can point me to the right direction :)
It will be around the comment that I linked to.

I think you will need to make sure that `PyCFunctionNode.defaults_tuple` and `PyCFunctionNode.default_kwargs` get populated since they're the two used for introspection.

You don't want to change is `PyCFunctionNode.defaults_struct` gets populated because that's what's used when the function is called.
ok, if my reading of the code is correct, [this block](https://github.com/cython/cython/blob/8609e0fa7d361f1392823ff6e1a618720cd62df3/Cython/Compiler/ExprNodes.py#L9382-L9395) should become:
```
for arg in self.def_node.args:
    if arg.default:
        if not must_use_constants:
            if not arg.default.is_literal:
                arg.is_dynamic = True
                if arg.type.is_pyobject:
                    nonliteral_objects.append(arg)
                else:
                    nonliteral_other.append(arg)
            else:
                arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
        if arg.kw_only:
            default_kwargs.append(arg)
        else:
            default_args.append(arg)    
```
so `arg` is default value is always added into `default_kwargs` or `default_args` and `must_use_constants` is only applied to whether item is added into `nonliteral_objects` or `nonliteral_other` (that are later used to populate `defaults_struct`).

Commit message: Preserve default arguments values on methods for introspection

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index 2714d3e4d7a..9be6d2b8324 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -9371,21 +9371,22 @@ def analyse_default_args(self, env):
         # so their optional arguments must be static, too.
         # TODO: change CyFunction implementation to pass both function object and owning object for method calls
         must_use_constants = env.is_c_class_scope or (self.def_node.is_wrapper and env.is_module_scope)
-
         for arg in self.def_node.args:
-            if arg.default and not must_use_constants:
-                if not arg.default.is_literal:
-                    arg.is_dynamic = True
-                    if arg.type.is_pyobject:
-                        nonliteral_objects.append(arg)
+            if arg.default:
+                if not must_use_constants:
+                    if not arg.default.is_literal:
+                        arg.is_dynamic = True
+                        if arg.type.is_pyobject:
+                            nonliteral_objects.append(arg)
+                        else:
+                            nonliteral_other.append(arg)
                     else:
-                        nonliteral_other.append(arg)
-                else:
-                    arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
-                if arg.kw_only:
-                    default_kwargs.append(arg)
-                else:
-                    default_args.append(arg)
+                        arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
+                if arg.type.is_pyobject or arg.type.is_numeric:
+                    if arg.kw_only:
+                        default_kwargs.append(arg)
+                    else:
+                        default_args.append(arg)
             if arg.annotation:
                 arg.annotation = arg.annotation.analyse_types(env)
                 annotations.append((arg.pos, arg.name, arg.annotation.string))
diff --git a/tests/run/cyfunction_defaults.pyx b/tests/run/cyfunction_defaults.pyx
index 2fe592a23c9..157b8aa7eaa 100644
--- a/tests/run/cyfunction_defaults.pyx
+++ b/tests/run/cyfunction_defaults.pyx
@@ -251,3 +251,51 @@ def test_func_default_scope_local():
         return arg
     print i  # genexprs don't leak
     return func
+
+cdef class C:
+    def f1(self, a, b=1, c=[]):
+        pass
+    def f2(self, a, b=1,/, c=[1]):
+        pass
+    def f3(self, a, /, b=1, *, c=[1]):
+        pass
+    cpdef f4(self, a, char*c=NULL):
+        pass
+    cpdef f5(self, a, str s = "123"):
+        pass
+    cpdef f6(self, a, int s = 4):
+        pass
+    cpdef f7(self, a, dict s = {'a':22}):
+        pass
+    cpdef f8(self, a, list s = [15]):
+        pass
+
+
+def check_defaults_on_methods_for_introspection():
+    """
+    >>> C.f1.__defaults__
+    (1, [])
+    >>> C.f1.__kwdefaults__
+    >>> C.f2.__defaults__
+    (1, [1])
+    >>> C.f2.__kwdefaults__
+    >>> C.f3.__defaults__
+    (1,)
+    >>> C.f3.__kwdefaults__
+    {'c': [1]}
+    >>> C.f4.__defaults__
+    >>> C.f4.__kwdefaults__
+    >>> C.f5.__defaults__
+    ('123',)
+    >>> C.f5.__kwdefaults__
+    >>> C.f6.__defaults__
+    (4,)
+    >>> C.f6.__kwdefaults__
+    >>> C.f7.__defaults__
+    ({'a': 22},)
+    >>> C.f7.__kwdefaults__
+    >>> C.f8.__defaults__
+    ([15],)
+    >>> C.f8.__kwdefaults__
+    """
+    pass
diff --git a/tests/run/py34_signature.pyx b/tests/run/py34_signature.pyx
index 6bfaec677f6..14780ff7b49 100644
--- a/tests/run/py34_signature.pyx
+++ b/tests/run/py34_signature.pyx
@@ -101,19 +101,17 @@ cpdef cp1(a, b):
     """
 
 
-# Currently broken, see GH #1864
 cpdef cp2(a, b=True):
     """
     >>> def py_cp2(a, b=True): pass
 
-    #>>> signatures_match(cp2, py_cp2)
+    >>> signatures_match(cp2, py_cp2)
     """
 
 
-# Currently broken, see GH #1864
 cpdef cp3(a=1, b=True):
     """
     >>> def py_cp3(a=1, b=True): pass
 
-    #>>> signatures_match(cp3, py_cp3)
+    >>> signatures_match(cp3, py_cp3)
     """


Code review comments:
diff_hunk: @@ -9371,21 +9371,22 @@ def analyse_default_args(self, env):
         # so their optional arguments must be static, too.
         # TODO: change CyFunction implementation to pass both function object and owning object for method calls
         must_use_constants = env.is_c_class_scope or (self.def_node.is_wrapper and env.is_module_scope)
-
         for arg in self.def_node.args:
-            if arg.default and not must_use_constants:
-                if not arg.default.is_literal:
-                    arg.is_dynamic = True
-                    if arg.type.is_pyobject:
-                        nonliteral_objects.append(arg)
+            if arg.default:
+                if not must_use_constants:
+                    if not arg.default.is_literal:
+                        arg.is_dynamic = True
+                        if arg.type.is_pyobject:
+                            nonliteral_objects.append(arg)
+                        else:
+                            nonliteral_other.append(arg)
                     else:
-                        nonliteral_other.append(arg)
-                else:
-                    arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
-                if arg.kw_only:
-                    default_kwargs.append(arg)
-                else:
-                    default_args.append(arg)
+                        arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
+                if arg.type.is_pyobject or arg.type.is_numeric:
path: Cython/Compiler/ExprNodes.py
text: @user1:
Why only these two? Why is this constraint needed?

@author:
This was added to make sure that only primitives/pyobjects (which also includes `unicode` and `str`) could be added to `__(kw)defaults__` which would  block cases like `cpdef f(char* arg = NULL)`. 

Alternative would be to do nothing and in which case this example will end up being compile time error but it might break some existing code  that used to compile  fine before.

@user2:
I think `arg.type` should have a function `can_coerce_to_pyobject`?

@author:
I think I've tried to use it however it does not  return what I would expect for `CPtrType` - in `CType` `can_coerce_to_pyobject` [is defined as](https://github.com/cython/cython/blob/master/Cython/Compiler/PyrexTypes.py#L1635) 
```
    def create_from_py_utility_code(self, env):
        return self.from_py_function is not None

    def can_coerce_to_pyobject(self, env):
        return self.create_to_py_utility_code(env)
```
and for `CPtrType` `from_py_function` will not be `None`. As a result `can_coerce_to_pyobject` for `CPtrType` returns `True` which lead to compile time error downstream.

@user2:
Does `arg.default` have a type? I think it may sometimes be different from `arg.type` (e.g. null pointers).

@author:
Yes, in this case it is `<CPtrType <CVoidType>>` (code in question is `cpdef f(char* arg = NULL)`)

diff_hunk: @@ -9371,21 +9371,22 @@ def analyse_default_args(self, env):
         # so their optional arguments must be static, too.
         # TODO: change CyFunction implementation to pass both function object and owning object for method calls
         must_use_constants = env.is_c_class_scope or (self.def_node.is_wrapper and env.is_module_scope)
-
         for arg in self.def_node.args:
-            if arg.default and not must_use_constants:
-                if not arg.default.is_literal:
-                    arg.is_dynamic = True
-                    if arg.type.is_pyobject:
-                        nonliteral_objects.append(arg)
+            if arg.default:
+                if not must_use_constants:
+                    if not arg.default.is_literal:
+                        arg.is_dynamic = True
+                        if arg.type.is_pyobject:
+                            nonliteral_objects.append(arg)
+                        else:
+                            nonliteral_other.append(arg)
                     else:
-                        nonliteral_other.append(arg)
-                else:
-                    arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
-                if arg.kw_only:
-                    default_kwargs.append(arg)
-                else:
-                    default_args.append(arg)
+                        arg.default = DefaultLiteralArgNode(arg.pos, arg.default)
+                if arg.type.is_pyobject or arg.type.is_numeric:
path: Cython/Compiler/ExprNodes.py
text: @user1:
```suggestion
                if arg.default.type and arg.default.type.can_coerce_to_pyobject(env):
```

This seems to pass `python runtests.py cyfunction default signature` (i.e. all the tests in this question, plus some related ones)