bpo-34822: Simplify AST for subscription. #9605
Conversation
* Remove the slice type. * Make Slice a kind of the expr type instead of the slice type. * Replace ExtSlice(slices) with Tuple(slices, Load()). * Replace Index(value) with a value itself. All non-terminal nodes in AST for expressions will be now of the expr type.
|
This enhancement is quite exciting, in fact, I've found the same problem at the midnight of yesterday, and it looks like a surprise to me that you made this PR now. I'm working at an implementation of However, for many projects depends on current AST structures, the changes might cause lots of incompatibilities. For instance, Hy project uses AFAICS, we could add some facilities to ast module to guarantee compatibility to avoid the breakage of some 3-party packages that take advantage of python ast. |
|
Hy project currently doesn't use PonyORM doesn't use Any project that just creates AST using functional style ( |
Doc/library/ast.rst
Outdated
|
|
||
| Old classes :class:`ast.Index` and :class:`ast.ExtSlice` are still | ||
| available, but they will be removed in future Python releases. | ||
| In the meanwhile, instantiating them will return an instance of |
There was a problem hiding this comment.
I Googled this expression, and there seems to be a preference for either "In the meantime" or "Meanwhile" over "In the meantime". Up to you though, it's not wrong.
Doc/library/ast.rst
Outdated
| ctx=Load())) | ||
|
|
||
|
|
||
| .. class:: Slice(lower, upper, step) | ||
|
|
||
| Regular slicing (on the form x:y). | ||
| Regular slicing (on the form ``lower:upper`` or ``lower:upper:step``). | ||
| Can occur only in the *slice* field of :class:`Subscript`. |
There was a problem hiding this comment.
Maybe clarify that it can occur inside a Tuple that's directly inside a Subscript's slice field?
There was a problem hiding this comment.
Done (not sure about correctness of my wording).
Lib/ast.py
Outdated
| @@ -557,6 +558,15 @@ def __new__(cls, *args, **kwargs): | |||
| type(...): 'Ellipsis', | |||
| } | |||
|
|
|||
| class Index(AST): | |||
| def __new__(cls, value, *args, **kwargs): | |||
There was a problem hiding this comment.
Are there ever any additional positional args? (I presume kwargs could be lineno etc.)
Lib/ast.py
Outdated
| return value | ||
|
|
||
| class ExtSlice(AST): | ||
| def __new__(cls, dims=(), *args, **kwargs): |
There was a problem hiding this comment.
Why not make dims a mandatory argument, like value for Index?
There was a problem hiding this comment.
Index without value cannot work. This change breaks the following code
node = Index()
node.value = Constant(1)
but we cannot keep it working. We should have something to return from Index().
On other side, ExtSlice with empty dims is invalid, and the following legal code
node = ExtSlice()
node.dims.append(Ellipsis())
will be broken because Tuple has the elts filed instead of dims. We can add an alias dims in Tuple to make the above example working. Should we? If not, than making dims a required parameter will not break anything that is not broken.
There was a problem hiding this comment.
P.S. There is a test (test_field_attr_existence) which tests that all node classes are callable without arguments (Index is an exception now). Should we make ExtSlice an exception too?
Lib/ast.py
Outdated
| if (isinstance(node.slice, Index) | ||
| and isinstance(node.slice.value, Tuple) | ||
| and node.slice.value.elts): | ||
| if len(node.slice.value.elts) == 1: | ||
| elt = node.slice.value.elts[0] | ||
| if isinstance(node.slice, Tuple) and node.slice.elts: | ||
| if len(node.slice.elts) == 1: | ||
| elt = node.slice.elts[0] | ||
| self.traverse(elt) | ||
| self.write(",") | ||
| else: | ||
| self.interleave(lambda: self.write(", "), self.traverse, node.slice.value.elts) | ||
| self.interleave(lambda: self.write(", "), self.traverse, node.slice.elts) |
There was a problem hiding this comment.
I'm guessing you'll get a merge conflict here if GH-17892 is merged first (which I think it should be).
| Simplified AST for subscription. Simple indices will be represented by their | ||
| value, extended slices will be represented as tuples. :mod:`ast` classes | ||
| ``Index`` and ``ExtSlice`` are considered deprecated and will be removed in | ||
| future Python versions. In meanwhile, ``Index(value)`` will return a ``value`` | ||
| itself, ``ExtSlice(slices)`` will return ``Tuple(slices, Load())``. |
There was a problem hiding this comment.
Maybe replace "will be" with "are now"? Also change "will return" into "now returns". (But "will be removed" must stay, since it is truly in the future.)
|
Thank you for your review @gvanrossum! |
|
You're welcome. Thanks for caring so much about Python. |
The structure of the AST for subscripts was changed in python/cpython#9605 Fixes #8627
The structure of the AST for subscripts was changed in python/cpython#9605. This also fixes some systemic issues with the tests under Python 3.9, but not all. Fixes #8627
* Remove the slice type. * Make Slice a kind of the expr type instead of the slice type. * Replace ExtSlice(slices) with Tuple(slices, Load()). * Replace Index(value) with a value itself. All non-terminal nodes in AST for expressions are now of the expr type.
All non-terminal nodes in AST for expressions will be now of the expr type.
https://bugs.python.org/issue34822