Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gh-108901: Add
Signature.from_code
method #108902New 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-108901: Add
Signature.from_code
method #108902Changes from all commits
566790c
20875cb
604f25b
d69623b
b3e7978
efdb617
77da9c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I worry that the reason this wasn't implemented in the original PEP-362 implementation is because the signatures produced for
func1
,func2
andfunc5
here are all incorrect, due to the fact that certain parameters have default values but this isn't reflected in the produced signatures :/inspect.getargs()
is used a fair bit in third-party projects. In the vast majority of these uses, they already have access to the function object, so they should probably just useSignature.from_callable()
instead of this new method. But there are a few uses where they don't have access to the function object -- so I agree that something like this new method is probably needed, as a replacement for those uses:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... well, they are actually valid signatures for the code object, since if you pass the code object to
exec()
, it doesn't take any notice of whether the original function had a default value or not:Why anybody would care what the signature of the code object itself is, though, I'm not sure -- ISTM that everybody trying to get the signature from a code object is just using the code object as the next-best-thing for the function the code object belongs to. I don't think many people are passing code objects directly to
exec()
It's not even really possible to pass a parameter to a code object you're calling viaexec()
AFAIK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just need to flag more forcefully in the docs the limitations of this new constructor method. I can offer some suggestions once you've addressed the review comments I've already posted 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a
.. note::
notation to highlight it even better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more concerned I am :/
The existing
getargs()
function that we have ininspect
gives accurate information. It doesn't claim to be able to tell you everything about the signature: it only claims to be able to tell you some information about the signature:*vararg
argument, if there is one**kwarg
argument, if there is oneThis new function, however, claims to be able to give you a complete and accurate signature from a code object (in that it constructs a
Signature
object from a code object). That's impossible, since code objects don't have information about which arguments have default values, and so the function gives you inaccurate information.I'm not sure having a prominent note in the docs is sufficient to fix this issue: adding a new constructor that we know gives inaccurate results seems very troubling to me. Perhaps a better solution would be to document
getargs
and promote it to public API?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. This new API would provide helpful information that is not currently offered by
getargs
.However. Does it make sense to think of code objects even having a "signature"? Functions have signatures, and you can construct functions from code objects. But as your snippet above demonstrates, you can't directly call code objects (except via
exec()
, but nobody does that really) -- you have to create a new function from the code object in order to get something that can be called.Which "signature" are we trying to get here? The signature of the function from which the code object originally came (if it came from a code object), or the signature a new function object would have if we created one from the code object? Getting a "signature from a code object" feels like quite a confusing concept to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option: we could document that if people want to get "signature information" from a code object
co
, they just need to do this:That way, they are forced to be explicit that code objects don't really have a "signature": you need to construct a new function from the code object in order to get a code object that actually has a signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is up to you to decide whether or not we should do it. I consider this PR done from my side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I'm leaning towards not adding this alternative constructor, and just documenting that people should call
inspect.signature(types.FunctionType(co, {}))
if they need to get signature information from a code objectco
, and the function object is not available for whatever reason.One of the third-party projects that uses
getargs
is Quora: https://github.com/quora/qcore/blob/e12a4929c8e75ec2dd878c06270cedcac72a643e/qcore/inspection.py#L130-L148. I'd be curious to know if @JelleZijlstra has any thoughts on whethergetargs
should be documented or deprecated and, if the latter, whether we should add thisSignature.from_code()
method or just add documentation along the lines of my sugestion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any code using that qcore function should really be migrated to use
inspect.signature
directly. As the docstring notes, the function doesn't deal correctly with kwonly arguments.I think we don't need
Signature.from_code
. My experience is that it is rare that you have only a code object without a function it came from, and code that manipulates code objects directly should generally be migrated to a more modern inspect API.