msg312482 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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...
|
msg362121 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2020-02-17 09:03 |
New changeset 85a2eef473a2c9ed3ab9c6ee339891fe99adbbc9 by Serhiy Storchaka in branch 'master':
bpo-32892: Update the documentation for handling constants in AST. (GH-18514)
https://github.com/python/cpython/commit/85a2eef473a2c9ed3ab9c6ee339891fe99adbbc9
|
msg362123 - (view) |
Author: miss-islington (miss-islington) |
Date: 2020-02-17 09:09 |
New changeset 988aeba94bf1dab81dd52fc7b02dca7a57ea8ba0 by Miss Islington (bot) in branch '3.8':
bpo-32892: Update the documentation for handling constants in AST. (GH-18514)
https://github.com/python/cpython/commit/988aeba94bf1dab81dd52fc7b02dca7a57ea8ba0
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:58 | admin | set | github: 77073 |
2020-02-17 09:09:53 | miss-islington | set | nosy:
+ miss-islington messages:
+ msg362123
|
2020-02-17 09:03:52 | miss-islington | set | pull_requests:
+ pull_request17910 |
2020-02-17 09:03:29 | serhiy.storchaka | set | messages:
+ msg362121 |
2020-02-15 12:35:04 | serhiy.storchaka | set | pull_requests:
+ pull_request17891 |
2019-05-14 16:52:56 | Anthony Sottile | set | nosy:
+ Anthony Sottile messages:
+ msg342486
|
2019-05-14 14:40:59 | mbussonn | set | nosy:
+ mbussonn
|
2019-04-22 17:31:02 | hroncok | set | nosy:
+ hroncok messages:
+ msg340664
|
2018-11-21 07:58:48 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages:
+ msg330179
stage: patch review -> resolved |
2018-10-28 15:08:41 | gvanrossum | set | nosy:
- gvanrossum
|
2018-10-28 11:43:07 | serhiy.storchaka | set | messages:
+ msg328685 |
2018-10-17 18:10:39 | serhiy.storchaka | set | pull_requests:
+ pull_request9285 |
2018-10-17 15:07:41 | vstinner | set | messages:
+ msg327903 |
2018-10-17 15:00:44 | vstinner | set | messages:
+ msg327902 |
2018-10-02 10:29:06 | serhiy.storchaka | set | messages:
+ msg326873 |
2018-10-01 08:21:02 | jwilk | set | nosy:
+ jwilk messages:
+ msg326767
|
2018-09-30 07:31:25 | serhiy.storchaka | set | messages:
+ msg326712 |
2018-09-30 07:14:08 | njs | set | nosy:
+ njs messages:
+ msg326711
|
2018-09-27 15:29:36 | vstinner | set | messages:
+ msg326568 |
2018-09-27 14:42:43 | serhiy.storchaka | set | messages:
+ msg326566 |
2018-09-27 14:39:15 | serhiy.storchaka | set | messages:
+ msg326565 |
2018-09-24 22:01:33 | nascheme | set | messages:
+ msg326295 |
2018-09-24 08:59:56 | vstinner | set | messages:
+ msg326209 |
2018-09-23 03:39:39 | thautwarm | set | messages:
+ msg326130 |
2018-09-22 13:25:40 | serhiy.storchaka | set | messages:
+ msg326087 |
2018-09-22 05:40:07 | thautwarm | set | nosy:
+ thautwarm messages:
+ msg326078
|
2018-09-21 21:15:17 | vstinner | set | messages:
+ msg326043 |
2018-09-21 20:59:34 | serhiy.storchaka | set | messages:
+ msg326041 |
2018-09-21 20:16:53 | vstinner | set | messages:
+ msg326030 |
2018-09-21 16:40:09 | nascheme | set | nosy:
+ nascheme messages:
+ msg326006
|
2018-09-20 16:53:21 | gvanrossum | set | messages:
+ msg325904 |
2018-09-20 15:08:30 | vstinner | set | messages:
+ msg325884 |
2018-09-20 14:53:15 | serhiy.storchaka | set | messages:
+ msg325882 |
2018-09-20 14:43:58 | serhiy.storchaka | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request8860 |
2018-07-03 17:56:15 | serhiy.storchaka | link | issue32888 dependencies |
2018-02-21 11:31:41 | ncoghlan | set | messages:
+ msg312491 |
2018-02-21 11:08:43 | serhiy.storchaka | set | messages:
+ msg312490 |
2018-02-21 10:32:55 | ncoghlan | set | messages:
+ msg312487 |
2018-02-21 09:16:37 | serhiy.storchaka | create | |