classification
Title: Remove specific constant AST types in favor of ast.Constant
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Anthony Sottile, benjamin.peterson, brett.cannon, hroncok, jwilk, mbussonn, nascheme, ncoghlan, njs, serhiy.storchaka, thautwarm, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2018-02-21 09:16 by serhiy.storchaka, last changed 2019-05-14 16:52 by Anthony Sottile. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9445 merged serhiy.storchaka, 2018-09-20 14:43
PR 9934 merged serhiy.storchaka, 2018-10-17 18:10
Messages (29)
msg312482 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-21 09:16
Currently different literals are represented by different types in AST:

Num -- for int, float and complex
Str -- for str
Bytes -- for bytes
Ellipsis -- for Ellipsis
NameConstant -- for True, False and None

And Constant (added in 3.6, issue26146) can represent any immutable value, including tuples and frozensets of constants. Instances of Constant are not created by the AST parser, they are created by the builtin AST optimizer and can be created manually.

These AST types don't have one-to-one correspondence with Python types, since Num represents three numeric types, NameConstant represents bool and NoneType, and any constant type can be represented as Constant.

I propose to get rid of Num, Str, Bytes, Ellipsis and NameConstant and always use Constant. This will simplify the code which currently needs to repeat virtually identical code for all types.

I have almost ready patch, the only question is whether it is worth to keep deprecated aliases Num, Str, Bytes, Ellipsis and NameConstant.
msg312487 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-02-21 10:32
As pure aliases, those wouldn't necessarily be that useful, as aside from NameConstant, the field names are different (Num.n, Str.s, Bytes.s).

