Skip to content

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

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

Merged
merged 4 commits into from
Feb 19, 2025
Merged

gh-128396: Fix a crash when inline comprehension has the same local variable as the outside scope #130235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49ffaea
Fix a crash when inline comprehension has the same local variable
gaogaotiantian Feb 17, 2025
5b6afbb
Add regression test
gaogaotiantian Feb 17, 2025
4758bd1
📜🤖 Added by blurb_it.
blurb-it[bot] Feb 17, 2025
13142cc
Fix lint
gaogaotiantian Feb 17, 2025
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
6 changes: 6 additions & 0 deletions Lib/test/test_frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ def f():
self.assertEqual(x, 2)
self.assertEqual(y, 3)

def test_closure_with_inline_comprehension(self):
lambda: k
k = 1
lst = [locals() for k in [0]]
self.assertEqual(lst[0]['k'], 0)

def test_as_dict(self):
x = 1
y = 2
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-02-17-18-59-33.gh-issue-128396.iVtoYY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a crash that occurs when calling :func:`locals` inside an inline comprehension that uses the same local variable as the outer frame scope where the variable is a free or cell var.
11 changes: 9 additions & 2 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,15 @@ framelocalsproxy_getval(_PyInterpreterFrame *frame, PyCodeObject *co, int i)
if (kind == CO_FAST_FREE || kind & CO_FAST_CELL) {
// The cell was set when the frame was created from
// the function's closure.
assert(PyCell_Check(value));
cell = value;
// GH-128396: With PEP 709, it's possible to have a fast variable in
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible this will do the wrong thing if the value comes from within the list comprehension, but happens to hold a cell object?

Something like this:

    def test_closure_with_inline_comprehension(self):
        lambda: k
        k = 1
        lst = [locals() for k in [types.CellType()]]
        self.assertEqual(lst[0]['k'], 0)

Copy link
Member Author

@gaogaotiantian gaogaotiantian Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's possible. Even though we do not recommend using CellType directly, but that's always a possibility. The good news is that this PR does not make it worse - it does not work properly without this PR either. Also the existing C API PyFrame_GetVar probably suffers from this as well.

To solve this, we need a reliable way to figure out if we are in an inlined comprehension - I think that's a loophole PEP 709 left us.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fundamental reason of this issue is the inconsistency between the variable kind that's static in the code object and the actual kind that is dynamic with inline comprehension. PEP 667 is one way to expose that but anything that relies on the kind of a variable in an inline comprehension will expose the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carljm , when PEP 709 was implemented, did the idea of creating a new iteration variable on the code object with the same name came up in some way? So

def f():
    x = 1
    [x for x in [0]]

actually compiles to a code object with co_varnames = ['x', 'x']

Copy link
Member

Choose a reason for hiding this comment

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

I think I ruled this out on the assumption that there is too much code (possibly even in CPython) relying on not having two different variable indices with the same varname in a code object, and I didn't think it was necessary. But it's possible I ruled it out too hastily; it would certainly simplify comprehension inlining a lot to do this.

// an inlined comprehension that has the same name as the cell variable
// in the frame, where the `kind` obtained from frame can not guarantee
// that the variable is a cell.
// If the variable is not a cell, we are okay with it and we can simply
// return the value.
if (PyCell_Check(value)) {
cell = value;
}
}

if (cell != NULL) {
Expand Down
Loading