gh-87447: Fix walrus comprehension rebind checking #100581
Conversation
|
Sorry, I did not click on the second reviewer the first time! :( |
There was a problem hiding this comment.
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
| @@ -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))) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Misc/NEWS.d/next/Core and Builtins/2022-12-28-15-02-53.gh-issue-87447.7-aekA.rst
Show resolved
Hide resolved
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 = [ |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
Misc/NEWS.d/next/Core and Builtins/2022-12-28-15-02-53.gh-issue-87447.7-aekA.rst
Outdated
Show resolved
Hide resolved
…e-87447.7-aekA.rst
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
There was a problem hiding this comment.
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!
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!