Skip to content
This repository has been archived by the owner on Jul 5, 2023. It is now read-only.

[2.7, 3.5] Support the per-argument function comment syntax #5

Merged
merged 7 commits into from
Jun 22, 2016
Merged

[2.7, 3.5] Support the per-argument function comment syntax #5

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8325e39
[2.7] Support the per-argument function comment syntax
ddfisher Jun 10, 2016
3aa01a1
[3.5] Support the per-argument function comment syntax
ddfisher Jun 15, 2016
4c8a2ec
[conversions] Convert Python 2 per-argument type comments
ddfisher Jun 21, 2016
314d17d
Respond to review feedback
ddfisher Jun 22, 2016
97915b0
Fix final nits
ddfisher Jun 22, 2016
78206fc
tabs
ddfisher Jun 22, 2016
87e46fb
update readme
ddfisher Jun 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ast27/Grammar/Grammar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ decorators: decorator+
decorated: decorators (classdef | funcdef)
funcdef: 'def' NAME parameters ':' [TYPE_COMMENT] suite
parameters: '(' [varargslist] ')'
varargslist: ((fpdef ['=' test] ',')*
('*' NAME [',' '**' NAME] | '**' NAME) |
fpdef ['=' test] (',' fpdef ['=' test])* [','])
varargslist: ((fpdef ['=' test] ',' [TYPE_COMMENT])*
('*' NAME [',' [TYPE_COMMENT] '**' NAME] [TYPE_COMMENT] | '**' NAME [TYPE_COMMENT]) |
fpdef ['=' test] (',' [TYPE_COMMENT] fpdef ['=' test])* [','] [TYPE_COMMENT])
fpdef: NAME | '(' fplist ')'
fplist: fpdef (',' fpdef)* [',']

Expand Down
5 changes: 3 additions & 2 deletions ast27/Include/Python-ast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ struct _arguments {
identifier vararg;
identifier kwarg;
asdl_seq *defaults;
asdl_seq *type_comments;
};

struct _keyword {
Expand Down Expand Up @@ -531,9 +532,9 @@ comprehension_ty _Ta27_comprehension(expr_ty target, expr_ty iter, asdl_seq * if
#define ExceptHandler(a0, a1, a2, a3, a4, a5) _Ta27_ExceptHandler(a0, a1, a2, a3, a4, a5)
excepthandler_ty _Ta27_ExceptHandler(expr_ty type, expr_ty name, asdl_seq * body, int lineno, int
col_offset, PyArena *arena);
#define arguments(a0, a1, a2, a3, a4) _Ta27_arguments(a0, a1, a2, a3, a4)
#define arguments(a0, a1, a2, a3, a4, a5) _Ta27_arguments(a0, a1, a2, a3, a4, a5)
arguments_ty _Ta27_arguments(asdl_seq * args, identifier vararg, identifier kwarg, asdl_seq *
defaults, PyArena *arena);
defaults, asdl_seq * type_comments, PyArena *arena);
#define keyword(a0, a1, a2) _Ta27_keyword(a0, a1, a2)
keyword_ty _Ta27_keyword(identifier arg, expr_ty value, PyArena *arena);
#define alias(a0, a1, a2) _Ta27_alias(a0, a1, a2)
Expand Down
3 changes: 2 additions & 1 deletion ast27/Parser/Python.asdl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,9 @@ module Python version "$Revision$"
excepthandler = ExceptHandler(expr? type, expr? name, stmt* body)
attributes (int lineno, int col_offset)

-- the type comments are used only in the multiline type comment syntax
arguments = (expr* args, identifier? vararg,
identifier? kwarg, expr* defaults)
identifier? kwarg, expr* defaults, string* type_comments)

-- keyword arguments supplied to call
keyword = (identifier arg, expr value)
Expand Down
40 changes: 37 additions & 3 deletions ast27/Python/Python-ast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ static char *arguments_fields[]={
"vararg",
"kwarg",
"defaults",
"type_comments",
};
static PyTypeObject *keyword_type;
static PyObject* ast2obj_keyword(void*);
Expand Down Expand Up @@ -963,7 +964,7 @@ static int init_types(void)
ExceptHandler_type = make_type("ExceptHandler", excepthandler_type, ExceptHandler_fields,
3);
if (!ExceptHandler_type) return 0;
arguments_type = make_type("arguments", &AST_type, arguments_fields, 4);
arguments_type = make_type("arguments", &AST_type, arguments_fields, 5);
if (!arguments_type) return 0;
keyword_type = make_type("keyword", &AST_type, keyword_fields, 2);
if (!keyword_type) return 0;
Expand Down Expand Up @@ -2090,7 +2091,8 @@ ExceptHandler(expr_ty type, expr_ty name, asdl_seq * body, int lineno, int col_o
}

