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

gaogaotiantian · 2024-06-19T01:30:15Z

Change is:
- To an Accepted or Final PEP, with Steering Council approval
- To fix editorial issues (mostly missing context for changes originally discussed under PEP 558)
PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

📚 Documentation preview 📚: https://pep-previews--3845.org.readthedocs.build/

carljm

This looks really good!

I think it might also make sense to add a "Rejected Alternatives" section, with a sub-section "Maintaining existing behavior of exec", discussing some of what we talked about in Discourse about the problems with trying to do this.

peps/pep-0667.rst

ncoghlan · 2024-06-19T06:19:29Z

The new section looks good to me. FWIW, What's New in 3.13 does discuss this aspect of the change (https://docs.python.org/dev/whatsnew/3.13.html#defined-mutation-semantics-for-locals), but having more details in the PEP definitely won't hurt.

ncoghlan · 2024-06-19T06:28:55Z

It may also be worth referencing back to the PEP 558 discussions of the impact of changing the way locals() works:

And while PEP 667's text didn't explicitly mention the impact on exec() and eval(), PEP 558 did: https://peps.python.org/pep-0558/#what-happens-with-the-default-args-for-eval-and-exec

This is a case where the use of competing PEPs may have let us down a bit - PEP 667 (quite reasonably) didn't bother repeating the rationale for the aspects where it agreed with the design decisions in PEP 558, so reading it in isolation from PEP 558 doesn't quite give the full picture of the change.

markshannon

Looks good.
I've nothing to add beyond what @carljm and @ncoghlan say.

gaogaotiantian · 2024-06-20T17:11:29Z

So should I post this on disclosure first?

willingc · 2024-06-20T19:10:32Z

peps/pep-0667.rst

+Impact on ``exec()``
+====================
+
+Even though this PEP does not modify ``exec()`` directly, the semantic change


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.

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.

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.

@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?

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.

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 :)

carljm · 2024-06-20T21:52:53Z

So should I post this on disclosure first?

Yes, this should be posted on Discourse to get community feedback, but it probably makes sense to address the existing feedback first?

Co-authored-by: Carl Meyer <[email protected]>

gaogaotiantian · 2024-06-21T00:35:59Z

Yes, this should be posted on Discourse to get community feedback, but it probably makes sense to address the existing feedback first?

Of course, just want to make sure about the procedure. I'm a bit swamped recently.

ncoghlan · 2024-06-21T16:00:32Z

I ticked the SC approval box in the template, since we have that in python/steering-council#245 (comment)

carljm · 2024-06-21T16:03:07Z

since we have that

I could be misunderstanding, but I don't see an indication in that comment that the SC considered the issue of exec() at all. I thought that comment was about C APIs. My understanding is that we don't yet have SC approval (or even consideration) of the issues discussed in this PR.

peps/pep-0667.rst

carljm · 2024-06-21T16:21:06Z

peps/pep-0667.rst

+Impact on ``exec()``
+====================
+
+Even though this PEP does not modify ``exec()`` directly, the semantic change


@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?

Co-authored-by: Carl Meyer <[email protected]>

willingc · 2024-06-21T17:37:44Z

@gaogaotiantian I accepted @carljm's suggestions so that I could look at the prose as it would display to a reader. I am going to add a few suggestions re: grammar, and then I believe this will be acceptable from my viewpoint. Thanks so much for doing this. It adds important clarity for all. 💐

peps/pep-0667.rst

warsaw · 2024-07-03T23:06:44Z

peps/pep-0667.rst

+
+To work with a copy of ``locals()`` on all versions without making
+redundant copies on Python 3.13+, a helper function will need to
+be defined in the affected code::


And by "affected code", do you mean "optimized scopes"?

Reworded to hopefully be clearer

warsaw · 2024-07-03T23:11:55Z

peps/pep-0667.rst

+
+This was historically also equivalent to using the calling frame's
+``frame.f_globals`` and ``frame.f_locals`` attributes, but this PEP maps
+them to ``globals()`` and ``locals()`` in order to preserve the property


