8000 bpo-39235: Fix end location for genexp in call args by gvanrossum · Pull Request #17925 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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 2 commits into from
Jan 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47298c8
bpo-39235: fix end location for genexp in call args
gvanrossum Jan 9, 2020
7c7861b
Add blurb
gvanrossum Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
10000
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2020-01-09-10-01-18.bpo-39235.RYwjoc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix AST end location for lone generator expression in function call, e.g.
f(i for i in a).
16 changes: 8 additions & 8 deletions Python/ast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,13 +1028,13 @@ forbidden_name(struct compiling *c, identifier name, const node *n,
}

static expr_ty
copy_location(expr_ty e, const node *n)
copy_location(expr_ty e, const node *n, const node *end)
{
if (e) {
e->lineno = LINENO(n);
e->col_offset = n->n_col_offset;
e->end_lineno = n->n_end_lineno;
e->end_col_offset = n->n_end_col_offset;
e->end_lineno = end->n_end_lineno;
e->end_col_offset = end->n_end_col_offset;
}
return e;
}
Expand Down Expand Up @@ -2464,10 +2464,10 @@ ast_for_atom(struct compiling *c, const node *n)
}

if (TYPE(CHILD(ch, 1)) == comp_for) {
return copy_location(ast_for_genexp(c, ch), n);
return copy_location(ast_for_genexp(c, ch), n, n);
}
else {
return copy_location(ast_for_testlist(c, ch), n);
return copy_location(ast_for_testlist(c, ch), n, n);
}
case LSQB: /* list (or list comprehension) */
ch = CHILD(n, 1);
Expand All @@ -2486,7 +2486,7 @@ ast_for_atom(struct compiling *c, const node *n)
n->n_end_lineno, n->n_end_col_offset, c->c_arena);
}
else {
return copy_location(ast_for_listcomp(c, ch), n);
return copy_location(ast_for_listcomp(c, ch), n, n);
}
case LBRACE: {
/* dictorsetmaker: ( ((test ':' test | '**' test)
Expand Down Expand Up @@ -2527,7 +2527,7 @@ ast_for_atom(struct compiling *c, const node *n)
/* It's a dictionary display. */
res = ast_for_dictdisplay(c, ch);
}
return copy_location(res, n);
return copy_location(res, n, n);
}
}
default:
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!e)
return NULL;
asdl_seq_SET(args, nargs++, e);
Expand Down
0