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

PEP 667: Clarify specification ambiguities and the expected impact #3845

Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open
Changes from 4 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
ab71e9a
Add impact on exec() for PEP 667
gaogaotiantian Jun 19, 2024
3d86025
Fix lint
gaogaotiantian Jun 19, 2024
8729874
Apply suggestions from code review
gaogaotiantian Jun 21, 2024
e2f0b94
Apply @carljm suggestions from review
willingc Jun 21, 2024
98edffa
Incorporate feedback and PyEval_GetLocals PR
ncoghlan Jun 25, 2024
9a2a7d3
Syntax fixes, PEP 558 comparison tweaks
ncoghlan Jun 25, 2024
e67a37d
Move f_locals spec note to spec section
ncoghlan Jun 25, 2024
a2b6b33
Add missing space
ncoghlan Jun 25, 2024
6be7b28
Indent ctypes example
ncoghlan Jun 25, 2024
f2423f0
Minor syntax and wording edits
ncoghlan Jun 25, 2024
82f94fb
Reword mention of "real" locals.
ncoghlan Jun 25, 2024
4a85e39
PEP link fix, minor text edits
ncoghlan Jun 25, 2024
6115fbd
Add note about the extra PEP 558 C APIs being omitted
ncoghlan Jun 25, 2024
669a0bc
Another link syntax fix
ncoghlan Jun 25, 2024
c68d02c
Merge branch 'main' into pep-667-exec-impact
ncoghlan Jun 25, 2024
9a966dd
Fix incorrect prefix in function name
ncoghlan Jun 25, 2024
98eddaf
Improve locals() implementation note
ncoghlan Jun 25, 2024
a8a700d
Clarify the note about deoptimization
ncoghlan Jun 25, 2024
7808759
Be clear there is no `is_function` frame method
ncoghlan Jun 25, 2024
6a89a65
More syntax and wording tweaks
ncoghlan Jun 25, 2024
22494e4
Further updates
ncoghlan Jun 25, 2024
828daa2
No joy on mixed syntax highlighting
ncoghlan Jun 25, 2024
1bc2eaa
Remove leftover comma
ncoghlan Jun 27, 2024
a924f4b
Reword note about better optimization support
ncoghlan Jun 28, 2024
c5d35e1
Merge branch 'main' into pep-667-exec-impact
ncoghlan Jul 4, 2024
32fc5fb
Address review comments from Barry & Tian
ncoghlan Jul 4, 2024
6449585
Syntax fix
ncoghlan Jul 4, 2024
a0881b1
Fix typo
ncoghlan Jul 4, 2024
8dec696
f_locals is mutable, not read-only!
ncoghlan Jul 4, 2024
4151acb
Misc proofreading edits
ncoghlan Jul 4, 2024
4871a39
Define exec/eval, fix proxy mutability
ncoghlan Jul 9, 2024
ad391ea
Fix syntax, update implementation note
ncoghlan Jul 9, 2024
2269f65
Another syntax fix
ncoghlan Jul 9, 2024
1539941
Make snapshot code consistent
ncoghlan Jul 9, 2024
0c5aea9
Use 3.13 signature for eval spec
ncoghlan Jul 16, 2024
a846667
Merge branch 'main' into pep-667-exec-impact
ncoghlan Jul 26, 2024
8abca04
Delay `PyEval_GetLocals` hard deprecation to 3.14
ncoghlan Jul 27, 2024
d3bff7b
Apply suggestions from code review
ncoghlan Jul 30, 2024
40e59cb
Merge branch 'main' into pep-667-exec-impact
ncoghlan Jul 30, 2024
1d4447d
Move locals rationale details to PEP 558
ncoghlan Aug 5, 2024
80e115b
Merge remote-tracking branch 'origin/main' into pep-667-exec-impact
ncoghlan Aug 5, 2024
c84fc0d
Merge remote-tracking branch 'origin/main' into pep-667-exec-impact
ncoghlan Aug 5, 2024
b7b427f
Merge branch 'main' into pep-667-exec-impact
ncoghlan Aug 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions peps/pep-0667.rst
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,70 @@ C API
As with all functions that return a borrowed reference, care must be taken to
ensure that the reference is not used beyond the lifetime of the object.

Impact on ``exec()``
====================

Even though this PEP does not modify ``exec()`` directly, the semantic change
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the first pass at trying to document this behavior. I would suggest reframing this a bit to:

  • Call out the specific semantic change to locals() to give the reader context
  • Explain concisely how this PEP impacts or explicitly documents why a behavior change may be seen when using exec() with performance optimized locals().
  • Make clear the expected behavior before explaining the examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the semantic changes to locals(), it's mentioned in this PEP a bit earlier. We should assume the reader already reads that part right? How should I either refer back or summarize that?

