gh-87447: Fix walrus comprehension rebind checking by sobolevn · Pull Request #100581 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Sorry, I did not click on the second reviewer the first time! :(
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)
| @@ -1488,7 +1488,8 @@ symtable_extend_namedexpr_scope(struct symtable *st, expr_ty e) |
|---|
| */ |
| if (ste->ste_comprehension) { |
| 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))) { |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| exec(f"lambda: {code}", {}) # Function scope |
|---|
| def test_named_expression_valid_rebinding_list_comprehension_iteration_variable(self): |
| cases = [ |
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).
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
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.
Co-authored-by: Pablo Galindo Salgado Pablogsal@gmail.com
Co-authored-by: Shantanu 12621235+hauntsaninja@users.noreply.github.com
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!
18 tasks