gh-104374: Remove access to class scopes for inlined comprehensions by JelleZijlstra · Pull Request #104528 · python/cpython · GitHub

Issue: List comprehensions now have access to the enclosing class scope #104374

…ions

This doesn't look bad at all! Thanks for pursuing it. I guess we can present both this and #104519 as options.

Co-authored-by: Carl Meyer <carl@oddbird.net>

Found the following crash with a variation of Carl's fuzzer:

>>> class a: [a for a in [b] for b[[b for _ in [] if a if b]] in [2]]
... 
SystemError: compiler_lookup_arg(name='a') with reftype=1 failed in <module>; freevars of code a: ('a',)

This doesn't reproduce on main, so it's new from this PR.

Found a different issue trying to simplify it:

>>> class C: [a for a in [] if [a for _ in []]]
... 
SystemError: _PyST_GetScope(name='a') failed: unknown scope in unit <module> (5033699440); symbols: {'C': 2050}; locals: {}; globals: {}

The problem occurs if it's a cell var in the outer comprehension and a free var in the inner one. In that case we should just do LOAD_FAST, but right now we're probably emitting _DEREF opcodes that explode because those cells don't exist in the class namespace.

I pushed a fix for the above issues; it's just a matter of correcting how we decide if a comprehension is "in a class block." Looking only at c->u->u_ste->ste_type gives the wrong answer for nested comprehensions; they are inside their outer comprehension, so never in a class block. We need to use c->u->u_ste->ste_type == ClassBlock && !c->u->u_in_inlined_comp.

Would be nice if we could test here that the name resolution in the inner listcomp here is right. I'll try to come up with some more tests around nested listcomps.

Are you still planning to add more tests here, or improve this one?

Yes, will push some more tests today or tomorrow. If you'd prefer to merge this now we can also do that; I can just write more tests in another PR.

Yes, I think if we get known fixes merged sooner it's easier to do more effective fuzz testing; if we find more issues we can fix them in separate PRs, and we can add more tests that we think of in separate PRs.

Just added two more tests and turned on automerge.

Now this segfaults:

class a:
    def a():
        class a:
            [(lambda : (a := a[(a := 2)])[b]) for b in (lambda b, a: 7)[a]]
            [][2] = b
            (1)[lambda a: a] = 4
            (2)[2] = b = a
        (4)[lambda b, a: b] = a = lambda : 1

Haven't tried to minimize it, but it's a compiler crash, something related to the cell/freevars:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x000000010008b33c python.exe`Py_TYPE(ob=0x0000000000000000) at object.h:204:16 [opt]
    frame #1: 0x000000010008ca8c python.exe`Py_IS_TYPE(ob=<unavailable>, type=0x000000010042d148) at object.h:235:12 [opt]
    frame #2: 0x000000010008db38 python.exe`_PyCode_ConstantKey(op=0x0000000000000000) at codeobject.c:2163:11 [opt]
    frame #3: 0x000000010008dd00 python.exe`_PyCode_ConstantKey(op=0x0000000103557a70) at codeobject.c:2225:24 [opt]
    frame #4: 0x0000000100173334 python.exe`_PyCompile_ConstCacheMergeOne(const_cache=0x0000000103023890, obj=0x000000016fdfedd8) at compile.c:7380:21 [opt]
    frame #5: 0x000000010014d88c python.exe`makecode(umd=0x0000000103407bb0, a=0x000000016fdfee50, const_cache=0x0000000103023890, constslist=0x000000010354b750, maxdepth=5, nlocalsplus=2, code_flags=0, filename=0x0000000103077e20) at assemble.c:569:9 [opt]
    frame #6: 0x000000010014d5b8 python.exe`_PyAssemble_MakeCodeObject(umd=0x0000000103407bb0, const_cache=0x0000000103023890, consts=0x000000010354b750, maxdepth=5, instrs=<unavailable>, nlocalsplus=2, code_flags=0, filename=0x0000000103077e20) at assemble.c:598:14 [opt]

Reduced:

def a():
    class a:
        [(lambda : b) for b in [a]]
        print(b)

This also reproduces on main (without this PR) and with two functions, so it's not just related to class scopes:

def a():
    def a():
        [(lambda : b) for b in [a]]
        print(b)

Crashes on current main with the same backtrace as above.

Thanks, I'll open a separate issue and PR for this.

Are you still planning to add more tests here, or improve this one?

