_yaml: Move 'value' of Node in each sub node

This allows us to type its value more strictly, allowing for more
efficient access.

- Also implement 'node_at' on 'SequenceNode', to allow some tests to
test some internals generically.

3 files changed, 71 insertions, 39 deletions

diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd
index f0f3a4267..a3edd83f0 100644
--- a/src/buildstream/_yaml.pxd
+++ b/src/buildstream/_yaml.pxd
@@ -22,7 +22,6 @@
 
 cdef class Node:
 
-    cdef object value
     cdef int file_index
     cdef int line
     cdef int column
@@ -38,6 +37,8 @@ cdef class Node:
 
 
 cdef class MappingNode(Node):
+    cdef dict value
+
     cpdef void composite(self, MappingNode target) except *
     cpdef void composite_under(self, MappingNode target) except *
     cdef Node get(self, str key, default, default_constructor)
@@ -61,6 +62,8 @@ cdef class MappingNode(Node):
 
 
 cdef class ScalarNode(Node):
+    cdef str value
+
     cpdef bint as_bool(self) except *
     cpdef int as_int(self) except *
     cpdef str as_str(self)
@@ -68,8 +71,11 @@ cdef class ScalarNode(Node):
 
 
 cdef class SequenceNode(Node):
+    cdef list value
+
     cpdef void append(self, object value)
     cpdef MappingNode mapping_at(self, int index)
+    cpdef Node node_at(self, int index, list allowed_types=*)
     cpdef SequenceNode sequence_at(self, int index)
     cpdef list as_str_list(self)
 
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index ba58b39f0..606db0a4a 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -54,7 +54,6 @@ _sentinel = object()
 # than a plain tuple, to distinguish them in things like node_sanitize)
 #
 # Members:
-#   value (str/list/dict): The loaded value.
 #   file_index (int): Index within _FILE_LIST (a list of loaded file paths).
 #                     Negative indices indicate synthetic nodes so that
 #                     they can be referenced.
@@ -63,8 +62,7 @@ _sentinel = object()
 #
 cdef class Node:
 
-    def __init__(self, object value, int file_index, int line, int column):
-        self.value = value
+    def __init__(self, int file_index, int line, int column):
         self.file_index = file_index
         self.line = line
         self.column = column
@@ -72,7 +70,7 @@ cdef class Node:
     @classmethod
     def from_dict(cls, dict value):
         if value:
-            return _new_node_from_dict(value, Node(None, _SYNTHETIC_FILE_INDEX, 0, next_synthetic_counter()))
+            return _new_node_from_dict(value, MappingNode({}, _SYNTHETIC_FILE_INDEX, 0, next_synthetic_counter()))
         else:
             # We got an empty dict, we can shortcut
             return MappingNode({}, _SYNTHETIC_FILE_INDEX, 0, next_synthetic_counter())
@@ -87,7 +85,7 @@ cdef class Node:
         # Delegate to the inner value, though this will likely not work
         # very well if the node is a list or string, it's unlikely that
         # code which has access to such nodes would do this.
-        return what in self.value
+        return what in (<MappingNode> self).value
 
     cpdef Node copy(self):
         raise NotImplementedError()
@@ -135,12 +133,25 @@ cdef class Node:
 cdef class ScalarNode(Node):
 
     def __init__(self, object value, int file_index, int line, int column):
-        if type(value) is str:
+        super().__init__(file_index, line, column)
+
+        cdef value_type = type(value)
+
+        if value_type is str:
             value = value.strip()
+        elif value_type is bool:
+            if value:
+                value = "True"
+            else:
+                value = "False"
+        elif value_type is int:
+            value = str(value)
+        elif value is None:
+            pass
+        else:
+            raise ValueError("ScalarNode can only hold str, int, bool or None objects")
+
         self.value = value
-        self.file_index = file_index
-        self.line = line
-        self.column = column
 
     cpdef ScalarNode copy(self):
         return self
@@ -207,10 +218,8 @@ cdef class ScalarNode(Node):
 cdef class MappingNode(Node):
 
     def __init__(self, dict value, int file_index, int line, int column):
+        super().__init__(file_index, line, column)
         self.value = value
-        self.file_index = file_index
-        self.line = line
-        self.column = column
 
     cpdef MappingNode copy(self):
         cdef dict copy = {}
@@ -472,22 +481,22 @@ cdef class MappingNode(Node):
             if prefix is not None:
                 target.value["(<)"] = prefix
             elif "(<)" in target.value:
-                target.value["(<)"].value.clear()
+                (<SequenceNode> target.value["(<)"]).value.clear()
             if suffix is not None:
                 target.value["(>)"] = suffix
             elif "(>)" in target.value:
-                target.value["(>)"].value.clear()
+                (<SequenceNode> target.value["(>)"]).value.clear()
         else:
             # Not clobbering, so prefix the prefix and suffix the suffix
             if prefix is not None:
                 if "(<)" in target.value:
                     for v in reversed(prefix.value):
-                        target.value["(<)"].value.insert(0, v)
+                        (<SequenceNode> target.value["(<)"]).value.insert(0, v)
                 else:
                     target.value["(<)"] = prefix
             if suffix is not None:
                 if "(>)" in target.value:
-                    target.value["(>)"].value.extend(suffix.value)
+                    (<SequenceNode> target.value["(>)"]).value.extend(suffix.value)
                 else:
                     target.value["(>)"] = suffix
 
@@ -580,10 +589,8 @@ cdef class MappingNode(Node):
 
 cdef class SequenceNode(Node):
     def __init__(self, list value, int file_index, int line, int column):
+        super().__init__(file_index, line, column)
         self.value = value
-        self.file_index = file_index
-        self.line = line
-        self.column = column
 
     cpdef void append(self, object value):
         if type(value) in [MappingNode, ScalarNode, SequenceNode]:
@@ -612,6 +619,17 @@ cdef class SequenceNode(Node):
                             .format(provenance, path, MappingNode.__name__))
         return value
 
+    cpdef Node node_at(self, int key, list allowed_types = None):
+        cdef value = self.value[key]
+
+        if allowed_types and type(value) not in allowed_types:
+            provenance = node_get_provenance(self)
+            raise LoadError(LoadErrorReason.INVALID_DATA,
+                            "{}: Value of '{}' is not one of the following: {}.".format(
+                                provenance, key, ", ".join(allowed_types)))
+
+        return value
+
     cpdef SequenceNode sequence_at(self, int index):
         value = self.value[index]
 
@@ -679,6 +697,8 @@ cdef class SequenceNode(Node):
         return reversed(self.value)
 
     def __setitem__(self, int key, object value):
+        cdef Node old_value
+
         if type(value) in [MappingNode, ScalarNode, SequenceNode]:
             self.value[key] = value
         else:
@@ -914,14 +934,14 @@ cdef class Representer:
 
     cdef RepresenterState _handle_wait_value_ScalarEvent(self, object ev):
         key = self.keys.pop()
-        (<dict> (<Node> self.output[-1]).value)[key] = \
+        (<MappingNode> self.output[-1]).value[key] = \
             ScalarNode(ev.value, self._file_index, ev.start_mark.line, ev.start_mark.column)
         return RepresenterState.wait_key
 
     cdef RepresenterState _handle_wait_value_MappingStartEvent(self, object ev):
         cdef RepresenterState new_state = self._handle_doc_MappingStartEvent(ev)
         key = self.keys.pop()
-        (<dict> (<MappingNode> self.output[-2]).value)[key] = self.output[-1]
+        (<MappingNode> self.output[-2]).value[key] = self.output[-1]
         return new_state
 
     cdef RepresenterState _handle_wait_key_MappingEndEvent(self, object ev):
@@ -929,7 +949,7 @@ cdef class Representer:
         # unless it's the last one in which case we leave it
         if len(self.output) > 1:
             self.output.pop()
-            if type((<Node> self.output[-1]).value) is list:
+            if type(self.output[-1]) is SequenceNode:
                 return RepresenterState.wait_list_item
             else:
                 return RepresenterState.wait_key
@@ -938,13 +958,13 @@ cdef class Representer:
 
     cdef RepresenterState _handle_wait_value_SequenceStartEvent(self, object ev):
         self.output.append(SequenceNode([], self._file_index, ev.start_mark.line, ev.start_mark.column))
-        (<dict> (<Node> self.output[-2]).value)[self.keys[-1]] = self.output[-1]
+        (<MappingNode> self.output[-2]).value[self.keys[-1]] = self.output[-1]
         return RepresenterState.wait_list_item
 
     cdef RepresenterState _handle_wait_list_item_SequenceStartEvent(self, object ev):
-        self.keys.append(len((<Node> self.output[-1]).value))
+        self.keys.append(len((<SequenceNode> self.output[-1]).value))
         self.output.append(SequenceNode([], self._file_index, ev.start_mark.line, ev.start_mark.column))
-        (<list> (<Node> self.output[-2]).value).append(self.output[-1])
+        (<SequenceNode> self.output[-2]).value.append(self.output[-1])
         return RepresenterState.wait_list_item
 
     cdef RepresenterState _handle_wait_list_item_SequenceEndEvent(self, object ev):
