[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
pull_requests: +15197
pull_request: https://github.com/python/cpython/pull/15511

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[miss-islington]

Change by miss-islington :


--
pull_requests: +15195
pull_request: https://github.com/python/cpython/pull/15509

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
keywords: +patch
pull_requests: +15177
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/15490

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Daniel Hilst Selli]

Change by Daniel Hilst Selli :


--
nosy: +dhilst

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Tyler Wince]

Tyler Wince  added the comment:

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?

--
nosy: +Tyler Wince

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Batuhan]

Batuhan  added the comment:

What about adding visit_Constant to NodeVisitor for at least one relase period 
and call visit_Str, visit_Num etc?

--
nosy: +BTaskaya

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile  added the comment:

> 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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Matthias Bussonnier]

Matthias Bussonnier  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Matthias Bussonnier]

Matthias Bussonnier  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile  added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Matthias Bussonnier]

Matthias Bussonnier  added the comment:

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 ?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

Anthony Sottile  added the comment:

wrong bpo, this is the right one: https://bugs.python.org/issue32892

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Matthias Bussonnier]

Change by Matthias Bussonnier :


--
nosy: +mbussonn

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36917] ast.NodeVisitor no longer calls visit_Str

[Anthony Sottile]

New submission from Anthony Sottile :

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]

--
components: Library (Lib)
messages: 342463
nosy: Anthony Sottile, serhiy.storchaka
priority: normal
severity: normal
status: open
title: ast.NodeVisitor no longer calls visit_Str
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com