bpo-33416: Add end positions to Python AST #11605

ilevkivskyi · 2019-01-18T09:46:26Z

The majority of this PR is tediously passing end_lineno and end_col_offset everywhere. Here are non-trivial points:

It is not possible to reconstruct end positions in AST "on the fly", some information is lost after an AST node is constructed, so we need two more attributes for every AST node end_lineno and end_col_offset.
I add end position information to both CST and AST. Although it may be technically possible to avoid adding end positions to CST, the code becomes more cumbersome and less efficient.
Since the end position is not known for non-leaf CST nodes while the next token is added, this requires a bit of extra care (see _PyNode_FinalizeEndPos). Unless I made some mistake, the algorithm should be linear.
For statements, I "trim" the end position of suites to not include the terminal newlines and dedent (this seems to be what people would expect), for example in
```
class C:
    pass

pass
```
the end line and end column for the class definition is (2, 8).
For end_col_offset I use the common Python convention for indexing, for example for pass the end_col_offset is 4 (not 3), so that [0:4] gives one the source code that corresponds to the node.
I added a helper function ast.get_source_segment(), to get source text segment corresponding to a given AST node. It is also useful for testing.

An (inevitable) downside of this PR is that AST now takes almost 25% more memory. I think however it is probably justified by the benefits.

https://bugs.python.org/issue33416

Conflicts: Parser/parsetok.c

gvanrossum · 2019-01-18T16:32:21Z

Never mind, that was SyntaxError. The AST uses 0-based column offsets.

asottile

I'm very excited for this change

asottile · 2019-01-18T19:06:37Z

Doc/library/ast.rst

+      The UTF-8 offset is recorded because the parser uses UTF-8 internally.
+
+      Note that the end positions are not required by the compiler and are
+      therefore optional. The end offset if *after* the last symbol, for example


Suggested change

therefore optional. The end offset if *after* the last symbol, for example

therefore optional. The end offset is *after* the last symbol, for example

asottile · 2019-01-18T19:07:18Z

Doc/library/ast.rst

-   This is useful to "move code" to a different location in a file.
+   Increment the line number and end line number of each node in the tree
+   starting at *node* by *n*. This is useful to "move code" to a different
+   location in a file..


Suggested change

location in a file..

location in a file.

gvanrossum

Phew! I don't have much to add, this all looks great. I don't think in these days 20% extra space to represent CST+AST for a single module is a big deal (though it would be nice to see what the difference is in absolute number of bytes for Lib/tkinter/init.py, which AFAIK is still the largest stdlib module).

gvanrossum · 2019-01-18T16:50:40Z

Doc/library/ast.rst

-   to *new_node* if possible, and return *new_node*.
+   Copy source location (:attr:`lineno`, :attr:`col_offset`, :attr:`end_lineno`,
+   and :attr:`end_col_offset`) from *old_node* to *new_node* if possible,
+   and return *new_node*.


Does this only affect the node, or the whole tree? I've never used this so I don't know what to expect from context.

This one (unlike some others) is non-recursive, one can even copy from a node of a different kind.

gvanrossum · 2019-01-18T16:52:15Z

Include/Python-ast.h

@@ -438,152 +446,188 @@ mod_ty _Py_Interactive(asdl_seq * body, PyArena *arena);
 mod_ty _Py_Expression(expr_ty body, PyArena *arena);


A few lines up, I wonder if it would be a good idea to add a comment explaining that these macros also change the definition of the functions (not just the calls).

Added a comment.

gvanrossum · 2019-01-18T16:56:51Z

Lib/ast.py

-            _fix(child, lineno, col_offset)
-    _fix(node, 1, 0)
+            _fix(child, lineno, col_offset, end_lineno, end_col_offset)
+    _fix(node, 1, 0, 1, 0)


This whole function looks a bit suspicious.Shouldn't it at least ensure that (end_lineno, end_col_offset) > (lineno, col_offset)? (Again, I guess I don't know the use case.)

This function is used to compile manually generated AST to bytecode. Although technically end positions are not needed for the compiler (only start positions are recorded in the byte code), it is probably good to have them fixed. The algorithm for fixing is quite naive (take either a position of nearest root-side node if known, otherwise use start of the file), but it never intended to be robust, it exist just to allow compilation in those case where a user doesn't care about line numbers.

gvanrossum · 2019-01-18T17:34:09Z

Lib/ast.py

+
+    if end_lineno == lineno:
+        source_line = source.splitlines()[lineno]
+        return source_line.encode()[col_offset:end_col_offset].decode(coding)


