classification
Title: ast.NodeVisitor no longer calls visit_Str
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Anthony Sottile, BTaskaya, Tyler Wince, dhilst, mbussonn, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2019-05-14 14:21 by Anthony Sottile, last changed 2019-08-26 12:14 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 15490 merged serhiy.storchaka, 2019-08-25 12:00
PR 15509 merged miss-islington, 2019-08-26 07:13
PR 15511 merged serhiy.storchaka, 2019-08-26 07:53
Messages (17)
msg342463 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-14 14:21
More fallout from the Constant change in https://bugs.python.org/issue32892

minimal reproduction:


import ast


class V(ast.NodeVisitor):
    def visit_Str(self, node):
        print(node.s)


def main():
    V().visit(ast.parse('x = "hi"'))


if __name__ == '__main__':
    exit(main())


$ python3.7 t.py
hi
$ python3.8 t.py
$ python3.8 --version --version
Python 3.8.0a4 (default, May  8 2019, 01:43:53) 
[GCC 7.4.0]
msg342474 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-14 15:26
wrong bpo, this is the right one: https://bugs.python.org/issue32892
msg342494 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2019-05-14 18:12
Would it be useful to add a default implementation of `visit_Constant(self, node)` on NodeVisitor that go through all the isinstance() check and call the appropriate backward compatible method ? 

We would still have the simplification without having any breakage in backward compatibility. 

That feels like best of both worlds ?
msg342497 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-14 18:30
There would still be a breakage for that if someone was defining py36+ `visit_Constant` (which would clobber the `ast.NodeVisitor.visit_Constant` if we were to add it)
msg342501 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2019-05-14 18:41
> There would still be a breakage for that if someone was defining py36+ `visit_Constant` (which would clobber the `ast.NodeVisitor.visit_Constant` if we were to add it)


Ok, it would still break in some cases, but that would still be a net improvement for codebase that do not override visit_Constant; right ? 

We could also push the patch further if we really want and call explicitly NodeVisitor.visit_Constant before the overridden method in NodeVisitor.visit.

Not advocating for the second part, but minimal breakage and code repetition in libraries when we can.
msg342572 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-15 13:32
spent some more time thinking about this and I think we should strongly consider reverting.  simplification in the core interpreter should not be weighed lightly against complexity and breaking changes for users.

the change is also unfortunate because it reduces the expressivity of the ast

as we discuss shims, I believe the intention is to remove the `Num` / `Str` / etc. layer eventually so it's really just delaying that.

so my vote is to revert, keep the complexity in the compiler and out of user code
msg342578 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2019-05-15 15:44
> so my vote is to revert, keep the complexity in the compiler and out of user code


Playing Devil advocate here, but the complexity depends on what transformations user want to do; with the current state of 3.8, a user that wants to visit all immutable types can do with a single visit_Constant; once 32892 reverted; they would need to implement all of visit_Str,NamedConstant,Num,Bytes, and have it called the same visit_Constant.
msg342583 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-15 16:05
This is not a new case. This is exactly the same problem which has already been fixed in multiple other projects.

Could you show your real code Anthony? In all known opensource examples (astroid, pyflakes, genshi, chameleon, mako) the compatibility fix is as simple as

    def visit_Constant(self, node):
        if node.value is Ellipsis:
            self._write('...')
        else:
            self._write(repr(node.value))

In return, you can remove several methods, such as visit_Str, visit_Num, etc, once you drop the support of Python versions <3.8.

AST is not stable. Virtually in every version the new nodes are added: for asynchronous constructions, for f-strings, etc, and the structure and meaning of existing nodes can be changed.
msg342584 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-15 16:29
The simplest case is just the addition of an `isinstance` check:

https://github.com/asottile/dead/blob/85f5edbb84b5e118beab4be3346a630e41418a02/dead.py#L165-L170

class V(ast.NodeVisitor):
    def visit_Str(self, node):
        ...

    def visit_Constant(self, node):
        if isinstance(node.value, str):
            self.visit_Str(node)
        else:
            self.generic_visit(node)


But I have another case where there's much more complex (sorry this one's private, I'll give the pseudo-code of it though which ends up in a whole mess of slow `isinstance(...)` calls


class V(ast.NodeVisitor):
    def visit_Str(self, node):
        ...

    def visit_Bytes(self, node):
        ...

    def visit_Num(self, node):
        ...

    def visit_Constant(self, node):
        if isinstance(node.value, str):
             return self.visit_Str(node)
        # Annoying special case, bools are ints, before this wouldn't call
        # `visit_Num` because there was a separate `visit_NameConstant` for `True` / `False`
        elif isinstance(node.value, int) and not isinstance(node.value, bool):
            return self.visit_Num(node)
        elif isinstance(node.value, bytes):
            return self.visit_Bytes(node)
        else:
            return self.generic_visit(node)


Whereas the opposite case (where handling all constants the same) is much much easier to simplify the code and not need the `Constant` mess (if required)

class V(ast.NodeVisitor):
    def _visit_constant(self, node):
        # generic handling of constant _if you want it_

    visit_Str = visit_Num = visit_Bytes = visit_NameConstant = visit_Ellipsis = _visit_constant


