Merge branch 'bschubert/new-node-compose' into 'bschubert/new-node-api'

Rework node compose API

See merge request BuildStream/buildstream!1442

9 files changed, 218 insertions, 225 deletions

diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py
index 35ed95e11..524305177 100644
--- a/src/buildstream/_context.py
+++ b/src/buildstream/_context.py
@@ -201,7 +201,7 @@ class Context():
         if config:
             self.config_origin = os.path.abspath(config)
             user_config = _yaml.load(config)
-            _yaml.composite(defaults, user_config)
+            user_config.composite(defaults)
 
         # Give obsoletion warnings
         if 'builddir' in defaults:
diff --git a/src/buildstream/_includes.py b/src/buildstream/_includes.py
index ea2bf484e..4c1eabb7e 100644
--- a/src/buildstream/_includes.py
+++ b/src/buildstream/_includes.py
@@ -81,7 +81,7 @@ class Includes:
                 finally:
                     included.remove(file_path)
 
-                _yaml.composite_and_move(node, include_node)
+                include_node.composite_under(node)
 
         for value in node.values():
             self._process_value(value,
diff --git a/src/buildstream/_options/optionpool.py b/src/buildstream/_options/optionpool.py
index eae03f181..5155f62f4 100644
--- a/src/buildstream/_options/optionpool.py
+++ b/src/buildstream/_options/optionpool.py
@@ -290,7 +290,7 @@ class OptionPool():
 
                 # Apply the yaml fragment if its condition evaluates to true
                 if apply_fragment:
-                    _yaml.composite(node, value)
+                    value.composite(node)
 
             return True
 
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index ba3b92207..1a22e0b06 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -570,7 +570,7 @@ class Project():
                 raise
 
         pre_config_node = self._default_config_node.copy()
-        _yaml.composite(pre_config_node, self._project_conf)
+        self._project_conf.composite(pre_config_node)
 
         # Assert project's format version early, before validating toplevel keys
         format_version = pre_config_node.get_int('format-version')
@@ -616,7 +616,7 @@ class Project():
         project_conf_first_pass = self._project_conf.copy()
         self._project_includes.process(project_conf_first_pass, only_local=True)
         config_no_include = self._default_config_node.copy()
-        _yaml.composite(config_no_include, project_conf_first_pass)
+        project_conf_first_pass.composite(config_no_include)
 
         self._load_pass(config_no_include, self.first_pass_config,
                         ignore_unknown=True)
@@ -640,7 +640,7 @@ class Project():
         project_conf_second_pass = self._project_conf.copy()
         self._project_includes.process(project_conf_second_pass)
         config = self._default_config_node.copy()
-        _yaml.composite(config, project_conf_second_pass)
+        project_conf_second_pass.composite(config)
 
         self._load_pass(config, self.config)
 
diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd
index 146f14a22..32a39dfd4 100644
--- a/src/buildstream/_yaml.pxd
+++ b/src/buildstream/_yaml.pxd
@@ -31,11 +31,15 @@ cdef class Node:
     cpdef object strip_node_info(self)
 
     cpdef void _assert_fully_composited(self) except *
+    cdef void _compose_on(self, str key, MappingNode target, list path) except *
+    cdef bint _is_composite_list(self) except *
     cdef bint _shares_position_with(self, Node target)
     cdef bint _walk_find(self, Node target, list path) except *
 
 
 cdef class MappingNode(Node):
+    cpdef void composite(self, MappingNode target) except *
+    cpdef void composite_under(self, MappingNode target) except *
     cdef Node get(self, str key, default, default_constructor)
     cpdef MappingNode get_mapping(self, str key, default=*)
     cpdef Node get_node(self, str key, list allowed_types=*, bint allow_none=*)
@@ -49,6 +53,9 @@ cdef class MappingNode(Node):
     cpdef void safe_del(self, str key)
     cpdef object values(self)
 
+    cdef void _composite(self, MappingNode target, list path=*) except *
+    cdef void _compose_on_composite_dict(self, MappingNode target)
+    cdef void _compose_on_list(self, SequenceNode target)
     cpdef list _find(self, Node target)
 
 
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index 2c45057ee..6420474eb 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -101,6 +101,25 @@ cdef class Node:
     cpdef void _assert_fully_composited(self) except *:
         raise NotImplementedError()
 
+    # _is_composite_list
+    #
+    # Checks if the node is a Mapping with array composition
+    # directives.
+    #
+    # Returns:
+    #    (bool): True if node was a Mapping containing only
+    #            list composition directives
+    #
+    # Raises:
+    #    (LoadError): If node was a mapping and contained a mix of
+    #                 list composition directives and other keys
+    #
+    cdef bint _is_composite_list(self) except *:
+        raise NotImplementedError()
+
+    cdef void _compose_on(self, str key, MappingNode target, list path) except *:
+        raise NotImplementedError()
+
     def __json__(self):
         raise ValueError("Nodes should not be allowed when jsonify-ing data", self)
 
@@ -159,6 +178,20 @@ cdef class ScalarNode(Node):
     cpdef void _assert_fully_composited(self) except *:
         pass
 
+    cdef void _compose_on(self, str key, MappingNode target, list path) except *:
+        cdef Node target_value = target.value.get(key)
+
+        if target_value is not None and type(target_value) is not ScalarNode:
+            raise CompositeError(path,
+                                 "{}: Cannot compose scalar on non-scalar at {}".format(
+                                    node_get_provenance(self),
+                                    node_get_provenance(target_value)))
+
+        target.value[key] = self
+
+    cdef bint _is_composite_list(self) except *:
+        return False
+
     cdef bint _walk_find(self, Node target, list path) except *:
         return self._shares_position_with(target)
 
@@ -199,6 +232,43 @@ cdef class MappingNode(Node):
             return path
         return None
 
+    # composite()
+    #
+    # Compose one mapping node onto another
+    #
+    # Args:
+    #    target (Node): The target to compose into
+    #
+    # Raises: LoadError
+    #
+    cpdef void composite(self, MappingNode target) except *:
+        try:
+            self._composite(target, [])
+        except CompositeError as e:
+            source_provenance = node_get_provenance(self)
+            error_prefix = ""
+            if source_provenance:
+                error_prefix = "{}: ".format(source_provenance)
+            raise LoadError(LoadErrorReason.ILLEGAL_COMPOSITE,
+                            "{}Failure composing {}: {}"
+                            .format(error_prefix,
+                                    e.path,
+                                    e.message)) from e
+
+    # Like composite(target, source), but where target overrides source instead.
+    #
+    cpdef void composite_under(self, MappingNode target) except *:
+        target.composite(self)
+
+        cdef str key
+        cdef Node value
+        cdef list to_delete = [key for key in target.value.keys() if key not in self.value]
+
+        for key, value in self.value.items():
+            target.value[key] = value
+        for key in to_delete:
+            del target.value[key]
+
     cdef Node get(self, str key, object default, object default_constructor):
         value = self.value.get(key, _sentinel)
 
@@ -306,6 +376,111 @@ cdef class MappingNode(Node):
 
         return {key: value.strip_node_info() for key, value in self.value.items()}
 
+    cdef void _composite(self, MappingNode target, list path=None) except *:
+        cdef str key
+        cdef Node value
+
+        for key, value in self.value.items():
+            path.append(key)
+            value._compose_on(key, target, path)
+            path.pop()
+
+    cdef void _compose_on(self, str key, MappingNode target, list path) except *:
+        cdef Node target_value
+
+        if self._is_composite_list():
+            if key not in target.value:
+                # Composite list clobbers empty space
+                target.value[key] = self
+            else:
+                target_value = target.value[key]
+
+                if type(target_value) is SequenceNode:
+                    # Composite list composes into a list
+                    self._compose_on_list(target_value)
+                elif target_value._is_composite_list():
+                    # Composite list merges into composite list
+                    self._compose_on_composite_dict(target_value)
+                else:
+                    # Else composing on top of normal dict or a scalar, so raise...
+                    raise CompositeError(path,
+                                         "{}: Cannot compose lists onto {}".format(
+                                             node_get_provenance(self),
+                                             node_get_provenance(target_value)))
+        else:
+            # We're composing a dict into target now
+            if key not in target.value:
+                # Target lacks a dict at that point, make a fresh one with
+                # the same provenance as the incoming dict
+                target.value[key] = MappingNode({}, self.file_index, self.line, self.column)
+
+            self._composite(target.value[key], path)
+
+    cdef void _compose_on_list(self, SequenceNode target):
+        cdef SequenceNode clobber = self.value.get("(=)")
+        cdef SequenceNode prefix = self.value.get("(<)")
+        cdef SequenceNode suffix = self.value.get("(>)")
+
+        if clobber is not None:
+            target.value.clear()
+            target.value.extend(clobber.value)
+        if prefix is not None:
+            for v in reversed(prefix.value):
+                target.value.insert(0, v)
+        if suffix is not None:
+            target.value.extend(suffix.value)
+
+    cdef void _compose_on_composite_dict(self, MappingNode target):
+        cdef SequenceNode clobber = self.value.get("(=)")
+        cdef SequenceNode prefix = self.value.get("(<)")
+        cdef SequenceNode suffix = self.value.get("(>)")
+
+        if clobber is not None:
+            # We want to clobber the target list
+            # which basically means replacing the target list
+            # with ourselves
+            target.value["(=)"] = clobber
+            if prefix is not None:
+                target.value["(<)"] = prefix
+            elif "(<)" in target.value:
+                target.value["(<)"].value.clear()
+            if suffix is not None:
+                target.value["(>)"] = suffix
+            elif "(>)" in target.value:
+                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)
+                else:
+                    target.value["(<)"] = prefix
+            if suffix is not None:
+                if "(>)" in target.value:
+                    target.value["(>)"].value.extend(suffix.value)
+                else:
+                    target.value["(>)"] = suffix
+
+    cdef bint _is_composite_list(self) except *:
+        cdef bint has_directives = False
+        cdef bint has_keys = False
+        cdef str key
+
+        for key in self.value.keys():
+            if key in ['(>)', '(<)', '(=)']:
+                has_directives = True
+            else:
+                has_keys = True
+
+        if has_keys and has_directives:
+            provenance = node_get_provenance(self)
+            raise LoadError(LoadErrorReason.INVALID_DATA,
+                            "{}: Dictionary contains array composition directives and arbitrary keys"
+                            .format(provenance))
+
+        return has_directives
+
     def __delitem__(self, str key):
         del self.value[key]
 
