Title: Simplify AST for slices
Type: enhancement Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.9
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BTaskaya, benjamin.peterson, brett.cannon, gvanrossum, nascheme, ncoghlan, pablogsal, serhiy.storchaka, thautwarm, veky
Priority: normal Keywords: patch

Created on 2018-09-27 16:17 by serhiy.storchaka, last changed 2020-03-10 16:54 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9605 merged serhiy.storchaka, 2018-09-27 17:11
Messages (15)
msg326570 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-27 16:17
Currently an AST for expressions can have non-terminal nodes of two types: expr and slice. Every of them can be a one of several kinds. A slice can be of kind Index (just an expression), Slice (optional expressions separated by ":") or ExtSlice (slices separated by ",").

For example, the expression "d[a, ..., b:c]" is represented as:

        Name('d', Load()),
                Index(Name('a', Load())),
                Slice(Name('b', Load()), Name('c', Load()), None)

and the expression "d[a, ..., b]" is represented as:

        Name('d', Load()),
                    Name('a', Load()),
                    Name('b', Load())

(note that ExtSlice is used only if there are slices in subexpressions).

I suggest to get rid of the separate slice type. The Slice kind will be a kind of the expr type instead of the slice type. The ExtSlice kind will be always replaced with a Tuple, even if subexpressions contain slices. Nodes of the Index kind will be replaced with expr nodes to which they are referred. For example, the expression "d[a, ..., b:c]" will be represented as:

        Name('d', Load()),
                Name('a', Load()),
                Slice(Name('b', Load()), Name('c', Load()), None)

This will simplify the code for handling AST, especially the C code. The following PR removes around 400 lines of code (a half of them are generated, but others are handwritten). I hope that this regularization will help to write a general code for walking AST for expressions and remove more duplicated code in ast_opt.c, ast_unparse.c, and symtable.c. This even can help to solve a problem with crashes in handling too deep AST if implement the recursion without using the C stack (but this is dreams).

This change is more breaking than a change in issue32892. What will continue to work:

* The code for creating an AST when pass values as arguments: `Index(value)` will return just `value`, `ExtSlice(slices)` will return `Tuple(slices, Load())`.

* NodeVisitor based processing. Methods visit_Index() and visit_ExtSlice() will be never invoked. The semantic of visit_Slice() will be not changed. visit_Tuple() will be invoked instead of visit_ExtSlice() for extended slices.

* isinstance() and issubclass() checks for Slice. Subclassing of Slice.

What will no longer work (with the current implementation):

* The code that creates empty AST nodes and sets their attributes. `node = Index(); node.value = value` will no longer work.

* The code that reads attributes of just created Index and ExtSlice nodes. `Index(value)` will return just `value` instead of a new object whose attribute "value" is a specified value. A list of subexpressions of ExtSlice(slices) will be accessible as the "elts" attribute instead of "dims" (because it is a Tuple).

* isinstance() and issubclass() checks for Index and ExtSlice will always return False.

* Subclassing of Index and ExtSlice. Instantiating subclasses of Index and ExtSlice will return the same result as for Index and ExtSlice, i.e. not instance of these classes.

* The code that produces a Python code from AST will need to handle indexing with tuples specially (see Tools/parser/ because d[(a, b)] is valid syntax (although parenthesis are redundant), but d[(a, b:c)] is not.

Some limitations are caused by the desire for simplicity and can be removed. For example it is possible to add the "dims" alias of "elts" to Tuple, and make subclasses working as before. It is more hard and inefficient to keep isinstance() checks and attribute access for Index. If some breakage for Index is not avoidable, I'm not sure that it is worth to spent efforts for imitating the old behavior for ExtSlice.
msg326991 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-10-03 16:19
Hello Serhiy,

I've not reviewed the patch but I trust that if you say it simplifies things, that's so.  Perhaps the important question is if it is okay to change the AST in backwards incompatible ways within 3.x releases.  As a library author who could be affected, I think it is okay. The AST is mostly an implementation detail and should not required to maintain compatibility.  We expose it via the 'ast' module for low level tools that want to manipulate it.  It is up to those users to handle backwards incompatible changes.

That said, we shouldn't just change it for trivial reasons.  That just causes work for 3rd party libraries.  Removing 400 lines of code seems like sufficient reason.
msg361997 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python triager) Date: 2020-02-14 20:49
Serhiy, any plans to bump this patch to 3.9 and continue / merge? In general the benefits looks great, but on the other hand this might cause some breakage which I guess as @nascheme is OK in AST.
msg362016 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-02-15 12:50
Updated the PR.

I can obviously be biased about my changes, so I need an approval of other core developer to merge them.

I created several PRs to popular third-party projects which work with AST to support both old and new AST schemes.
msg362224 - (view) Author: Vedran Čačić (veky) * Date: 2020-02-18 16:13
I wrote some AST analyzers, and this would have simplified my code. So I welcome it. :-)

