TransformVisitor (and StrConv) are slightly out-of-sync with node definitions

[Michael0x2a]

I recently noticed that mypy.treetransform.TransformVisitor seems to be slightly out-of-sync with the nodes within mypy.nodes.

For example...

• visit_func_def isn't copying over the is_generator, is_coroutine, and is_awaitable_coroutine attributes
• visit_var isn't copying over the is_suppressed_import attribute
• visit_with_statement isn't copying over is_async
• visit_class_def isn't copying over has_incompatible_baseclass

Currently, it doesn't seem like these omissions are too harmful since the only place we use TransformVisitor is within TypeTransformVisitor (which only expands types). However, I could see this might lead to subtle bugs in the future, so it's probably worth fixing this now and perhaps coming up with some sort of test that will automatically catch these sorts of discrepancies in the future.

mypy.strconv.StrConv also seems to have similar issues, though it's probably even less of a priority. I haven't really checked the other visitors, so I don't know if they're similarly desynched.

(I'm filing this issue to make sure I don't forget about it).

[gvanrossum]

Good auditing! I wonder if this could cause subtle issues when -i is used for code containing async def? I wonder what we could do to prevent more regressions like these in the future? Some kind of checklist for when someone adds an attribute to a type class? (I wonder if some kind of assert could be used for this, where you'd have a list of known classes and attributes that the visitor knows about, which you update when you add support for an attribute to the visitor.)

[Michael0x2a]

I think having a checklist might be a good idea -- we have so many visitors that keeping track of them all can be difficult to do.

I was maybe thinking of creating some sort of test that would inspect the symbol table of a given node class or examine the available class attributes/instance attributes at runtime, automagically set each attribute to some sort of sentinel value, run a transform, then check to make sure the sentinel values are all present in the new tree.

Though, this approach seems like a little too much magic/potentially fragile (and it also wouldn't necessarily work for StrConv or other visitors).

[elazarg]

To the point, I think unit tests can easily check at least some of these discrepancies, by using reflection. I don't know if that's considered good style. If it's acceptable I can try to write it.

dreamy thoughts alert

Having a Struct metaclass or attr decorator, with automatic generation of __init__ _fields and serialization could solve some of these issues, and save much of the boilerplate.

It will not solve the other problem, not mentioned here, of visitors not being updated when Nodes are added. This could be solved by putting all class definition in a huge enum, effectively constructing a variant-type.

Together we have hierarchy whose visitor base class can be automatically generated and which can be checked for unused visit_ methods in subclasses. I have never tried that, though.

[JukkaL]

Please add no metaclasses to the mypy codebase :-)

Starting with a checklist looks like the obvious first step -- it's uncontroversial and not a lot of work to do, and it would likely solve a good fraction of the issues we now have.

Reflection magic has the problem that typically things aren't quite consistent enough for this to be simple, and complex reflection magic rarely is a net win. A lot of simple code generally wins over a shorter but sufficiently complex bit of code. If the magic is trivial to explain and implement and doesn't have complex logic or exceptions, it may be okay, at least if it's only used for tests.

TransformVisitor actually needs to preserve everything, even if we only use it to transform types, since the transformed function bodies will be type checked.

Here are some other ideas that might be simple enough and not too fragile:

• Update mypy/test/testtransform.py to include more semantic analysis test cases, which are used to test identity transforms. I suspect it hasn't been kept up-to-date. Some features such as NewType actually don't seem to have semantic analysis test cases, so they won't be tested here at all. We should probably have semantic analysis test cases for all AST node types.
• Maybe we could somehow automatically verify that all AST node types are covered by semantic analysis test cases.
• We could have a test that verifies that certain visitors such as StrConv and TransformVisitor override all visitor methods. Not sure if making the base class visit methods abstract would be a good idea.
• This is less likely to be useful, but what about looking for all attributes of all node classes and making sure that they are at least mentioned (maybe we can just run a grep) in certain visitors. If some attributes don't need to be processed, we could add a comment whitelisting them to they'd still appear in the text of the visitor.

[elazarg]

I guess assert str(tree) == str(copy(tree)) might help keeping these two in sync.

[JukkaL]

Yeah, that's basically what the tests in testtransform.py do. But for this we need a bunch of trees, and we reuse the semantic analysis test cases to generate the syntax trees used in the test cases, so that it's not necessary to maintain an additional set of test cases just for testing identity transforms.

[gvanrossum]

A meta comment: we should really have these discussions in a dedicated issue, not in the tail of an unrelated closed issue.

…

--Guido (mobile)