I do wonder whether it might be worth keeping "NameConstant", though, and use that for Ellipsis as well, so the singletons all use NameConstant, while regular constants use Constant.
msg312490 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-21 11:08
Right, they shouldn't be just aliases, but Constant subclasses with __new__ which return Constant and __instancecheck__ which checks the type of the value. And the Constant class should have writable properties n and s. All these operations should emit a deprecation warning at runtime. Even this doesn't preserve perfect compatibility. issubclass(type(node), Num) will not work, and compile(Num('123')) will raise en exception in the Num constructor instead of compile().
msg312491 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2018-02-21 11:31
Ah, right - in that case, I think the subclasses would be worthwhile, as they shouldn't be too much hassle to maintain for a release, and will provide a more graceful migration for folks doing their own AST processing and generation.
msg325882 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-20 14:53
PR 9445 is an implementation of this idea. For simplicity and for reducing affect on tests no deprecation warning is raised, and there is no validation of the type of argument for old classes. Num('123') will just return Constant('123') without errors and warnings. isinstance(Constant('123'), Num) will return False, and isinstance(Constant('123'), Str) will return True.
msg325884 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-20 15:08
+1. I wanted to propose this change while I was working on FAT Python, but I waited until I "completed" this project. Sadly, I abandonned the FAT Python. And I wasn't brave enough to propose to break the AST.
msg325904 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-09-20 16:53
I don't feel confident reviewing the code, but I'm okay with the change. Can you describe what usages of the old API will continue to work, and what part will break? (It would seem that code that creates a tree using e.g. Num(42) will still work, but code inspecting a tree won't see any Num instances, only Constant instances.)
msg326006 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-09-21 16:40
As someone who does AST manipulation (Quixote PTL template support), I'm interested in this patch. I think what is proposed sounds like a good change.  I.e. having the extra node types is not useful and makes the compiler harder to understand.  Providing subclasses for backwards compatibility would be nice.  I will test my Quixote package with Serhiy's patch.
msg326030 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-21 20:16
Hum, I'm not sure that I understood correctly: does "isinstance(node, ast.Num)" still work with the patch? If yes, I see no reason to not merge the change ;-)
msg326041 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-21 20:59
> Can you describe what usages of the old API will continue to work, and what part will break?

Very good question!

What will continue to work:

* Instantiating. Both `Num(42)` and `Num(n=42)` will continue to work, but they will return an instance of Constant: Constant(value=42).

* Attribute access. Constant will get attributes "n" and "s" as aliases to the attribute "value". So Num(42).n will continue to return 42. 

* isinstance() check. Although `isinstance(Num(42), Num)` will continue to return True, and `isinstance(Str('42'), Num)` will continue to return False. Also `isinstance(Constant(42), Num)` will return True and `isinstance(Constant('42'), Num)` will return False.

* Subclassing. Subclasses of these classes will continue to behave as normal classes. Instantiating them will return instances of that classes, not Constant, and the isinstance() check will not have any magic.

What will no longer work:

* issubclass() check and exact type check. `issubclass(type(node), Num)` and `type(node) is Num` will return False for node = Num(42). But seems isinstance() is a more common way of checking the type of a node.

* If the user of NodeVisitor implemented visit_Num(), but not implemented visit_Constant(), his handler will not be triggered. This can be easily fixed by implementing visit_Constant(). Constant was introduced in 3.6.

* isinstance() check for underinitialized node. `isinstance(Num(), Num)` will return False.
msg326043 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-21 21:15
> * If the user of NodeVisitor implemented visit_Num(), but not implemented visit_Constant(), his handler will not be triggered. This can be easily fixed by implementing visit_Constant(). Constant was introduced in 3.6.

IHMO that's an acceptable tradeoff. We should just make sure that it's properly documented in the Porting section of What's New in Python 3.8.

> * issubclass() check and exact type check. `issubclass(type(node), Num)` and `type(node) is Num` will return False for node = Num(42). But seems isinstance() is a more common way of checking the type of a node.

In the AST code that I read, isinstance() was the most popular way to check the type of a node. I don't recall any AST code using issubclass.

> isinstance() check for underinitialized node. `isinstance(Num(), Num)` will return False.

I don't think that it's an issue. 

--

In term of optimization, I'm curious, do you know if your PR optimize more cases than previously? Or do you expect futher optimizations later thanks to this change?
msg326078 - (view) Author: thautwarm (thautwarm) * Date: 2018-09-22 05:40
I'm in favor of this idea for prospective extensions that could be implemented through this brand-new ast.Constant.  

Actually I used to expect a more general ast.Constant when I was manipulating ASTs half a year ago, at that time my job is to make staging functions that take some user-defined types as constants(in these scenes, these types are definitely immutable and permanent) to gain performance improvements and avoid redundant allocations, and what I exactly wanted is such an ast.Constant.
msg326087 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-22 13:25
thautwarm, ast.Constant is not new. You can use it since 3.8. What is new in this issue -- ast.parse() will produce ast.Constant instead of ast.Num, ast.Str, etc.
msg326130 - (view) Author: thautwarm (thautwarm) * Date: 2018-09-23 03:39
Thank you, Serhiy. 

I learned python ast through ast.parse and pretty print, which prevented me from knowing this useful one.

In fact, I wonder if we could support more species of constant values accepted by ast.Constant?
msg326209 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-24 08:59
Serhiy Storchaka: "ast.Constant is not new. You can use it since 3.8."

Since Python 3.6 ;-)
msg326295 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2018-09-24 22:01
I've checked Quixote PTL module support.  I will have to make some changes to support the removal of Str and other node types.  E.g. I have to change visit_Str to visit_Constant.  The fixes are pretty easy though and I think it is reasonable that this kind of AST manipulation is dependent on Python version and may get broken between releases.
msg326565 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-27 14:39
> Since Python 3.6 ;-)

Thank you for correction Victor. It is what I meant. Why I had written 3.8? :-?
msg326566 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-27 14:42
New changeset 3f22811fef73aec848d961593d95fa877f77ecbf by Serhiy Storchaka in branch 'master':
bpo-32892: Use ast.Constant instead of specific constant AST types. (GH-9445)
https://github.com/python/cpython/commit/3f22811fef73aec848d961593d95fa877f77ecbf
msg326568 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-27 15:29
Thank you Serhiy :-D I really love ast.Constant, it simplify many things when writing code modifying the AST.
msg326711 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-09-30 07:14
As a point of information, this did in fact break pylint/astroid: https://github.com/PyCQA/astroid/issues/617
msg326712 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-30 07:31
Yesterday I submitted a PR that should fix astroid in Python 3.8: https://github.com/PyCQA/astroid/issues/616
msg326767 - (view) Author: Jakub Wilk (jwilk) Date: 2018-10-01 08:21
Also broke pyflakes: https://github.com/PyCQA/pyflakes/issues/367
msg326873 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-02 10:29
Submitted a fix for Pyflakes: https://github.com/PyCQA/pyflakes/pull/369. It had more problems due to changes in line and column numbers in SyntaxError.
msg327902 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-17 15:00
This change broke two templating engines:

* Genshi: https://github.com/python/performance/issues/46
* Chameleon: https://github.com/python/performance/issues/47
msg327903 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-10-17 15:07
> This change broke two templating engines:
> * Genshi: https://github.com/python/performance/issues/46

