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

Allow for's target expression to be starred #2879

Merged
merged 2 commits into from Mar 5, 2022

Conversation

isidentical
Copy link
Collaborator

@isidentical isidentical commented Feb 11, 2022

Fixes #2878

@github-actions
Copy link

@github-actions github-actions bot commented Feb 11, 2022

diff-shades reports zero changes comparing this PR (71b5708) to main (07a2e6f).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Thanks, some minor feedback.

Also, I noticed that [x for x in *a, *b] isn't allowed. Just curious, do you know why that is?

@@ -62,6 +62,7 @@
]

PY310_CASES: List[str] = [
"starred_for_target",
Copy link
Collaborator

@JelleZijlstra JelleZijlstra Feb 11, 2022

Choose a reason for hiding this comment

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

I don't think this needs to be 310-specific

Copy link
Collaborator Author

@isidentical isidentical Feb 11, 2022

Choose a reason for hiding this comment

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

Since the PEP 617 was opt-out in 3.9, the given examples are rejected by the official AST when the user starts the python interpreter with -Xoldparser. It is why I've added tests to 3.10 group.

CHANGES.md Outdated Show resolved Hide resolved
@isidentical
Copy link
Collaborator Author

@isidentical isidentical commented Feb 11, 2022

@JelleZijlstra this seems to be something that was introduced during the PEG parser merge, but there is no official reference in anywhere beside the grammar. I have no knowledge on the comprehensions, but perhaps @pablogsal can shed more light in that.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@isidentical isidentical requested a review from JelleZijlstra Feb 11, 2022
@pablogsal
Copy link
Contributor

@pablogsal pablogsal commented Feb 11, 2022

@JelleZijlstra this seems to be something that was introduced during the PEG parser merge, but there is no official reference in anywhere beside the grammar. I have no knowledge on the comprehensions, but perhaps @pablogsal can shed more light in that.

Hummm, maybe I am misunderstanding this, but this syntax has never been allowed:

Python 3.8.12 (default, Nov  1 2021, 12:21:59)
>>> [y for y in *x, *y]
  File "<stdin>", line 1
    [y for y in *x, *y]
                ^
SyntaxError: invalid syntax
Python 3.10.2 (main, Jan 17 2022, 12:00:35)
>>> [y for y in *x, *y]
  File "<stdin>", line 1
    [y for y in *x, *y]
                ^
SyntaxError: invalid syntax

@pablogsal
Copy link
Contributor

@pablogsal pablogsal commented Feb 11, 2022

This is In the same spirit as [x,y for x,y in range(10)] being invalid. We have an error for this in 3.10:

>>> [x,y for x,y in x]
  File "<stdin>", line 1
    [x,y for x,y in x]
     ^^^
SyntaxError: did you forget parentheses around the comprehension target?

@JelleZijlstra
Copy link
Collaborator

@JelleZijlstra JelleZijlstra commented Feb 11, 2022

My thinking process was just "hey, I didn't know this was allowed, I wonder where else it could be allowed". But it does make sense that we're a bit stricter in comprehensions than in standalone for loops. Thanks for the extra information!

@isidentical
Copy link
Collaborator Author

@isidentical isidentical commented Feb 11, 2022

@pablogsal no, I have meant the regular for syntax:

$ python -c "for a in *[], *[]: pass"
$ python -Xoldparser -c "for a in *[], *[]: pass"
  File "<string>", line 1
    for a in *[], *[]: pass
             ^
SyntaxError: invalid syntax

The git bisect tells me that this behavior initially got introduced in the PEP 617 merge. Since with 3.9/3.10 this is now supported, what we wonder is why these two are inconsistent.

@pablogsal
Copy link
Contributor

@pablogsal pablogsal commented Feb 11, 2022

The git bisect tells me that this behavior initially got introduced in the PEP 617 merge.

Ah, sorry, I misunderstood your comment. Yes, that was introduced on PEP 617 merge but I am not actually sure that was on purpose as I cannot find any previous discussion.

Maybe we need to revert that :(

@isidentical
Copy link
Collaborator Author

@isidentical isidentical commented Feb 11, 2022

Maybe we need to revert that :(

It might be a bit too late, no? (it was part of both 3.9/3.10 stable). But yea, let's create a bpo ticket to discuss further.

@pablogsal
Copy link
Contributor

@pablogsal pablogsal commented Feb 11, 2022

It might be a bit too late, no? (it was part of both 3.9/3.10 stable). But yea, let's create a bpo ticket to discuss further.

It may be, but if we treat it like a bug, you are technically not allowed to rely on the bug :)

@pablogsal
Copy link
Contributor

@pablogsal pablogsal commented Feb 11, 2022

Opened https://bugs.python.org/issue46725

@JelleZijlstra
Copy link
Collaborator

@JelleZijlstra JelleZijlstra commented Feb 11, 2022

Let's hold off on merging this until the BPO is resolved. If CPython's decision is to remove the syntax, I think it makes sense for Black to disallow it too.

Copy link
Collaborator

@ichard26 ichard26 left a comment

Has been documented in python/cpython#31481. Thanks for fixing this Batuhan!

@JelleZijlstra JelleZijlstra merged commit 6f4976a into psf:main Mar 5, 2022
38 checks passed
Copy link

@AIvinAt169922K AIvinAt169922K left a comment

Good luck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants