[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 13d52c268699f199a8e917a0f1dc4c51e5346c42 by Serhiy Storchaka in 
branch 'master':
bpo-34822: Simplify AST for subscription. (GH-9605)
https://github.com/python/cpython/commit/13d52c268699f199a8e917a0f1dc4c51e5346c42


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Guido van Rossum]

Guido van Rossum  added the comment:

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 https://github.com/python/cpython/pull/17892 should go in regardless.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Batuhan Taskaya]

Batuhan Taskaya  added the comment:

> Sorry, I did not know about your PR Batuhan and fixed this bug in issue39889.

No problem.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Batuhan Taskaya]

Batuhan Taskaya  added the comment:

Yes, there is an already PR about that the bug.

Related PR: https://github.com/python/cpython/pull/17892

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Vedran Čačić]

Vedran Čačić  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

It was added to produce nicer output.

Currently:

>>> 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,]')))

a[i:j]

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Guido van Rossum]

Guido van Rossum  added the comment:

Haven’t looked at the code but I welcome the simplification.

--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

No, this PR does not change the Python syntax. It only changes the AST 
representation.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Vedran Čačić]

Vedran Čačić  added the comment:

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]asa[(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?

--
nosy: +veky

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--
versions: +Python 3.9 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Batuhan Taskaya]

Batuhan Taskaya  added the comment:

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.

--
nosy: +BTaskaya, pablogsal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Guido van Rossum]

Change by Guido van Rossum :


--
nosy:  -gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Yury Selivanov]

Change by Yury Selivanov :


--
nosy:  -yselivanov

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Neil Schemenauer]

Neil Schemenauer  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
keywords: +patch
pull_requests: +9002
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34822] Simplify AST for slices

[Serhiy Storchaka]

New submission from Serhiy Storchaka :

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:

Subscript(
Name('d', Load()),
ExtSlice(
[
Index(Name('a', Load())),
Index(Constant(Ellipsis)),
Slice(Name('b', Load()), Name('c', Load()), None)
]
),
Load()
)

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

Subscript(
Name('d', Load()),
Index(
Tuple(
[
Name('a', Load()),
Constant(Ellipsis),
Name('b', Load())
],
Load()
)
),
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:

Subscript(
Name('d', Load()),
Tuple(
[
Name('a', Load()),
Constant(Ellipsis),
Slice(Name('b', Load()), Name('c', Load()), None)
],
Load()
),
Load()
)

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/unparse.py) 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.

--
components: Interpreter Core, Library (Lib)
messages: 326570
nosy: benjamin.peterson, brett.cannon, gvanrossum, nascheme, ncoghlan, 
serhiy.storchaka, thautwarm, vstinner, yselivanov
priority: normal
severity: normal
status: open
title: Simplify AST for slices
type: enhancement
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com