Repository: neogeny/TatSu

Issue:
Unusual use of `__new__()` in `NodeWalker`
Hunting for old-style `super()` calls I bumped into the `__new__()` method of the `NodeWalker` class:
```python
class NodeWalker:
    def __new__(cls, *args, **kwargs):
        cls._walker_cache = {}
        return super(NodeWalker, cls).__new__(cls)
```
As the class does not inherit from other classes the use of `super()` seems redundant here as it is equivalent to simply calling `cls()` but this is not what is unusual. What is unusual is modifying the class definition in the `__new__()` method. This means that all `NodeWalker` instances share the same `_walker_cache` dictionary and that this dictionary in reset every time a new `NodeWalker` instance is created. I cannot find a reason why this could be desirable.

Shouldn't this `__new__()` method be turned into a `__init__()` method?

Discussion:

Patch:
diff --git a/tatsu/walkers.py b/tatsu/walkers.py
index e236501..12ccc63 100644
--- a/tatsu/walkers.py
+++ b/tatsu/walkers.py
@@ -8,10 +8,18 @@ from tatsu.objectmodel import Node
 from tatsu.util import is_list
 
 
-class NodeWalker:
-    def __new__(cls, *args, **kwargs):
-        cls._walker_cache = {}
-        return super(NodeWalker, cls).__new__(cls)
+class NodeWalkerMeta(type):
+    def __new__(cls, name, bases, dct):
+        class_ = super().__new__(cls, name, bases, dct)
+        class_._walker_cache = {}
+        return class_
+
+
+class NodeWalker(metaclass=NodeWalkerMeta):
+    def __init__(self):
+        super().__init__()
+        # copy the class attribute to avoid linter warnings
+        self._walker_cache = type(self)._walker_cache  # pylint: disable=no-member
 
     def walk(self, node: Node|list[Node], *args, **kwargs) -> Any:
         if isinstance(node, list):
@@ -30,7 +38,8 @@ class NodeWalker:
         def pythonize_match(m):
             return '_' + m.group().lower()
 
-        classid = id(node.__class__)
+        cls = node.__class__
+        classid = id(cls)
 
         if classid in self._walker_cache:
             return self._walker_cache[classid]


Test patch:
diff --git a/test/walker_test.py b/test/walker_test.py
index cdcc022..6461f25 100644
--- a/test/walker_test.py
+++ b/test/walker_test.py
@@ -2,7 +2,7 @@ import json
 from collections import defaultdict
 
 import tatsu
-from tatsu.walkers import DepthFirstWalker
+from tatsu.walkers import NodeWalker, DepthFirstWalker
 from tatsu.util import asjson
 
 
@@ -41,3 +41,18 @@ def test_walk_node_ast():
     print(json.dumps(asjson(model), indent=2))
     PW().walk(model)
     assert seen == {'SampleExpression': 2, 'TemporalSeq': 1}
+
+
+def test_cache_per_class():
+    class PW(DepthFirstWalker):
+        pass
+
+    assert isinstance(NodeWalker._walker_cache, dict)  # pylint: disable=no-member
+    assert isinstance(DepthFirstWalker._walker_cache, dict)  # pylint: disable=no-member
+    assert isinstance(PW._walker_cache, dict)  # pylint: disable=no-member
+
+    assert id(NodeWalker._walker_cache) != id(DepthFirstWalker._walker_cache)  # pylint: disable=no-member
+    assert id(PW._walker_cache) != id(DepthFirstWalker._walker_cache)  # pylint: disable=no-member
+
+    walker = PW()  # pylint: disable=no-member
+    assert id(walker._walker_cache) == id(PW._walker_cache)  # pylint: disable=no-member
diff --git a/test/zzz_bootstrap/bootstrap_test.py b/test/zzz_bootstrap/bootstrap_test.py
index 724f823..a09b32d 100644
--- a/test/zzz_bootstrap/bootstrap_test.py
+++ b/test/zzz_bootstrap/bootstrap_test.py
@@ -155,6 +155,7 @@ class BootstrapTests(unittest.TestCase):
 
         class PrintNameWalker(DepthFirstWalker):
             def __init__(self):
+                super().__init__()
                 self.walked = []
 
             def walk_default(self, o, children):