Do not check for all variables in junctionsvalentindavid/partial-variables

Junctions are loaded before all variables can loaded. So there should
not be errors for unused variables that are defined using undefined
variables.

Also this checks for undefined variables in the same time as looking
for cycles. This will allow to show more errors to the user when both
type of errors happen in the same time. This also simplify error
handling in `_variables.pyx`.

9 files changed, 82 insertions, 38 deletions

diff --git a/src/buildstream/_variables.pyx b/src/buildstream/_variables.pyx
index e3fcfc3a0..bc4b92d33 100644
--- a/src/buildstream/_variables.pyx
+++ b/src/buildstream/_variables.pyx
@@ -22,6 +22,7 @@
 
 import re
 import sys
+import itertools
 
 from ._exceptions import LoadError
 from .exceptions import LoadErrorReason
@@ -72,11 +73,18 @@ cdef class Variables:
     def __init__(self, MappingNode node):
         self.original = node
         self._expstr_map = self._resolve(node)
-        self._check_for_missing()
-        self._check_for_cycles()
+
+    def check(self):
+        self._check_variables()
 
     def __getitem__(self, str name):
-        return _expand_var(self._expstr_map, name)
+        if name not in self._expstr_map:
+            raise KeyError(name)
+        try:
+            return _expand_var(self._expstr_map, name)
+        except (KeyError, RecursionError):
+            self._check_variables(subset=[name])
+            raise
 
     def __contains__(self, str name):
         return name in self._expstr_map
@@ -105,7 +113,7 @@ cdef class Variables:
     cpdef str get(self, str name):
         if name not in self._expstr_map:
             return None
-        return _expand_var(self._expstr_map, name)
+        return self[name]
 
     # expand()
     #
@@ -146,7 +154,9 @@ cdef class Variables:
 
         try:
             return _expand_expstr(self._expstr_map, expstr)
-        except KeyError:
+        except (KeyError, RecursionError):
+            # We also check for unmatch for recursion errors since _check_variables expects
+            # subset to be defined
             unmatched = []
 
             # Look for any unmatched variable names in the expansion string
@@ -161,6 +171,9 @@ cdef class Variables:
                 )
 
                 raise LoadError(message, LoadErrorReason.UNRESOLVED_VARIABLE)
+
+            # Otherwise the missing key is from a variable definition
+            self._check_variables(subset=expstr[1::2])
             # Otherwise, re-raise the KeyError since it clearly came from some
             # other unknowable cause.
             raise
@@ -186,39 +199,41 @@ cdef class Variables:
             ret[sys.intern(key)] = _parse_expstr(value)
         return ret
 
-    def _check_for_missing(self):
-        # First the check for anything unresolvable
+
+    def _check_variables(self, *, subset=None):
         summary = []
-        for key, expstr in self._expstr_map.items():
-            for var in expstr[1::2]:
-                if var not in self._expstr_map:
-                    line = "  unresolved variable '{unmatched}' in declaration of '{variable}' at: {provenance}"
-                    provenance = self.original.get_scalar(key).get_provenance()
-                    summary.append(line.format(unmatched=var, variable=key, provenance=provenance))
-        if summary:
-            raise LoadError("Failed to resolve one or more variable:\n{}\n".format("\n".join(summary)),
-                            LoadErrorReason.UNRESOLVED_VARIABLE)
 
-    def _check_for_cycles(self):
-        # And now the cycle checks
-        def cycle_check(expstr, visited, cleared):
+        def rec_check(name, expstr, visited, cleared):
             for var in expstr[1::2]:
                 if var in cleared:
                     continue
-                if var in visited:
-                    raise LoadError("{}: ".format(self.original.get_scalar(var).get_provenance()) +
-                                    ("Variable '{}' expands to contain a reference to itself. " +
-                                     "Perhaps '{}' contains '%{{{}}}").format(var, visited[-1], var),
-                                     LoadErrorReason.RECURSIVE_VARIABLE)
-                visited.append(var)
-                cycle_check(self._expstr_map[var], visited, cleared)
-                visited.pop()
-                cleared.add(var)
+                elif var in visited:
+                    chain = list(itertools.takewhile(lambda x: x != var, reversed(visited)))
+                    chain.append(var)
+                    chain.reverse()
+                    for i in range(len(chain)-1):
+                        line = "  Variable '{variable}' recusively uses '{rec}' at: {provenance}"
+                        provenance = self.original.get_scalar(chain[i]).get_provenance()
+                        summary.append(line.format(rec=chain[i+1], variable=chain[i], provenance=provenance))
+                elif var not in self._expstr_map:
+                    line = "  unresolved variable '{unmatched}' in declaration of '{variable}' at: {provenance}"
+                    provenance = self.original.get_scalar(name).get_provenance()
+                    summary.append(line.format(unmatched=var, variable=name, provenance=provenance))
+                else:
+                    visited.append(var)
+                    rec_check(var, self._expstr_map[var], visited, cleared)
+                    visited.pop()
+                    cleared.add(var)
 
         cleared = set()
