Skip to content

gh-133357: Fix SyntaxError carret location on invalid starred exprs #133455

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented May 5, 2025

I went with the simplest possible change:
Снимок экрана 2025-05-05 в 19 40 47

Now carret always points to the star itself.

Why?

  1. Because RAISE_SYNTAX_ERROR_KNOWN_RANGE requires the second argument, it is rather hard to match. Because simple a='*' b=expression does not work. Because, for example, (1:2) is not a valid expression it is a slice in ().
  2. Changing starred_expressions also seems rather hard:
starred_expression[expr_ty]:
    | invalid_starred_expression_unpacking
    | '*' a=expression { _PyAST_Starred(a, Load, EXTRA) }
    | invalid_starred_expression

So, I decided to keep it simple.

@pablogsal
Copy link
Member

Yeah I don't want to over complicate this because not only is very easy to make it slower for no real gain in most cases but the more complex the rules are the more edge cases they will have and with peg that can also affect other rules

@lysnikolaou
Copy link
Member

Agreed on the point about complexity. Is this an improvement over the status quo though?

@sobolevn
Copy link
Member Author

sobolevn commented May 5, 2025

Is this an improvement over the status quo though?

Yes, I think it is. Compare my current branch:

Снимок экрана 2025-05-05 в 19 40 47

With main:

Снимок экрана 2025-05-05 в 21 12 42

Currently, we point at different chars in seemingly random places: :, ]
When the problem always starts with *

@pablogsal
Copy link
Member

@lysnikolaou are you ok if we go with this?

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

Can we add a test that actually verifys the location to prevent a future regression?

@sobolevn
Copy link
Member Author

sobolevn commented May 5, 2025

Adding such test can be done, but is rather hard to do, since there's no infrastructure for such tests as far as I know.

@pablogsal
Copy link
Member

We normally either add them in test exceptions or test syntax

@StanFromIreland
Copy link
Contributor

I proposed a test in a PR against your branch.

@lysnikolaou
Copy link
Member

lysnikolaou commented May 5, 2025

I'm okay to go with this if everyone else agrees, but I'm not convinced this is clearer. The caret should point to the first character that does not adhere to our syntax and this breaks that assumption.

The current caret position is not random. It actually points to the correct place. In the above examples @sobolevn showed, if A is an ndarray, it's the ] or the : that make this invalid syntax since the following would be a valid index:

A[*(1,)]

@sobolevn
Copy link
Member Author

sobolevn commented May 6, 2025

@lysnikolaou yes, this is a good point. Maybe we can come up with a better solution? Do you have an idea about what we can do?

@lysnikolaou
Copy link
Member

@lysnikolaou yes, this is a good point. Maybe we can come up with a better solution? Do you have an idea about what we can do?

Ideally we would be able to place tildes below the whole expression and a caret below the position that breaks the syntax. I don't think we have that in the parser though and the complexity would be too much.

Maybe just keep this as is?

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

Successfully merging this pull request may close these issues.

4 participants