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.

Sign up for GitHub

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

Jump to bottom

gh-74929: Implement PEP 667 #115153

Merged
merged 44 commits into from May 4, 2024
Merged

gh-74929: Implement PEP 667 #115153

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
42d7186
Basic prototype for frame proxy
gaogaotiantian Feb 7, 2024
7eeab1b
Fix some lint and remove oprun check
gaogaotiantian Feb 8, 2024
60e70e7
Not entirely work yet
gaogaotiantian Feb 8, 2024
1454ce4
Fix a bug
gaogaotiantian Feb 8, 2024
0045274
Change code style and add GC
gaogaotiantian Feb 13, 2024
de73bc9
Clean up code
gaogaotiantian Feb 13, 2024
ca92393
Disable all fast/local functions
gaogaotiantian Mar 1, 2024
ff886ff
Update tests for the new f_locals
gaogaotiantian Mar 2, 2024
9690a2d
Comment out the pop for now
gaogaotiantian Mar 2, 2024
6e9848a
Convert f_locals to dict first
gaogaotiantian Mar 2, 2024
b84b0df
Add static to static functions, add interface for new C API
gaogaotiantian Apr 24, 2024
bebff28
Add some tests and a few methods
gaogaotiantian Apr 26, 2024
d846de9
Implement all methods
gaogaotiantian Apr 27, 2024
d00a742
Make f_extra_locals extra lazy
gaogaotiantian Apr 27, 2024
1a4344d
Merge branch 'main' into pep667
gaogaotiantian Apr 27, 2024
f720e12
Fix typo
gaogaotiantian Apr 27, 2024
64d3772
Remove print debugging
gaogaotiantian Apr 27, 2024
2eadbf0
Fix some styling issue
gaogaotiantian Apr 27, 2024
cbae199
Update generated files for cAPI
gaogaotiantian Apr 27, 2024
9e7edf8
Remove f_fast_as_locals and useless calls for sys.settrace
gaogaotiantian Apr 27, 2024
523cb75
📜🤖 Added by blurb_it.
blurb-it[bot] Apr 27, 2024
4b83311
Add the new type to static types
gaogaotiantian Apr 27, 2024
ae2db7c
Remove internal APIs for fast locals
gaogaotiantian Apr 27, 2024
bf45c02
Add extra tests for closure
gaogaotiantian Apr 27, 2024
026e15e
Add the type to globals-to-fix
gaogaotiantian Apr 27, 2024
f42980d
Add CAapi test
gaogaotiantian Apr 27, 2024
e693ad0
Polish lint
gaogaotiantian Apr 27, 2024
5dd045b
Apply some simple changes
gaogaotiantian Apr 28, 2024
30ecd4d
Update Misc/NEWS.d/next/Core and Builtins/2024-04-27-21-44-40.gh-issu…
gaogaotiantian Apr 28, 2024
f35c5e3
Abstract the key index part
gaogaotiantian Apr 28, 2024
5844fb4
Fix error handling
gaogaotiantian Apr 28, 2024
06277f9
Make key index work better
gaogaotiantian Apr 28, 2024
e1c3f56
Add comments for GetHiddenLocals
gaogaotiantian Apr 30, 2024
b672d84
Add global test
gaogaotiantian Apr 30, 2024
3e32572
Remove unsupported methods
gaogaotiantian May 2, 2024
8dc4664
Support non-string keys
gaogaotiantian May 2, 2024
652f641
Use static function for setitem
gaogaotiantian May 2, 2024
f29e6a3
Fix the list comp
gaogaotiantian May 3, 2024
e0ca4fe
Fix mapping check
gaogaotiantian May 3, 2024
f78156a
Fix frame_getlocals
gaogaotiantian May 3, 2024
4503145
Fix test error
gaogaotiantian May 3, 2024
cdac22c
Change the new ref for getcode
gaogaotiantian May 3, 2024
49287ff
Avoid creating the frame object if possible
gaogaotiantian May 3, 2024
378aacf
Remove a single blank line
gaogaotiantian May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Include/cpython/frameobject.h
View file
Open in desktop
Expand Up @@ -27,3 +27,9 @@ PyAPI_FUNC(int) _PyFrame_IsEntryFrame(PyFrameObject *frame);

PyAPI_FUNC(int) PyFrame_FastToLocalsWithError(PyFrameObject *f);
PyAPI_FUNC(void) PyFrame_FastToLocals(PyFrameObject *);
ncoghlan marked this conversation as resolved.
Show resolved Hide resolved


