Repository navigation
Revert __signature__ to being a cache, remove string and callable support #116110
Description
Feature or enhancement
Proposal:
#100039 modified the behavior of __signature__ as used by the inspect.signature function :
- if the fields contains a Signature object, it is returned by the function (that doesn't change)
- if it contains None, it is ignored (that doesn't change)
- if it contains a string, it is converted to a signature (new)
- if it contains a callable, it is called with no parameters and expected to return either a signature object or a string which will be treated as the above (new)
- if it contains any other value, a TypeError is raised (this used to include strings and callables, it doesn't anymore)
As for strings :
The __text_signature__ attribute's purpose is to pass a string to inform the signature. It's also only used for C-written functions, and iirc there's no way to use it in pure python : I understand that, but if the ability to specify a text override of inspect.signature must happen, then it should go through __text_signature__ and not __signature__.
As for callables :
The Signature object qualifies, and the signature function inspects, callables. So, if when asked for the signature of a given callable you return another callable, it would make more sense to consider the second callable's signature as the signature of the first callable, than considering the second callable to return the signature of the first. And why no parameters ? Why not pass the first callable as parameter ? Or the actual signature of the first callable ? That implementation raises a lot of questions of arbitrariness, which makes it a bad design in my opinion.
As for the purpose :
The use case, which was adding signature overrides to Enums, does not require any change to the inspect module, in fact a perfectly good alternative implementation can be found in #115937. It should also make for a considerably faster execution.
I suggest reverting these changes.
Links to previous discussion of this feature:
This was discussed in #115937 and #116086 in which it was advised to separate the question of the implementation from that of the documentation.
Activity
skirpichev commentedon Feb 29, 2024
Perhaps, #115984 may be an alternative implementation. But this pr was rejected.
Gouvernathor commentedon Feb 29, 2024
My sentiment reading #116086 is that it was only closed in favor of the pure-doc PR. But there was no actual argument formulated against the merits of your PR, so I think it should be merged as fixing this issue.
merwok commentedon Feb 29, 2024
I think the discussion taking place should reach a conclusion before more changes are done
https://discuss.python.org/t/signatures-for-extension-modules-pep-draft/43914
skirpichev commentedon Mar 1, 2024
Yes, I would appreciate, if @erlend-aasland could do this. I don't think we should reopen this pr without permission from some core developer.
My guess is, that this will introduce some incompatibility. But on another hand, the documentation now says explicitly, that we can break things here. So, I would prefer #115984 for a simpler logic.
I feel that the discussed proposal has a very little support, so far. But more complex logic for handling the
__signature__attribute is not too important for one.erlend-aasland commentedon Mar 1, 2024
I don't have an opinion about that particular change, and I don't have the bandwidth nor desire to involve myself in such a discussion. But such a change would need to be discussed with a broader audience than on a GitHub issue. Please open a discussion on Discourse.
skirpichev commentedon Mar 1, 2024
So, in principle, you will not object if I reopen mentioned PR as a solution for this issue?
pythongh-116110: remove extra processing for the __signature__ attribute
erlend-aasland commentedon Mar 1, 2024
I will not object. Anyone is free to open and re-open PRs. Of course, it is good practice not to repeatedly try to push a change that has been rejected1, as that is seen as pushy or demanding behaviour. I still think your change needs a wider discussion (for example on Discourse).
Footnotes
generally speaking, not specifically regarding your PR ↩
skirpichev commentedon Mar 1, 2024
Thanks.
@Gouvernathor opened d.p.o discussion about docs, I just left comment on possible changes in logic. I hope, it's better than starting a new thread.
Gouvernathor commentedon Mar 1, 2024
I think it's better to a new discourse thread, separate both from this issue and the pure-doc one I opened (since it's located in the documentation part of the forum, it's not really the place to discuss implementation changes).
I also think it would be better not to have two discussions in parallel, so it should probably wait for the other discussion (with the PEP draft) to settle. So, I can either open it now and present it as "on hold", or open it after the other discussion is done. Probably the former, and I'll @ you in it anyway.
I suggest you reopen your PR as a draft (so it doesn't grab attention as something to be reviewed), and link it to this issue if possible.
pythongh-116110: remove extra processing for the __signature__ attribute
Revert "pythongh-116110: remove extra processing for the __signature_…
skirpichev commentedon Oct 7, 2024
FYI: https://discuss.python.org/t/66906
gh-116110: remove extra processing for the __signature__ attribute (G…
pythongh-116110: remove extra processing for the __signature__ attrib…