-        for key, expstr in self._expstr_map.items():
-            if key not in cleared:
-                cycle_check(expstr, [key], cleared)
+        for key in subset if subset is not None else self._expstr_map:
+            visited = []
+            rec_check(key, self._expstr_map[key], visited, cleared)
+            cleared.add(key)
+
+        if summary:
+            raise LoadError("Failed to resolve one or more variable:\n{}\n".format("\n".join(summary)),
+                            LoadErrorReason.UNRESOLVED_VARIABLE)
 
 # Cache for the parsed expansion strings.  While this is nominally
 # something which might "waste" memory, in reality each of these
diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index 6a0fa5fab..d15263000 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -285,6 +285,8 @@ class Element(Plugin):
         variables = self.__extract_variables(project, meta)
         variables["element-name"] = self.name
         self.__variables = Variables(variables)
+        if not meta.is_junction:
+            self.__variables.check()
 
         # Collect the composited environment now that we have variables
         unexpanded_env = self.__extract_environment(project, meta)
diff --git a/src/buildstream/exceptions.py b/src/buildstream/exceptions.py
index 2c455be84..6e14d2fd5 100644
--- a/src/buildstream/exceptions.py
+++ b/src/buildstream/exceptions.py
@@ -131,13 +131,10 @@ class LoadErrorReason(Enum):
     RECURSIVE_INCLUDE = 21
     """A recursive include has been encountered"""
 
-    RECURSIVE_VARIABLE = 22
-    """A recursive variable has been encountered"""
-
-    PROTECTED_VARIABLE_REDEFINED = 23
+    PROTECTED_VARIABLE_REDEFINED = 22
     """An attempt was made to set the value of a protected variable"""
 
-    DUPLICATE_DEPENDENCY = 24
+    DUPLICATE_DEPENDENCY = 23
     """A duplicate dependency was detected"""
 
     LINK_FORBIDDEN_DEPENDENCIES = 25
diff --git a/tests/format/variables.py b/tests/format/variables.py
index c5e8eebad..5fad104d1 100644
--- a/tests/format/variables.py
+++ b/tests/format/variables.py
@@ -67,7 +67,7 @@ def test_simple_cyclic_variables(cli, datafiles):
     print_warning("Performing cyclic test, if this test times out it will " + "exit the test sequence")
     project = str(datafiles)
     result = cli.run(project=project, silent=True, args=["build", "simple-cyclic.bst"])
-    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE)
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE)
 
 
 @pytest.mark.timeout(15, method="signal")
@@ -76,7 +76,7 @@ def test_cyclic_variables(cli, datafiles):
     print_warning("Performing cyclic test, if this test times out it will " + "exit the test sequence")
     project = str(datafiles)
     result = cli.run(project=project, silent=True, args=["build", "cyclic.bst"])
-    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.RECURSIVE_VARIABLE)
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE)
 
 
 @pytest.mark.parametrize("protected_var", PROTECTED_VARIABLES)
@@ -168,3 +168,13 @@ def test_variables_resolving_errors_in_public_section(cli, datafiles):
 
     result = cli.run(project=project, args=["show", "--format", "%{public}", "public_unresolved.bst"])
     result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.UNRESOLVED_VARIABLE)
+
+
+@pytest.mark.datafiles(os.path.join(DATA_DIR, "partial_context"))
+def test_partial_context_junctions(cli, datafiles):
+    project = str(datafiles)
+
+    result = cli.run(project=project, args=["show", "--format", "%{vars}", "test.bst"])
+    result.assert_success()
+    result_vars = _yaml.load_data(result.output)
+    assert result_vars.get_str("eltvar") == "/bar/foo/baz"
diff --git a/tests/format/variables/partial_context/base.bst b/tests/format/variables/partial_context/base.bst
new file mode 100644
index 000000000..10ce559a9
--- /dev/null
+++ b/tests/format/variables/partial_context/base.bst
@@ -0,0 +1,4 @@
+kind: junction
+sources:
+- kind: local
+  path: base
diff --git a/tests/format/variables/partial_context/base/project.conf b/tests/format/variables/partial_context/base/project.conf
new file mode 100644
index 000000000..91511bfe9
--- /dev/null
+++ b/tests/format/variables/partial_context/base/project.conf
@@ -0,0 +1,3 @@
+name: base
+min-version: 2.0
+
diff --git a/tests/format/variables/partial_context/base/vars.yml b/tests/format/variables/partial_context/base/vars.yml
new file mode 100644
index 000000000..3a91e1310
--- /dev/null
+++ b/tests/format/variables/partial_context/base/vars.yml
@@ -0,0 +1,2 @@
+variables:
+  subvar: "/bar"
diff --git a/tests/format/variables/partial_context/project.conf b/tests/format/variables/partial_context/project.conf
new file mode 100644
index 000000000..e8a8ed038
--- /dev/null
+++ b/tests/format/variables/partial_context/project.conf
@@ -0,0 +1,8 @@
+name: test
+min-version: 2.0
+
+(@): base.bst:vars.yml
+
+variables:
+  var: "%{subvar}/foo"
+
diff --git a/tests/format/variables/partial_context/test.bst b/tests/format/variables/partial_context/test.bst
new file mode 100644
index 000000000..8276763c3
--- /dev/null
+++ b/tests/format/variables/partial_context/test.bst
@@ -0,0 +1,3 @@
+kind: manual
+variables:
+  eltvar: "%{var}/baz"