So if I read this well, source is bytes, and the col_offsets are byte offsets? And a tab counts as one space? Is that explicit somewhere in the ast module docs? (This is easy to miss when writing code since most source code is ASCII only so the difference doesn't matter, but it would break whenever a token appears after a non-ASCII character in an identifier or string literal.)

All you say is right, and yes it is already mentioned in the docs that they are UTF-8 byte offsets.

gvanrossum · 2019-01-18T18:46:36Z

Lib/ast.py

+    last = lines[end_lineno].encode()[:end_col_offset].decode(coding)
+    middle_lines = lines[lineno+1:end_lineno]
+
+    return '\n'.join([first] + middle_lines + [last])


This would avoid two list copies if you used .insert() and .append() to merge first and last into the middle_lines list.

gvanrossum · 2019-01-18T18:47:25Z

Lib/test/test_ast.py

+        self.assertEqual(ast.get_source_segment(s_orig, cdef.body[0], padded=True),
+                         s_method)
+
+


FWIW I trust your tests. If they pass, they're good. :-)

gvanrossum · 2019-01-18T18:48:27Z

Misc/NEWS.d/next/Core and Builtins/2019-01-18-09-49-17.bpo-33416.rrj9a5.rst

@@ -0,0 +1 @@
+Add end line and end column position information to the Python AST nodes.


Maybe add that this is a backwards incompatible API change for anything that creates AST nodes? (It's fine if you just call ast.parse(), but not if you ever create a new node.)

Actually most of the things still work:

All existing manual AST creation code will work, ast.Constant(value=42, lineno=1, col_offset=0) is still valid.

Compilation of existing manually generated ASTs will still work, the end position are int? in Python.asdl, so they are allowed to be absent and/or be None.

However, some C-level compatibility is broken. I am not sure what is the source of truth for "official" C API. I checked docs.python.org and none of the functions I updated is listed there.

I will try to re-formulate this.

gvanrossum · 2019-01-18T18:58:00Z

Python/ast.c

@@ -2864,7 +2918,12 @@ ast_for_call(struct compiling *c, const node *n, expr_ty func,
        }
    }

-    return Call(func, args, keywords, func->lineno, func->col_offset, c->c_arena);
+    // Ideally call ends where closing parenthesis is placed.


(When does a call not have a close par?)

It (close par) was NULL for "fake" calls used in analyzing class definitions, but I just fixed this and removed the comment.

gvanrossum · 2019-01-18T19:52:18Z

Python/ast.c

+get_last_end_pos(asdl_seq *s, int *end_lineno, int *end_col_offset)
+{
+    int tot = asdl_seq_LEN(s);
+    stmt_ty last = asdl_seq_GET(s, tot - 1);


Does this need a check that tot > 0?

Added this just in case (to be future proof against any AST optimizers).

serhiy-storchaka

This is a part of comments.

serhiy-storchaka · 2019-01-19T10:01:24Z

Doc/library/ast.rst

@@ -162,6 +170,19 @@ and classes for traversing abstract syntax trees:
      :class:`AsyncFunctionDef` is now supported.


+.. function:: get_code_segment(source, node, *, padded=False, coding='utf-8')


I think encoding is more common term in this context.

serhiy-storchaka · 2019-01-19T10:04:29Z

Doc/library/ast.rst

+      The UTF-8 offset is recorded because the parser uses UTF-8 internally.
+
+      Note that the end positions are not required by the compiler and are
+      therefore optional. The end offset if *after* the last symbol, for example


What does "optional" mean? Is it equal to None or an absent attribute?

Actually both are allowed. An optional attribute in Python.asdl can either be absent or None.

serhiy-storchaka · 2019-01-19T10:18:34Z

Lib/ast.py

+    If *padded* is `True`, the first line of a multi-line statement will
+    be padded with spaces to match its original position.
+    """
+    if not hasattr(node, 'lineno'):


This can be simplified:

try: # get loneno, end_lineno, etc. except AttributeError: return None

serhiy-storchaka · 2019-01-19T10:20:39Z

Lib/ast.py

+
+    if end_lineno == lineno:
+        source_line = source.splitlines()[lineno]
+        return source_line.encode()[col_offset:end_col_offset].decode(coding)


I think coding should not be used here. Always use UTF-8.

You are totally right.

serhiy-storchaka · 2019-01-19T10:26:12Z

Lib/ast.py

+    lines = source.splitlines()
+
+    if padded:
+        padding = ' ' * len(lines[lineno].encode()[:col_offset].decode(coding))


I suggest to keep tabs and spaces (and maybe '\f'?) and replace all other characters with spaces. There may be similar code in C, look how the position of the caret is calculated.

Oh yes. It looks like there are bunch of corner cases around \r\n\f\t. I tried to update the function (including your suggestions) so that it mimics closely what happens in C code.

serhiy-storchaka · 2019-01-19T10:41:20Z

Lib/test/test_asdl_parser.py

@@ -62,14 +62,14 @@ def test_product(self):

    def test_attributes(self):
        stmt = self.types['stmt']
-        self.assertEqual(len(stmt.attributes), 2)
+        self.assertEqual(len(stmt.attributes), 4)
        self.assertEqual(str(stmt.attributes[0]), 'Field(int, lineno)')
        self.assertEqual(str(stmt.attributes[1]), 'Field(int, col_offset)')


Add tests for stmt.attributes[2] and stmt.attributes[3]?

serhiy-storchaka · 2019-01-19T10:47:43Z

Parser/node.c

 int
-PyNode_AddChild(node *n1, int type, char *str, int lineno, int col_offset)
+PyNode_AddChild(node *n1, int type, char *str, int lineno, int col_offset,


Is not this a part of the C API? Should not we add a new function?

What is the "official" source of truth for C API? I didn't find this function on docs.python.org.

serhiy-storchaka · 2019-01-19T10:54:25Z

Parser/parser.c

@@ -225,7 +229,9 @@ future_hack(parser_state *ps)

 int
 PyParser_AddToken(parser_state *ps, int type, char *str,


Is not this a part of the C API?

ilevkivskyi · 2019-01-19T19:42:16Z

@asottile @gvanrossum @serhiy-storchaka Thank you for reviewing! I think I addressed all your comments.

ilevkivskyi · 2019-01-19T19:51:26Z

Lib/ast.py

@@ -213,6 +226,77 @@ def get_docstring(node, clean=True):
    return text


+def _splitlines_no_ff(source):


Another option is to use .splitlines(keepends=True) (which considers \f as an endline) and then re-join those that aren't ending in \r or \n, I am not sure which approach is better.

ilevkivskyi · 2019-01-19T19:59:18Z

@gvanrossum I just tried to compare the sizes of Lib/tkinter/__init__.py AST before and after this PR. Here are the numbers: before 5.8 Mbytes, after 7.2Mbytes (24% increase).

gvanrossum · 2019-01-19T20:22:18Z

@gvanrossum I just tried to compare the sizes of Lib/tkinter/__init__.py AST before and after this PR. Here are the numbers: before 5.8 Mbytes, after 7.2Mbytes (24% increase).

How did you measure this? IIUC there's also a size increase for the CST, and during translation from CST to AST both are in memory. Is it possible to measure the peak memory usage during this translation?

ilevkivskyi · 2019-01-20T01:18:16Z

I just looked at the difference in allocated memory reported in sys._debugmallocstats(). I just tried another way using tracemalloc. It reports that the line in question:

compile(source, filename, mode, PyCF_ONLY_AST)

allocates 3.7 Mbytes before and 4.6 Mbytes after this PR (still the same 24% relative increase). Also this number (24%) is quite reasonable taking into account that all nodes in CST have exactly the same size: 40 bytes before, 48 bytes after (at least on my 64-bit Linux).

gvanrossum · 2019-01-20T02:03:06Z

I do think that's a very reasonable increase (we're not talking MicroPython here :-).

Also this number (24%) is quite reasonable taking into account that all nodes in CST have exactly the same size: 40 bytes before, 48 bytes after (at least on my 64-bit Linux).

That's sizeof what exactly? While the node types indeed are big unions, there are several different ones: struct _stmt, struct _expr and some minor ones (all defined in Include/Python-ast.h).

(I tried this myself on my Mac, and it looks like sizeof(struct _stmt) is 72 bytes, and sizeof(struct _expr) is 48 bytes. An int is 4 bytes.)

ilevkivskyi · 2019-01-20T02:17:07Z

That's sizeof what exactly?

This is a size of _node. There is only one "main" struct in CST struct _node from node.h.

gvanrossum

I like this! Let's land this. I combed through the code (both the tedious parts and the interesting parts) and it all looks good to me. I am not worried about the 25% increase in CST and AST tree sizes.

ilevkivskyi · 2019-01-22T11:15:26Z

@gvanrossum
Great, thanks! I will merge this now, so that we can move with the typed_ast PR, but there is one question that Serhiy raised and I would like to double check: does the fact that PyNode_AddChild and PyParser_AddToken are not documented on docs.python.org mean they are not part of C-API? Also I just checked the files where they are declared (node.h and parser.h) are not included in Python.h.

gvanrossum · 2019-01-22T16:19:12Z

does the fact that PyNode_AddChild and PyParser_AddToken are not documented on docs.python.org mean they are not part of C-API?

I'm not sure. Their names don't have _ prefixes so I think they live in some nebulous area. It would be good to bring this up in another forum where more core devs can think about the issue (some of whome have probably thought about it more).

In the worst case scenario, if it's deemed an unacceptable backwards incompatibility, we can always add new functions that add end_lineno and end_col_offset arguments, and make the old ones call the new ones with some kind of default values computed. But I would only resort to this if during the alpha/beta release we get actual complaints.

ilevkivskyi · 2019-01-22T22:32:00Z

OK, I will post to Python-Dev about this.

ilevkivskyi added 29 commits January 6, 2019 21:16

Some initial infra

ba4ba82

Regenerate nodes

3e343e3

Mindless implementation: known bugs, notably in fstrings

1684c17

Some test fixes

514d4ea

More test fixes

3ab2516

Add a TODO

1d3e352

Switch to better algorithm for finding end position

dbf9cc9

Merge remote-tracking branch 'upstream/master' into add-end-line-col

a44207b

Conflicts: Parser/parsetok.c

Be consistent for line_num

5af33da

Minor fixes; start adding tests

ce7f5ce

Update two failing tests

2171eb9

Fix multiline strings

10cf4bd

Fix end position for if statement

ed05305

Adjust end positions in while and for

58fbfa6

Add also with

f2589ff

Fix try end position (concludes fixing suites)

7d5ca5e

Some formatting plus minor fixes

aa62e3c

More formatting; fix import from

96a0ec0

Fix f-strings

c169025

Add few more tests

553a772

Add final bunch of tests

5cc01e9

Update docstrings

dce260e

Update docs

9ba6604

Add get_source_segment() helper

69a6280

Consistent formatting in docstring; use new helper in tests

4af426f

Fix bug

e5a12c3

Split few long lines

0275a93

Add tests and docs gor the helper

f20635b

Fix missing comma

c9da8f5

ilevkivskyi requested a review from gvanrossum January 18, 2019 09:46

asottile reviewed Jan 18, 2019

View reviewed changes

gvanrossum reviewed Jan 18, 2019

View reviewed changes

serhiy-storchaka reviewed Jan 19, 2019

View reviewed changes

ilevkivskyi and others added 5 commits January 19, 2019 17:26

Fix get_source_segment

48936b9

More CR

ac5b5cb

rst fixes

4726f17

Remove unused vars

eeea87d

📜🤖 Added by blurb_it.

027a4ca

Remove old NEWS file

ff361f2

ilevkivskyi commented Jan 19, 2019

View reviewed changes

gvanrossum mentioned this pull request Jan 22, 2019

bpo-35766: Merge typed_ast back into CPython #11645

Merged

gvanrossum approved these changes Jan 22, 2019

View reviewed changes

ilevkivskyi merged commit 9932a22 into python:master Jan 22, 2019

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

ilevkivskyi deleted the add-end-line-col branch January 22, 2019 11:18

iamdefinitelyahuman mentioned this pull request Aug 1, 2019

Add end offsets to AST and source map vyperlang/vyper#1557

Closed

theodoretliu mentioned this pull request Aug 16, 2019

Pylint not underlining full expressions in VS Code pylint-dev/pylint#3061

Closed

iamdefinitelyahuman mentioned this pull request Aug 24, 2019

AST end offsets and compressed source map vyperlang/vyper#1580

Merged

jacobtylerwalls mentioned this pull request Jun 27, 2023

AST nodes for PEP 695 type param syntax do not require end_lineno nor end_col_offset #106145

Closed

	therefore optional. The end offset if after the last symbol, for example
	therefore optional. The end offset is after the last symbol, for example

		@@ -438,152 +446,188 @@ mod_ty _Py_Interactive(asdl_seq * body, PyArena *arena);
		mod_ty _Py_Expression(expr_ty body, PyArena *arena);

		self.assertEqual(ast.get_source_segment(s_orig, cdef.body[0], padded=True),
		s_method)

		@@ -0,0 +1 @@
		Add end line and end column position information to the Python AST nodes.

		@@ -162,6 +170,19 @@ and classes for traversing abstract syntax trees:
		:class:`AsyncFunctionDef` is now supported.


		.. function:: get_code_segment(source, node, *, padded=False, coding='utf-8')

		@@ -225,7 +229,9 @@ future_hack(parser_state *ps)

		int
		PyParser_AddToken(parser_state ps, int type, char str,

		@@ -213,6 +226,77 @@ def get_docstring(node, clean=True):
		return text


		def _splitlines_no_ff(source):

bpo-33416: Add end positions to Python AST #11605

bpo-33416: Add end positions to Python AST #11605

Conversation

ilevkivskyi commented Jan 18, 2019 • edited Loading

gvanrossum commented Jan 18, 2019

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi Jan 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Jan 19, 2019

Choose a reason for hiding this comment

ilevkivskyi commented Jan 19, 2019

gvanrossum commented Jan 19, 2019

ilevkivskyi commented Jan 20, 2019

gvanrossum commented Jan 20, 2019

ilevkivskyi commented Jan 20, 2019

gvanrossum left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Jan 22, 2019

gvanrossum commented Jan 22, 2019

ilevkivskyi commented Jan 22, 2019

ilevkivskyi commented Jan 18, 2019 •

edited

Loading

ilevkivskyi Jan 19, 2019 •

edited

Loading