Skip to content

bpo-36256: Fix bug in parsermodule when parsing if statements #12477

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

Merged
merged 7 commits into from
Mar 21, 2019

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Mar 21, 2019

This one was tricky! :)

In the parser module, when validating nodes before starting the parsing with to create a ST in "parser_newstobject" there is a problem that appears when two arcs in the same DFA state has transitions with labels with the same type. For example, the DFA for if_stmt has a state with
two labels with the same type: "elif" and "else" (type NAME). The algorithm tries one by one the arcs until the label that starts the arc transition has a label with the same type of the current child label we are
triying to accept. In this case, the arc for "elif" comes before the arc for "else"and passes this test (because the current child label is "else" and has the same type as "elif"). This lead to expecting a namedexpr_test (305) instead of a colon (11). The solution is to compare also the string representation (in case there is one) of the labels to see if the transition that we have is the correct one.

https://bugs.python.org/issue36256

In the parser module, when validating nodes before starting the parsing
with to create a ST in "parser_newstobject" there is a problem that
appears when two arcs in the same DFA state has transitions with labels
with the same type. For example, the DFA for if_stmt has a state with
two labels with the same type: "elif" and "else" (type NAME). The
algorithm tries one by one the arcs until the label that starts the arc
transition has a label with the same type of the current child label we are
triying to accept. In this case, the arc for "elif" comes before the arc
for "else"and passes this test (because the current child label is "else"
and has the same type as "elif"). This lead to expecting a namedexpr_test
(305) instead of a colon (11). The solution is to compare also the string
representation (in case there is one) of the labels to see if the transition
that we have is the correct one.
@tyomitch
Copy link
Contributor

Note that the generation of error message at https://github.com/python/cpython/pull/12477/files#diff-73f51bbc1366ee12a4f041d90bbb902dR700 wouldn't handle mismatching NAMEs correctly.

Please see https://github.com/python/cpython/pull/10995/files#diff-73f51bbc1366ee12a4f041d90bbb902dR698 for the remaining part of the fix

@tyomitch
Copy link
Contributor

An illustration of what I mean:

Python 3.8.0a2+ (remotes/pablogsal/36256:ca3e88dca0, Mar 21 2019, 08:15:48) 
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import parser
>>> parser.sequence2st((257,(269,(295,(297,(1,'fi'),(305,(306,(310,(311,(312,(313,(316,(317,(318,(319,(320,(321,(322,(323,(324,(325,(1,'True'))))))))))))))))),(11,':'),(304,(4,''),(5,''),(269,(270,(271,(277,(1,'pass'))),(4,''))),(6,''))))),(4,''),(0,'')))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
parser.ParserError: Illegal terminal: expected NAME.

I.e., it complains on a NAME saying that it expected NAME instead: not very informative!

@tyomitch
Copy link
Contributor

Very good, thanks!

brettcannon and others added 3 commits March 21, 2019 21:24
Co-Authored-By: pablogsal <Pablogsal@gmail.com>
Co-Authored-By: pablogsal <Pablogsal@gmail.com>
Co-Authored-By: pablogsal <Pablogsal@gmail.com>
Co-Authored-By: pablogsal <Pablogsal@gmail.com>
@pablogsal pablogsal merged commit 9a0000d into python:master Mar 21, 2019
@pablogsal pablogsal deleted the 36256 branch March 21, 2019 23:33
@pablogsal
Copy link
Member Author

Thank you, everyone, for your review and code suggestions!

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-12488 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 21, 2019
…GH-12477)

bpo-36256: Fix bug in parsermodule when parsing if statements

In the parser module, when validating nodes before starting the parsing with to create a ST in "parser_newstobject" there is a problem that appears when two arcs in the same DFA state has transitions with labels with the same type. For example, the DFA for if_stmt has a state with
two labels with the same type: "elif" and "else" (type NAME). The algorithm tries one by one the arcs until the label that starts the arc transition has a label with the same type of the current child label we are trying to accept. In this case, the arc for "elif" comes before the arc for "else"and passes this test (because the current child label is "else" and has the same type as "elif"). This lead to expecting a namedexpr_test (305) instead of a colon (11). The solution is to compare also the string representation (in case there is one) of the labels to see if the transition that we have is the correct one.
(cherry picked from commit 9a0000d)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
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.

7 participants