GeezerVison

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

GeezerVison is showing github.com — gh-87447: Fix walrus comprehension rebind checking by sobolevn · Pull Request #100581 · python/cpython

Conversation

sobolevn commented Dec 28, 2022 •

edited by AlexWaygood

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

I went with the most straightforward approach. I am not very qualified in this module, so I would love to hear about any possible drawbacks this solution might have!

Issue: Walrus comprehension rebind checking behavior #87447

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-87447: Fix walrus comprehension rebind checking

4a9c194

bedevere-bot mentioned this pull request

Dec 28, 2022

Walrus comprehension rebind checking behavior #87447

Closed

bedevere-bot added the awaiting review label

Dec 28, 2022

sobolevn requested review from emilyemorehouse and pablogsal

December 28, 2022 12:06

sobolevn commented Dec 28, 2022

Copy link

Copy Markdown

Member Author

Sorry, I did not click on the second reviewer the first time! :(

All reactions

Sorry, something went wrong.

Uh oh!

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

hauntsaninja reviewed

Dec 29, 2022

View reviewed changes

hauntsaninja left a comment

Copy link

Copy Markdown

Contributor

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.

Thank you! I'm not very familiar with symtable, but left one comment.

I think it might also need a little more consensus that this is something that should be fixed; I tried to restart discussion in #87447 (comment)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/symtable.c Outdated

		long target_in_scope = _PyST_GetSymbol(ste, target_name);
		if (target_in_scope & DEF_COMP_ITER) {
		if ((target_in_scope & DEF_COMP_ITER) &&
		(target_in_scope & (DEF_LOCAL \| DEF_GLOBAL))) {

hauntsaninja Dec 29, 2022

Copy link

Copy Markdown

Contributor

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.

Hmm, should this also check DEF_NONLOCAL?

Actually, looking closer, I think DEF_GLOBAL might be impossible, because we already check for it when setting DEF_COMP_ITER. Or at least, tests pass without checking DEF_GLOBAL here.

Sorry, something went wrong.

Uh oh!

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

All reactions

sobolevn Dec 29, 2022

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, you are right.

Sorry, something went wrong.

Uh oh!

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

All reactions

sobolevn commented

Dec 29, 2022

View reviewed changes

Comment thread

Python/symtable.c Resolved

Uh oh!

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


          Update Python/symtable.c

hauntsaninja reviewed

Jan 1, 2023

View reviewed changes

hauntsaninja left a comment •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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.

Thanks! With the fix, the code seems reasonable to me, but I'm not comfortable enough with this module for that to mean much.

I think this merits a What's New entry, along the lines of https://docs.python.org/3/whatsnew/3.11.html#other-language-changes

Sorry, something went wrong.

Uh oh!

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

All reactions

pablogsal reviewed

Jan 3, 2023

View reviewed changes

Comment thread

Python/symtable.c Resolved

Uh oh!

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

pablogsal reviewed

Jan 3, 2023

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2022-12-28-15-02-53.gh-issue-87447.7-aekA.rst Resolved

Uh oh!

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

pablogsal reviewed

Jan 3, 2023

View reviewed changes

Comment thread

Lib/test/test_named_expressions.py Outdated

		exec(f"lambda: {code}", {}) # Function scope

		def test_named_expression_valid_rebinding_list_comprehension_iteration_variable(self):
		cases = [

pablogsal Jan 3, 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.

Can you add a comment at the start of the test explaining that we are cheching that anything that is not directly a comprehension iteration variable (a or b) can be assigned to?

Also, we probably should ensure ALL comprehensions are covered by the test (add another for loop that changes the brackets).

Sorry, something went wrong.

Uh oh!

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

All reactions

sobolevn Jan 4, 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.

Done! Except for dict comprehensions. Looks like they are not tested in test_named_expressions at all. I've opened a new task for it here: #100746

Sorry, something went wrong.

Uh oh!

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

All reactions


          Address review

8e167b0

sobolevn mentioned this pull request

Jan 4, 2023

Dict comprehensions are not tested at all in test_named_expressions.py #100746

Closed

pablogsal approved these changes

Jan 4, 2023

View reviewed changes

pablogsal 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.

Fantastic work @sobolevn!

Let's leave some time for @emilyemorehouse in case she wants to take a look and we can land it then. If this is not landed in a week or so, please ping me and I will land it.

Sorry, something went wrong.

Uh oh!

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

sobolevn reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot added awaiting merge and removed awaiting review labels

Jan 4, 2023

pablogsal reviewed

Jan 4, 2023

View reviewed changes

Comment thread

Doc/whatsnew/3.12.rst Resolved

Uh oh!

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


          Update Doc/whatsnew/3.12.rst

84a413a

Co-authored-by: Pablo Galindo Salgado <[email protected]>

hauntsaninja reviewed

Jan 5, 2023

View reviewed changes

Comment thread

Doc/whatsnew/3.12.rst Resolved

Uh oh!

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

hauntsaninja reviewed

Jan 5, 2023

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2022-12-28-15-02-53.gh-issue-87447.7-aekA.rst Resolved

Uh oh!

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

hauntsaninja and others added 4 commits

January 5, 2023 13:46


          Update Misc/NEWS.d/next/Core and Builtins/2022-12-28-15-02-53.gh-issu…

da61b6b

…e-87447.7-aekA.rst


          Merge branch 'main' into issue-87447

f152ae3


          Update Doc/whatsnew/3.12.rst

e941439

Co-authored-by: Shantanu <[email protected]>


          Merge branch 'main' into issue-87447

73e370e

emilyemorehouse approved these changes

Jan 8, 2023

View reviewed changes

emilyemorehouse 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.

The implementation of this looks great, thanks @sobolevn!

I went back and forth a bit on whether the underlying issue should be fixed in this way and ultimately agree – the current behavior is certainly confusing and while I'm not highly motivated by the use cases, it makes sense given that we allow the use of local variables in assignment expressions in other scenarios. I'm also in favor of leaving this as a change for 3.12 and not backporting.

Thanks, all!

Sorry, something went wrong.

Uh oh!

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

sobolevn, gvanrossum, tushar-deepsource, and Be3y4uu-K0T reacted with thumbs up emoji

All reactions

👍 4 reactions

emilyemorehouse merged commit bc0a686 into python:main

Jan 8, 2023

bedevere-bot removed the awaiting merge label

Jan 8, 2023

Ismaelkadir mentioned this pull request

Apr 16, 2023

GH-103484: Docs: add linkcheck allowed redirects entries for most cases #103569

Merged

hauntsaninja mentioned this pull request

May 21, 2023

Python 3.12 tracking issue python/mypy#15277

Closed

7 of 9 tasks

AlexWaygood mentioned this pull request

Jun 24, 2024

Remove E999 as a rule, disallow any disablement methods for syntax error astral-sh/ruff#11901

Merged

9 of 13 tasks

AlexWaygood mentioned this pull request

Feb 26, 2025

Emit diagnostics for new syntax as per the target Python version astral-sh/ruff#6591

Closed

16 tasks done

ntBre mentioned this pull request

Mar 5, 2025

☂️ Syntax errors raised by the compiler astral-sh/ruff#11934

Closed

18 tasks done

Reviewers

pablogsal

pablogsal approved these changes

hauntsaninja

hauntsaninja left review comments

emilyemorehouse

emilyemorehouse approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

5 participants

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.

GeezerVison

Conversation

sobolevn commented Dec 28, 2022 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Dec 28, 2022

Uh oh!

hauntsaninja left a comment

Choose a reason for hiding this comment

Uh oh!

hauntsaninja Dec 29, 2022

Choose a reason for hiding this comment

Uh oh!

sobolevn Dec 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hauntsaninja left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pablogsal Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

sobolevn Jan 4, 2023

Choose a reason for hiding this comment

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emilyemorehouse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sobolevn commented Dec 28, 2022 •

edited by AlexWaygood

Loading

Uh oh!

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

hauntsaninja left a comment •

edited

Loading

Uh oh!

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