@@ -959,13 +979,13 @@ cdef class Representer:
             return RepresenterState.wait_key
 
     cdef RepresenterState _handle_wait_list_item_ScalarEvent(self, object ev):
-        (<Node> self.output[-1]).value.append(
+        (<SequenceNode> self.output[-1]).value.append(
            ScalarNode(ev.value, self._file_index, ev.start_mark.line, ev.start_mark.column))
         return RepresenterState.wait_list_item
 
     cdef RepresenterState _handle_wait_list_item_MappingStartEvent(self, object ev):
         cdef RepresenterState new_state = self._handle_doc_MappingStartEvent(ev)
-        (<list> (<Node> self.output[-2]).value).append(self.output[-1])
+        (<SequenceNode> self.output[-2]).value.append(self.output[-1])
         return new_state
 
     cdef RepresenterState _handle_doc_DocumentEndEvent(self, object ev):
@@ -1109,11 +1129,11 @@ cpdef ProvenanceInformation node_get_provenance(Node node, str key=None, list in
         return ProvenanceInformation(node)
 
     if key and not indices:
-        return ProvenanceInformation(node.value.get(key))
+        return ProvenanceInformation((<MappingNode> node).value.get(key))
 
-    cdef Node nodeish = <Node> node.value.get(key)
+    cdef Node nodeish = <Node> (<MappingNode> node).value.get(key)
     for idx in indices:
-        nodeish = <Node> nodeish.value[idx]
+        nodeish = <Node> (<SequenceNode> nodeish).value[idx]
 
     return ProvenanceInformation(nodeish)
 
@@ -1179,7 +1199,6 @@ cdef Node _new_node_from_list(list inlist, Node ref_node):
             ret.value.append(_new_node_from_list(v, ref_node))
         else:
             ret.value.append(ScalarNode(str(v), ref_node.file_index, ref_node.line, next_synthetic_counter()))
-
     return ret
 
 
diff --git a/tests/internals/yaml.py b/tests/internals/yaml.py
index f50d68585..37a2c9931 100644
--- a/tests/internals/yaml.py
+++ b/tests/internals/yaml.py
@@ -21,7 +21,7 @@ def test_load_yaml(datafiles):
                             'basics.yaml')
 
     loaded = _yaml.load(filename)
-    assert loaded.value.get('kind').value == 'pony'
+    assert loaded.get_str('kind') == 'pony'
 
 
 def assert_provenance(filename, line, col, node, key=None, indices=None):
@@ -42,7 +42,7 @@ def test_basic_provenance(datafiles):
                             'basics.yaml')
 
     loaded = _yaml.load(filename)
-    assert loaded.value.get('kind').value == 'pony'
+    assert loaded.get_str('kind') == 'pony'
 
     assert_provenance(filename, 1, 0, loaded)
 
@@ -55,7 +55,7 @@ def test_member_provenance(datafiles):
                             'basics.yaml')
 
     loaded = _yaml.load(filename)
-    assert loaded.value.get('kind').value == 'pony'
+    assert loaded.get_str('kind') == 'pony'
     assert_provenance(filename, 2, 13, loaded, 'description')
 
 
@@ -67,7 +67,7 @@ def test_element_provenance(datafiles):
                             'basics.yaml')
 
     loaded = _yaml.load(filename)
-    assert loaded.value.get('kind').value == 'pony'
+    assert loaded.get_str('kind') == 'pony'
     assert_provenance(filename, 5, 2, loaded, 'moods', [1])
 
 
@@ -101,7 +101,7 @@ def test_node_get(datafiles):
                             'basics.yaml')
 
     base = _yaml.load(filename)
-    assert base.value.get('kind').value == 'pony'
+    assert base.get_str('kind') == 'pony'
 
     children = base.get_sequence('children')
     assert isinstance(children, _yaml.SequenceNode)
@@ -504,9 +504,16 @@ def test_node_find_target(datafiles, case):
     # laid out.  Client code should never do this.
     def _walk(node, entry, rest):
         if rest:
-            return _walk(node.value[entry], rest[0], rest[1:])
+            if isinstance(entry, int):
+                new_node = node.node_at(entry)
+            else:
+                new_node = node.get_node(entry)
+
+            return _walk(new_node, rest[0], rest[1:])
         else:
-            return node.value[entry]
+            if isinstance(entry, int):
+                return node.node_at(entry)
+            return node.get_node(entry)
 
     want = _walk(loaded, case[0], case[1:])
     found_path = toplevel._find(want)