However, it means people might be tempted to write 

    a[b:c, d]    as    a[(b:c, d)]

(or at least expect it to work -- same as `a[b, c]` can now be written as `a[(b, c)]`). We aren't going to allow this?
msg362234 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-02-18 20:33
No, this PR does not change the Python syntax. It only changes the AST representation.
msg363583 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-03-07 06:40
Haven’t looked at the code but I welcome the simplification.
msg363592 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2020-03-07 13:12
The one thing in the PR that makes me slightly wary is the point Vedran raised: in the old AST _Unparser code, the fact that index tuples containing slices should be printed without parentheses was encapsulated in the ExtSlice node type, but with Index and ExtSlice removed, that behaviour is now instead implemented by duplicating the visit_Tuple logic directly in visit_Subscript, purely to omit the surrounding parens.

So I agree that the parse tree simplification is an improvement, but I'm wondering if it might make sense to add an "is_subscript" attribute to expression nodes.

That way the Tuple vs ExtSlice and Expr vs Index distinctions would be preserved, but the representation of those distinctions would change in a way that meant that most consuming code didn't need to care that the distinctions existed (ExtSlice would become a Tuple with is_subscript set to True, and similarly, Index would become Expr instances with is_subscript set to True).

It's been so long since I changed the AST, though, that I'm not sure how painful adding that attribute would be.
msg363593 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-07 13:35
It was added to produce nicer output.


>>> print(ast.unparse(ast.parse('a[i, j]')))

a[(i, j)]

With PR 9605:

>>> print(ast.unparse(ast.parse('a[i, j]')))

a[i, j]

The current code is not consistent with outputting parenthesis:

>>> print(ast.unparse(ast.parse('a[i:j, k]')))

a[i:j, k]

It also produces the same output for a[i:j] and a[i:j,] which have different AST and compiled to different bytecode (this is a bug).

>>> print(ast.unparse(ast.parse('a[i:j,]')))

msg363597 - (view) Author: Vedran Čačić (veky) * Date: 2020-03-07 14:57
Agree with the idea, but think the name is too narrow. How about `parethesized`? There are many contexts where parentheses look weird and can be omitted (e.g. after return statement), although subscripts are currently the only place where they are an outright error.
msg363598 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python triager) Date: 2020-03-07 15:00
Yes, there is an already PR about that the bug.

Related PR:
msg363648 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-08 10:51
Sorry, I did not know about your PR Batuhan and fixed this bug in issue39889.

After fixing the bug in the current code PR 9605 only simplifies the code. So in this case the new code is not more complex than correctly written old code.
msg363650 - (view) Author: Batuhan Taskaya (BTaskaya) * (Python triager) Date: 2020-03-08 10:59
> Sorry, I did not know about your PR Batuhan and fixed this bug in issue39889.

No problem.
msg363756 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-03-09 17:51
I'm going to review the actual code next.

Regarding the omission of parentheses in various contexts, I am all for that, but I consider it a separate issue (as it only pertains to ast.unparse()). The fix in should go in regardless.
msg363829 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-10 16:52
New changeset 13d52c268699f199a8e917a0f1dc4c51e5346c42 by Serhiy Storchaka in branch 'master':
bpo-34822: Simplify AST for subscription. (GH-9605)
Date User Action Args
2020-03-10 16:54:21serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-03-10 16:52:55serhiy.storchakasetmessages: + msg363829
2020-03-09 17:51:40gvanrossumsetmessages: + msg363756
2020-03-08 10:59:04BTaskayasetmessages: + msg363650
2020-03-08 10:51:36serhiy.storchakasetmessages: + msg363648
2020-03-07 15:00:33BTaskayasetmessages: + msg363598
2020-03-07 14:57:40vekysetmessages: + msg363597
2020-03-07 13:35:00serhiy.storchakasetmessages: + msg363593
2020-03-07 13:12:05ncoghlansetmessages: + msg363592
2020-03-07 06:40:17gvanrossumsetnosy: + gvanrossum
messages: + msg363583
2020-02-18 20:33:25serhiy.storchakasetmessages: + msg362234
2020-02-18 16:13:04vekysetnosy: + veky
messages: + msg362224
2020-02-17 09:40:20vstinnersetnosy: - vstinner
2020-02-15 12:50:56serhiy.storchakasetmessages: + msg362016
versions: + Python 3.9, - Python 3.8
2020-02-14 20:49:46BTaskayasetnosy: + pablogsal, BTaskaya
messages: + msg361997
2018-10-03 17:07:21gvanrossumsetnosy: - gvanrossum
2018-10-03 16:33:45yselivanovsetnosy: - yselivanov
2018-10-03 16:19:07naschemesetmessages: + msg326991
2018-09-27 17:11:20serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request9002
2018-09-27 16:17:40serhiy.storchakacreate