gh-101293: Fix support of custom callables and types in inspect.Signature.from_callable()

[serhiy-storchaka]

Support callables with the __call__() method and types with __new__() and __init__() methods set to class methods, static methods, bound methods, partial functions, and other types of methods and descriptors.

Add tests for numerous types of callables and descriptors.

• Issue: inspect.signature returns incorrect result on a @staticmethod and a @classmethod #101293

[gaogaotiantian]

In what scenario, is f.__wrapped__ not callable? It might worth some comments as it's not easy to follow.

[serhiy-storchaka]

When it is a data descriptor. For example staticmethod.__wrapped__.

It is actually a workaround of issue #112006. We need obj to be a callable, so we can use this workaround here. But more general solution can replace it.

[gaogaotiantian]

Okay sure that makes sense. I still believe that worth some comments about it (the comments above covered the other cases listed and they are much easier to follow than f.__wrapped__)

[serhiy-storchaka]

I'll add a comment.

[serhiy-storchaka]

This workaround no longer needed.

[gaogaotiantian]

It seems like __new__ is the only exemption that continues to use this original method. All the others are replaced with the new logic which uses getattr_static to avoid descriptor resolving. Is there a reason for that? Can we simply change _signature_get_user_defined_method?

[serhiy-storchaka]

It reproduces the logic in the C code (see slot_tp_call, slot_tp_new and slot_tp_init). getattr_static() should be used for __call__ and __init__, and getattr() for __new__.

[gaogaotiantian]

I think _signature_get_user_defined_method is still a pretty self-explanatory name for the method. The only usage for this private method is to get user defined __new__, __init__ and __call__ methods. Can we do the check logic inside the method, asserting the input method to check, and use getattr() / getattr_static() accordingly? The only difference between __new__ and the other two is which function to use right? We don't even need the _descriptor_get function, and we don't need to change the logic in this function.

[serhiy-storchaka]

We can do this, but it will split the logic on two parts, 600 lines apart one from other. There are other differences:

• Note that the code for __new__ is followed by break (to call _signature_bound_method() if skip_bound_arg is true), while the code for other methods immediately returns. So some logic will be here, and some in _signature_get_user_defined_method().

• The code for non-type's __call__ does not have restrictions for builtin types. In future, I want to loosen restrictions for other methods too, so isinstance(meth, _NonUserDefinedCallables) will be different for every method, and we may need to check the type of the descriptor before resolving it.

[gaogaotiantian]

Okay I still think the logic is hard to follow. It took me a while to figure out that the key thing changed is that init and call are currently bound methods, so they do not need the strip of the first parameter - this logic for new however, is far down.

I still believe that we should put the new descriptor/getattr_static logic into _signature_bound_method() because that's exactly what they do. You still have the old function and that's having two super similar routine far away from each other.

I think the logic in _signature_from_callable should be as simple as

call = _signature_get_user_defined_method(type(obj), '__call__')
new = _signature_get_user_defined_method(obj, '__new__')
init = _signature_get_user_defined_method(obj, '__init__')

However, we need to deal with the bound-issue, which is what slot_tp_call does - we can just return another indicator:

call, unbound = _signature_get_user_defined_method(type(obj), '__call__')

We would significantly simplify the logic in _signature_from_callable and get what we need (function and whether it's bounded) with a single function call.

Then, inside isinstance(obj, type) check, before if sig is None:, we can simply do

if sig is not None:
    if unbound and skip_bound_arg:
        return _signature_bound_method(sig)
    return sig

Then of course the following code does not need the if check protection as we ensured sig is not None at that point.

This way, we have all the related logic in two places:

1. _signature_get_user_defined_method that "magically" does the check and resolves the descriptor.
2. A continuous block in if isinstance(obj, type): that solves everything.

[serhiy-storchaka]

My goal was to minimize the diffs for easier review and do refactoring in the following PR. But if you prefer, I do some refactoring in this PR (complete refactoring will be possible after making some other behavior changes).

I tried to implement your suggestions, except returning unbound, because I do not see how it can help.

[gaogaotiantian]

I have some doubts listing the bug as a test case. If we know it's a wrong answer, we should not test that the answer is accurately generated. It's possible in the future something changed and this wrong result is corrected, or converted to another wrong one, and the test would fail - and people will be confused unless they look at the source test code and realize it was labeled "BUG". It may help for the "backward compatibility" if we consider the bug as a compatibility we need to keep track of, but I still have concerns for this. If we do want to keep track of it, I think a #TODO would be much better.

[serhiy-storchaka]

This is definitely a bug. The current code ignores __call__ signature and gets the signature from __init__, which is not even called. I used label TODO if signature() currently raises ValueError, but can get a right result, and label BUG if it successfully returns wrong result. The test is added to show what the right answer is. If in the future something changed and this wrong result is corrected, it will be good, we will remove assertRaises() and the comment.

We can also split these tests on small methods and use the @expectedFailure decorator, if it matters.

BTW, I have a patch that fixes these TODOs and BUGs, but I am not certain about it, so I kept only safer changes here.

[gaogaotiantian]

I understand that this is to test whether the method works without an exception, but this statement is a bit weird by itself. What are the possible use cases for a @staticmethod __init__? To be honest all the decorators on __init__ and __new__ are so weird that I doubt if there's anyone actually using them.

[serhiy-storchaka]

They are weird because they are the only examples that can be created using existing builtin classes. But in future we could implement __init__, __new__ and __call__ in C for some classes. By testing with different types of callables and descriptors we increase confidence in working with arbitrary appropriate types.

[gaogaotiantian]

For the first sentence I actually meant the C(1) statement - it does not explicitly check against anything. I understand that it tests whether an exception will be raised when instantiating the object, but it still looks a bit weird.

[serhiy-storchaka]

I'll add comments.

[serhiy-storchaka]

@gaogaotiantian, which of my comments answered your concerns and which ones were unconvincing?

[gaogaotiantian]

Thank you for your patience. I approved the change for now because obviously you have future plans with the code and some of them will be refactored soon, so it's not worth it to debate on the best shape of the current situation. It's completely acceptable now.

However, as I'm not the core-dev so you probably need another person to take a look at the code?

[serhiy-storchaka]

Thank you for your review @gaogaotiantian.

[miss-islington-app]

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[miss-islington-app]

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-app]

GH-116197 is a backport of this pull request to the 3.11 branch.

[bedevere-app]

GH-116198 is a backport of this pull request to the 3.12 branch.