"them" is ambiguous. Does it refer to frame.f_globals and frame.f_locals or the arguments to exec() and eval()?

Reworded to remove the ambiguity

peps/pep-0667.rst

warsaw · 2024-07-03T23:15:40Z

peps/pep-0667.rst

+``exec()`` refreshes the cached dict with the actual values on the frame.
+This means that, unlike the "fake" locals created by writing back to ``locals()``
+(including via previous calls to ``exec()``), the real locals known by the
+compiler can't easily be modified by ``exec()``.


"easily" or at all? What would "difficult" code look like?

I've added a reference back to the ctypes code that's part of the Rationale section above.

peps/pep-0667.rst

warsaw

@gaogaotiantian Thank you so much for this PEP update. To me, it really clarifies all the (known!) ambiguities that have been discovered, especially around exec() and eval(). Really fantastic work on this, and thanks also to everyone who has contributed comments in this PR.

I left a number of little one-off comments and suggestions. I don't think any of them are blockers, but to me would help add just a little more clarity in a few places. Please consider them.

In my role as a member of the SC, I'm approving these changes. Before merging, please wait for at least a few other SC members to weigh in. I will ping them privately and we'll reach resolution asap.

gaogaotiantian · 2024-07-03T23:36:20Z

Thanks @warsaw , but the majority of the work was done by @ncoghlan so she deserves the credit.

gaogaotiantian · 2024-07-03T23:41:10Z

peps/pep-0667.rst

+each other. Writes to the ``f_locals`` frame atribute may not show up as
+modifications to local variables if ``PyFrame_LocalsToFast()`` is never
+called. Writes to local variables can get lost if the dictionary snapshot
+is not refreshed before being written back to the frame.


I was a bit confused here. I thought writes to local variables get lost if the snapshot is refreshed before being written back to the frame. So if you do exec("x = 1") then locals() (to refresh the snapshot), your x=1 is lost?

Reworded to hopefully be clearer.

peps/pep-0667.rst

ncoghlan

Latest update should cover Barry & Tian's review comments (I'll go through and resolve the conversions I'm 100% sure are addressed, but there are a few more ambiguous ones that I'll leave unresolved until they've had a chance to look at the updated wording)

peps/pep-0667.rst

ncoghlan · 2024-07-04T02:41:02Z

peps/pep-0667.rst

+each other. Writes to the ``f_locals`` frame atribute may not show up as
+modifications to local variables if ``PyFrame_LocalsToFast()`` is never
+called. Writes to local variables can get lost if the dictionary snapshot
+is not refreshed before being written back to the frame.


Reworded to hopefully be clearer.

peps/pep-0667.rst

ncoghlan · 2024-07-04T02:48:09Z

peps/pep-0667.rst

+
+To work with a copy of ``locals()`` on all versions without making
+redundant copies on Python 3.13+, a helper function will need to
+be defined in the affected code::


Reworded to hopefully be clearer

ncoghlan · 2024-07-04T02:50:45Z

peps/pep-0667.rst

+
+This was historically also equivalent to using the calling frame's
+``frame.f_globals`` and ``frame.f_locals`` attributes, but this PEP maps
+them to ``globals()`` and ``locals()`` in order to preserve the property


Reworded to remove the ambiguity

ncoghlan · 2024-07-04T02:55:32Z

peps/pep-0667.rst

+``exec()`` refreshes the cached dict with the actual values on the frame.
+This means that, unlike the "fake" locals created by writing back to ``locals()``
+(including via previous calls to ``exec()``), the real locals known by the
+compiler can't easily be modified by ``exec()``.


I've added a reference back to the ctypes code that's part of the Rationale section above.

peps/pep-0667.rst

markshannon · 2024-07-08T11:58:16Z

This PR mores than doubles the size of PEP 667. That seems excessive.
Despite that, the actual changes to eval and exec are not made explicit.

The impact on eval and exec can be specified as something like:

