Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Walrus comprehension rebind checking behavior #87447

Closed
gousaiyang mannequin opened this issue Feb 21, 2021 · 9 comments
Closed

Walrus comprehension rebind checking behavior #87447

gousaiyang mannequin opened this issue Feb 21, 2021 · 9 comments
Labels
type-feature A feature request or enhancement

Comments

@gousaiyang
Copy link
Mannequin

gousaiyang mannequin commented Feb 21, 2021

BPO 43281
Nosy @gvanrossum, @emilyemorehouse, @gousaiyang

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-02-21.01:20:04.826>
labels = ['type-bug', '3.8', '3.9', '3.10']
title = 'Walrus comprehension rebind checking behavior'
updated_at = <Date 2021-02-27.05:46:20.917>
user = 'https://github.com/gousaiyang'

bugs.python.org fields:

activity = <Date 2021-02-27.05:46:20.917>
actor = 'gvanrossum'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2021-02-21.01:20:04.826>
creator = 'gousaiyang'
dependencies = []
files = []
hgrepos = []
issue_num = 43281
keywords = []
message_count = 2.0
messages = ['387433', '387768']
nosy_count = 3.0
nosy_names = ['gvanrossum', 'emilyemorehouse', 'gousaiyang']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue43281'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

@gousaiyang
Copy link
Mannequin Author

gousaiyang mannequin commented Feb 21, 2021

# test

PEP-572 disallows walrus use cases such as [(i := 1) for i in [1]], because for i implies that i is local to the comprehension but (i := 1) implies that i is "exported", which is a contradiction. However, I noticed the following behavior both in 3.8 and 3.9:

>>> [(a := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'a'
>>> [(b := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'b'
>>> [(c := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'c'
>>> [(d := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'd'
>>> [(e := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'e'
>>> [(f := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'f'
>>> [(g := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'g'
>>> [(h := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'h'
>>> [(i := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in <listcomp>
TypeError: cannot unpack non-iterable int object

Among all of these assignment expressions, only the assignment to i (attribute name) is allowed. However, in my understanding, only a and b should be disallowed for assignment. This comprehension example only creates two new variables, a and b, which are local to the comprehension. Assignment expressions assigning to c, d, e, f, g and h should be allowed. c and h are already in the outer scope. d, e, f and g are just names inside the expression in the subscript part. They are not "comprehension iteration variables" and not local to the comprehension.

The same behavior is also observed when detecting inner loop variable rebinding outer loop assignment expression targets:

>>> [i for i in range(5) if (j := 0) for k[j + 1] in range(5)]
  File "<stdin>", line 1
SyntaxError: comprehension inner loop cannot rebind assignment expression target 'j'

@gousaiyang gousaiyang mannequin added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error labels Feb 21, 2021
@gvanrossum
Copy link
Member

I'd like Emily to have a look at that.

@sobolevn
Copy link
Member

I had some free time to learn more about symtable.c and fix this :)
So, I hope my PR is in the right direction!

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Dec 29, 2022

I think it probably needs a little more discussion whether this should be fixed.

I found it useful to look at a slightly simpler example than the test case, like:

index = 0
a = [0, 0, 0]
[(index := i) for i, a[index] in enumerate([9,8,7])]

Arguments in favour of changing:

  • Like OP points out, the rationale in PEP 572 for outlawing these only applies to a and b; the scoping is clear for the other variables (so at least we'd have to invent new reasons to keep this outlawed)
  • I think it's a little surprising that one gets SyntaxError: assignment expression cannot rebind comprehension iteration variable 'index' from the above
  • It's seemingly a small change to fix in CPython
  • I could squint my eyes to believing there's some use case for it

Arguments against changing:

  • Any comprehension that does this is going to be at least a little cursed. No concrete use case has been proposed
  • It'd maybe be a little weird that you could do [(a := [i]) for i, a[0] in enumerate([9,8,7])] but not be able to do [(a := i) for i, a in enumerate([9,8,7])]
  • There are stricter-than-strictly-necessary restrictions on assignment expressions in comprehensions that we're okay with, that I find more surprising than this (disallowing them entirely in the iterable)
  • Python 3.8 has been around for a couple years and this hasn't really come up. Changing this could necessitate changes in other Python implementations or linters

Overall, I think I lean against, but I could be convinced otherwise!

@sobolevn
Copy link
Member

sobolevn commented Dec 29, 2022

My main argument to fix this is that error message does not show the reality:

>>> [(h := 1) for a, (*b, c[d+e::f(g)], h.i) in [1]]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot rebind comprehension iteration variable 'h'

But, h is not a comprehension iteration variable, h.i is.

So, in my opinion it is clearly a bug. But, there are two ways of fixing it:

  1. Allow to rebind h if h.i is used, as in my PR
  2. Try changing the error message to something like assignment expression cannot rebind variable 'h' that is used in comprehension iteration expression (or whatever)

@gvanrossum
Copy link
Member

I feel for the OP and I agree with several arguments in favor of this fix, given that it's a simple fix. Shantanu's arguments against feel pretty weak (probably intentionally so: "at least a little", "maybe a little").

Maybe the other thing (disallowing named expressions in the iterable altogether) can be fixed in another PR?

@sobolevn
Copy link
Member

sobolevn commented Dec 29, 2022

Maybe the other thing (disallowing named expressions in the iterable altogether) can be fixed in another PR?

Are you talking about forbidding := in [(h := 1) for a part? If I understand you correctly, this would be a breaking change, because even PEP 572 references this use-case: https://peps.python.org/pep-0572/#scope-of-the-target

# Compute partial sums in a list comprehension
total = 0
partial_sums = [total := total + v for v in values]
print("Total:", total)

However, I probably just didn't get your idea :)

@gvanrossum
Copy link
Member

gvanrossum commented Dec 29, 2022

I was talking about this from Shantanu:

  • There are stricter-than-strictly-necessary restrictions on assignment expressions in comprehensions that we're okay with, that I find more surprising than this (disallowing them entirely in the iterable)

This is about things like:

>>> [x for x in (a := range(3))]
  File "<stdin>", line 1
SyntaxError: assignment expression cannot be used in a comprehension iterable expression
>>> 

emilyemorehouse pushed a commit that referenced this issue Jan 8, 2023
Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@emilyemorehouse emilyemorehouse added type-feature A feature request or enhancement and removed 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error labels Jan 8, 2023
@emilyemorehouse
Copy link
Member

Fixed in #100581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants