On Mon, Aug 23, 2021 at 8:46 AM Mark Shannon <mark@hotpy.org> wrote:
Hi Guido,

On 23/08/2021 3:53 pm, Guido van Rossum wrote:
> On Mon, Aug 23, 2021 at 4:38 AM Mark Shannon <mark@hotpy.org
> <mailto:mark@hotpy.org>> wrote:
>
>     Hi Nick,
>
>     On 22/08/2021 4:51 am, Nick Coghlan wrote:
>
>      > If Mark's claim that PyEval_GetLocals() could not be fixed was
>     true then
>      > I would be more sympathetic to his proposal, but I know it isn't
>     true,
>      > because it still works fine in the PEP 558 implementation (it even
>      > immediately sees changes made via proxies, and proxies see
>     changes to
>      > extra variables). The only truly unfixable public API is
>      > PyFrame_LocalsToFast().
>
>     You are making claims that seem inconsistent with each other.
>     Namely, you are claiming that:
>
>     1. That the result of locals() is ephemeral.
>     2. That PyEval_GetLocals() returns a borrowed reference.
>
>     This seems impossible, as you can't return a borrowed reference to
>     an emphemeral object. That's just a pointer to freed memory.
>
>     Do `locals()` and `PyEval_GetLocals()` behave differently?
>
>
> That is my understanding, yes. in PEP 558 locals() returns a snapshot
> dict, the Python-level f_locals property returns a fresh proxy that has
> no state except a pointer to the frame, and PyEval_GetLocals() returns a
> borrowed reference to the dict that's stored on the frame's C-level
> f_locals attribute

Can we avoid describing the C structs in any of these PEPs?

It confuses readers having Python attributes and "C-level attributes"
(C struct fields?).
It also restricts the implementation unnecessarily.

(E.g. the PyFrameObject doesn't have a `f_locals` field in 3.11:
https://github.com/python/cpython/blob/main/Include/cpython/frameobject.h#L7)

I'd be happy to. Nick's PEP still references it (and indeed it is very confusing) and I took it from him. And honestly it would be nice to have a specific short name for it, rather than circumscribing it with "an internal dynamic snapshot stored on the frame object " :-)
 
>
> (In my "crazy" proposal all that is the same.)

>
>     Is the result of `PyEval_GetLocals()` cached, but `locals()` not?
>
>
> I wouldn't call it a cache -- deleting it would affect the semantics,
> not just the performance. But yes, it returns a reference to an object
> that is owned by the frame, just as it does in 3.10 and before.
>
>     If that were the case, then it is a bit confusing, but could work.
>
>
> Yes, see my "crazy" proposal.
>
>     Would PyEval_GetLocals() be defined as something like this?
>
>     (add _locals_cache attribute to the frame which is initialized to NULL).
>
>     def PyEval_GetLocals():
>           frame._locals_cache attribute = locals()
>           return borrow(frame._locals_cache attribute)
>
>
> Nah, the dict returned by PyEval_GetLocals() is stored in the frame's
> C-level f_locals attribute, which is consulted by the Python-level
> f_locals proxy -- primarily to store "extra" variables, but IIUC in
> Nick's latest version it is also still used to cache by that proxy.
> Nick's locals() just returns dict(sys._getframe().f_locals).

The "extra" variables must be distinct from the result of locals() as
that includes both extras and "proper" variables.
If we want to cache the locals(), it needs to be distinct from the extra
variables.

I don't care that much about caching locals(), but it seems we're bound to cache any non-NULL result from PyEval_GetLocals(), since it returns a borrowed reference. So they may be different things, with different semantics, if we don't cache locals().
 
A debugger setting extra variables in a function that that is also
accessed by a C call to PyEval_GetLocals() is going to be incredibly
rare. Let's not worry about efficiency here.

Agreed.

>
>     None of this is clear (at least not to me) from PEP 558.
>
>
> One problem with PEP 558 is that it's got too many words, and it's
> lacking a section that crisply describes the semantics of the proposed
> implementation. I've suggested to Nick that he add a section with
> pseudo-code for the implementation, like you did in yours.
>
> (PS, did you read my PS about what locals() should do in class scope
> when __prepare__ returns a non-dict?)

Yes, but no harm in a reminder :)
I'll update my PEP to fix the semantics of locals().

I'm in suspense as to what semantics you chose. :-)

PS. Another point where you seem to have missed some detail is in the mapping from (proper) variable names to "frame locals" -- you use co_varnames, but that (currently, at least) doesn't include cells.

--
--Guido van Rossum (python.org/~guido)