@@ -423,6 +598,24 @@ cdef class SequenceNode(Node):
         for value in self.value:
             value._assert_fully_composited()
 
+    cdef void _compose_on(self, str key, MappingNode target, list path) except *:
+        # List clobbers anything list-like
+        cdef Node target_value = target.value.get(key)
+
+        if not (target_value is None or
+                type(target_value) is SequenceNode or
+                target_value._is_composite_list()):
+            raise CompositeError(path,
+                                 "{}: List cannot overwrite {} at: {}"
+                                 .format(node_get_provenance(self),
+                                         key,
+                                         node_get_provenance(target_value)))
+        # Looks good, clobber it
+        target.value[key] = self
+
+    cdef bint _is_composite_list(self) except *:
+        return False
+
     cdef bint _walk_find(self, Node target, list path) except *:
         cdef int i
         cdef Node v
@@ -1046,213 +1239,6 @@ cdef Node __new_node_from_list(list inlist):
     return SequenceNode(ret, _SYNTHETIC_FILE_INDEX, 0, next_synthetic_counter())
 
 
-# _is_composite_list
-#
-# Checks if the given node is a Mapping with array composition
-# directives.
-#
-# Args:
-#    node (value): Any node
-#
-# Returns:
-#    (bool): True if node was a Mapping containing only
-#            list composition directives
-#
-# Raises:
-#    (LoadError): If node was a mapping and contained a mix of
-#                 list composition directives and other keys
-#
-cdef bint _is_composite_list(Node node):
-    cdef bint has_directives = False
-    cdef bint has_keys = False
-    cdef str key
-
-    if type(node) is MappingNode:
-        for key in (<MappingNode> node).keys():
-            if key in ['(>)', '(<)', '(=)']:  # pylint: disable=simplifiable-if-statement
-                has_directives = True
-            else:
-                has_keys = True
-
-            if has_keys and has_directives:
-                provenance = node_get_provenance(node)
-                raise LoadError(LoadErrorReason.INVALID_DATA,
-                                "{}: Dictionary contains array composition directives and arbitrary keys"
-                                .format(provenance))
-        return has_directives
-
-    return False
-
-
-# _compose_composite_list()
-#
-# Composes a composite list (i.e. a dict with list composition directives)
-# on top of a target list which is a composite list itself.
-#
-# Args:
-#    target (Node): A composite list
-#    source (Node): A composite list
-#
-cdef void _compose_composite_list(Node target, Node source):
-    clobber = source.value.get("(=)")
-    prefix = source.value.get("(<)")
-    suffix = source.value.get("(>)")
-    if clobber is not None:
-        # We want to clobber the target list
-        # which basically means replacing the target list
-        # with ourselves
-        target.value["(=)"] = clobber
-        if prefix is not None:
-            target.value["(<)"] = prefix
-        elif "(<)" in target.value:
-            target.value["(<)"].value.clear()
-        if suffix is not None:
-            target.value["(>)"] = suffix
-        elif "(>)" in target.value:
-            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)
-            else:
-                target.value["(<)"] = prefix
-        if suffix is not None:
-            if "(>)" in target.value:
-                target.value["(>)"].value.extend(suffix.value)
-            else:
-                target.value["(>)"] = suffix
-
-
-# _compose_list()
-#
-# Compose a composite list (a dict with composition directives) on top of a
-# simple list.
-#
-# Args:
-#    target (Node): The target list to be composed into
-#    source (Node): The composition list to be composed from
-#
-cdef void _compose_list(Node target, Node source):
-    clobber = source.value.get("(=)")
-    prefix = source.value.get("(<)")
-    suffix = source.value.get("(>)")
-    if clobber is not None:
-        target.value.clear()
-        target.value.extend(clobber.value)
-    if prefix is not None:
-        for v in reversed(prefix.value):
-            target.value.insert(0, v)
-    if suffix is not None:
-        target.value.extend(suffix.value)
-
-
-# composite_dict()
-#
-# Compose one mapping node onto another
-#
-# Args:
-#    target (Node): The target to compose into
-#    source (Node): The source to compose from
-#    path   (list): The path to the current composition node
-#
-# Raises: CompositeError
-#
-cpdef void composite_dict(Node target, Node source, list path=None) except *:
-    cdef str k
-    cdef Node v, target_value
-
-    if path is None:
-        path = []
-    for k, v in source.value.items():
-        path.append(k)
-        if type(v.value) is list:
-            # List clobbers anything list-like
-            target_value = target.value.get(k)
-            if not (target_value is None or
-                    type(target_value.value) is list or
-                    _is_composite_list(target_value)):
-                raise CompositeError(path,
-                                     "{}: List cannot overwrite {} at: {}"
-                                     .format(node_get_provenance(source, k),
-                                             k,
-                                             node_get_provenance(target, k)))
-            # Looks good, clobber it
-            target.value[k] = v
-        elif _is_composite_list(v):
-            if k not in target.value:
-                # Composite list clobbers empty space
-                target.value[k] = v
-            elif type(target.value[k].value) is list:
-                # Composite list composes into a list
-                _compose_list(target.value[k], v)
-            elif _is_composite_list(target.value[k]):
-                # Composite list merges into composite list
-                _compose_composite_list(target.value[k], v)
-            else:
-                # Else composing on top of normal dict or a scalar, so raise...
-                raise CompositeError(path,
-                                     "{}: Cannot compose lists onto {}".format(
-                                         node_get_provenance(v),
-                                         node_get_provenance(target.value[k])))
-        elif type(v.value) is dict:
-            # We're composing a dict into target now
-            if k not in target.value:
-                # Target lacks a dict at that point, make a fresh one with
-                # the same provenance as the incoming dict
-                target.value[k] = MappingNode({}, v.file_index, v.line, v.column)
-            if type(target.value) is not dict:
-                raise CompositeError(path,
-                                     "{}: Cannot compose dictionary onto {}".format(
-                                         node_get_provenance(v),
-                                         node_get_provenance(target.value[k])))
-            composite_dict(target.value[k], v, path)
-        else:
-            target_value = target.value.get(k)
-            if target_value is not None and type(target_value.value) is not str:
-                raise CompositeError(path,
-                                     "{}: Cannot compose scalar on non-scalar at {}".format(
-                                         node_get_provenance(v),
-                                         node_get_provenance(target.value[k])))
-            target.value[k] = v
-        path.pop()
-
-
-# Like composite_dict(), but raises an all purpose LoadError for convenience
-#
-cpdef void composite(MappingNode target, MappingNode source) except *:
-    assert type(source.value) is dict
-    assert type(target.value) is dict
-
-    try:
-        composite_dict(target, source)
-    except CompositeError as e:
-        source_provenance = node_get_provenance(source)
-        error_prefix = ""
-        if source_provenance:
-            error_prefix = "{}: ".format(source_provenance)
-        raise LoadError(LoadErrorReason.ILLEGAL_COMPOSITE,
-                        "{}Failure composing {}: {}"
-                        .format(error_prefix,
-                                e.path,
-                                e.message)) from e
-
-
-# Like composite(target, source), but where target overrides source instead.
-#
-def composite_and_move(MappingNode target, MappingNode source):
-    composite(source, target)
-
-    cdef str key
-    cdef Node value
-    cdef list to_delete = [key for key in target.value.keys() if key not in source.value]
-    for key, value in source.value.items():
-        target.value[key] = value
-    for key in to_delete:
-        del target.value[key]
-
-
 # node_validate()
 #
 # Validate the node so as to ensure the user has not specified
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index a11ba21b9..a4496e192 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -2504,7 +2504,7 @@ class Element(Plugin):
 
             splits = project._splits.copy()
             # Extend project wide split rules with any split rules defined by the element
