bpo-35766: Merge typed_ast back into CPython #11645

gvanrossum · 2019-01-22T02:26:06Z

[UPDATE: I've rewritten this comment to match the latest state of the PR.]

See the bpo issue for motivation. This is the minimal code I hope to merge into CPython. The PR here is complete except for docs. To test this, use ast.parse(source, type_comments=True).

(Speaking of backwards compatibility, my bpo issue suggests that I'd also like some features supporting older grammar versions. The only thing that I would really appreciate is being able to parse code that uses async or await as a non-keyword. But I could structure this as a follow-up PR, much smaller in size.)

https://bugs.python.org/issue35766

…anything

Demo: `compile(sample, "<sample>", "exec", PyCF_ONLY_AST|0x1000)` This will crash if type comments are present. It doesn't *work* yet -- type comments cause crashes in ast.c and type ignore comments aren't passed on to Module yet.

This works, it just needs tests

gvanrossum · 2019-01-22T17:23:13Z

I updated the PR to be merged with master now #11605 has been merged. Everything still works, and there are no more "ghost commits" from #11605.

gvanrossum · 2019-01-22T19:01:00Z

I'll try to fix all failing tests before trying again. In my local branch I have test_ast and test_asdl_parser working; the rest is more mysterious (it seems bad indents are parsed differently somehow).

gvanrossum · 2019-01-23T01:10:18Z

I may have to put this off for a while (certainly I won't get it into 3.8a1). The main problem at this point is the two failing tests, test_syntax and test_exceptions. Both are failing because the grammar changes have made it so that

if 1:
foo()

reports SyntaxError instead of IndentationError at the start of line 2. The reason for that is this definition in Grammar:

# the TYPE_COMMENT in suites is only parsed for funcdefs, but can't go elsewhere due to ambiguity
suite: simple_stmt | NEWLINE [TYPE_COMMENT NEWLINE] INDENT stmt+ DEDENT

Previously this was

suite: simple_stmt | NEWLINE INDENT stmt+ DEDENT

and there is code in Python/pythonrun.c::err_input that depends on INDENT being the only possibility after the NEWLINE.

I haven't figured out how to fix this yet.

This fixes if 1: foo() but does nothing about def f(): foo() The latter still reports 'SyntaxError: invalid syntax'. So this is not a real solution, just a stop-gap measure until I find a better solution.

Also: - remove redundant 'c' argument from new_type_comment() - double-check that TYPE_COMMENT in a suite is followed by NL

gvanrossum · 2019-01-28T19:51:36Z

Sorry for the flood of comments. I believe I've done or responded to every code review comment. Hopefully tests will pass now.

ilevkivskyi

Thanks for updates! This looks ready, I left a bunch of really minor comments.

ilevkivskyi · 2019-01-30T00:40:08Z

Parser/tokenizer.c

+            while (*prefix && p < tok->cur) {
+                if (*prefix == ' ') {
+                    while (*p == ' ' || *p == '\t')
+                        p++;


I am not sure what are the rules of single line bodies. I would prefer to have them always enclosed in curly braces {...}. It looks like you mix both styles. I will label them so that you can deiced what to do.

I added a few more braces. I don't know what PEP 7 says, but the code here seems to mostly use the convention that a single break or goto or return doesn't need to be surrounded by braces (except when part of an if-else if-etc. sequence where most other bodies braced). So I added braces to this specific case, but not around several of the others you flagged.

ilevkivskyi · 2019-01-30T00:40:37Z

Parser/parsetok.c

+        arr->size *= 2;
+        arr->items = realloc(arr->items, arr->size * sizeof(*arr->items));
+        if (!arr->items)
+            return 0;


Single line body.

ilevkivskyi · 2019-01-30T00:41:14Z

Parser/tokenizer.c

+                p += 6;
+                while (is_type_ignore && p < tok->cur) {
+                  if (*p == '#')
+                      break;


Single line body.

ilevkivskyi · 2019-01-30T00:42:56Z

Python/ast.c

+            num = NCH(ch);
+            type_ignores = _Py_asdl_seq_new(num, arena);
+            if (!type_ignores)
+                goto out;


Single line body.

ilevkivskyi · 2019-01-30T00:43:12Z

Python/ast.c

+            for (i = 0; i < num; i++) {
+                type_ignore_ty ti = TypeIgnore(LINENO(CHILD(ch, i)), arena);
+                if (!ti)
+                   goto out;


Single line body.

ilevkivskyi · 2019-01-30T00:44:02Z

Python/ast.c

+                num = 0;
+                for (i = 0; i < NCH(ch); i++) {
+                    if (TYPE(CHILD(ch, i)) == test)
+                        num++;


One more single line body. OK, there are probably too many of them.

This one I fixed.

ilevkivskyi · 2019-01-30T00:46:12Z

Python/ast.c

+
+                    if (i < NCH(n) && TYPE(CHILD(n, i)) == TYPE_COMMENT) {
+                        ast_error(c, CHILD(n, i),
+                                "bare * has associated type comment");


This looks like an inconsistent indent, should be two more spaces.

Fixed, and fixed indents for all other ast_error calls. (I wonder if it used to have a shorter name?)

ilevkivskyi · 2019-01-30T00:51:18Z

Python/ast.c

@@ -3045,15 +3182,16 @@ ast_for_expr_stmt(struct compiling *c, const node *n)
 {
    REQ(n, expr_stmt);
    /* expr_stmt: testlist_star_expr (annassign | augassign (yield_expr|testlist) |
-                            ('=' (yield_expr|testlist_star_expr))*)
+                           ('=' (yield_expr|testlist_star_expr))* [TYPE_COMMENT])


This comment needs to be updated to match the grammar.

ilevkivskyi · 2019-01-30T00:52:18Z

Python/ast.c


-    if (NCH(n) == 1) {
+    if (num == 1 || (num == 2 && TYPE(CHILD(n, 1)) == TYPE_COMMENT)) {


It seems to me the (num == 2 && TYPE(CHILD(n, 1)) == TYPE_COMMENT) condition can never be true now (with the updated grammar).

gvanrossum · 2019-01-30T19:39:44Z

@ambv Should I just merge this, or do you want to review it?

ambv · 2019-01-30T19:52:13Z

Unless you're in a hurry now, I'll review and stamp it tomorrow morning my time. Mostly to familiarize myself with the changes.

gvanrossum · 2019-01-30T19:52:42Z

That's fine!

gvanrossum · 2019-01-30T19:58:23Z

Unsure why the Appveyor build isn't running?

zooba · 2019-01-30T23:39:04Z

      Unsure why the Appveyor build isn't running?

It looks it decided to skip the build, maybe because the last commit is a merge?

ambv

Let's merge this. It's a bit of a bigger change than promised but such is life 😉

I left one comment, @gvanrossum, that you might want to respond to.

ambv · 2019-01-31T11:33:10Z

Parser/asdl_c.py

 mod_ty PyAST_obj2mod(PyObject* ast, PyArena* arena, int mode)
 {
    mod_ty res;
    PyObject *req_type[3];
-    char *req_name[] = {"Module", "Expression", "Interactive"};
+    char *req_name[] = {"Module", "Expression", "Interactive", "FunctionType"};
    int isinstance;

    req_type[0] = (PyObject*)Module_type;
    req_type[1] = (PyObject*)Expression_type;
    req_type[2] = (PyObject*)Interactive_type;


Shouldn't we be also populating req_type[3] here to FunctionType_type? The assymetry looks brittle to me.

Hm... I misunderstood what this is for. Fortunately the code in builtin_compile_impl() prevents this to be reached with mode==3. I propose to just drop support for FunctionType here.

pablogsal · 2019-02-01T14:04:57Z

@ambv @gvanrossum I have detected some refleaks in the buildbots after this PR and I started https://bugs.python.org/issue35881 to track this problem. I already started debugging, but I would like so input in case you can see something obvious right away.

pythonGH-11645 added the fourth option for `mode` argument; updating the comment accordingly.

tyomitch · 2019-02-05T20:15:21Z

This PR added a fourth option for mode argument of builtin_compile_impl(), but didn't update its documentation; please see GH-11762

This is mostly cribbed from python/cpython#11645, though it also adds a new error check to new_type_comment itself.

gvanrossum requested review from ambv and ilevkivskyi January 22, 2019 02:26

bedevere-bot added the awaiting merge label Jan 22, 2019

gvanrossum added 13 commits January 22, 2019 08:58

Add TYPE_IGNORE and TYPE_COMMENT without regenerating anything

77de614

Regenerated files (make regen-token)

8e0fd8a

Tentative parsetok.c changes to handle TYPE_IGNORE

cb93ba5

Unconditionally recognize type comments in tokenizer.c

232f2b6

Changes to Grammar without regenerating anything

c80197e

Regenerated files (make regen-grammar)

b440842

Add type_comment fields to various asdl classes without regenerating …

9db5e34

…anything

Regenerated files (make regen-ast)

6cc5a55

Make ast.c compile by adding NULL for all type_comment arguments

e7f207d

Make recognition of type comments conditional on a flag (off by default)

8b426b9

Define flags to turn on parsing type comments

72e4f4d

Demo: `compile(sample, "<sample>", "exec", PyCF_ONLY_AST|0x1000)` This will crash if type comments are present. It doesn't *work* yet -- type comments cause crashes in ast.c and type ignore comments aren't passed on to Module yet.

Support passing type ignores to Module object

3d19dbb

Support TYPE_COMMENT in ast.c for all nodes that need it

d273d67

This works, it just needs tests

gvanrossum force-pushed the ast-type-comments branch from e1bf919 to d273d67 Compare January 22, 2019 17:21

gvanrossum added 2 commits January 22, 2019 09:32

Regenerate Lib/symbol.py

830d63a

Make growable_int_array functions static

71aecd5

blurb-it bot and others added 3 commits January 22, 2019 11:21

News blurb (thank you blurb-it)

53ceee6

Fix test_ast.py

6ddca66

Fix test_asdl_parser.py

36c212b

gvanrossum force-pushed the ast-type-comments branch from a40849b to 36c212b Compare January 22, 2019 20:56

gvanrossum mentioned this pull request Jan 23, 2019

[DO NOT MERGE] Fix def line numbers for decorated nodes python/typed_ast#61

Closed

gvanrossum added 3 commits January 22, 2019 20:30

Hacky way to get tests to pass

d2091ed

This fixes if 1: foo() but does nothing about def f(): foo() The latter still reports 'SyntaxError: invalid syntax'. So this is not a real solution, just a stop-gap measure until I find a better solution.

Regen Lib/symbol.py (again)

6d1f1de

Fix assert in ast_for_suite() -- n can be a func_body_suite too now

567f341

gvanrossum added 3 commits January 28, 2019 11:31

Add comment clarifying the func_body_suite switcheroo

98ee20a

Check for errors from _Py_asdl_seq_new()

4abedd3

Check for NULL from NEW_TYPE_COMMENT()

551f2a6

Also: - remove redundant 'c' argument from new_type_comment() - double-check that TYPE_COMMENT in a suite is followed by NL

Fix markup bug

b68270c

ilevkivskyi approved these changes Jan 30, 2019

View reviewed changes

gvanrossum added 4 commits January 30, 2019 09:18

Brace block in parsetok.c

dcf86e8

Brace a block and fix indentation of another block

87812ad

Brace block, update grammar comment, fix indents of many ast_error calls

5633403

Merge branch 'master' into new-ast-type-comments

2062e27

Merge remote-tracking branch 'origin/master' into ast-type-comments

03310fc

Break up long comment to trigger Appveyor build

229874c

ambv approved these changes Jan 31, 2019

View reviewed changes

ambv merged commit dcfcd14 into python:master Jan 31, 2019

bedevere-bot removed the awaiting merge label Jan 31, 2019

gvanrossum mentioned this pull request Feb 1, 2019

bpo-35879: Fix type comment leaks #11728

Merged

gvanrossum deleted the ast-type-comments branch February 1, 2019 23:29

tyomitch added a commit to tyomitch/cpython that referenced this pull request Feb 5, 2019

Update comment for builtin_compile_impl

31327c8

pythonGH-11645 added the fourth option for `mode` argument; updating the comment accordingly.

tyomitch mentioned this pull request Feb 5, 2019

Update comment for builtin_compile_impl #11762

Closed

msullivan added a commit to python/typed_ast that referenced this pull request Feb 6, 2019

Check NEW_TYPE_COMMENT calls for failure

76511c0

This is mostly cribbed from python/cpython#11645, though it also adds a new error check to new_type_comment itself.

msullivan mentioned this pull request Feb 6, 2019

Check NEW_TYPE_COMMENT calls for failure python/typed_ast#93

Merged

gvanrossum pushed a commit to python/typed_ast that referenced this pull request Feb 6, 2019

Check NEW_TYPE_COMMENT calls for failure (#93)

f2acb2b

This is mostly cribbed from python/cpython#11645, though it also adds a new error check to new_type_comment itself.


		if (NCH(n) == 1) {
		if (num == 1 \|\| (num == 2 && TYPE(CHILD(n, 1)) == TYPE_COMMENT)) {

Uh oh!

bpo-35766: Merge typed_ast back into CPython #11645

bpo-35766: Merge typed_ast back into CPython #11645

Uh oh!

Conversation

gvanrossum commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 22, 2019

Uh oh!

gvanrossum commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 28, 2019

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Jan 30, 2019

Uh oh!

ambv commented Jan 30, 2019

Uh oh!

gvanrossum commented Jan 30, 2019

Uh oh!

gvanrossum commented Jan 30, 2019

Uh oh!

zooba commented Jan 30, 2019

Uh oh!

ambv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Feb 1, 2019

Uh oh!

tyomitch commented Feb 5, 2019

Uh oh!

Uh oh!

gvanrossum commented Jan 22, 2019 •

edited

Loading

gvanrossum commented Jan 22, 2019 •

edited

Loading

gvanrossum commented Jan 23, 2019 •

edited

Loading