arguments_ty
arguments(asdl_seq * args, identifier vararg, identifier kwarg, asdl_seq * defaults, PyArena *arena)
arguments(asdl_seq * args, identifier vararg, identifier kwarg, asdl_seq * defaults, asdl_seq *
type_comments, PyArena *arena)
{
arguments_ty p;
p = (arguments_ty)PyArena_Malloc(arena, sizeof(*p));
Expand All @@ -2100,6 +2102,7 @@ arguments(asdl_seq * args, identifier vararg, identifier kwarg, asdl_seq * defau
p->vararg = vararg;
p->kwarg = kwarg;
p->defaults = defaults;
p->type_comments = type_comments;
return p;
}

Expand Down Expand Up @@ -3304,6 +3307,11 @@ ast2obj_arguments(void* _o)
if (PyObject_SetAttrString(result, "defaults", value) == -1)
goto failed;
Py_DECREF(value);
value = ast2obj_list(o->type_comments, ast2obj_string);
if (!value) goto failed;
if (PyObject_SetAttrString(result, "type_comments", value) == -1)
goto failed;
Py_DECREF(value);
return result;
failed:
Py_XDECREF(value);
Expand Down Expand Up @@ -6617,6 +6625,7 @@ obj2ast_arguments(PyObject* obj, arguments_ty* out, PyArena* arena)
identifier vararg;
identifier kwarg;
asdl_seq* defaults;
asdl_seq* type_comments;

if (PyObject_HasAttrString(obj, "args")) {
int res;
Expand Down Expand Up @@ -6690,7 +6699,32 @@ obj2ast_arguments(PyObject* obj, arguments_ty* out, PyArena* arena)
PyErr_SetString(PyExc_TypeError, "required field \"defaults\" missing from arguments");
return 1;
}
*out = arguments(args, vararg, kwarg, defaults, arena);
if (PyObject_HasAttrString(obj, "type_comments")) {
int res;
Py_ssize_t len;
Py_ssize_t i;
tmp = PyObject_GetAttrString(obj, "type_comments");
if (tmp == NULL) goto failed;
if (!PyList_Check(tmp)) {
PyErr_Format(PyExc_TypeError, "arguments field \"type_comments\" must be a list, not a %.200s", tmp->ob_type->tp_name);
goto failed;
}
len = PyList_GET_SIZE(tmp);
type_comments = asdl_seq_new(len, arena);
if (type_comments == NULL) goto failed;
for (i = 0; i < len; i++) {
string value;
res = obj2ast_string(PyList_GET_ITEM(tmp, i), &value, arena);
if (res != 0) goto failed;
asdl_seq_SET(type_comments, i, value);
}
Py_XDECREF(tmp);
tmp = NULL;
} else {
PyErr_SetString(PyExc_TypeError, "required field \"type_comments\" missing from arguments");
return 1;
}
*out = arguments(args, vararg, kwarg, defaults, type_comments, arena);
return 0;
failed:
Py_XDECREF(tmp);
Expand Down
48 changes: 38 additions & 10 deletions ast27/Python/ast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,17 +732,18 @@ static arguments_ty
ast_for_arguments(struct compiling *c, const node *n)
{
/* parameters: '(' [varargslist] ')'
varargslist: (fpdef ['=' test] ',')* ('*' NAME [',' '**' NAME]
| '**' NAME) | fpdef ['=' test] (',' fpdef ['=' test])* [',']
varargslist: ((fpdef ['=' test] ',' [TYPE_COMMENT])*
('*' NAME [',' [TYPE_COMMENT] '**' NAME] [TYPE_COMMENT] | '**' NAME [TYPE_COMMENT]) |
fpdef ['=' test] (',' [TYPE_COMMENT] fpdef ['=' test])* [','] [TYPE_COMMENT])
*/
int i, j, k, n_args = 0, n_defaults = 0, found_default = 0;
asdl_seq *args, *defaults;
int i, j, k, l, n_args = 0, n_all_args = 0, n_defaults = 0, found_default = 0;
asdl_seq *args, *defaults, *type_comments = NULL;
identifier vararg = NULL, kwarg = NULL;
node *ch;

if (TYPE(n) == parameters) {
if (NCH(n) == 2) /* () as argument list */
return arguments(NULL, NULL, NULL, NULL, c->c_arena);
return arguments(NULL, NULL, NULL, NULL, NULL, c->c_arena);
n = CHILD(n, 1);
}
REQ(n, varargslist);
Expand All @@ -754,7 +755,10 @@ ast_for_arguments(struct compiling *c, const node *n)
n_args++;
if (TYPE(ch) == EQUAL)
n_defaults++;
if (TYPE(ch) == STAR || TYPE(ch) == DOUBLESTAR)
n_all_args++;
}
n_all_args += n_args;
args = (n_args ? asdl_seq_new(n_args, c->c_arena) : NULL);
if (!args && n_args)
return NULL;
Expand All @@ -768,6 +772,7 @@ ast_for_arguments(struct compiling *c, const node *n)
i = 0;
j = 0; /* index for defaults */
k = 0; /* index for args */
l = 0; /* index for type comments */
while (i < NCH(n)) {
ch = CHILD(n, i);
switch (TYPE(ch)) {
Expand Down Expand Up @@ -834,7 +839,7 @@ ast_for_arguments(struct compiling *c, const node *n)
asdl_seq_SET(args, k++, name);

}
i += 2; /* the name and the comma */
i += 1 + (TYPE(CHILD(n, i + 1)) == COMMA); /* the name and the comma, if present */
if (parenthesized && Py_Py3kWarningFlag &&
!ast_warn(c, ch, "parenthesized argument names "
"are invalid in 3.x"))
Expand All @@ -848,15 +853,32 @@ ast_for_arguments(struct compiling *c, const node *n)
vararg = NEW_IDENTIFIER(CHILD(n, i+1));
if (!vararg)
return NULL;
i += 3;
i += 2 + (TYPE(CHILD(n, i + 2)) == COMMA);
break;
case DOUBLESTAR:
if (!forbidden_check(c, CHILD(n, i+1), STR(CHILD(n, i+1))))
return NULL;
kwarg = NEW_IDENTIFIER(CHILD(n, i+1));
if (!kwarg)
return NULL;
i += 3;
i += 2 + (TYPE(CHILD(n, i + 2)) == COMMA);
break;
case TYPE_COMMENT:
assert(l < k + !!vararg + !!kwarg);

if (!type_comments) {
/* lazily allocate the type_comments seq for perf reasons */
type_comments = asdl_seq_new(n_all_args, c->c_arena);
if (!type_comments)
return NULL;
}

while (l < k + !!vararg + !!kwarg - 1) {
asdl_seq_SET(type_comments, l++, NULL);
}

asdl_seq_SET(type_comments, l++, NEW_TYPE_COMMENT(ch));
i += 1;
Copy link

Choose a reason for hiding this comment

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

All this logic looks sensible.

I'd like to have a comment somewhere describing the intended semantics of this type_comment sequence -- something like that it's either NULL, or has one entry for each parameter including one for varargs and one for kwargs if applicable, any of which may be NULL for an absent type comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. Added!

break;
default:
PyErr_Format(PyExc_SystemError,
Expand All @@ -866,7 +888,13 @@ ast_for_arguments(struct compiling *c, const node *n)
}
}

return arguments(args, vararg, kwarg, defaults, c->c_arena);
if (type_comments) {
while (l < n_all_args) {
asdl_seq_SET(type_comments, l++, NULL);
}
}

return arguments(args, vararg, kwarg, defaults, type_comments, c->c_arena);
}

