bpo-40334: Correctly generate C parser when assigned var is None #20296
Conversation
When there are 2 negative lookaheads in a same rule, let's say
`!"(" blabla "," !")"`, there will the 2 `FunctionCall`'s where
assigned value is None. Currently when the `add_var` is called
the first one will be ignored (since it will be returned as same
from dedupe because there aren't any `None` named variables in
locals, but when the second lookahead's var is sent to dedupe it
will be returned as `None_1` and this wont be ignored by the
declaration generator in the `visit_Alt`. This patch adds an explicit
check to `add_var` to distinguish whether if there is a variable or not.
|
Could you add a test? |
|
I couldnt find the suitable place to add test, would you give me a hint about it? |
|
or alternatively https://github.com/python/cpython/blob/master/Lib/test/test_peg_generator/test_c_parser.py if you cannot reproduce it in the Python version. |
|
I dont know if this test is suitable, but I couldn't find a better way to test this. |
|
Also, why mypy has not catched this? |
| grammar = parse_string(grammar_source, GrammarParser) | ||
| parser_source = generate_c_parser_source(grammar) | ||
|
|
||
| self.assertNotIn("int None_1", parser_source) |
There was a problem hiding this comment.
Hummmm. I don't like this test because is checking the implementation. What goes wrong exactly now when there are two negative lookaheads?
There was a problem hiding this comment.
What goes wrong exactly now when there are two negative lookaheads?
gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Parser/pegen/parse.o Parser/pegen/parse.c
Parser/pegen/parse.c: In function ‘invalid_import_from_targets_rule’:
Parser/pegen/parse.c:12249:13: warning: unused variable ‘None_1’ [-Wunused-variable]
12249 | int None_1;
| ^~~~~~
There was a problem hiding this comment.
Hummmm. I don't like this test because is checking the implementation.
I agree with you but this is an implementation problem so I dont know exactly how to write a better test for this
There was a problem hiding this comment.
I agree with you but this is an implementation problem so I dont know exactly how to write a better test for this
Yeah, let's remove the test and let's focus on why mypy has not found this.
There was a problem hiding this comment.
because of CCallMakerVisitor's return value is Any, mypy kind a ignores it.
There was a problem hiding this comment.
🤦
Ok, will prepare a PR to fix mypy, I found a couple of other stuff.
This reverts commit 5440280.
…honGH-20296) When there are 2 negative lookaheads in the same rule, let's say `!"(" blabla "," !")"`, there will the 2 `FunctionCall`'s where assigned value is None. Currently when the `add_var` is called the first one will be ignored but when the second lookahead's var is sent to dedupe it will be returned as `None_1` and this won't be ignored by the declaration generator in the `visit_Alt`. This patch adds an explicit check to `add_var` to distinguish whether if there is a variable or not.
4 participants
When there are 2 negative lookaheads in a same rule, let's say
!"(" blabla "," !")", there will the 2FunctionCall's whereassigned value is None. Currently when the
add_varis calledthe first one will be ignored (since it will be returned as same
from dedupe because there aren't any
Nonenamed variables inlocals, but when the second lookahead's var is sent to dedupe it
will be returned as
None_1and this wont be ignored by thedeclaration generator in the
visit_Alt. This patch adds an explicitcheck to
add_varto distinguish whether if there is a variable or not.https://bugs.python.org/issue40334