Skip to content

Full Grammar specification lists invalid syntax #118235

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

Closed
encukou opened this issue Apr 24, 2024 · 2 comments
Closed

Full Grammar specification lists invalid syntax #118235

encukou opened this issue Apr 24, 2024 · 2 comments
Labels
docs Documentation in the Doc dir

Comments

@encukou
Copy link
Member

encukou commented Apr 24, 2024

The Full Grammar specification lists these production rules:

 type_param:  
     | NAME [type_param_bound]
     | '*' NAME ':' expression
     | '*' NAME 
     | '**' NAME ':' expression 
     | '**' NAME 
 
 for_if_clause:
     | 'async' 'for' star_targets 'in' ~ disjunction ('if' disjunction )* 
     | 'for' star_targets 'in' ~ disjunction ('if' disjunction )* 
     | 'async'? 'for' (bitwise_or (',' bitwise_or)* [',']) !'in' 

 starred_expression:
     | '*' expression 
     | '*' 

However, some of the alternatives are actually invalid syntax: in the grammar file we can find that they're handled by raising a syntax error. For example:

for_if_clause[comprehension_ty]:
    [...]
    | 'async'? 'for' (bitwise_or (',' bitwise_or)* [',']) !'in' {
        RAISE_SYNTAX_ERROR("'in' expected after for-loop variables") }

These should be hidden in the same way as other invalid rules.

Linked PRs

@encukou encukou added the docs Documentation in the Doc dir label Apr 24, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 26, 2024
encukou added a commit that referenced this issue Apr 26, 2024
…H-118237) (GH-118309)

gh-118235: Skip RAISE_SYNTAX_ERROR rules in the grammar spec (GH-118237)
(cherry picked from commit ef940de)

Co-authored-by: Petr Viktorin <[email protected]>
encukou added a commit to encukou/cpython that referenced this issue May 1, 2024
…validate

Add a validator that makes sure RAISE_SYNTAX_ERROR only appears in
invalid rules. This looks at the action text, as a string.

Move all such actions to invalid rules. This works for two of them,
but not the type_param rules, where the SyntaxError that ends up
being raised is the one from &&'(' in the function definition.
@encukou
Copy link
Member Author

encukou commented May 1, 2024

@pablogsal said on #118237 (comment):

Another alternative it's to move all these rules to be inside an invalid_ rule and in general prevent by review or automation rules that have RAISE_SYNTAX_ERROR in the actions.

I tried to do this, but I got stuck moving the type_param syntax errors to invalid rules.
The SyntaxError from the forced token &&'(' from function_def_raw is the one that gets reported.

Whenever you have time, could you look at main...encukou:cpython:invalid-grammar and give me a hint at what I'm doing wrong?

That PR fails with:

FAIL: test_typevartuple_01 (test.test_type_params.TypeParamsTypeVarTupleTest.test_typevartuple_01)
----------------------------------------------------------------------
  File "<test string>", line 1
    def func1[*A: str](): pass
             ^
SyntaxError: expected '('

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  ...
AssertionError: "cannot use bound with TypeVarTuple" does not match "expected '(' (<test string>, line 1)"

(No rush at all.)

@encukou
Copy link
Member Author

encukou commented May 7, 2024

I just found the discussion about that issue in #118091; I'll read it when I get back to grammar-in-docs.

encukou added a commit to encukou/cpython that referenced this issue May 29, 2024
… spec (pythonGH-118237)"

This reverts commit ef940de.

The highlighter change is no longer needed, since all RAISE_SYNTAX_ERROR
actions are in invalid rules.
encukou added a commit that referenced this issue May 30, 2024
…sure they stay there (GH-119731)

The Full Grammar specification in the docs omits rule actions, so grammar rules that raise a syntax error looked like valid syntax.
This was solved in ef940de by hiding those rules in the custom syntax highlighter.

This moves all syntax-error alternatives to invalid rules, adds a validator that ensures that actions containing RAISE_SYNTAX_ERROR are in invalid rules, and reverts the syntax highlighter hack.
@encukou encukou closed this as completed May 30, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
… make sure they stay there (pythonGH-119731)

The Full Grammar specification in the docs omits rule actions, so grammar rules that raise a syntax error looked like valid syntax.
This was solved in ef940de by hiding those rules in the custom syntax highlighter.

This moves all syntax-error alternatives to invalid rules, adds a validator that ensures that actions containing RAISE_SYNTAX_ERROR are in invalid rules, and reverts the syntax highlighter hack.
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
… make sure they stay there (pythonGH-119731)

The Full Grammar specification in the docs omits rule actions, so grammar rules that raise a syntax error looked like valid syntax.
This was solved in ef940de by hiding those rules in the custom syntax highlighter.

This moves all syntax-error alternatives to invalid rules, adds a validator that ensures that actions containing RAISE_SYNTAX_ERROR are in invalid rules, and reverts the syntax highlighter hack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

No branches or pull requests

1 participant