_yaml: Add 'as_bool()' and 'is_none()' to ScalarNode

- 'as_bool()' casts a ScalarNode into a boolean, understanding both
  'True' and 'False' as truthy-falsy values, as per node_get(type=bool)
  behavior
- 'is_none()' allwos checking whether the scalar node contains a 'None'
  value. Since 'None' cannot be used when working with booleans, we need
  to have a way of checking for 'None' when we actually need the
  information of whether the value is unset.
- Adapt all call places to use the new API

16 files changed, 58 insertions, 34 deletions

diff --git a/src/buildstream/_cas/casremote.py b/src/buildstream/_cas/casremote.py
index 0195e217a..56cfea20f 100644
--- a/src/buildstream/_cas/casremote.py
+++ b/src/buildstream/_cas/casremote.py
@@ -33,7 +33,7 @@ class CASRemoteSpec(namedtuple('CASRemoteSpec', 'url push server_cert client_key
     def _new_from_config_node(spec_node, basedir=None):
         _yaml.node_validate(spec_node, ['url', 'push', 'server-cert', 'client-key', 'client-cert', 'instance-name'])
         url = spec_node.get_str('url')
-        push = _yaml.node_get(spec_node, bool, 'push', default_value=False)
+        push = spec_node.get_bool('push', default=False)
         if not url:
             provenance = _yaml.node_get_provenance(spec_node, 'url')
             raise LoadError(LoadErrorReason.INVALID_DATA,
diff --git a/src/buildstream/_context.py b/src/buildstream/_context.py
index e285f6b6a..f32f5aa91 100644
--- a/src/buildstream/_context.py
+++ b/src/buildstream/_context.py
@@ -273,7 +273,7 @@ class Context():
         self.remote_execution_specs = SandboxRemote.specs_from_config_node(defaults)
 
         # Load pull build trees configuration
-        self.pull_buildtrees = _yaml.node_get(cache, bool, 'pull-buildtrees')
+        self.pull_buildtrees = cache.get_bool('pull-buildtrees')
 
         # Load cache build trees configuration
         self.cache_buildtrees = _node_get_option_str(
@@ -287,8 +287,8 @@ class Context():
             'debug', 'element-format', 'message-format'
         ])
         self.log_key_length = _yaml.node_get(logging, int, 'key-length')
-        self.log_debug = _yaml.node_get(logging, bool, 'debug')
-        self.log_verbose = _yaml.node_get(logging, bool, 'verbose')
+        self.log_debug = logging.get_bool('debug')
+        self.log_verbose = logging.get_bool('verbose')
         self.log_error_lines = _yaml.node_get(logging, int, 'error-lines')
         self.log_message_lines = _yaml.node_get(logging, int, 'message-lines')
         self.log_element_format = logging.get_str('element-format')
@@ -423,7 +423,7 @@ class Context():
             # so work out if we should be strict, and then cache the result
             toplevel = self.get_toplevel_project()
             overrides = self.get_overrides(toplevel.name)
-            self._strict_build_plan = _yaml.node_get(overrides, bool, 'strict', default_value=True)
+            self._strict_build_plan = overrides.get_bool('strict', default=True)
 
         # If it was set by the CLI, it overrides any config
         # Ditto if we've already computed this, then we return the computed
diff --git a/src/buildstream/_gitsourcebase.py b/src/buildstream/_gitsourcebase.py
index eefa36b1d..61ecd36eb 100644
--- a/src/buildstream/_gitsourcebase.py
+++ b/src/buildstream/_gitsourcebase.py
@@ -388,7 +388,7 @@ class _GitSourceBase(Source):
             self.node_validate(tag_node, ['tag', 'commit', 'annotated'])
 
         tags = self._load_tags(node)
-        self.track_tags = self.node_get_member(node, bool, 'track-tags', False)
+        self.track_tags = node.get_bool('track-tags', default=False)
 
         self.original_url = node.get_str('url')
         self.mirror = self.BST_MIRROR_CLASS(self, '', self.original_url, ref, tags=tags, primary=True)
@@ -405,7 +405,7 @@ class _GitSourceBase(Source):
             raise SourceError("{}: Git sources require a ref and/or track".format(self),
                               reason="missing-track-and-ref")
 
-        self.checkout_submodules = self.node_get_member(node, bool, 'checkout-submodules', True)
+        self.checkout_submodules = node.get_bool('checkout-submodules', default=True)
         self.submodules = []
 
         # Parse a dict of submodule overrides, stored in the submodule_overrides
@@ -423,7 +423,7 @@ class _GitSourceBase(Source):
 
             self.submodule_overrides[path] = url
             if 'checkout' in submodule:
-                checkout = self.node_get_member(submodule, bool, 'checkout')
+                checkout = submodule.get_bool('checkout')
                 self.submodule_checkout_overrides[path] = checkout
 
         self.mark_download_url(self.original_url)
@@ -667,7 +667,7 @@ class _GitSourceBase(Source):
         for tag_node in tags_node:
             tag = tag_node.get_str('tag')
             commit_ref = tag_node.get_str('commit')
-            annotated = self.node_get_member(tag_node, bool, 'annotated')
+            annotated = tag_node.get_bool('annotated')
             tags.append((tag, commit_ref, annotated))
         return tags
 
diff --git a/src/buildstream/_options/optionbool.py b/src/buildstream/_options/optionbool.py
index caeff9711..1e5935e48 100644
--- a/src/buildstream/_options/optionbool.py
+++ b/src/buildstream/_options/optionbool.py
@@ -34,13 +34,13 @@ class OptionBool(Option):
 
         super().load(node)
         _yaml.node_validate(node, OPTION_SYMBOLS + ['default'])
-        self.value = _yaml.node_get(node, bool, 'default')
+        self.value = node.get_bool('default')
 
     def load_value(self, node, *, transform=None):
         if transform:
             self.set_value(transform(node.get_str(self.name)))
         else:
-            self.value = _yaml.node_get(node, bool, self.name)
+            self.value = node.get_bool(self.name)
 
     def set_value(self, value):
         if value in ('True', 'true'):
diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py
index bb1b06b43..6157206ff 100644
--- a/src/buildstream/_project.py
+++ b/src/buildstream/_project.py
@@ -687,13 +687,10 @@ class Project():
         self._splits = config.get_mapping('split-rules')
 
         # Support backwards compatibility for fail-on-overlap
-        fail_on_overlap = _yaml.node_get(config, bool, 'fail-on-overlap', default_value=None)
-
-        if (CoreWarnings.OVERLAPS not in self._fatal_warnings) and fail_on_overlap:
-            self._fatal_warnings.append(CoreWarnings.OVERLAPS)
+        fail_on_overlap = config.get_scalar('fail-on-overlap', None)
 
         # Deprecation check
-        if fail_on_overlap is not None:
+        if not fail_on_overlap.is_none():
             self._context.message(
                 Message(
                     None,
@@ -703,6 +700,9 @@ class Project():
                 )
             )
 
+            if (CoreWarnings.OVERLAPS not in self._fatal_warnings) and fail_on_overlap.as_bool():
+                self._fatal_warnings.append(CoreWarnings.OVERLAPS)
+
         # Load project.refs if it exists, this may be ignored.
         if self.ref_storage == ProjectRefStorage.PROJECT_REFS:
             self.refs.load(self.options)
diff --git a/src/buildstream/_variables.pyx b/src/buildstream/_variables.pyx
index c41ff5c3f..e8f083912 100644
--- a/src/buildstream/_variables.pyx
+++ b/src/buildstream/_variables.pyx
@@ -120,7 +120,7 @@ cdef class Variables:
         # element, then override max-jobs to be 1.
         # Initialize it as a string as all variables are processed as strings.
         #
-        if _yaml.node_get(node, bool, 'notparallel', None, False):
+        if node.get_bool('notparallel', False):
             _yaml.node_set(node, 'max-jobs', str(1))
 
         cdef dict ret = {}
diff --git a/src/buildstream/_workspaces.py b/src/buildstream/_workspaces.py
index a4f7abc76..bcf0ac92c 100644
--- a/src/buildstream/_workspaces.py
+++ b/src/buildstream/_workspaces.py
@@ -630,7 +630,7 @@ class Workspaces():
     #
     def _load_workspace(self, node):
         dictionary = {
-            'prepared': _yaml.node_get(node, bool, 'prepared', default_value=False),
+            'prepared': node.get_bool('prepared', default=False),
             'path': node.get_str('path'),
             'last_successful': node.get_str('last_successful', default=None),
             'running_files': _yaml.node_sanitize(
diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd
index b2237f075..d308a23a8 100644
--- a/src/buildstream/_yaml.pxd
+++ b/src/buildstream/_yaml.pxd
@@ -32,11 +32,14 @@ cdef class MappingNode(Node):
     cdef Node get(self, str key, default, default_constructor)
     cpdef MappingNode get_mapping(self, str key, default=*)
     cpdef ScalarNode get_scalar(self, str key, default=*)
+    cpdef bint get_bool(self, str key, default=*) except *
     cpdef str get_str(self, str key, object default=*)
 
 
 cdef class ScalarNode(Node):
+    cpdef bint as_bool(self) except *
     cpdef str as_str(self)
+    cpdef bint is_none(self)
 
 
 cdef class ProvenanceInformation:
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index a7779c41e..eb73ef4d6 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -85,6 +85,25 @@ cdef class ScalarNode(Node):
         self.line = line
         self.column = column
 
+    cpdef bint is_none(self):
+        return self.value is None
+
+    cpdef bint as_bool(self) except *:
+        if type(self.value) is bool:
+            return self.value
+
+        # Don't coerce booleans to string, this makes "False" strings evaluate to True
+        if self.value in ('True', 'true'):
+            return True
+        elif self.value in ('False', 'false'):
+            return False
+        else:
+            provenance = node_get_provenance(self)
+            path = node_find_target(provenance.toplevel, self)[-1]
+            raise LoadError(LoadErrorReason.INVALID_DATA,
+                "{}: Value of '{}' is not of the expected type '{}'"
+                .format(provenance, path, bool.__name__, self.value))
+
     cpdef str as_str(self):
         # We keep 'None' as 'None' to simplify the API's usage and allow chaining for users
         if self.value is None:
@@ -132,7 +151,7 @@ cdef class MappingNode(Node):
 
         if type(value) is not ScalarNode:
             if value is None:
-                value = ScalarNode(None, _SYNTHETIC_FILE_INDEX, 0, next_synthetic_counter())
+                value = ScalarNode(None, self.file_index, 0, next_synthetic_counter())
             else:
                 provenance = node_get_provenance(value)
                 raise LoadError(LoadErrorReason.INVALID_DATA,
@@ -141,6 +160,11 @@ cdef class MappingNode(Node):
 
         return value
 
+    cpdef bint get_bool(self, str key, object default=_sentinel) except *:
+        # TODO: don't go through 'get_scalar', we can directly get everything and optimize
+        cdef ScalarNode scalar = self.get_scalar(key, default)
+        return scalar.as_bool()
+
     cpdef str get_str(self, str key, object default=_sentinel):
         # TODO: don't go through 'get_scalar', we can directly get everything and optimize
         cdef ScalarNode scalar = self.get_scalar(key, default)
@@ -574,8 +598,6 @@ def dump(object contents, str filename=None):
 # Returns: The Provenance of the dict, member or list element
 #
 cpdef ProvenanceInformation node_get_provenance(Node node, str key=None, list indices=None):
-    assert type(node.value) is dict
-
     if key is None:
         # Retrieving the provenance for this node directly
         return ProvenanceInformation(node)
diff --git a/src/buildstream/plugin.py b/src/buildstream/plugin.py
index a12ff61ec..e209e5f4e 100644
--- a/src/buildstream/plugin.py
+++ b/src/buildstream/plugin.py
@@ -894,10 +894,10 @@ class Plugin():
             project = self.__project
 
             for key, value in self.node_items(project.element_overrides):
-                if _yaml.node_get(value, bool, 'suppress-deprecation-warnings', default_value=False):
+                if value.get_bool('suppress-deprecation-warnings', default=False):
                     silenced_warnings.add(key)
             for key, value in self.node_items(project.source_overrides):
-                if _yaml.node_get(value, bool, 'suppress-deprecation-warnings', default_value=False):
+                if value.get_bool('suppress-deprecation-warnings', default=False):
                     silenced_warnings.add(key)
 
             return self.get_kind() in silenced_warnings
diff --git a/src/buildstream/plugins/elements/compose.py b/src/buildstream/plugins/elements/compose.py
index b672cde0c..b2e52170e 100644
--- a/src/buildstream/plugins/elements/compose.py
+++ b/src/buildstream/plugins/elements/compose.py
@@ -65,10 +65,10 @@ class ComposeElement(Element):
 
         # We name this variable 'integration' only to avoid
         # collision with the Element.integrate() method.
-        self.integration = self.node_get_member(node, bool, 'integrate')
+        self.integration = node.get_bool('integrate')
         self.include = self.node_get_member(node, list, 'include')
         self.exclude = self.node_get_member(node, list, 'exclude')
-        self.include_orphans = self.node_get_member(node, bool, 'include-orphans')
+        self.include_orphans = node.get_bool('include-orphans')
 
     def preflight(self):
         pass
diff --git a/src/buildstream/plugins/elements/filter.py b/src/buildstream/plugins/elements/filter.py
index 7fdb96d7a..e62a9d49b 100644
--- a/src/buildstream/plugins/elements/filter.py
+++ b/src/buildstream/plugins/elements/filter.py
@@ -173,7 +173,7 @@ class FilterElement(Element):
 
         self.include = self.node_get_member(node, list, 'include')
         self.exclude = self.node_get_member(node, list, 'exclude')
-        self.include_orphans = self.node_get_member(node, bool, 'include-orphans')
+        self.include_orphans = node.get_bool('include-orphans')
         self.include_provenance = self.node_provenance(node, member_name='include')
         self.exclude_provenance = self.node_provenance(node, member_name='exclude')
 
diff --git a/src/buildstream/plugins/elements/script.py b/src/buildstream/plugins/elements/script.py
index 0d194dcc1..df03bad17 100644
--- a/src/buildstream/plugins/elements/script.py
+++ b/src/buildstream/plugins/elements/script.py
@@ -60,8 +60,7 @@ class ScriptElement(buildstream.ScriptElement):
 
         self.set_work_dir()
         self.set_install_root()
-        self.set_root_read_only(self.node_get_member(node, bool,
-                                                     'root-read-only', False))
+        self.set_root_read_only(node.get_bool('root-read-only', default=False))
 
 
 # Plugin entry point
diff --git a/src/buildstream/plugins/sources/remote.py b/src/buildstream/plugins/sources/remote.py
index 4e9fe4f7d..bb4c11325 100644
--- a/src/buildstream/plugins/sources/remote.py
+++ b/src/buildstream/plugins/sources/remote.py
@@ -63,7 +63,7 @@ class RemoteSource(DownloadableFileSource):
         super().configure(node)
 
         self.filename = node.get_str('filename', os.path.basename(self.url))
-        self.executable = self.node_get_member(node, bool, 'executable', False)
+        self.executable = node.get_bool('executable', default=False)
 
         if os.sep in self.filename:
             raise SourceError('{}: filename parameter cannot contain directories'.format(self),
diff --git a/tests/format/include.py b/tests/format/include.py
index 82667f19f..7c5a35a65 100644
--- a/tests/format/include.py
+++ b/tests/format/include.py
@@ -28,7 +28,7 @@ def test_include_project_file(cli, datafiles):
         'element.bst'])
     result.assert_success()
     loaded = _yaml.load_data(result.output)
-    assert _yaml.node_get(loaded, bool, 'included')
+    assert loaded.get_bool('included')
 
 
 def test_include_missing_file(cli, tmpdir):
@@ -87,7 +87,7 @@ def test_include_junction_file(cli, tmpdir, datafiles):
         'element.bst'])
     result.assert_success()
     loaded = _yaml.load_data(result.output)
-    assert _yaml.node_get(loaded, bool, 'included')
+    assert loaded.get_bool('included')
 
 
 @pytest.mark.datafiles(DATA_DIR)
@@ -310,4 +310,4 @@ def test_local_to_junction(cli, tmpdir, datafiles):
         'element.bst'])
     result.assert_success()
     loaded = _yaml.load_data(result.output)
-    assert _yaml.node_get(loaded, bool, 'included')
+    assert loaded.get_bool('included')
diff --git a/tests/sources/git.py b/tests/sources/git.py
index 2b8a12997..e3f803469 100644
--- a/tests/sources/git.py
+++ b/tests/sources/git.py
@@ -855,7 +855,7 @@ def test_git_describe(cli, tmpdir, datafiles, ref_storage, tag_type):
             assert 'tag' in tag
             assert 'commit' in tag
             assert 'annotated' in tag
-            assert _yaml.node_get(tag, bool, 'annotated') == (tag_type == 'annotated')
+            assert tag.get_bool('annotated') == (tag_type == 'annotated')
 
         assert {(tag.get_str('tag'),
                  tag.get_str('commit'))
@@ -970,7 +970,7 @@ def test_git_describe_head_is_tagged(cli, tmpdir, datafiles, ref_storage, tag_ty
         assert 'tag' in tag
         assert 'commit' in tag
         assert 'annotated' in tag
-        assert _yaml.node_get(tag, bool, 'annotated') == (tag_type == 'annotated')
+        assert tag.get_bool('annotated') == (tag_type == 'annotated')
 
         tag_name = tag.get_str('tag')
         commit = tag.get_str('commit')