-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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-86682: Adds sys._getframemodulename as a lightweight alternative to _getframe #99520
Conversation
…ame calls in collections, doctest, enum, and typing modules
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 typing change is fine, thanks for picking this up!
I might like your original version (sys._get_calling_module_name
) better. It exposes less data, so it's easier to keep the interface if interpreter internals change.
Yeah, turns out we can't use |
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.
IMO the news file should mention the new function too.
I’d like a review by @markshannon .
I’m surprised the calling function isn’t obtainable, but the use cases are there, so no complaints about that.
I'd like a review from Mark, too. The calling function is obtainable, even more easily than the module name. But in every case we already use this, we'd then have to go to If there's a use case for both, I'll happily add both, but right now, we don't need the function object for anything. |
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.
Really need to get @markshannon to answer the question. But he’s on vacation this week.
I'm thinking aloud here, so bear with me... I don't think that It feels to me that this API is doing two things, getting the frame, then the module name. Why not split the parts?
or, potentially
Unfortunately this doesn't achieve the stated goal of removing (almost) all uses of Which leads us back to adding a function to get the caller function.
Which seems cleaner to me. |
@zooba You state in an earlier comment that |
My cursory scan of the code suggests that |
Even in the repl, it seems that functtions have |
I didn't trace down exactly why it wasn't being filled in correctly, but I'm willing to bet it's a much more complicated compatibility issue to change it. It's definitely a bit of an odd API though, I agree. That's why I happily tried returning the function object instead. Perhaps it was only doctests that didn't set it correctly? Also keen on your thoughts on the |
You need to check for invalid frames when walking the frame stack. |
OOI, what tests fail if you rely on |
I've been looking into the The only reliable workaround is to get it from I added a better test to compare |
That sounds like a bug. Let's fix it rather than working around it. |
Seems easy enough to fix, there's just a constructor that needs to pull Even so, I still don't see a reason why we'd return function objects. Our code either needs the module name, or else it needs actual frame information (e.g. current lineno). This is a private API, so if we need something different in the future we can always change it. This is also why I made it not raise an error but return |
Two main reasons:
My feeling is that |
I started writing a reply earlier making the argument that only returning the name wouldn't require a red flag (I think it still deserves an event, but it's far less of a risk than if random code is unexpectedly doing The most interesting thing you can do with a function object is mess with But because we don't use it, it means instead of |
If there are no other concerns raised I'd like to merge this sometime this week. |
@@ -93,7 +93,10 @@ _PyFunction_FromConstructor(PyFrameConstructor *constr) | |||
op->func_doc = Py_NewRef(Py_None); | |||
op->func_dict = NULL; | |||
op->func_weakreflist = NULL; | |||
op->func_module = NULL; | |||
op->func_module = Py_XNewRef(PyDict_GetItem(op->func_globals, &_Py_ID(__name__))); |
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.
PyDict_GetItem is broken by design and should not be used. See #106033.
Replaces _getframe calls in collections, doctest, enum, and typing modules