-
Notifications
You must be signed in to change notification settings - Fork 229
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
Allow function definition locals() to be modified in the shell #571
base: main
Are you sure you want to change the base?
Conversation
This still requires more testing, especially for the different shells. For the IPython shell, this requires not making a copy of locals(), but this also causes the IPython shell to pollute the locals() namespace with all its internal variables (like _0 and so on), so a better fix is needed. Fixes inducer#26.
This also causes IPython to inject all its magic variables (_, _0, get_ipython, etc.) into the locals namespace every time it is started. Unless there is some way to disable this, this is not worth this functionality, since it pollutes the variables view and also would break any code that happens to use those same variable names.
Probably going to just revert the change to the IPython shell, meaning this won't work there (but it will work in the inline shell). Unless IPython has a method to turn this off, we would have to manually remove them every time IPython is started. It would also break any code that happens to use the same variable names as the ones IPython injects (which includes |
So based on some (very basic) testing:
We'll probably want some way to disable this behavior, as you might not actually want the variables you type in the shell to inject themselves into your code. Although note that this is already the behavior at the module level because writing to What should the config look like? We can force it to work with any of the shells, even the ones that inject a lot of variables. So what should the default look like? Ideally for the internal and classic shells it would be enabled by default and for IPython it should be disabled by default. |
This could probably use some more testing, but for now, the main thing I know is missing is a configuration option, which I'd like feedback on how to best do (see my previous comment). Should it just default to the behavior being on (or off) or try to set a "best guess" default depending on which shell is enabled? Or should it default to on but only actually work in the shells that don't inject variables (the rest would always copy locals() to prevent it from working). CC @inducer |
I would be OK with just removing ptipython.
I would vote for keeping this simple to start. Either propagate or don't. And if ipython is being used with "yes, propagate", maybe print a one-line warning every time it starts, stating that IPython spews copious amounts of variables. |
@@ -327,6 +327,7 @@ def set_frame_index(self, index): | |||
return | |||
|
|||
self.curframe, lineno = self.stack[index] | |||
self.curframe_locals = self.curframe.f_locals |
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 add a comment explaining what it's good for? TBH, I'm not sure I understand why the proposed changes have the described effect. (at least with just looking at the diff)
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 might try to dig deeper to figure out exactly what is going on. From what I can tell, as long as you reuse the same locals object, any modifications you make to it are reflected when the frame is run. But if you access frame.f_locals
again, it recomputes the locals and resets them. I'm unclear exactly how it works, but it does.
I'm actually unclear why your example at #26 (comment) didn't work for you. I tested it and even in Python 2, it has the desired effect when you set i
on the given line.
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, you've nerd-sniped me. Grr. 🙂
Here's what I've found so far:
- Accessing
f_locals
winds up here: https://github.com/python/cpython/blob/c450c8c9ed6e420025f39d0e4850a79f8160cdcd/Objects/frameobject.c#L25-L32 - Which ends up overwriting
f_locals
, which is the same object every time you access it, here: https://github.com/python/cpython/blob/v3.11.0/Objects/frameobject.c#L1097 - I'm still a bit hazy on how changes in
f_locals
make their way from there back to the "fast" local variable storage. I lost the trail atPyFrame_LocalsToFast
, in that I wasn't totally able to pin down what causes it to get called.
Some notes:
- Interestingly, it seems that all this code was significantly rewritten for 3.12.
- There also seem to have been some changes here for 3.11. Search for
PyFrame_LocalsToFast
here.
Whatever we do here, we should make sure to preserve a summary of it in comments, so that we don't have to rediscover it some other time.
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 found https://peps.python.org/pep-0558/, which mentions this behavior (footnote 3), although the link there is broken.
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.
That PEP is very technical, but I think what happens is:
- locals is designed to be snapshotted. That's why when you do something like
it prints
def f(): x = 1 locals()['x'] = 2 print(x) f()
1
instead of2
. frame.f_locals
is designed to be a write-through proxy (probably exactly for this feature).- What I think happens is that the descriptor for
f_locals
computes this write-through proxy from a snapshot. That means that when you access it, it recomputes it from what it was, and throws away whatever was there before. Since it's a write-through proxy, this effectively resets the locals in the frame.
But I'm not completely sure about this. Honestly I think a core dev would need to confirm the details. I think I'll just cross-reference this PEP in the comments since that should be a good jumping off point for anyone who is interested in what is going on.
This still requires more testing, especially for the different shells.
For the IPython shell, this requires not making a copy of locals(), but this also causes the IPython shell to pollute the locals() namespace with all its internal variables (like _0 and so on), so a better fix is needed.
Fixes #26.