Fix children duplicating nodes and unchecked nullptr in pyuast.cc

Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>

Author: Juanjo Alvarez

bblfsh/node.py

@@ -47,7 +47,26 @@ def offset(self, v: int) -> None:
 
 class CompatChildren(MutableSequence):
     def __init__(self, parent: "Node") -> None:
-        self._children = parent.get_dict()["@children"]
+        self._par_dict = parent.get_dict()
+        self._children = self._sync_children()
+
+    def _sync_children(self) -> None:
+        if "_children" not in self._par_dict:
+            self._par_dict["_children"] = []
+        children = self._par_dict["_children"]
+        for k, v in self._par_dict.items():
+            if k in ("_children", "@pos", "@role", "@type"):
+                continue
+
+            tv = type(v)
+            if tv in (Node, dict):
+                if v not in children:
+                    children.append(v)
+            elif tv in (list, tuple):
+                # Get all node|dict types inside the list and add to children
+                children.extend([i for i in v if type(i) in (Node, dict) and i not in children])
+            # else ignore it
+        return children
 
     @staticmethod
     def _node2dict(n: Union['Node', dict]) -> dict:
@@ -66,13 +85,16 @@ def __delitem__(self, idx: Union[int, slice]) -> None:
         del self._children[idx]
 
     def __setitem__(self, idx: Union[int, slice], val: Union['Node', dict]) -> None:
-        self._children[idx] = self._node2dict(val)
+        self._par_dict["_children"].__setitem__(idx, self._node2dict(val))
+        self._children = self._sync_children()
 
     def insert(self, idx: int, val: Union['Node', dict]) -> None:
-        self._children.insert(idx, self._node2dict(val))
+        self._par_dict["_children"].insert(idx, self._node2dict(val))
+        self._children = self._sync_children()
 
     def append(self, val: Union['Node', dict]) -> None:
-        self._children.append(self._node2dict(val))
+        self._par_dict["_children"].append(self._node2dict(val))
+        self._children = self._sync_children()
 
     def extend(self, items: List[Union['Node', dict]]) -> None:
         for i in items:
@@ -112,32 +134,6 @@ def __init__(self, node_ext: NodeExt = None, value: ResultMultiType = None) -> N
 
         self.node_ext = node_ext
 
-        if isinstance(self.internal_node, dict):
-            self._load_children()
-
-    # This is for v1 "node.children" compatibility. It will update the children
-    # property with the dict or Node objects in properties or list/tuple properties
-    # when .children is accessed (because the user could change the node using get_dict()
-    # or .properties).
-    # Also, all these " in children" are O(1) so this will be slow for frequently accessing
-    # the children property on big nodes.
-    def _load_children(self) -> None:
-        """Get all properties of type node or dict and load them into the list"""
-        d = self.get_dict()
-        children = d.get("@children", [])
-        for k, v in d.items():
-            if k in ("@children", "@pos", "@role", "@type"):
-                continue
-
-            tv = type(v)
-            if tv in (Node, dict):
-                if v not in children:
-                    children.append(v)
-            elif tv in (list, tuple):
-                # Get all node|dict types inside the list and add to children
-                children.extend([i for i in v if type(i) in (Node, dict) and i not in children])
-            # else ignore it
-
     def __str__(self) -> str:
         return str(self.get())
 
@@ -200,7 +196,6 @@ def _is_dict_list(self, key: str) -> Optional[List]:
 
     @property
     def children(self) -> CompatChildren:
-        self._load_children()
         return CompatChildren(self)
 
     @property

bblfsh/pyuast.cc

@@ -551,6 +551,7 @@ class Node : public uast::Node<Node*> {
         if (!keys) return nullptr;
 
         PyObject* key = PyList_GetItem(keys, i); // borrows
+        if (!key) return nullptr;
         const char * k = PyUnicode_AsUTF8(key);
 
         std::string* s = new std::string(k);

bblfsh/test.py

@@ -36,7 +36,6 @@ def _parse_fixture(self) -> ResultContext:
         self._validate_ctx(ctx)
         return ctx
 
-    """
     def testVersion(self) -> None:
         version = self.client.version()
         self.assertTrue(hasattr(version, "version"))
@@ -188,7 +187,6 @@ def testFilterBadType(self) -> None:
     def testRoleIdName(self) -> None:
         self.assertEqual(role_id(role_name(1)), 1)
         self.assertEqual(role_name(role_id("IDENTIFIER")),  "IDENTIFIER")
-    """
 
     @staticmethod
     def _itTestTree() -> dict:
@@ -258,7 +256,6 @@ def testIteratorPostOrder(self) -> None:
         self.assertListEqual(expanded, ['son1_1', 'son1_2', 'son1', 'son2_1',
                                         'son2_2', 'son2', 'root'])
 
-    """
     def testIteratorLevelOrder(self) -> None:
         root = self._itTestTree()
         it = iterator(root, TreeOrder.LEVEL_ORDER)
@@ -338,7 +335,6 @@ def testSupportedLanguages(self) -> None:
             for key in ('language', 'version', 'status', 'features'):
                 self.assertTrue(hasattr(l, key))
                 self.assertIsNotNone(getattr(l, key))
-    """
 
 
 if __name__ == "__main__":

bblfsh/test_compat.py

@@ -226,7 +226,7 @@ def testIteratorPositionOrder(self):
                                         'son1_2', 'son2_2', 'son2'])
 
     def testFilterInsideIter(self):
-        root = self.client.parse(__file__).uast
+        root = self._parse_fixture().uast
         it = iterator(root, TreeOrder.PRE_ORDER)
         self.assertIsNotNone(it)
         for n in it:
@@ -275,7 +275,6 @@ def testManyParsesAndFilters(self):
             xpath_filter(root, "//*[@role='Identifier']")
 
         after = resource.getrusage(resource.RUSAGE_SELF)
-
         self.assertLess(after[2] / before[2], 4.0)
 
     def testSupportedLanguages(self):
@@ -304,6 +303,16 @@ def testChildren(self):
         self.assertDictEqual(n.children[3], l[0])
         self.assertDictEqual(n.children[4], l[1])
 
+    def testChildrenFile(self):
+        root = self._parse_fixture().uast
+        self.assertEqual(len(root.children), 10)
+        n = Node()
+        n.internal_type = 'child_node'
+        root.children.append(n)
+        self.assertEqual(len(root.children), 11)
+        last = root.children[-1]
+        self.assertDictEqual(last, n.internal_node)
+
 
 
 if __name__ == "__main__":