upstream issue: https://genshi.edgewall.org/ticket/612

> * Chameleon: https://github.com/python/performance/issues/47

upstream issue: https://github.com/malthe/chameleon/issues/272
msg328685 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-28 11:43
New changeset 6015cc50bc38b9e920ce4986ee10658eaa14f561 by Serhiy Storchaka in branch 'master':
bpo-32892: Support subclasses of base types in isinstance checks for AST constants. (GH-9934)
https://github.com/python/cpython/commit/6015cc50bc38b9e920ce4986ee10658eaa14f561
msg330179 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-21 07:58
Chameleon has been fixed. Genshi needs more work (seems it has issues with 3.7 too). There are other third-party projects broken after this change, but it is not hard to fix them.

Closed this issue. Open new issues for new problems.
msg340664 - (view) Author: Miro Hrončok (hroncok) * Date: 2019-04-22 17:31
Mako (now fixed) https://github.com/sqlalchemy/mako/issues/287
msg342486 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-14 16:52
hitting this in https://bugs.python.org/issue36917?

Is the simplification here really worth the breaking change to consumers?

I now have to write something that's essentially this to work around this which feels more like the complexity has just been pushed to users instead of the stdlib:


def visit_Constant(self, node):
    if isinstance(node.value, str):
        self.visit_Str(node)
    elif isinstance(node.value, bytes):
        self.visit_Bytes(node)
    elif node.value in {True, False}:
        self.visit_NameConstant(node)
    elif node.value is Ellipsis:
        self.visit_Ellipsis(node)
    elif isinstance(node.value, (int, float)):
        self.visit_Num(node)
    else:
        # etc...
History
Date User Action Args
2019-05-14 16:52:56Anthony Sottilesetnosy: + Anthony Sottile
messages: + msg342486
2019-05-14 14:40:59mbussonnsetnosy: + mbussonn
2019-04-22 17:31:02hroncoksetnosy: + hroncok
messages: + msg340664
2018-11-21 07:58:48serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg330179

stage: patch review -> resolved
2018-10-28 15:08:41gvanrossumsetnosy: - gvanrossum
2018-10-28 11:43:07serhiy.storchakasetmessages: + msg328685
2018-10-17 18:10:39serhiy.storchakasetpull_requests: + pull_request9285
2018-10-17 15:07:41vstinnersetmessages: + msg327903
2018-10-17 15:00:44vstinnersetmessages: + msg327902
2018-10-02 10:29:06serhiy.storchakasetmessages: + msg326873
2018-10-01 08:21:02jwilksetnosy: + jwilk
messages: + msg326767
2018-09-30 07:31:25serhiy.storchakasetmessages: + msg326712
2018-09-30 07:14:08njssetnosy: + njs
messages: + msg326711
2018-09-27 15:29:36vstinnersetmessages: + msg326568
2018-09-27 14:42:43serhiy.storchakasetmessages: + msg326566
2018-09-27 14:39:15serhiy.storchakasetmessages: + msg326565
2018-09-24 22:01:33naschemesetmessages: + msg326295
2018-09-24 08:59:56vstinnersetmessages: + msg326209
2018-09-23 03:39:39thautwarmsetmessages: + msg326130
2018-09-22 13:25:40serhiy.storchakasetmessages: + msg326087
2018-09-22 05:40:07thautwarmsetnosy: + thautwarm
messages: + msg326078
2018-09-21 21:15:17vstinnersetmessages: + msg326043
2018-09-21 20:59:34serhiy.storchakasetmessages: + msg326041
2018-09-21 20:16:53vstinnersetmessages: + msg326030
2018-09-21 16:40:09naschemesetnosy: + nascheme
messages: + msg326006
2018-09-20 16:53:21gvanrossumsetmessages: + msg325904
2018-09-20 15:08:30vstinnersetmessages: + msg325884
2018-09-20 14:53:15serhiy.storchakasetmessages: + msg325882
2018-09-20 14:43:58serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request8860
2018-07-03 17:56:15serhiy.storchakalinkissue32888 dependencies
2018-02-21 11:31:41ncoghlansetmessages: + msg312491
2018-02-21 11:08:43serhiy.storchakasetmessages: + msg312490
2018-02-21 10:32:55ncoghlansetmessages: + msg312487
2018-02-21 09:16:37serhiy.storchakacreate