"""
Because this PEP changes the behavior of locals(), the behavior of eval and exec are also changed.
Assuming a function _eval which performs the job of eval with explicit arguments for globals and locals, eval can be defined as follows:

def eval(expression, the_globals=None, the_locals=None):
    if the_globals is None:
        the_globals = globals()
    if the_locals is None:
        the_locals = locals()
    return _eval(expression, the_globals, the_locals)

Similarly, for exec.
"""

All the extra history and exposition only serves to obscure the semantics, IMO.

Fleshing the out the rationale a bit seems reasonable, but not too much.

markshannon · 2024-07-08T11:59:49Z

peps/pep-0667.rst


 ``frame.f_locals`` will return a view object on the frame that
-implements the ``collections.abc.Mapping`` interface.
+implements the ``collections.abc.MutableMapping`` interface.


This is incorrect. It should implement the Mapping interface. It may be mutable, but it is not a fully mutable mapping. You can't delete some values, and only strings are supported as keys.

No harm in adding some clarification that is has some mutability, but not the full MutableMapping interface.

Hmm, that's not the current situation either. The current implementation does not allow any deletion from the object. It also supports all kinds of keys per your request : ) .

Personally, I believe it's nicer to only support pure strings (we do have an open issue about this, allowing other types is kind of a mess for the locals proxy). Originally it was implemented that way because the PEP says so, but if we are editing the PEP, should we reconsider that decision? If we still support all types of keys, we need to answer the issue mentioned - what happens if something that looks like a string got passed in?

After checking at the 3.14 interactive prompt, I moved this paragraph to a clearer location and changed it to say:

The view objects fully implement the ``collections.abc.Mapping`` interface, and also implement the following mutable mapping operations: * using assignment to add new key/value pairs * using assignment to update the value associated with a key * conditional assignment via the `setdefault()` method * bulk updates via the `update()` method Removing keys with `del` statements or the `pop()` method is NOT supported.

