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-108901: Add inspect.Signature.from_frame #116537

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Mar 9, 2024

I had to go with a bigger diff, but easier code and probably more optimal one.
The main reason was that creating types.FunctionType is not trivial, for example, it required the correct amount of __closure__ vars for a code object. So, let's not create it: it will reduce the amount of possible errors.

First PR: #112639


📚 Documentation preview 📚: https://cpython-previews--116537.org.readthedocs.build/

Doc/library/inspect.rst Outdated Show resolved Hide resolved
Lib/inspect.py Outdated Show resolved Hide resolved
Lib/test/test_inspect/test_inspect.py Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexWaygood
Copy link
Member

I haven't really got the bandwidth to look at this right now, sorry :)

@AlexWaygood AlexWaygood removed their request for review March 10, 2024 08:59
@vstinner
Copy link
Member

cc @serhiy-storchaka @pitrou

@sobolevn
Copy link
Member Author

sobolevn commented Mar 10, 2024

I agree, we should drop the defaults part.

>>> import sys, inspect
>>> def f(x=1):
...    print(inspect.getargvalues(sys._getframe()))
...
>>> f()
ArgInfo(args=['x'], varargs=None, keywords=None, locals={'x': 1})

Current API does not show defaults, only locals. But, locals are stored as .f_locals, it is a stable API. Users can use .f_locals to populate defaults, it is quite easy. Later we can add defaults support if users ask for it.

Showing defaults is also not safe in this case by default, for example we can leak things like passwords or secret keys.

@vstinner
Copy link
Member

I agree, we should drop the defaults part.

I looked at tests where the frame is captured whereas frame locals are not modified, so "it just works" magically. But if I look at the test which modify a variable, I now understand how wrong it is. A frame local is not a function default parameter value: they are two different things. signature() is the signature, before the function is called. Frame locals is the "live state" of a frame, it's unrelated.

Moreover, frame locals can contain sensitive information (like a password) which should not be exposed in a signature.

@sobolevn
Copy link
Member Author

Updated, now defaults are not shown. Right now this is very close to getargvalues:

>>> import sys, inspect
>>> def f(x=1):
...    print(inspect.getargvalues(sys._getframe()))
...
>>> f()
ArgInfo(args=['x'], varargs=None, keywords=None, locals={'x': 1})

And we are trying to replace this old function, so I guess it is fine :)

@hugovk hugovk removed their request for review October 24, 2024 18:00
@hugovk
Copy link
Member

hugovk commented Oct 24, 2024

(This has a merge conflict)

@vstinner
Copy link
Member

@sobolevn: Can you please try to fix the merge conflict?

in a function inside ``__defaults__``, ``__kwdefaults__``,
and ``__annotations__`` attributes.

.. versionadded:: 3.13
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.. versionadded:: 3.13
.. versionadded:: next

inspect
-------

* Add :meth:`inspect.Signature.from_frame` to get signatures from frame objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need the "Contributed by" part? (to have the issue number)

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

Successfully merging this pull request may close these issues.

6 participants