[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile <asottile at umich.edu> added the comment:

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

----------

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue36917>
_______________________________________