GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-104374: Remove access to class scopes for inlined comprehensions by JelleZijlstra · Pull Request #104528 · python/cpython

Conversation

@JelleZijlstra

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: List comprehensions now have access to the enclosing class scope #104374

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JelleZijlstra
pythongh-104374: Remove access to class scopes for inlined comprehens…
3fad7b0
…ions
@JelleZijlstra
JelleZijlstra requested a review from carljm May 16, 2023 04:58
@bedevere-bot bedevere-bot mentioned this pull request May 16, 2023
List comprehensions now have access to the enclosing class scope #104374
Closed
carljm
carljm reviewed May 16, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_listcomps.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_listcomps.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra and others added 5 commits May 16, 2023 06:34
@JelleZijlstra @carljm
Update Lib/test/test_listcomps.py
fa11ca9
Co-authored-by: Carl Meyer <[email protected]>
@JelleZijlstra
Remove unnecessary change
1d776fd
@JelleZijlstra
Better tests, copied from Carl's PR
932a2da
@JelleZijlstra
A few more tests
d5653ae
@JelleZijlstra
Merge remote-tracking branch 'upstream/main' into classcomp2
ba3bacf
@carljm carljm added the skip news label May 16, 2023
@carljm carljm mentioned this pull request May 16, 2023
PEP 709, for the third and (hopefully) final time python/steering-council#192
Closed
@JelleZijlstra
JelleZijlstra marked this pull request as ready for review May 17, 2023 01:07
@JelleZijlstra
JelleZijlstra requested review from iritkatriel and markshannon as code owners May 17, 2023 01:07
@bedevere-bot bedevere-bot added the awaiting core review label May 17, 2023
@JelleZijlstra

JelleZijlstra commented May 17, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented May 17, 2023

Copy link
Copy Markdown
Member Author

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: {}
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented May 17, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@carljm
fix definition of 'in a class block'
948bc1a
@carljm

carljm commented May 17, 2023

Copy link
Copy Markdown
Member

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.

JelleZijlstra and mokko reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@carljm
a couple more tests
045478f
JelleZijlstra
JelleZijlstra commented May 17, 2023
View reviewed changes
Comment thread Lib/test/test_listcomps.py

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]]

@JelleZijlstra JelleZijlstra May 17, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@carljm carljm May 18, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JelleZijlstra JelleZijlstra May 18, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@carljm carljm May 18, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@JelleZijlstra JelleZijlstra May 18, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just added two more tests and turned on automerge.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@JelleZijlstra

JelleZijlstra commented May 17, 2023

Copy link
Copy Markdown
Member Author

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]
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented May 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Reduced:

def a():
    class a:
        [(lambda : b) for b in [a]]
        print(b)
carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented May 17, 2023

Copy link
Copy Markdown
Member Author

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.

carljm, itamaro, and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@carljm

carljm commented May 17, 2023

Copy link
Copy Markdown
Member

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

JelleZijlstra, itamaro, erlend-aasland, and mokko reacted with thumbs up emoji
All reactions
  • 👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra JelleZijlstra mentioned this pull request May 18, 2023
PEP 709: Crash with lambda + nested scope #104602
Closed
carljm
carljm approved these changes May 18, 2023
View reviewed changes
Comment thread Lib/test/test_listcomps.py

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]]

@carljm carljm May 18, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 18, 2023
JelleZijlstra added 2 commits May 17, 2023 21:48
@JelleZijlstra
Merge remote-tracking branch 'upstream/main' into classcomp2
fc63272
@JelleZijlstra
A few more tests
afe0277
@JelleZijlstra
JelleZijlstra enabled auto-merge (squash) May 18, 2023 04:55
@JelleZijlstra
JelleZijlstra merged commit 662aede into python:main May 18, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label May 18, 2023
@JelleZijlstra
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 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
JelleZijlstra restored the classcomp2 branch September 10, 2024 23:37
@JelleZijlstra
JelleZijlstra deleted the classcomp2 branch December 3, 2024 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@JelleZijlstra @carljm @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.