Merge branch 'danielsilverstone-ct/fix-node-provenance' into 'master'

Improve node provenance in error reporting

Closes #1059

See merge request BuildStream/buildstream!1551

8 files changed, 59 insertions, 2 deletions

diff --git a/src/buildstream/node.pxd b/src/buildstream/node.pxd
index 02e95d06f..f2244a18f 100644
--- a/src/buildstream/node.pxd
+++ b/src/buildstream/node.pxd
@@ -70,7 +70,7 @@ cdef class MappingNode(Node):
     cdef void _compose_on_list(self, SequenceNode target)
 
     # Private Methods
-    cdef void __composite(self, MappingNode target, list path=*) except *
+    cdef void __composite(self, MappingNode target, list path) except *
     cdef Node _get(self, str key, default, default_constructor)
 
 
diff --git a/src/buildstream/node.pyx b/src/buildstream/node.pyx
index 98a785868..73911634c 100644
--- a/src/buildstream/node.pyx
+++ b/src/buildstream/node.pyx
@@ -1098,7 +1098,7 @@ cdef class MappingNode(Node):
     #   path (list): path from the root of the target when composing recursively
     #                in order to give accurate error reporting.
     #
-    cdef void __composite(self, MappingNode target, list path=None) except *:
+    cdef void __composite(self, MappingNode target, list path) except *:
         cdef str key
         cdef Node value
 
@@ -1107,6 +1107,13 @@ cdef class MappingNode(Node):
             value._compose_on(key, target, path)
             path.pop()
 
+        # Clobber the provenance of the target mapping node if we're not
+        # synthetic.
+        if self.file_index != __SYNTHETIC_FILE_INDEX:
+            target.file_index = self.file_index
+            target.line = self.line
+            target.column = self.column
+
     # _get(key, default, default_constructor)
     #
     # Internal helper method to get an entry from the underlying dictionary.
diff --git a/tests/format/invalid-keys/included-source.bst b/tests/format/invalid-keys/included-source.bst
new file mode 100644
index 000000000..559031bcd
--- /dev/null
+++ b/tests/format/invalid-keys/included-source.bst
@@ -0,0 +1,4 @@
+kind: import
+
+sources:
+  - (@): somesource.yaml
diff --git a/tests/format/invalid-keys/no-path-specified.bst b/tests/format/invalid-keys/no-path-specified.bst
new file mode 100644
index 000000000..85570b64d
--- /dev/null
+++ b/tests/format/invalid-keys/no-path-specified.bst
@@ -0,0 +1,4 @@
+kind: import
+
+sources:
+  - kind: local
diff --git a/tests/format/invalid-keys/optional-source.bst b/tests/format/invalid-keys/optional-source.bst
new file mode 100644
index 000000000..3234969a9
--- /dev/null
+++ b/tests/format/invalid-keys/optional-source.bst
@@ -0,0 +1,6 @@
+kind: import
+
+sources:
+  - (?):
+      - not thingy:
+          kind: local
diff --git a/tests/format/invalid-keys/project.conf b/tests/format/invalid-keys/project.conf
new file mode 100644
index 000000000..c24f5789e
--- /dev/null
+++ b/tests/format/invalid-keys/project.conf
@@ -0,0 +1,7 @@
+name: test
+
+options:
+  thingy:
+    type: bool
+    description: A thingy
+    default: false
diff --git a/tests/format/invalid-keys/somesource.yaml b/tests/format/invalid-keys/somesource.yaml
new file mode 100644
index 000000000..753a3bc40
--- /dev/null
+++ b/tests/format/invalid-keys/somesource.yaml
@@ -0,0 +1 @@
+kind: local
diff --git a/tests/format/invalid_keys.py b/tests/format/invalid_keys.py
new file mode 100644
index 000000000..861cfeabd
--- /dev/null
+++ b/tests/format/invalid_keys.py
@@ -0,0 +1,28 @@
+# Pylint doesn't play well with fixtures and dependency injection from pytest
+# pylint: disable=redefined-outer-name
+
+import os
+import pytest
+from buildstream._exceptions import ErrorDomain, LoadErrorReason
+from buildstream.testing.runcli import cli  # pylint: disable=unused-import
+
+# Project directory
+DATA_DIR = os.path.join(
+    os.path.dirname(os.path.realpath(__file__)),
+    'invalid-keys'
+)
+
+
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.parametrize(("element", "location"), [
+    ("no-path-specified.bst", "line 4 column 4"),
+    ("optional-source.bst", "line 6 column 10"),
+    ("included-source.bst", "line 4 column 4"),
+])
+def test_compositied_node_fails_usefully(cli, datafiles, element, location):
+    project = str(datafiles)
+    result = cli.run(project=project, args=['show', element])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
+
+    assert "synthetic node" not in result.stderr
+    assert "{} [{}]: Dictionary did not contain expected key 'path'".format(element, location) in result.stderr