Skip to content
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.

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

Default arguments in methods are not preserved for introspection #4061

Closed
vladima opened this issue Mar 23, 2021 · 12 comments · Fixed by #4118
Closed

Default arguments in methods are not preserved for introspection #4061

vladima opened this issue Mar 23, 2021 · 12 comments · Fixed by #4118

Comments

@vladima
Copy link
Contributor

vladima commented Mar 23, 2021

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)
@da-woods
Copy link
Collaborator

I think the underlying cause of this is a known limitation

# For global cpdef functions and def/cpdef methods in cdef classes, we must use global constants
# for default arguments to avoid the dependency on the CyFunction object as 'self' argument
# in the underlying C function. Basically, cpdef functions/methods are static C functions,
# 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)

It might be possible to change it to make the introspection work without using the __defaults__ attribute in the actual function though.

@da-woods
Copy link
Collaborator

da-woods commented Mar 23, 2021

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

@vladima
Copy link
Contributor Author

vladima commented Apr 12, 2021

Sorry, can you please explain why def in cdef class is special so rules for global functions cannot be applied for it?

@da-woods
Copy link
Collaborator

da-woods commented Apr 12, 2021

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.

@vladima
Copy link
Contributor Author

vladima commented Apr 13, 2021

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=[])

@da-woods
Copy link
Collaborator

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.

@da-woods
Copy link
Collaborator

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 so there's really very little reason not to populate the attributes.

@vladima
Copy link
Contributor Author

vladima commented Apr 13, 2021

I can try to fix this if somebody can point me to the right direction :)

@da-woods
Copy link
Collaborator

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.

@vladima
Copy link
Contributor Author

vladima commented Apr 14, 2021

ok, if my reading of the code is correct, this block 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).

@da-woods
Copy link
Collaborator

da-woods commented Apr 14, 2021

That looks reasonable to me (but that doesn't guarantee it works!)

@vladima
Copy link
Contributor Author

vladima commented Apr 14, 2021

At glance with this change simple test now outputs correct signature. Will try with more exhaustive testing tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants