Hi,
First of all let me say that I agree with the aims of PEP 558 and most
of the design.
I do find the wording of the PEP itself a bit confusing in a couple of
places.
This critique is based on my understanding of the PEP.
If I am mistaken in my misunderstanding, then treat that as an implied
request for clarification of the PEP :)
Critique of PEP 558
===
Layout of the PEP
-
[This is largely to help the reader, it doesn't change the nature of the
PEP]
Could we replace the section "CPython Implementation Changes" with a
section that states what the behavior will be, not how it changes.
Having to mentally apply the suggested changes to the existing (and
convoluted) implementation makes it hard to work out what the proposed
behavior will be.
Could the design discussion be moved to an appendix or another document?
The key parts of it should be moved to the Rationale or Motivation.
There should be a Motivation section explaining why this PEP is
necessary, for those not familiar with the weirdness of `locals()`.
Much of what is in the Rationale should perhaps be in the Motivation.
Proposal
[Ditto; this is largely to help the reader, it doesn't change the nature
of the PEP]
Drop the definitions of the type of scope. They are (at least they
should be) clearly defined in existing documentation.
Why "largely" eliminate the concept of a separate tracing mode? Wasn't
the plan to eliminate it entirely?
Rather than specifying what the documentation will be, could you specify
the semantics. The language here should be precise.
The documentation needs to explain the behavior to the majority of
users, but the PEP should be worded so that it can be implemented
correctly from just reading the PEP.
There is no reason to remove the sugested documentation changes, but
they should be secondary to the more formal specification.
Resolving the issues with tracing mode behaviour
According to this section the `f_locals` object (_PyFastLocalsProxy_Type
in C) will have *two* fields.
It should only have *one*, the pointer to the underlying frame object.
In order to maximize backwards compatibility and, more importantly,
avoid the synchronization issues that lead to obscure bugs, all the
state of the `f_locals` object should be kept on the frame.
Any changes to `f_locals` should be instantly visible to any thread both
through other `f_locals` objects (for the same frame) and the underlying
local variable (if it exists).
E.g.
def __getitem__(self, name):
f = self.frame
if name in f.f_code.co_varnames:
index = f.f_code.co_varnames.index(name)
obj = f.locals[index] # f.locals refers to the fast array of
variables
if obj is NULL:
raise KeyError(name)
return obj
else:
return f._extra_locals[name]
def __setitem__(self, name, value):
f = self.frame
if name in f.f_code.co_varnames:
index = f.f_code.co_varnames.index(name)
CLEAR(f.locals[index])
f.locals[index] = value
else:
f._extra_locals[name] = value
def items(self):
f = self.frame
for index, name in enumerate(f.f_code.co_varnames)
obj = f.locals[index]
if obj is not NULL:
yield name, obj
yield from f._extra_locals.items()
Where `_extra_locals` is a normal dictionary that is not visible to
either Python or the C API.
C API changes
-
The PEP suggests adding four new functions to the stable API.
Then in the "Changes to the public CPython C API" section, another five
functions are added for a total of nine new functions!
At the Python level, there are two ways to access a locals mapping.
1. locals()
2. frame.f_locals
We only need two C functions, one for each of the above.
`PyEval_GetLocals()` should be equivalent to `locals()`.
It will have to return a new reference. It cannot return a borrowed
reference as it would be returning freed memory.
There is nothing to borrow the reference from, for a function scope.
`PyFrame_GetLocals(PyFrameObject *)` should be equivalent to
`frame.f_locals`.
The PEP doesn't explicitly state why `PyLocals_GetKind()` is needed, but
I believe it is avoid unnecessary copying?
Why is creating an extra copy an issue? It takes a microsecond or so to
create the copy.
If a function that returns a copy of the local namespace is really
needed, then why not offer that functionality directly?
E.g. `PyFrame_GetLocalsCopy()` which is roughly:
PyObject *locals = PyEval_GetLocals();
if (current_scope_is_function())
return locals;
return PyDict_Copy(locals); // This leaks, need to decref locals
Reducing the runtime overhead of trace hooks
I'm confused by this part.
Since `_PyFrame_FastToLocals` and friends do not alter the logical state
of the frame object (or anything else), they
