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

Incorrect description of the __signature__ attribute in docs #115937

Closed
skirpichev opened this issue Feb 26, 2024 · 8 comments · Fixed by #116086
Closed

Incorrect description of the __signature__ attribute in docs #115937

skirpichev opened this issue Feb 26, 2024 · 8 comments · Fixed by #116086
Labels
docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error

Comments

@skirpichev
Copy link
Member

skirpichev commented Feb 26, 2024

Bug report

Bug description:

Since #106311 its documented in the inspect.signature() sphinx docs as:
"If the passed object has a __signature__ attribute, this function returns it without further computations."

That looks correct per PEP 362, see the implementation section. But actual code logic goes far away from this simple description:

cpython/Lib/inspect.py

Lines 2549 to 2562 in 8e8ab75

if sig is not None:
# since __text_signature__ is not writable on classes, __signature__
# may contain text (or be a callable that returns text);
# if so, convert it
o_sig = sig
if not isinstance(sig, (Signature, str)) and callable(sig):
sig = sig()
if isinstance(sig, str):
sig = _signature_fromstr(sigcls, obj, sig)
if not isinstance(sig, Signature):
raise TypeError(
'unexpected object {!r} in __signature__ '
'attribute'.format(o_sig))
return sig

Either we should document this properly (then this is a documentation issue). Or we could change the code and remove undocumented logic. As this attribute was documented recently, I'll suggest the second option.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

@skirpichev skirpichev added the type-bug An unexpected behavior, bug, or error label Feb 26, 2024
@merwok merwok added the docs Documentation in the Doc dir label Feb 26, 2024
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Feb 26, 2024

Either we should document this properly (then this is a documentation issue). Or we could change the code and remove undocumented logic. As this attribute was documented recently, I'll suggest the second option.

Instead of rewriting the implementation in prose, another option is to word the recently added sentence more vague.

Also, in the case of discrepancies between docs and implementation, we (almost) never align the code with the docs, as that in most cases is bound to be a breaking change.

@Gouvernathor
Copy link
Contributor

Gouvernathor commented Feb 27, 2024

Explaining the current implementation as presented here, while it may not be the best choice for other reasons, is not very hard to explain in a simple manner. __signature__ takes either :

  • a signature object,
  • a string matching the signature of a function or method,
  • or a callable taking no argument and returning either of those things.

inspect.signature then makes a signature object out of it.

What would be weird though, is that there would be no documented way of turning a string like "(self, /, *args)" into a signature object directly, but there would be a roundabout way by setting that string to be the __signature__ parameter of a random object and passing that to inspect.signature.

@skirpichev
Copy link
Member Author

another option is to word the recently added sentence more vague.

Isn't documentation expected to be as precise as possible?

that in most cases is bound to be a breaking change

Perhaps, this case is a special one.

  1. The attribute was documented recently, see __signature__ in the inspect module #106310.
  2. Its behaviour was mentioned before only in the PEP 362 (If the object has a __signature__ attribute and if it is not None - return it)

For a background, new logic was introduced recently (since 3.12) in #100039. Maybe @ethanfurman could explain why parsing text signatures was chosen instead of using the Signature class in a pure-Python code? Recent discussion (https://discuss.python.org/t/43914/) reveals, that the __text_signature__-like interface, probably, is a bad one even for extensions.

a string matching the signature of a function or method,

This forces us to document the format for the __text_signature__, isn't? It worth to note, that repr/str formats for the Signature are different from that one.

@hugovk
Copy link
Member

hugovk commented Feb 27, 2024

Isn't documentation expected to be as precise as possible?

No, the style guide advises economy of expression:

More documentation is not necessarily better documentation. Err on the side of being succinct.

It is an unfortunate fact that making documentation longer can be an impediment to understanding and can result in even more ways to misread or misinterpret the text. Long descriptions full of corner cases and caveats can create the impression that a function is more complex or harder to use than it actually is.

@skirpichev
Copy link
Member Author

skirpichev commented Feb 27, 2024

Hmm, I think we have a working alternative for #100168, that has no need in support callables or parsing text. There is also type check, added to solve #66000. I think, adding a setter method in typeobject.c might be an alternative.

Long descriptions full of corner cases and caveats can create the impression that a function is more complex

That's hardly the case. Missing parts aren't just "corner cases" or something "complex". Logic is simple. Just... undocumented.

@Gouvernathor
Copy link
Contributor

Gouvernathor commented Feb 27, 2024

For a background, new logic was introduced recently (since 3.12) in 100039. Maybe ethanfurman could explain why parsing text signatures was chosen instead of using the Signature class in a pure-Python code?

I think that's what we should ascertain in the first place.
I agree with Hugo -it seems- that documenting the behavior in full details is not always the best solution, and I think that's especially true here. For now, the 100039 behavior is undocumented and while it is explainable in a simple way, it may not be something we want to commit to in the long-term.
Also, I'm sorry to say that about something that has been merged and publiqhed as part of a full Release (3.12 it seems), but this evolution is a breaking change, which goes against a behavior that has worked since the beginning I believe, which was introduced in a PEP, and whose proper documentation was backported. #115984 needs to be considered as a solution and as things currently stand, that's the way out I'm supporting.

Edit: it's actually not a breaking change, since a type check on __signature__ dates back from 2014. But after considering the single-use context it was added for, I even more strongly agree with reverting it, for two reasons:

  • the string-parsing behavior is a double of __text_signature__, if we want it to be more workable that's the one that should be changed, not __signature__
  • if __signature__ contains a callable, I wouldn't expect it to return a signature object when called with no arguments (?), I would only assume the signature function to be told to return the signature of that callable. It may pose other problems (loop recursion being one of them), but in any case I don't think the no-param callable use case makes sense.

@skirpichev
Copy link
Member Author

BTW, #115984 is in a simplest version. We could do proper deprecation for changes, introduced in the 3.12 release.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Feb 28, 2024
…() docs

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
erlend-aasland added a commit that referenced this issue Feb 29, 2024
#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 29, 2024
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 29, 2024
…() docs (pythonGH-116086)

(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
gpshead added a commit that referenced this issue Feb 29, 2024
…e() docs (GH-116086) (#116106)

gh-115937: Remove implementation details from inspect.signature() docs (GH-116086)
(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
gpshead added a commit that referenced this issue Feb 29, 2024
…e() docs (GH-116086) (#116107)

gh-115937: Remove implementation details from inspect.signature() docs (GH-116086)
(cherry picked from commit fb2e17b)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
@skirpichev
Copy link
Member Author

@erlend-aasland, could you, please remove "bug" label? Apparently, parsing a string, taken from the __signature__ doesn't considered as an issue by CPython devs.

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this issue Mar 4, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…() docs (python#116086)

Co-authored-by: Carol Willing <[email protected]>
Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants