bpo-39235: Fix end location for genexp in call args 8000 #17925
Conversation
| @@ -3146,7 +3146,7 @@ ast_for_call(struct compiling *c, const node *n, expr_ty func, | |||
| } | |||
| else if (TYPE(CHILD(ch, 1)) == comp_for) { | |||
| /* the lone generator expression */ | |||
| e = copy_location(ast_for_genexp(c, ch), maybegenbeg); | |||
| e = copy_location(ast_for_genexp(c, ch), maybegenbeg, closepar); | |||
There was a problem hiding this comment.
Note that if maybegenbeg and closepar are passed here and used as start and end line/col info we will need to work around this in pegen, since the parser there does not consider the parentheses part of the generator expression and uses its first and last character for the node metadata.
There was a problem hiding this comment.
Clearly this is the intention of the original code -- it started out pointing the start location to the open parenthesis. This is apparently so that the location for a genexp node always includes the surrounding parentheses -- which are considered part of the syntax (just like for a list comprehension the [...] are considered part of the syntax).
There is some discussion of this in bpo-31241 (though not for function calls).
We can probably handle this in pegen by writing a separate bit of function call syntax, e.g.
| ...
| primary lone_genexp
| primary '(' [arguments] ')'
| ...
lone_genexp:
| '(' expression for_if_clauses ')'
(We'll need a similar thing for call_tail in the targets section.)
|
Could you add some tests to index 55b91cfa23..e72df9b64d 100644
--- a/Lib/test/test_ast.py
+++ b/Lib/test/test_ast.py
@@ -235,7 +235,8 @@ eval_tests = [
"()",
# Combination
"a.b.c.d(a.b[1:2])",
-
+ # Function call with generator expr
+ "f(x for x in y)",
]
# TODO: expr_context, slice, boolop, operator, unaryop, cmpop, comprehension
@@ -879,6 +880,13 @@ Module(
self.assertEqual(elif_stmt.lineno, 3)
self.assertEqual(elif_stmt.col_offset, 0)
+ def test_generatorexp_inside_call_col_offsets(self):
+ expression = 'func(x for x in y)'
+ node = ast.parse(expression)
+ genexp = node.body[0].value.args[0]
+ genexp_str = expression[genexp.col_offset:genexp.end_col_offset]
+ self.assertEqual(genexp_str, '(x for x in y)')
+
def test_starred_expr_end_position_within_call(self):
node = ast.parse('f(*[0, 1])')
starred_expr = node.body[0].value.args[0]
@@ -1952,5 +1960,6 @@ eval_results = [
('Expression', ('Tuple', (1, 0), [('Constant', (1, 1), 1, None), ('Constant', (1, 3), 2, None), ('Constant', (1, 5), 3, None)], ('Load',))),
('Expression', ('Tuple', (1, 0), [], ('Load',))),
('Expression', ('Call', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Name', (1, 0), 'a', ('Load',)), 'b', ('Load',)), 'c', ('Load',)), 'd', ('Load',)), [('Subscript', (1, 8), ('Attribute', (1, 8), ('Name', (1, 8), 'a', ('Load',)), 'b', ('Load',)), ('Slice', ('Constant', (1, 12), 1, None), ('Constant', (1, 14), 2, None), None), ('Load',))], [])),
+('Expression', ('Call', (1, 0), ('Name', (1, 0), 'f', ('Load',)), [('GeneratorExp', (1, 1), ('Name', (1, 2), 'x', ('Load',)), [('comprehension', ('Name', (1, 8), 'x', ('Store',)), ('Name', (1, 13), 'y', ('Load',)), [], 0)])], [])),
]
main() |
|
I propose to merge this PR without tests and backport it to 3.8. Then merge tests in #17926 without backporting them, because I am not sure that all other end positions are calculated correctly and do not want to break alternate implementations which do it correctly. |
|
@pablogsal Given GH-17926 should I still add the test you suggested? |
|
Thanks @gvanrossum for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
The fix changes copy_location() to require an extra node from which to extract the end location, and fixing all 5 call sites. https://bugs.python.org/issue39235 (cherry picked from commit a796d8e) Co-authored-by: Guido van Rossum <guido@python.org>
|
GH-17927 is a backport of this pull request to the 3.8 branch. |
The fix changes copy_location() to require an extra node from which to extract the end location, and fixing all 5 call sites. https://bugs.python.org/issue39235 (cherry picked from commit a796d8e) Co-authored-by: Guido van Rossum <guido@python.org>
I am fine as with GH-17926 we will be able to detect if a future change breaks/changes what this PR is fixing. |
bpo-39235: Fix end location for genexp in call args (pythonGH-17925)
The fix changes copy_location() to require an extra node from which to extract the end location, and fixing all 5 call sites. https://bugs.python.org/issue39235
The fix changes copy_location() to require an extra node from which to extract the end location, and fixing all 5 call sites.
https://bugs.python.org/issue39235
Automerge-Triggered-By: @gvanrossum