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

gh-73536: Add support of multi-signatures #117671

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Apr 9, 2024

  • Add inspect.MultiSignature which is a subclass of inspect.Signature.
  • Add inspect.signatures().
  • inspect.signature() can now return a multi-signature.
  • Support multi-signatures in pydoc.
  • Support multi-signatures in IDLE calltips.
  • Support multi-signatures in dataclasses docstrings.
  • Allow multiple @text_signature in Argument Clinic.
  • Add representable signatures for all builtin functions and methods of builtin classes except type() and super().

* Add inspect.MultiSignature which is a subclass of inspect.Signature.
* Add inspect.signatures().
* inspect.signature() can now return a multi-signature.
* Support multi-signatures in pydoc.
* Support multi-signatures in IDLE calltips.
* Support multi-signatures in dataclasses docstrings.
* Allow multiple @text_signature in Argument Clinic.
* Add representable signatures for all builtin functions and methods of
  builtin classes except type() and super().
Objects/memoryobject.c Outdated Show resolved Hide resolved
Python/bltinmodule.c Outdated Show resolved Hide resolved
@@ -2126,11 +2131,29 @@ def _signature_get_partial(wrapped_sig, partial, extra_args=()):
return wrapped_sig.replace(parameters=new_params.values())


def _multisignature_get_partial(wrapped_sig, partial, extra_args=()):
last_exc = None
Copy link
Contributor

@Gouvernathor Gouvernathor Apr 10, 2024

Choose a reason for hiding this comment

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

Unless the case of the list being empty is handled elsewhere, this could result in a raise None. Maybe that None should instead be something like Exception("Empty MultiSignature list") ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is checked in the constructor. I think even about removing this assignment, NameError is clearer indication if something goes wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Lib/inspect.py Outdated
return True
if isinstance(other, MultiSignature):
return self._signatures == other._signatures
if len(self._signatures) == 1 and isinstance(other, Signature):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be

Suggested change
if len(self._signatures) == 1 and isinstance(other, Signature):
if isinstance(other, Signature):
return len(self._signatures) == 1

and the addition to Signature's eq should be removed, above.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not correct, it would make a 1-element MultiSignature equal to any Signature.

You perhaps mistyped, I wrote more correct fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did. That's good. (I would suggest removing the line about MultiSignature in Signature's eq as I believe it's dead code now, but up to you).

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not dead. It is called in SignatureSubclass() == MultiSignature(). Although it is not covered by tests yet.

return '\n'.join(sig.format(max_width=max_width)
for sig in self._signatures)


def signature(obj, *, follow_wrapped=True, globals=None, locals=None, eval_str=False):
"""Get a signature object for the passed callable."""
return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the original Signature.from_callable is not changed and does the exact same call to _from_callable, that means both signature and Signature.from_callable may return a MultiSignature object, right ?

Strictly speaking this is not a violation of backwards compatibility as MultiSignature is a subclass of Signature, but I find it weird and it may break some strict-type behaviors. Is there a way to have those only return pure Signature objects, without breaking everything this update brings to the table ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. The idea was that inspect.signature() will start to support previously not supported cases and return a MultiSignature which should be compatible with Signature. But now I am no longer sure. They are not so well compatible as I expected, and in most cases I needed to rewrite the code to explicitly handle MultiSignature. This is why I added inspect.signatures(). Now the code is in the intermediate state -- either MultiSignature will be made more compatible with Signature and inspect.signatures() may disappear, or they will became completely different things.

Python/bltinmodule.c Outdated Show resolved Hide resolved
Objects/memoryobject.c Outdated Show resolved Hide resolved
@@ -2126,11 +2131,29 @@ def _signature_get_partial(wrapped_sig, partial, extra_args=()):
return wrapped_sig.replace(parameters=new_params.values())


def _multisignature_get_partial(wrapped_sig, partial, extra_args=()):
last_exc = None
Copy link
Member Author

Choose a reason for hiding this comment

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

It is checked in the constructor. I think even about removing this assignment, NameError is clearer indication if something goes wrong.

Lib/inspect.py Outdated Show resolved Hide resolved
Lib/inspect.py Outdated
return True
if isinstance(other, MultiSignature):
return self._signatures == other._signatures
if len(self._signatures) == 1 and isinstance(other, Signature):
Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not correct, it would make a 1-element MultiSignature equal to any Signature.

You perhaps mistyped, I wrote more correct fix.

return '\n'.join(sig.format(max_width=max_width)
for sig in self._signatures)


def signature(obj, *, follow_wrapped=True, globals=None, locals=None, eval_str=False):
"""Get a signature object for the passed callable."""
return Signature.from_callable(obj, follow_wrapped=follow_wrapped,
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. The idea was that inspect.signature() will start to support previously not supported cases and return a MultiSignature which should be compatible with Signature. But now I am no longer sure. They are not so well compatible as I expected, and in most cases I needed to rewrite the code to explicitly handle MultiSignature. This is why I added inspect.signatures(). Now the code is in the intermediate state -- either MultiSignature will be made more compatible with Signature and inspect.signatures() may disappear, or they will became completely different things.

@skirpichev
Copy link
Member

@serhiy-storchaka, now this again has conflicts with the main. I did pr serhiy-storchaka#22 that address this, hope it helps.

Are you thinking about writing a PEP as Petr suggested?

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

Successfully merging this pull request may close these issues.

4 participants