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

Created on 2019-05-14 14:21 by Anthony Sottile, last changed 2019-05-19 01:00 by BTaskaya.

Messages (12)
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?
History
Date User Action Args
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