NotificationsYou must be signed in to change notification settings
Fork33.7k
Star70.4k

gh-87447: Fix walrus comprehension rebind checking#100581

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

emilyemorehouse merged 8 commits intopython:mainfromsobolevn:issue-87447

Jan 8, 2023

Merged

gh-87447: Fix walrus comprehension rebind checking#100581

emilyemorehouse merged 8 commits intopython:mainfromsobolevn:issue-87447

Jan 8, 2023

Conversation

Copy link

Member

sobolevn commentedDec 28, 2022•
edited by AlexWaygood
Loading

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

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 fromemilyemorehouse andpablogsal

December 28, 2022 12:06

Copy link

MemberAuthor

sobolevn commentedDec 28, 2022

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

hauntsaninja reviewed

Dec 29, 2022

View reviewed changes

Copy link

Contributor

hauntsaninja left a 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)

Python/symtable.c Outdated

		longtarget_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))) {

Copy link

Contributor

Choose a reason for hiding this comment

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

Hmm, should this also checkDEF_NONLOCAL?

Actually, looking closer, I thinkDEF_GLOBAL might be impossible, because we already check for it when settingDEF_COMP_ITER. Or at least, tests pass without checkingDEF_GLOBAL here.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Yes, you are right.

sobolevn commented

Dec 29, 2022

View reviewed changes

Python/symtable.c OutdatedShow resolvedHide resolved

Update Python/symtable.c

1481678

hauntsaninja reviewed

Jan 1, 2023

View reviewed changes

Copy link

Contributor

hauntsaninja left a comment•
edited
Loading

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 ofhttps://docs.python.org/3/whatsnew/3.11.html#other-language-changes

pablogsal reviewed

Jan 3, 2023

View reviewed changes

Python/symtable.cShow resolvedHide resolved

pablogsal reviewed

Jan 3, 2023

View reviewed changes

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

pablogsal reviewed

Jan 3, 2023

View reviewed changes

Lib/test/test_named_expressions.py Outdated

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

		deftest_named_expression_valid_rebinding_list_comprehension_iteration_variable(self):
		cases= [

Copy link

Member

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

Address review

8e167b0

sobolevn mentioned this pull request

Jan 4, 2023

Dict comprehensions are not tested at all intest_named_expressions.py#100746

Closed

pablogsal approved these changes

Jan 4, 2023

View reviewed changes

Copy link

Member

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

bedevere-bot added awaiting merge and removed awaiting review labels

Jan 4, 2023

pablogsal reviewed

Jan 4, 2023

View reviewed changes

Doc/whatsnew/3.12.rst OutdatedShow resolvedHide resolved

Update Doc/whatsnew/3.12.rst

84a413a

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

hauntsaninja reviewed

Jan 5, 2023

View reviewed changes

Doc/whatsnew/3.12.rst OutdatedShow resolvedHide resolved

hauntsaninja reviewed

Jan 5, 2023

View reviewed changes

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

hauntsaninjaand others added4 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 <12621235+hauntsaninja@users.noreply.github.com>

Merge branch 'main' into issue-87447

73e370e

emilyemorehouse approved these changes

Jan 8, 2023

View reviewed changes

Copy link

Member

emilyemorehouse left a 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!