-            _yaml.composite(splits, element_splits)
+            element_splits.composite(splits)
 
         element_bst['split-rules'] = splits
         element_public['bst'] = element_bst
@@ -2537,7 +2537,7 @@ class Element(Plugin):
 
             overrides = elements.get_mapping(kind, default=None)
             if overrides:
-                _yaml.composite(defaults, overrides)
+                overrides.composite(defaults)
 
             # Set the data class wide
             cls.__defaults = defaults
@@ -2554,8 +2554,8 @@ class Element(Plugin):
         else:
             environment = project.base_environment.copy()
 
-        _yaml.composite(environment, default_env)
-        _yaml.composite(environment, meta.environment)
+        default_env.composite(environment)
+        meta.environment.composite(environment)
         environment._assert_fully_composited()
 
         return environment
@@ -2600,8 +2600,8 @@ class Element(Plugin):
         else:
             variables = project.base_variables.copy()
 
-        _yaml.composite(variables, default_vars)
-        _yaml.composite(variables, meta.variables)
+        default_vars.composite(variables)
+        meta.variables.composite(variables)
         variables._assert_fully_composited()
 
         for var in ('project-name', 'element-name', 'max-jobs'):
@@ -2623,7 +2623,7 @@ class Element(Plugin):
         config = cls.__defaults.get_mapping('config', default={})
         config = config.copy()
 