The first three could be simplified to item assignment (including via the setdefault() method), but calling out addition and rebinding separately seemed worthwhile (since they're genuinely different proxy operations)

ncoghlan · 2024-07-09T07:54:04Z

I had omitted exec() and eval() from the specification section since their default argument handling isn't actually changing in CPython. However, other implementations might have implemented these builtins to instead default to sys._getframe().f_globals and sys._getframe().f_locals, so you're right that it should be included.

Unfortunately, I don't see a neat way of expressing the semantics of exec() and eval() as equivalent Python code. Using globals() and locals() as suggested isn't right, as they end up being called relative to the wrong frame (it only works for the builtin function implementations because they don't create a new Python frame).

It's possible to get the semantics correct using sys._getframe(), but the resulting code seems less clear to me than the prose descriptions of:

"if globals is not specified, then the result of calling globals() in the calling namespace is used"; and
"if locals is not specified, then the result of calling locals() in the calling namespace is used"

However, I'll see how the code version looks in context, since I agree that precision is desirable here.

Edit: the semantically correct code version ended up looking fine, so I included it in the specification section (it's also useful for anyone else wanting to emulate this argument handling in their own Python code):

    FrameProxyType = type((lambda: sys._getframe().f_locals)())

    def eval(expression, the_globals=None, the_locals=None):
        if the_globals is None:
            # No globals -> use calling frame's globals
            _calling_frame = sys._getframe(1)
            the_globals = _calling_frame.f_globals
            if the_locals is None:
                # No globals or locals -> use calling frame's locals
                the_locals = _calling_frame.f_locals
                if isinstance(the_locals, FrameProxyType):
                    # Align with locals() builtin in optimized frame
                    the_locals = the_locals.copy()
        elif the_locals is None:
            # Globals but no locals -> use same namespace for both
            the_locals = the_globals
        return _eval(expression, the_globals, the_locals)

(Related: it's unfortunate it is too late in the release cycle to add a frame.locals() helper method to encapsulate calling f_locals.copy() on optimized frames and using f_locals directly otherwise. The fact there's no longer an entirely straightforward way to recreate the builtin code evaluation semantics in a pure Python function does feel like a genuine functional gap)

I agree the extra words in the backwards compatibility section aren't useful to implementers, but I do think they're potentially useful to regular Python users bitten by the change to make locals() return independent snapshots (as well as in reassuring the SC that some form of compatibility break is genuinely unavoidable, so it's a matter of choosing which one is the most acceptable rather than it being a gratuitous break).

* exec/eval semantics should be clearly specified * proxies do not fully implement MutableMapping

Add impact on exec() for PEP 667

ab71e9a

gaogaotiantian requested a review from markshannon as a code owner June 19, 2024 01:30

gaogaotiantian requested review from carljm and ncoghlan June 19, 2024 01:30

Fix lint

3d86025

carljm reviewed Jun 19, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

peps/pep-0667.rst Outdated Show resolved Hide resolved

peps/pep-0667.rst Outdated Show resolved Hide resolved

peps/pep-0667.rst Outdated Show resolved Hide resolved

peps/pep-0667.rst Outdated Show resolved Hide resolved

ncoghlan mentioned this pull request Jun 19, 2024

PEP 667: correct/clarify handling of PyEval_GetLocals python/steering-council#245

Open

markshannon reviewed Jun 20, 2024

View reviewed changes

willingc reviewed Jun 20, 2024

View reviewed changes

Apply suggestions from code review

8729874

Co-authored-by: Carl Meyer <[email protected]>

carljm reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

carljm reviewed Jun 21, 2024

View reviewed changes

Apply @carljm suggestions from review

e2f0b94

Co-authored-by: Carl Meyer <[email protected]>

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

willingc reviewed Jun 21, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw reviewed Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

warsaw approved these changes Jul 3, 2024

View reviewed changes

gaogaotiantian commented Jul 3, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

ncoghlan added 2 commits July 4, 2024 12:30

Merge branch 'main' into pep-667-exec-impact

c5d35e1

Address review comments from Barry & Tian

32fc5fb

ncoghlan reviewed Jul 4, 2024

View reviewed changes

ncoghlan added 3 commits July 4, 2024 13:12

Syntax fix

6449585

Fix typo

a0881b1

f_locals is mutable, not read-only!

8dec696

ncoghlan reviewed Jul 4, 2024

View reviewed changes

peps/pep-0667.rst Outdated Show resolved Hide resolved

Misc proofreading edits

4151acb

markshannon reviewed Jul 8, 2024

View reviewed changes

ncoghlan added 5 commits July 9, 2024 18:26

Define exec/eval, fix proxy mutability

4871a39

* exec/eval semantics should be clearly specified * proxies do not fully implement MutableMapping

Fix syntax, update implementation note

ad391ea

Another syntax fix

2269f65

Make snapshot code consistent

1539941

Use 3.13 signature for eval spec

0c5aea9

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

Are you sure you want to change the base?

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

Conversation

gaogaotiantian commented Jun 19, 2024 • edited by ncoghlan Loading

carljm left a comment

Choose a reason for hiding this comment

ncoghlan commented Jun 19, 2024

ncoghlan commented Jun 19, 2024

markshannon left a comment

Choose a reason for hiding this comment

gaogaotiantian commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

carljm commented Jun 20, 2024

gaogaotiantian commented Jun 21, 2024

ncoghlan commented Jun 21, 2024

carljm commented Jun 21, 2024

Choose a reason for hiding this comment

willingc commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warsaw left a comment

Choose a reason for hiding this comment

gaogaotiantian commented Jul 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markshannon commented Jul 8, 2024

markshannon Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan commented Jul 9, 2024 • edited Loading

gaogaotiantian commented Jun 19, 2024 •

edited by ncoghlan

Loading

ncoghlan Jun 25, 2024 •

edited

Loading

ncoghlan left a comment •

edited

Loading

markshannon Jul 8, 2024 •

edited

Loading

ncoghlan commented Jul 9, 2024 •

edited

Loading