static expr_ty
Expand Down Expand Up @@ -1038,7 +1066,7 @@ ast_for_lambdef(struct compiling *c, const node *n)
expr_ty expression;

if (NCH(n) == 3) {
args = arguments(NULL, NULL, NULL, NULL, c->c_arena);
args = arguments(NULL, NULL, NULL, NULL, NULL, c->c_arena);
if (!args)
return NULL;
expression = ast_for_expr(c, CHILD(n, 2));
Expand Down
55 changes: 38 additions & 17 deletions ast27/Python/graminit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,51 +159,72 @@ static arc arcs_8_0[3] = {
{31, 2},
{32, 3},
};
static arc arcs_8_1[3] = {
static arc arcs_8_1[4] = {
{28, 4},
{30, 5},
{24, 6},
{0, 1},
};
static arc arcs_8_2[1] = {
{21, 6},
{21, 7},
};
static arc arcs_8_3[1] = {
{21, 7},
{21, 8},
};
static arc arcs_8_4[1] = {
{29, 8},
{29, 9},
};
static arc arcs_8_5[4] = {
static arc arcs_8_5[5] = {
{27, 1},
{24, 10},
{31, 2},
{32, 3},
{0, 5},
};
static arc arcs_8_6[2] = {
{30, 9},
static arc arcs_8_6[1] = {
{0, 6},
};
static arc arcs_8_7[1] = {
static arc arcs_8_7[3] = {
{30, 11},
{24, 6},
{0, 7},
};
static arc arcs_8_8[2] = {
{30, 5},
{24, 6},
{0, 8},
};
static arc arcs_8_9[1] = {
static arc arcs_8_9[3] = {
{30, 5},
{24, 6},
{0, 9},
};
static arc arcs_8_10[4] = {
{27, 1},
{31, 2},
{32, 3},
{0, 10},
};
static arc arcs_8_11[2] = {
{24, 12},
{32, 3},
};
static arc arcs_8_12[1] = {
{32, 3},
};
static state states_8[10] = {
static state states_8[13] = {
{3, arcs_8_0},
{3, arcs_8_1},
{4, arcs_8_1},
{1, arcs_8_2},
{1, arcs_8_3},
{1, arcs_8_4},
{4, arcs_8_5},
{2, arcs_8_6},
{1, arcs_8_7},
{5, arcs_8_5},
{1, arcs_8_6},
{3, arcs_8_7},
{2, arcs_8_8},
{1, arcs_8_9},
{3, arcs_8_9},
{4, arcs_8_10},
{2, arcs_8_11},
{1, arcs_8_12},
};
static arc arcs_9_0[2] = {
{21, 1},
Expand Down Expand Up @@ -1971,7 +1992,7 @@ static dfa dfas[88] = {
"\000\000\020\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"},
{263, "parameters", 0, 4, states_7,
"\000\040\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"},
{264, "varargslist", 0, 10, states_8,
{264, "varargslist", 0, 13, states_8,
"\000\040\040\200\001\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"},
{265, "fpdef", 0, 4, states_9,
"\000\040\040\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"},
Expand Down
7 changes: 4 additions & 3 deletions ast35/Grammar/Grammar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ async_funcdef: ASYNC funcdef
funcdef: 'def' NAME parameters ['->' test] ':' [TYPE_COMMENT] suite

parameters: '(' [typedargslist] ')'
typedargslist: (tfpdef ['=' test] (',' tfpdef ['=' test])* [','
['*' [tfpdef] (',' tfpdef ['=' test])* [',' '**' tfpdef] | '**' tfpdef]]
| '*' [tfpdef] (',' tfpdef ['=' test])* [',' '**' tfpdef] | '**' tfpdef)
typedargslist: (tfpdef ['=' test] (',' [TYPE_COMMENT] tfpdef ['=' test])* [',' [TYPE_COMMENT]
['*' [tfpdef] (',' [TYPE_COMMENT] tfpdef ['=' test])* [',' [TYPE_COMMENT] '**' tfpdef] | '**' tfpdef]] [TYPE_COMMENT]
| '*' [tfpdef] (',' [TYPE_COMMENT] tfpdef ['=' test])* [',' [TYPE_COMMENT] '**' tfpdef] [TYPE_COMMENT]
| '**' tfpdef [TYPE_COMMENT])
Copy link

Choose a reason for hiding this comment

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

One consequence of this definition is that the type comment is required to come after the comma, if any, that follows the argument.

This seems fine to me -- thanks to Python's graceful handling of a redundant trailing comma, there's rarely a good reason to want to put the comma on the next line, in a form like this:

def do_something( options=None
                , config=None
                , force=True
                ):

such as might appear in other languages for the sake of uniformity between lines and avoiding unnecessary diff noise.

It's not clearly specified by PEP 484, though:
"""
Sometimes you have a long list of parameters and specifying their types in a single # type: comment would be awkward. To this end you may list the arguments one per line and add a # type: comment per line. To specify the return type use the ellipsis syntax. Not every argument needs to be given a type. A line with a # type: comment should contain exactly one argument. The type comment for the last argument (if any) should precede the close parenthesis.
"""

So we should say this clearly someplace like the README, and also update the PEP to clarify. (And once that's done, can take this point out of the README.)

Copy link

Choose a reason for hiding this comment

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

(Looks like this and the other PEP clarification are still open.)

tfpdef: NAME [':' test]
varargslist: (vfpdef ['=' test] (',' vfpdef ['=' test])* [','
['*' [vfpdef] (',' vfpdef ['=' test])* [',' '**' vfpdef] | '**' vfpdef]]
Expand Down
Loading