typedef struct {
PyObject_HEAD
PyFrameObject* frame;
} PyFrameLocalsProxyObject;
2 changes: 2 additions & 0 deletions Include/cpython/pyframe.h
View file
Open in desktop
Expand Up @@ -3,8 +3,10 @@
#endif

PyAPI_DATA(PyTypeObject) PyFrame_Type;
PyAPI_DATA(PyTypeObject) PyFrameLocalsProxy_Type;

#define PyFrame_Check(op) Py_IS_TYPE((op), &PyFrame_Type)
#define PyFrameLocalsProxy_Check(op) Py_IS_TYPE((op), &PyFrameLocalsProxy_Type)

PyAPI_FUNC(PyFrameObject *) PyFrame_GetBack(PyFrameObject *frame);
PyAPI_FUNC(PyObject *) PyFrame_GetLocals(PyFrameObject *frame);
Expand Down
6 changes: 5 additions & 1 deletion Include/internal/pycore_frame.h
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ struct _frame {
char f_trace_lines; /* Emit per-line trace events? */
char f_trace_opcodes; /* Emit per-opcode trace events? */
char f_fast_as_locals; /* Have the fast locals of this frame been converted to a dict? */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can f_fast_as_locals be dropped now? It was associated with the old dict conversion functions that are now no-ops.

PyObject *f_extra_locals; /* Dict for locals set by users using f_locals, could be NULL */
/* The frame data, if this frame object owns the frame */
PyObject *_f_frame_data[1];
};
Expand Down Expand Up @@ -233,7 +234,10 @@ int
_PyFrame_Traverse(_PyInterpreterFrame *frame, visitproc visit, void *arg);

PyObject *
_PyFrame_GetLocals(_PyInterpreterFrame *frame, int include_hidden);
_PyFrame_GetLocals(_PyInterpreterFrame *frame);

PyObject *
_PyFrame_GetHiddenLocals(_PyInterpreterFrame *frame);

int
_PyFrame_FastToLocalsWithError(_PyInterpreterFrame *frame);
Expand Down
5 changes: 3 additions & 2 deletions Lib/test/test_frame.py
View file
Open in desktop
Expand Up @@ -200,9 +200,10 @@ def inner():

def test_locals(self):
f, outer, inner = self.make_frames()
outer_locals = outer.f_locals
# TODO: Support pop for f_locals
outer_locals = dict(outer.f_locals)
self.assertIsInstance(outer_locals.pop('inner'), types.FunctionType)
self.assertEqual(outer_locals, {'x': 5, 'y': 6})
self.assertEqual(dict(outer_locals), {'x': 5, 'y': 6})
inner_locals = inner.f_locals
self.assertEqual(inner_locals, {'x': 5, 'z': 7})

Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_listcomps.py
View file
Open in desktop
Expand Up @@ -610,10 +610,10 @@ def test_exception_in_post_comp_call(self):

