bpo-39235: Fix end location for genexp in call args #17925

gvanrossum · 2020-01-09T17:55:54Z

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


        bpo-39235: fix end location for genexp in call args


        Add blurb

lysnikolaou · 2020-01-09T18:07:46Z

Python/ast.c

@@ -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);


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.

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.)

pablogsal · 2020-01-09T18:15:22Z

Could you add some tests to Lib/test_ast.py? Something like:

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()

serhiy-storchaka

LGTM.

As for tests, I can change existing tests to catch this error in separate PR.

serhiy-storchaka · 2020-01-09T19:07:09Z

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.

gvanrossum · 2020-01-09T19:15:50Z

@pablogsal Given GH-17926 should I still add the test you suggested?

lysnikolaou · 2020-01-09T19:16:12Z

Yes, backporting this is the only way since we need it for pegen, but there is a strong need to document these changes somewhere, because they could break code, like @asottile mentions in #17582.

I'm not sure if just the news entry is enough.

miss-islington · 2020-01-09T19:18:51Z

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


        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 (cherry picked from commit a796d8e) Co-authored-by: Guido van Rossum <guido@python.org>

bedevere-bot · 2020-01-09T19:19:03Z

GH-17927 is a backport of this pull request to the 3.8 branch.


        bpo-39235: Fix end location for genexp in call args (GH-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 (cherry picked from commit a796d8e) Co-authored-by: Guido van Rossum <guido@python.org>

pablogsal · 2020-01-09T20:03:10Z

@pablogsal Given GH-17926 should I still add the test you suggested?

I am fine as with GH-17926 we will be able to detect if a future change breaks/changes what this PR is fixing.


        Merge pull request #158 from python/master

bpo-39235: Fix end location for genexp in call args (pythonGH-17925)

bpo-39235: fix end location for genexp in call args

Loading status checks…

47298c8

the-knights-who-say-ni added the CLA signed label Jan 9, 2020

bedevere-bot added the awaiting core review label Jan 9, 2020

Add blurb

Loading status checks…

7c7861b

gvanrossum requested a review from pablogsal Jan 9, 2020

gvanrossum added the needs backport to 3.8 label Jan 9, 2020

lysnikolaou reviewed Jan 9, 2020

View changes

serhiy-storchaka approved these changes Jan 9, 2020

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 9, 2020

serhiy-storchaka added the type-bugfix label Jan 9, 2020

gvanrossum mentioned this pull request Jan 9, 2020

bpo-39235: Check end_lineno and end_col_offset of AST nodes. #17926

Merged

gvanrossum added the 🤖 automerge label Jan 9, 2020

miss-islington deleted the bpo-39235-genexp-endloc branch Jan 9, 2020

bedevere-bot removed the awaiting merge label Jan 9, 2020

bedevere-bot removed the needs backport to 3.8 label Jan 9, 2020

python / cpython

bpo-39235: Fix end location for genexp in call args #17925

bpo-39235: Fix end location for genexp in call args #17925

gvanrossum commented Jan 9, 2020 •

edited by miss-islington

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pablogsal commented Jan 9, 2020 •

edited

serhiy-storchaka left a comment

This comment has been minimized.

serhiy-storchaka commented Jan 9, 2020

This comment has been minimized.

gvanrossum commented Jan 9, 2020

This comment has been minimized.

lysnikolaou commented Jan 9, 2020 •

edited

This comment has been minimized.

miss-islington commented Jan 9, 2020

This comment has been minimized.

bedevere-bot commented Jan 9, 2020

This comment has been minimized.

pablogsal commented Jan 9, 2020

python / cpython

Sponsor python/cpython

Join GitHub today

bpo-39235: Fix end location for genexp in call args #17925

bpo-39235: Fix end location for genexp in call args #17925

Conversation

gvanrossum commented Jan 9, 2020 • edited by miss-islington

This comment has been minimized.

lysnikolaou Jan 9, 2020

This comment has been minimized.

gvanrossum Jan 9, 2020

This comment has been minimized.

pablogsal commented Jan 9, 2020 • edited

serhiy-storchaka left a comment

This comment has been minimized.

serhiy-storchaka commented Jan 9, 2020

This comment has been minimized.

gvanrossum commented Jan 9, 2020

This comment has been minimized.

lysnikolaou commented Jan 9, 2020 • edited

This comment has been minimized.

miss-islington commented Jan 9, 2020

This comment has been minimized.

bedevere-bot commented Jan 9, 2020

This comment has been minimized.

pablogsal commented Jan 9, 2020

gvanrossum commented Jan 9, 2020 •

edited by miss-islington

pablogsal commented Jan 9, 2020 •

edited

lysnikolaou commented Jan 9, 2020 •

edited