Skip to content

bpo-34822: Simplify AST for subscription. #9605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Mar 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions Doc/library/ast.rst
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,19 @@ Node classes
node = ast.UnaryOp(ast.USub(), ast.Constant(5, lineno=0, col_offset=0),
lineno=0, col_offset=0)

.. versionchanged:: 3.8

Class :class:`ast.Constant` is now used for all constants.
Simple indices are represented by their value, extended slices are
represented as tuples.

.. deprecated:: 3.8

Class :class:`ast.Constant` is now used for all constants. Old classes
:class:`ast.Num`, :class:`ast.Str`, :class:`ast.Bytes`,
:class:`ast.NameConstant` and :class:`ast.Ellipsis` are still available,
but they will be removed in future Python releases.
Old classes :class:`ast.Num`, :class:`ast.Str`, :class:`ast.Bytes`,
:class:`ast.NameConstant`, :class:`ast.Ellipsis`, :class:`ast.Index`
and :class:`ast.ExtSlice` are still available, but they will be removed
in future Python releases. In meanwhile, instantiating them will return
an instance of different class.


.. _abstract-grammar:
Expand Down Expand Up @@ -246,7 +253,7 @@ and classes for traversing abstract syntax trees:
def visit_Name(self, node):
return copy_location(Subscript(
value=Name(id='data', ctx=Load()),
slice=Index(value=Constant(value=node.id)),
slice=Constant(value=node.id),
ctx=node.ctx
), node)

Expand Down
19 changes: 19 additions & 0 deletions Doc/whatsnew/3.8.rst
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,12 @@ Deprecated
versions. :class:`~ast.Constant` should be used instead.
(Contributed by Serhiy Storchaka in :issue:`32892`.)

* :mod:`ast` classes ``Index`` and ``ExtSlice`` are considered deprecated
and will be removed in future Python versions. Just ``value`` should be
used instead of ``Index(value)``. ``Tuple(slices, Load())`` should be
used instead of ``ExtSlice(slices)``.
(Contributed by Serhiy Storchaka in :issue:`32892`.)


Removed
=======
Expand Down Expand Up @@ -370,6 +376,19 @@ Changes in the Python API
external entities by default.
(Contributed by Christian Heimes in :issue:`17239`.)

* Simplified AST for literals and subscription.

- All constants will be represented as :class:`ast.Constant` instances.
Instantiating old classes ``Num``, ``Str``, ``Bytes``, ``NameConstant``
and ``Ellipsis`` will return an instance of ``Constant``.
(Contributed by Serhiy Storchaka in :issue:`32892`.)

- Simple indices will be represented by their value, extended slices will
be represented as tuples. ``Index(value)`` will return a ``value`` itself,
``ExtSlice(slices)`` will return ``Tuple(slices, Load())``.
(Contributed by Serhiy Storchaka in :issue:`34822`.)


CPython bytecode changes
------------------------

Expand Down
36 changes: 8 additions & 28 deletions Include/Python-ast.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions Lib/ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,3 +384,11 @@ def __new__(cls, *args, **kwargs):
NameConstant: (type(None), bool),
Ellipsis: (type(...),),
}