def test_frame_locals(self):
code = """
val = [sys._getframe().f_locals for a in [0]][0]["a"]
val = "a" in [sys._getframe().f_locals for a in [0]][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we've changed the intent of the test here. It was previously checking that a was added to the f_locals snapshot during the listcomp, and now it's checking that it is not visible through the proxy afterwards.

To cover both aspects, I think we need two test cases (first one shows that "a" is accessible in the f_locals while the listcomp is running, second shows that it is gone afterwards):

    def test_frame_locals_in_listcomp(self):
        # Iteration variable is accessible via f_locals proxy while the listcomp is running
        code = """
            val = [sys._getframe().f_locals["a"] for a in [0]][0]
        """
        import sys
        self._check_in_scopes(code, {"val": 0}, ns={"sys": sys})

    def test_frame_locals_after_listcomp(self):
        # Iteration variable is no longer accessible via f_locals proxy after listcomp finishes
        code = """
            val = "a" in [sys._getframe().f_locals for a in [0]][0]
        """
        import sys
        self._check_in_scopes(code, {"val": False}, ns={"sys": sys})

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a very interesting point because it introduced a new issue - should we include the hidden fast local in f_locals when it's module/class scope?

val = [sys._getframe().f_locals["a"] for a in [0]][0]

What should the sys._getframe().f_locals inside the list comprehension return? It's it a dict or a proxy? Do we consider that "within" the function scope and return a proxy? Will that include the module-level variables as well? Do we return a dict? Should we include variable a? If so, writing to the key 'a' won't have any effects because it's a fast variable, how do we deal with that inconsistency?

@markshannon

Copy link
Member

Choose a reason for hiding this comment

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

Hidden fast-locals were designed to mimic the prior behavior when comprehensions were previously implemented as one-shot functions. So as much as is feasible, the behavior of f_locals with hidden fast-locals should mirror how it would behave if the comprehension were a function.

Copy link
Member

@carljm carljm Apr 30, 2024 •
edited

Choose a reason for hiding this comment

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

The second proposed test in @ncoghlan 's comment is not correct. Nor is the test as currently modified in the PR.

If you access f_locals on a frame you got while inside the comprehension, it should include the hidden fast-local (i.e. the comprehension iteration variable), and that variable should still be present (in the f_locals of the frame you got while inside the comprehension) even after the comprehension is done. This behavior is the same as getting a frame (or its f_locals) inside a function and returning it from the function. This is the current behavior in main, and this behavior is important to keep in PEP 667.

Of course if you access the frame outside the comprehension, the comprehension's hidden fast locals should not be present in its f_locals.

Currently in main, module globals are present in the f_locals of a frame accessed inside a module-level comprehension. This differs from a function, but this difference was called out in PEP 709 and determined to be OK, and I think it's still OK.

writing to the key 'a' won't have any effects because it's a fast variable

I suspect this is also not a problem in practice, though if we can reasonably return a proxy that can support it, it would be even better.

Copy link
Member

Choose a reason for hiding this comment

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

If you access f_locals on a frame you got while inside the comprehension, it should include the hidden fast-local (i.e. the comprehension iteration variable), and that variable should still be present (in the f_locals of the frame you got while inside the comprehension) even after the comprehension is done.

I don't think this is correct. f_locals is a proxy, so as the frame changes, so will the proxy. Inside the comprehension, the comprehension variables will be visible. After the comprehension has executed, the comprehension variables no longer exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the key here is "as if it's a one-shot function". That's infeasible with the current compiler because it's not a function anymore. For a function, we have a dedicated frame object that keeps the local variables, which can last longer than the intepreter frame as long as there are references on it. In that way, we can always access the variables on that frame even when the actual interpreter frame is gone.

However, with inline comprehension, it fakes the "function call" and clear the hidden variable - there's no mechanism currently to prevent the variable from being cleared. If we want this, we'll probably need to change the compiler and the interpreter which will definitely postpone this PEP to 3.14 (and would probably make the compiler code more complicated).

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 class and module level comprehensions, can we return a proxy if f_locals is accessed inside the comprehension and a dict if it's accessed outside which does not have the hidden variable? I feel like that's more consistent.

Copy link
Member

@carljm carljm Apr 30, 2024 •
edited

Choose a reason for hiding this comment

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

I don't think this is correct. f_locals is a proxy, so as the frame changes, so will the proxy. Inside the comprehension, the comprehension variables will be visible. After the comprehension has executed, the comprehension variables no longer exist.

It depends whether we are aiming for "correctness" for the actual situation (that the comprehension doesn't have its own frame), or for the backwards-compatible fiction (which PEP 709 only partially maintained, but did jump through hoops to maintain in terms of visibility of class-scoped variables) that comprehensions still behave as if they have their own frame. But as @gaogaotiantian points out, it may be very hard or impossible to maintain that fiction in this case, while still having f_locals be a proxy.

TBH in the long term I think it would be better to move away from that fiction anyway, but it will have backwards-compatibility implications. Today in main, sys._getframe.f_locals() inside a module-level comprehension returns a dictionary that includes the comprehension iteration variable, and that variable is still visible in that dictionary after the comprehension is done.

I don't know why someone relying on that couldn't just use locals() instead, though, and locals() should be fine for backwards-compatibility; it remains a dict. So I don't know how significant that backwards-incompatibility will be in practice.

For class and module level comprehensions, can we return a proxy if f_locals is accessed inside the comprehension and a dict if it's accessed outside which does not have the hidden variable? I feel like that's more consistent.

Certainly the dict returned when f_locals is accessed outside the comprehension should not have the hidden local in it.

I think returning a proxy inside the module- or class-scoped comprehension is more internally consistent for PEP 667; returning a dict (that includes the comprehension iteration variable) would be a bit more backward-compatible.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow the details, but I agree that people who access f_locals are better off being shown implementation details that may vary by Python version than a costly fiction.

"""
import sys
self._check_in_scopes(code, {"val": 0}, ns={"sys": sys})
self._check_in_scopes(code, {"val": False}, ns={"sys": sys})

def _recursive_replace(self, maybe_code):
if not isinstance(maybe_code, types.CodeType):
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_sys.py
View file
Open in desktop
Expand Up @@ -1550,7 +1550,7 @@ class C(object): pass
def func():
return sys._getframe()
x = func()
check(x, size('3Pi3c7P2ic??2P'))
check(x, size('3Pi3cP7P2ic??2P'))
# function
def func(): pass
check(func, size('15Pi'))
Expand Down