The reason of the behavior change is basically

The core difference is that the current implementation returns the same dict
when locals() is called multiple times in function scope, so the following
code magically works

About the expected behavior, do you mean something like "the local variable update will not be propagated between exec()"? I feel difficult to describe the exact behavior otherwise I would probably just summarize it in words instead of examples. This is a rather subtle change and the original behavior is kind of odd. I would love any help from people who are better at documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @gaogaotiantian. We can definitely iterate on this. Looking at @carljm's message, I think there are three concepts that the reader may confuse: locals(), locals argument, and frame.f_locals for functions such as exec().

I'm not sure what the best wording would be but I think we should iterate until it seems clear to folks. Mostly, I want to make sure that someone reading the PEP and using exec() has a clear idea of what the expected behavior should be.

P.S. I'm very happy that you are taking the time to make this PEP addition.

Copy link
Member

Choose a reason for hiding this comment

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

@willingc I feel that these concepts are pretty well communicated by the current draft, with my suggested changes incorporated. First the current behavior of exec() is explained (with an example), then the reason for that behavior (and the connection to locals() is explained, then some additional confusing implications of the current behavior are shown (with an example), and then the new behavior is explained (which doesn't take very long, since it's much simpler), and then examples are given to demonstrate how to accomplish some things that are currently possible, under the new behavior. Are there particular aspects of this presentation that you don't find sufficiently clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @carljm. Let's accept your suggestions, and I will give it another read. I'm primarily concerned that the expected behavior for 3.13+ is clear to readers. That may be so after your changes are included. I have a hard time following the wording flow until I see it as one prose document.

Copy link
Contributor

@ncoghlan ncoghlan Jun 25, 2024

Choose a reason for hiding this comment

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

After my Discord comments about even PEP 558 not explaining the locals() changes particularly well, I went back to PEP 558, the related python-dev thread, and even PEP 3100 to find the material that actually went into the decision to make locals() and exec() work as PEP 558 and PEP 667 both propose.

The early iterations of PEP 558 actually tried to standardise locals() without changing it, so hopefully the new text makes it clear why that approach didn't last long :)

to ``locals()`` impacts the behavior of ``exec()``.

``exec(object)`` has been equivalent to ``exec(object, globals(), locals())``
for a long time, but it's not officially documented. We plan to keep this
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
relation, which means existing code using ``exec()`` will see some different
behaviors.

The core difference is that the current implementation returns the same dict
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
when ``locals()`` is called multiple times in function scope, so the following
code magically works::

def f():
exec('a = 0') # equivalent to exec('a = 0', globals(), locals())
exec('print(a)') # equivalent to exec('print(a)', globals(), locals())
print(locals()) # {'a': 0}
# However, print(a) will not work here
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
f()

``locals()`` in a function currently returns the same persisted dict for every call. This allows stashing
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
extra "fake locals" in that dict, which aren't real locals known by the compiler, but can still be seen in
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
``locals()`` and shared between multiple ``exec()`` in the same function scope.

This behavior gets harder to predict when the code in ``exec()`` tries to write
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
to an existing local variable::
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved

def f():
a = None
exec('a = 0') # equivalent to exec('a = 0', globals(), locals())
exec('print(a)') # equivalent to exec('print(a)', globals(), locals())
print(locals()) # {'a': None}
f()

``print(a)`` will print ``None`` because the implicit ``locals()`` call in
``exec()`` refreshes the cached dict with the actual values on the frame.
So "real" locals, unlike "fake" locals created via ``exec()``, can't easily be
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
modified by ``exec()``.

With the semantic changes to ``locals()`` in this PEP, it's easier to explain the behavior
of ``exec()`` - it will never affect local function scopes. Any assignment
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved
to the local variables will be discarded when ``exec()`` returns.

However, you can still achieve a shared namespace across `exec()` calls with explicit namespaces::
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved

def f():
scope = {}
exec('a = 0', locals=scope)
exec('print(a)', locals=scope)
f()

You can even change the variables in the local scope by explicitly using
``frame.f_locals``, which was not possible before::

def f():
a = None
exec('a = 0', locals=sys._getframe().f_locals)
print(a) # 0
f()

The behavior of ``exec()`` for module and class scopes is not changed.

Impact on PEP 709 inlined comprehensions
========================================

Expand Down
Loading