pythongh-104374 : Remove access to class scopes for inlined comprehens…

3fad7b0

…ions

JelleZijlstra requested a review from carljm May 16, 2023 04:58

bedevere-bot mentioned this pull request May 16, 2023

List comprehensions now have access to the enclosing class scope #104374

Closed

carljm reviewed May 16, 2023

View reviewed changes

Comment thread Lib/test/test_listcomps.py Outdated

Comment thread Lib/test/test_listcomps.py Outdated

Comment thread Python/compile.c Outdated

JelleZijlstra and others added 5 commits May 16, 2023 06:34

Update Lib/test/test_listcomps.py

fa11ca9

Co-authored-by: Carl Meyer <carl@oddbird.net>

Remove unnecessary change

1d776fd

Better tests, copied from Carl's PR

932a2da

A few more tests

d5653ae

Merge remote-tracking branch 'upstream/main' into classcomp2

ba3bacf

carljm added the skip news label May 16, 2023

carljm mentioned this pull request May 16, 2023

PEP 709, for the third and (hopefully) final time python/steering-council#192

Closed

JelleZijlstra marked this pull request as ready for review May 17, 2023 01:07

JelleZijlstra requested review from iritkatriel and markshannon as code owners May 17, 2023 01:07

bedevere-bot added the awaiting core review label May 17, 2023

fix definition of 'in a class block'

948bc1a

a couple more tests

045478f

JelleZijlstra commented May 17, 2023

View reviewed changes

JelleZijlstra mentioned this pull request May 18, 2023

PEP 709: Crash with lambda + nested scope #104602

Closed

carljm approved these changes May 18, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 18, 2023

JelleZijlstra added 2 commits May 17, 2023 21:48

Merge remote-tracking branch 'upstream/main' into classcomp2

fc63272

A few more tests

afe0277

JelleZijlstra enabled auto-merge (squash) May 18, 2023 04:55

JelleZijlstra merged commit 662aede into python:main May 18, 2023

bedevere-bot removed the awaiting merge label May 18, 2023

JelleZijlstra deleted the classcomp2 branch May 18, 2023 05:22

This was referenced May 18, 2023

gh-104602: ensure all cellvars are known up front #104603

Merged

PEP 709 behavior change around mangling #104604

Closed

carljm mentioned this pull request May 18, 2023

gh-104374: don't inline comprehensions in non-function scopes #104519

Closed

This was referenced Apr 22, 2024

gh-109118: Allow lambdas in annotation scopes in classes #118019

Merged

gh-109118: Make comprehensions work within annotation scopes, but without inlining #118160

Merged

JelleZijlstra restored the classcomp2 branch September 10, 2024 23:37

JelleZijlstra deleted the classcomp2 branch December 3, 2024 02:02

+                  def test_nested_free_var_in_iter(self):
+                      code = """
+                          items = [_C for _C in [1] for [0, 1][[x for x in [1] if _C][0]] in [2]]

gh-104374: Remove access to class scopes for inlined comprehensions by JelleZijlstra · Pull Request #104528 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-104374: Remove access to class scopes for inlined comprehensions - #104528

gh-104374: Remove access to class scopes for inlined comprehensions#104528JelleZijlstra merged 10 commits intopython:mainpython/cpython:mainfrom JelleZijlstra:classcomp2JelleZijlstra/cpython:classcomp2Copy head branch name to clipboard

Conversation

JelleZijlstra commented May 16, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelleZijlstra commented May 17, 2023

Uh oh!

JelleZijlstra commented May 17, 2023

Uh oh!

JelleZijlstra commented May 17, 2023

Uh oh!

carljm commented May 17, 2023

Uh oh!

JelleZijlstra May 17, 2023

Choose a reason for hiding this comment

Uh oh!

carljm May 18, 2023

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra May 18, 2023

Choose a reason for hiding this comment

Uh oh!

carljm May 18, 2023

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra May 18, 2023

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented May 17, 2023

Uh oh!

JelleZijlstra commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelleZijlstra commented May 17, 2023

Uh oh!

carljm commented May 17, 2023

Uh oh!

carljm May 18, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-104374: Remove access to class scopes for inlined comprehensions#104528
JelleZijlstra merged 10 commits into
python:mainfrom
JelleZijlstra:classcomp2

JelleZijlstra commented May 16, 2023 •
edited by bedevere-bot
Loading

JelleZijlstra commented May 17, 2023 •
edited
Loading