Maybe I haven't been in the community long enough but this is the first *removal* of the ast I've seen, and it makes all my uses of these functions objectively worse, and none of the cases get simpler (whereas there's an existing easy way to simplify constants already, without breaking the ast)

github search isn't a great measure of this but it's currently showing 10k+ usages of `visit_{Str,Num,Bytes}` that would presumably be broken by this change: https://github.com/search?q=visit_Num+visit_Str+visit_Bytes&type=Code -- backward compatibility is very valuable, I would hope it isn't squandered for an internal cleanup
msg342595 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-05-15 19:34
You can not use the same implementation of the visitor for Num, Str, NameConstant and Ellipsis, because all these classes use different attribute for saving the value (Ellipsis does not have it at all). For the same reasons you can not just pass the argument of visit_Constant() to visit_Str() -- they have different types.

No need to call generic_visit() from visit_Constant() -- Constant nodes should not contain AST nodes.
msg342597 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-05-15 19:38
> You can not use the same implementation of the visitor for Num, Str, NameConstant and Ellipsis, because all these classes use different attribute for saving the value

ah yes, this is true -- maybe the better change would be to just add `@property value` to each of those instead of collapsing the classes? (If that's the actual convenience we're trying to achieve)

> No need to call generic_visit() from visit_Constant() -- Constant nodes should not contain AST nodes.

correct there's no need, but it's a best practice to always call `generic_visit` in all `visit_*` methods, I have a linter I haven't finished up yet that checks this.  And who knows if that'll be true in the future!
msg342827 - (view) Author: Batuhan (BTaskaya) * Date: 2019-05-19 01:00
What about adding visit_Constant to NodeVisitor for at least one relase period and call visit_Str, visit_Num etc?
msg346562 - (view) Author: Tyler Wince (Tyler Wince) Date: 2019-06-25 22:20
Another example of the breaking change here is in the security linter `PyCQA/bandit`. It may be a simple addition of a check in the meta parser for the ast but I would tend to agree with Anthony around whether or not it is worth it. 

Anyone else have any thoughts on this since the conversation ended last month?
msg350496 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-26 07:13
New changeset c3ea41e9bf100a5396b851488c3efe208e5e2179 by Serhiy Storchaka in branch 'master':
bpo-36917: Add default implementation of ast.NodeVisitor.visit_Constant(). (GH-15490)
https://github.com/python/cpython/commit/c3ea41e9bf100a5396b851488c3efe208e5e2179
msg350508 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-26 07:43
New changeset 522a394a72f107ca55701371529b5e4ed20c9fff by Serhiy Storchaka (Miss Islington (bot)) in branch '3.8':
[3.8] bpo-36917: Add default implementation of ast.NodeVisitor.visit_Constant(). (GH-15490) (GH-15509)
https://github.com/python/cpython/commit/522a394a72f107ca55701371529b5e4ed20c9fff
msg350527 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-26 11:49
New changeset c2388622923c81b5f06b0c9a5ce821fc03c624b9 by Serhiy Storchaka in branch '3.7':
bpo-36917: Backport basic test for ast.NodeVisitor. (GH-15511)
https://github.com/python/cpython/commit/c2388622923c81b5f06b0c9a5ce821fc03c624b9
msg350528 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-08-26 12:14
Thank you for your report and discussion Anthony.

Added the default implementation of visit_Constant which calls corresponding visitor for old constant nodes. It emits a deprecation warning (PendingDeprecationWarning in 3.8 and DeprecationWarning in 3.9) as a reminder that you finally will need to implement your own visit_Constant. You can temporary ignore them.

Note that the implementation in the stdlib is not future proof (and it should not, because visit_Constant will likely be removed at the same time or before removing classes Num and Str and removing properties "n" and "s" from Constant). In long term solution you should write something like:

class V(ast.NodeVisitor):
    def _visit_string(self, node, value):
        ...

    def visit_Str(self, node):
        return self._visit_string(node, node.s)

    def visit_Constant(self, node):
        if isinstance(node.value, str):
            return self._visit_string(node, node.value)
        ...

Otherwise you can get other deprecation warnings or errors in future releases.
History
Date User Action Args
2019-08-26 12:14:21serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg350528

stage: patch review -> resolved
2019-08-26 11:49:12serhiy.storchakasetmessages: + msg350527
2019-08-26 07:53:12serhiy.storchakasetpull_requests: + pull_request15197
2019-08-26 07:43:39serhiy.storchakasetmessages: + msg350508
2019-08-26 07:13:32miss-islingtonsetpull_requests: + pull_request15195
2019-08-26 07:13:25serhiy.storchakasetmessages: + msg350496
2019-08-25 12:00:49serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request15177
2019-06-26 01:48:22dhilstsetnosy: + dhilst
2019-06-25 22:20:36Tyler Wincesetnosy: + Tyler Wince
messages: + msg346562
2019-05-19 01:00:44BTaskayasetnosy: + BTaskaya
messages: + msg342827
2019-05-15 19:38:44Anthony Sottilesetmessages: + msg342597
2019-05-15 19:34:47serhiy.storchakasetmessages: + msg342595
2019-05-15 16:29:19Anthony Sottilesetmessages: + msg342584
2019-05-15 16:05:28serhiy.storchakasetmessages: + msg342583
2019-05-15 15:44:15mbussonnsetmessages: + msg342578
2019-05-15 13:32:48Anthony Sottilesetmessages: + msg342572
2019-05-14 18:41:33mbussonnsetmessages: + msg342501
2019-05-14 18:30:56Anthony Sottilesetmessages: + msg342497
2019-05-14 18:12:28mbussonnsetmessages: + msg342494
2019-05-14 15:26:23Anthony Sottilesetmessages: + msg342474
2019-05-14 14:41:08mbussonnsetnosy: + mbussonn
2019-05-14 14:21:22Anthony Sottilecreate