-        _yaml.composite(config, meta.config)
+        meta.config.composite(config)
         config._assert_fully_composited()
 
         return config
@@ -2649,8 +2649,8 @@ class Element(Plugin):
         sandbox_defaults = cls.__defaults.get_mapping('sandbox', default={})
         sandbox_defaults = sandbox_defaults.copy()
 
-        _yaml.composite(sandbox_config, sandbox_defaults)
-        _yaml.composite(sandbox_config, meta.sandbox)
+        sandbox_defaults.composite(sandbox_config)
+        meta.sandbox.composite(sandbox_config)
         sandbox_config._assert_fully_composited()
 
         # Sandbox config, unlike others, has fixed members so we should validate them
@@ -2685,7 +2685,7 @@ class Element(Plugin):
 
         # Allow elements to extend the default splits defined in their project or
         # element specific defaults
-        _yaml.composite(base_splits, element_splits)
+        element_splits.composite(base_splits)
 
         element_bst['split-rules'] = base_splits
         element_public['bst'] = element_bst
diff --git a/src/buildstream/source.py b/src/buildstream/source.py
index dfac3d4ec..7d27ac9a6 100644
--- a/src/buildstream/source.py
+++ b/src/buildstream/source.py
@@ -1291,7 +1291,7 @@ class Source(Plugin):
         config = cls.__defaults.get_mapping('config', default={})
         config = config.copy()
 
-        _yaml.composite(config, meta.config)
+        meta.config.composite(config)
         config._assert_fully_composited()
 
         return config
diff --git a/src/buildstream/testing/runcli.py b/src/buildstream/testing/runcli.py
index 16593fa8e..016f8a83a 100644
--- a/src/buildstream/testing/runcli.py
+++ b/src/buildstream/testing/runcli.py
@@ -574,7 +574,7 @@ class CliIntegration(Cli):
 
                 project_config = _yaml.load(temp_project)
 
-            _yaml.composite_dict(base_config, project_config)
+            project_config.composite(base_config)
 
             _yaml.roundtrip_dump(base_config, project_filename)