class Index(AST):
def __new__(cls, value, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there ever any additional positional args? (I presume kwargs could be lineno etc.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No!

return value

class ExtSlice(AST):
def __new__(cls, dims=(), *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make dims a mandatory argument, like value for Index?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return Tuple(list(dims), Load(), *args, **kwargs)
18 changes: 11 additions & 7 deletions Lib/test/test_ast.py
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ def test_base_classes(self):
def test_field_attr_existence(self):
for name, item in ast.__dict__.items():
if isinstance(item, type) and name != 'AST' and name[0].isupper():
if name == 'Index':
# Index(value) just returns value now.
# The argument is required.
continue
x = item()
if isinstance(x, ast.AST):
self.assertEqual(type(x._fields), tuple)
Expand Down Expand Up @@ -1056,21 +1060,21 @@ def test_attribute(self):
self.expr(attr, "must have Load context")

def test_subscript(self):
sub = ast.Subscript(ast.Name("x", ast.Store()), ast.Index(ast.Num(3)),
sub = ast.Subscript(ast.Name("x", ast.Store()), ast.Num(3),
ast.Load())
self.expr(sub, "must have Load context")
x = ast.Name("x", ast.Load())
sub = ast.Subscript(x, ast.Index(ast.Name("y", ast.Store())),
sub = ast.Subscript(x, ast.Name("y", ast.Store()),
ast.Load())
self.expr(sub, "must have Load context")
s = ast.Name("x", ast.Store())
for args in (s, None, None), (None, s, None), (None, None, s):
sl = ast.Slice(*args)
self.expr(ast.Subscript(x, sl, ast.Load()),
"must have Load context")
sl = ast.ExtSlice([])
self.expr(ast.Subscript(x, sl, ast.Load()), "empty dims on ExtSlice")
sl = ast.ExtSlice([ast.Index(s)])
sl = ast.Tuple([], ast.Load())
self.expr(ast.Subscript(x, sl, ast.Load()))
sl = ast.Tuple([s], ast.Load())
self.expr(ast.Subscript(x, sl, ast.Load()), "must have Load context")

def test_starred(self):
Expand Down Expand Up @@ -1306,13 +1310,13 @@ def main():
('Expression', ('Constant', (1, 0), 10)),
('Expression', ('Constant', (1, 0), 'string')),
('Expression', ('Attribute', (1, 0), ('Name', (1, 0), 'a', ('Load',)), 'b', ('Load',))),
('Expression', ('Subscript', (1, 0), ('Name', (1, 0), 'a', ('Load',)), ('Slice', ('Name', (1, 2), 'b', ('Load',)), ('Name', (1, 4), 'c', ('Load',)), None), ('Load',))),
('Expression', ('Subscript', (1, 0), ('Name', (1, 0), 'a', ('Load',)), ('Slice', (1, 2), ('Name', (1, 2), 'b', ('Load',)), ('Name', (1, 4), 'c', ('Load',)), None), ('Load',))),
('Expression', ('Name', (1, 0), 'v', ('Load',))),
('Expression', ('List', (1, 0), [('Constant', (1, 1), 1), ('Constant', (1, 3), 2), ('Constant', (1, 5), 3)], ('Load',))),
('Expression', ('List', (1, 0), [], ('Load',))),
('Expression', ('Tuple', (1, 0), [('Constant', (1, 0), 1), ('Constant', (1, 2), 2), ('Constant', (1, 4), 3)], ('Load',))),
('Expression', ('Tuple', (1, 1), [('Constant', (1, 1), 1), ('Constant', (1, 3), 2), ('Constant', (1, 5), 3)], ('Load',))),
('Expression', ('Tuple', (1, 0), [], ('Load',))),
('Expression', ('Call', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Name', (1, 0), 'a', ('Load',)), 'b', ('Load',)), 'c', ('Load',)), 'd', ('Load',)), [('Subscript', (1, 8), ('Attribute', (1, 8), ('Name', (1, 8), 'a', ('Load',)), 'b', ('Load',)), ('Slice', ('Constant', (1, 12), 1), ('Constant', (1, 14), 2), None), ('Load',))], [])),
('Expression', ('Call', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Attribute', (1, 0), ('Name', (1, 0), 'a', ('Load',)), 'b', ('Load',)), 'c', ('Load',)), 'd', ('Load',)), [('Subscript', (1, 8), ('Attribute', (1, 8), ('Name', (1, 8), 'a', ('Load',)), 'b', ('Load',)), ('Slice', (1, 12), ('Constant', (1, 12), 1), ('Constant', (1, 14), 2), None), ('Load',))], [])),
]
main()
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
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())``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

9 changes: 4 additions & 5 deletions Parser/Python.asdl
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,20 @@ module Python

-- the following expression can appear in assignment context
| Attribute(expr value, identifier attr, expr_context ctx)
| Subscript(expr value, slice slice, expr_context ctx)
| Subscript(expr value, expr slice, expr_context ctx)
| Starred(expr value, expr_context ctx)
| Name(identifier id, expr_context ctx)
| List(expr* elts, expr_context ctx)
| Tuple(expr* elts, expr_context ctx)

-- can appear only in Subscript
| Slice(expr? lower, expr? upper, expr? step)

-- col_offset is the byte offset in the utf8 string the parser uses
attributes (int lineno, int col_offset)

expr_context = Load | Store | Del | AugLoad | AugStore | Param

slice = Slice(expr? lower, expr? upper, expr? step)
| ExtSlice(slice* dims)
| Index(expr value)

boolop = And | Or

operator = Add | Sub | Mult | MatMult | Div | Mod | Pow | LShift
Expand Down
Loading