Skip to content

gh-111488: Changed error message in case of no 'in' keyword after 'for' in cmp#113656

Merged
pablogsal merged 3 commits into
python:mainfrom
grigoriev-semyon:main
Jan 6, 2024
Merged

gh-111488: Changed error message in case of no 'in' keyword after 'for' in cmp#113656
pablogsal merged 3 commits into
python:mainfrom
grigoriev-semyon:main

Conversation

@grigoriev-semyon

@grigoriev-semyon grigoriev-semyon commented Jan 2, 2024

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Jan 2, 2024

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@pablogsal

Copy link
Copy Markdown
Member

This approach doesn't work. Please read the discussion on the issue, particularly this comment:

#111488 (comment)

For example, this PR breaks this case:

>>> [x for a, b, (c + 1, d()) in y]
  File "<stdin>", line 1
    [x for a, b, (c + 1, d()) in y]
                  ^^^^^
SyntaxError: cannot assign to expressio

as it now shows:

>>> [x for a, b, (c + 1, d()) in y]
  File "<stdin>", line 1
    [x for a, b, (c + 1, d()) in y]
                              ^^
SyntaxError: 'in' expected after for-loop variables

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please check the discussion on the issue

@bedevere-app

bedevere-app Bot commented Jan 2, 2024

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@grigoriev-semyon

Copy link
Copy Markdown
Contributor Author

fixed

➜  ~ python3.13
Python 3.13.0a2+ (heads/main-dirty:7b74e4141b, Jan  4 2024, 01:39:25) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> [x for a, b, (c + 1, d()) in y]
  File "<stdin>", line 1
    [x for a, b, (c + 1, d()) in y]
                  ^^^^^
SyntaxError: cannot assign to expression
>>> [x for a, b, (c + 1, d()) if y]
  File "<stdin>", line 1
    [x for a, b, (c + 1, d()) if y]
                              ^^
SyntaxError: 'in' expected after for-loop variables
>>> 

@grigoriev-semyon

Copy link
Copy Markdown
Contributor Author

Hello, I have made the requested changes; please review again :)

@pablogsal

Copy link
Copy Markdown
Member

This is better but note lowering the grammar to not include expressions will still change existing errors. For example:

>>> [x for a>b if z else 42 in x]
  File "<stdin>", line 1
    [x for a>b if z else 42 in x]
SyntaxError: cannot assign to conditional expression

changes to

>>> [x for a>b if z else 42 in x]
  File "<stdin>", line 1
    [x for a>b if z else 42 in x]
            ^
SyntaxError: 'in' expected after for-loop variables

I think is not too bad because the error is still correct but this may affect other constructs, although I think the error will more or less be correct (maybe it will show AFTER some other real error, though). @lysnikolaou what do you think?

@pablogsal

Copy link
Copy Markdown
Member

An example of what I mean:

>>> [x for (yield z) with y]
  File "<stdin>", line 1
    [x for (yield z) with y]
            ^^^^^^^
SyntaxError: cannot assign to yield expression

will change to

>>> [x for (yield z) with y]
  File "<stdin>", line 1
    [x for (yield z) with y]
                     ^^^^
SyntaxError: 'in' expected after for-loop variables

here are two errors and now we show the one that's further from the start of the comprehension. So after fixing that there is another error reported BEFORE it.

I am not sure if we should care about that, but this is a difference.

@pablogsal

Copy link
Copy Markdown
Member

(@grigoriev-semyon don't change anything yet, let's check what @lysnikolaou thinks)

@lysnikolaou

Copy link
Copy Markdown
Member

I'm okay with both, although I'm incline to say let's fix this one and accept that there might be cases that become suboptimal because of it.

Either way we have to accept that there are error messages that are very hard to get right, and this might be one of them. I'm also kind of inclined to try and have a more sophisticated approach to this and other error messages, where you have a priority system where you can try more than one invalid_ rules and the error with the highest priority wins. Don't know how much effort that would take though.

@grigoriev-semyon

Copy link
Copy Markdown
Contributor Author

@pablogsal I tried to improve the result, but it seems difficult.

If you agree, I suggest merging this PR and creating a task to further improve the error in this place.
I am also ready to work on a future task.

@pablogsal

Copy link
Copy Markdown
Member

Yeah I think we will merge this and check how it goes. Thanks for the patience @grigoriev-semyon and the good work!

@pablogsal
pablogsal enabled auto-merge (squash) January 6, 2024 10:27
@pablogsal
pablogsal merged commit bb4c167 into python:main Jan 6, 2024
@sobolevn

sobolevn commented Jan 6, 2024

Copy link
Copy Markdown
Member

Congrats @grigoriev-semyon! Thanks everyone :)

kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

Confusing SyntaxError message for x for x if range(1)

4 participants