refactor: make `_make_when_spec()` private to `directives.py`

`make_when_spec()` was being used in the solver, but it has semantics that are specific
to parsing when specs from `package.py`. In particular, it returns `None` when the
`when` spec is `False`, and directives are responsible for ignoring that case and not
adding requirements, deps, etc. when there's an actual `False` passed in from
`package.py`.

In `asp.py`, we know that there won't ever be a raw boolean when spec or constraint, so
we know we can parse them without any of the special boolean handling. However, we
should report where in the file the error happened on error, so this adds some parsing
logic to extract the `mark` from YAML and alert the user where the bad parse is.

- [x] refactor `config.py` so that basic `spack_yaml` mark info is in its own method
- [x] refactor `asp.py` so that it uses the smarter YAML parsing routine
- [x] refactor `asp.py` so that YAML input validation for requirements is done up front

4 files changed, 81 insertions, 46 deletions

diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index ee48a261c8..2ff51e085c 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -1580,6 +1580,49 @@ def fetch_remote_configs(url: str, dest_dir: str, skip_existing: bool = True) ->
     raise ConfigFileError(f"Cannot retrieve configuration (yaml) from {url}")
 
 
+def get_mark_from_yaml_data(obj):
+    """Try to get ``spack.util.spack_yaml`` mark from YAML data.
+
+    We try the object, and if that fails we try its first member (if it's a container).
+
+    Returns:
+        mark if one is found, otherwise None.
+    """
+    # mark of object itelf
+    mark = getattr(obj, "_start_mark", None)
+    if mark:
+        return mark
+
+    # mark of first member if it is a container
+    if isinstance(obj, (list, dict)):
+        first_member = next(iter(obj), None)
+        if first_member:
+            mark = getattr(first_member, "_start_mark", None)
+
+    return mark
+
+
+def parse_spec_from_yaml_string(string: str) -> "spack.spec.Spec":
+    """Parse a spec from YAML and add file/line info to errors, if it's available.
+
+    Parse a ``Spec`` from the supplied string, but also intercept any syntax errors and
+    add file/line information for debugging using file/line annotations from the string.
+
+    Arguments:
+        string: a string representing a ``Spec`` from config YAML.
+
+    """
+    try:
+        spec = spack.spec.Spec(string)
+        return spec
+    except spack.parser.SpecSyntaxError as e:
+        mark = spack.config.get_mark_from_yaml_data(string)
+        if mark:
+            msg = f"{mark.name}:{mark.line + 1}: {str(e)}"
+            raise spack.parser.SpecSyntaxError(msg) from e
+        raise e
+
+
 class ConfigError(SpackError):
     """Superclass for all Spack config related errors."""
 
@@ -1625,23 +1668,9 @@ class ConfigFormatError(ConfigError):
     def _get_mark(self, validation_error, data):
         """Get the file/line mark fo a validation error from a Spack YAML file."""
 
-        def _get_mark_or_first_member_mark(obj):
-            # mark of object itelf
-            mark = getattr(obj, "_start_mark", None)
-            if mark:
-                return mark
-
-            # mark of first member if it is a container
-            if isinstance(obj, (list, dict)):
-                first_member = next(iter(obj), None)
-                if first_member:
-                    mark = getattr(first_member, "_start_mark", None)
-                    if mark:
-                        return mark
-
         # Try various places, starting with instance and parent
         for obj in (validation_error.instance, validation_error.parent):
-            mark = _get_mark_or_first_member_mark(obj)
+            mark = get_mark_from_yaml_data(obj)
             if mark:
                 return mark
 
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index e7fc6d9990..80aee968c8 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -91,7 +91,7 @@ Patcher = Callable[[Union["spack.package_base.PackageBase", Dependency]], None]
 PatchesType = Optional[Union[Patcher, str, List[Union[Patcher, str]]]]
 
 
-def make_when_spec(value: WhenType) -> Optional["spack.spec.Spec"]:
+def _make_when_spec(value: WhenType) -> Optional["spack.spec.Spec"]:
     """Create a ``Spec`` that indicates when a directive should be applied.
 
     Directives with ``when`` specs, e.g.:
@@ -471,7 +471,7 @@ def _depends_on(
     type: DepType = dt.DEFAULT_TYPES,
     patches: PatchesType = None,
 ):
-    when_spec = make_when_spec(when)
+    when_spec = _make_when_spec(when)
     if not when_spec:
         return
 
@@ -547,7 +547,7 @@ def conflicts(conflict_spec: SpecType, when: WhenType = None, msg: Optional[str]
 
     def _execute_conflicts(pkg: "spack.package_base.PackageBase"):
         # If when is not specified the conflict always holds
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
@@ -599,7 +599,7 @@ def extends(spec, when=None, type=("build", "run"), patches=None):
     """
 
     def _execute_extends(pkg):
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
@@ -627,7 +627,7 @@ def provides(*specs, when: Optional[str] = None):
     def _execute_provides(pkg):
         import spack.parser  # Avoid circular dependency
 
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
@@ -685,7 +685,7 @@ def patch(
                 "Patches are not allowed in {0}: package has no code.".format(pkg.name)
             )
 
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
@@ -813,7 +813,7 @@ def variant(
     description = str(description).strip()
 
     def _execute_variant(pkg):
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         when_specs = [when_spec]
 
         if not re.match(spack.spec.IDENTIFIER_RE, name):
@@ -855,7 +855,7 @@ def resource(**kwargs):
 
     def _execute_resource(pkg):
         when = kwargs.get("when")
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
@@ -921,17 +921,17 @@ def maintainers(*names: str):
 
 def _execute_license(pkg, license_identifier: str, when):
     # If when is not specified the license always holds
-    when_spec = make_when_spec(when)
+    when_spec = _make_when_spec(when)
     if not when_spec:
         return
 
     for other_when_spec in pkg.licenses:
         if when_spec.intersects(other_when_spec):
             when_message = ""
-            if when_spec != make_when_spec(None):
+            if when_spec != _make_when_spec(None):
                 when_message = f"when {when_spec}"
             other_when_message = ""
-            if other_when_spec != make_when_spec(None):
+            if other_when_spec != _make_when_spec(None):
                 other_when_message = f"when {other_when_spec}"
             err_msg = (
                 f"{pkg.name} is specified as being licensed as {license_identifier} "
@@ -992,7 +992,7 @@ def requires(*requirement_specs: str, policy="one_of", when=None, msg=None):
             )
             raise DirectiveError(err_msg)
 
-        when_spec = make_when_spec(when)
+        when_spec = _make_when_spec(when)
         if not when_spec:
             return
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 617a11b452..82eff23119 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -20,6 +20,7 @@ import archspec.cpu
 
 import spack.config as sc
 import spack.deptypes as dt
+import spack.parser
 import spack.paths as sp
 import spack.util.path as sup
 
@@ -862,9 +863,13 @@ class ErrorHandler:
 
 
 #: Data class to collect information on a requirement
-RequirementRule = collections.namedtuple(
-    "RequirementRule", ["pkg_name", "policy", "requirements", "condition", "kind", "message"]
-)
+class RequirementRule(NamedTuple):
+    pkg_name: str
+    policy: str
+    requirements: List["spack.spec.Spec"]
+    condition: "spack.spec.Spec"
+    kind: RequirementKind
+    message: str
 
 
 class PyclingoDriver:
@@ -1352,10 +1357,18 @@ class SpackSolverSetup:
                     constraints = [constraints]
                     policy = "one_of"
 
+                # validate specs from YAML first, and fail with line numbers if parsing fails.
+                constraints = [
+                    sc.parse_spec_from_yaml_string(constraint) for constraint in constraints
+                ]
+                when_str = requirement.get("when")
+                when = sc.parse_spec_from_yaml_string(when_str) if when_str else spack.spec.Spec()
+
+                # filter constraints
                 constraints = [
-                    x
-                    for x in constraints
-                    if not self.reject_requirement_constraint(pkg_name, constraint=x, kind=kind)
+                    c
+                    for c in constraints
+                    if not self.reject_requirement_constraint(pkg_name, constraint=c, kind=kind)
                 ]
                 if not constraints:
                     continue
@@ -1367,13 +1380,13 @@ class SpackSolverSetup:
                         requirements=constraints,
                         kind=kind,
                         message=requirement.get("message"),
-                        condition=requirement.get("when"),
+                        condition=when,
                     )
                 )
         return rules
 
     def reject_requirement_constraint(
-        self, pkg_name: str, *, constraint: str, kind: RequirementKind
+        self, pkg_name: str, *, constraint: "spack.spec.Spec", kind: RequirementKind
     ) -> bool:
         """Returns True if a requirement constraint should be rejected"""
         if kind == RequirementKind.DEFAULT:
@@ -1740,20 +1753,13 @@ class SpackSolverSetup:
             virtual = rule.kind == RequirementKind.VIRTUAL
 
             pkg_name, policy, requirement_grp = rule.pkg_name, rule.policy, rule.requirements
-
             requirement_weight = 0
-            # TODO: don't call make_when_spec here; do it in directives.
-            main_requirement_condition = spack.directives.make_when_spec(rule.condition)
-            if main_requirement_condition is False:
-                continue
 
             # Write explicitly if a requirement is conditional or not
-            if main_requirement_condition != spack.spec.Spec():
+            if rule.condition != spack.spec.Spec():
                 msg = f"condition to activate requirement {requirement_grp_id}"
                 try:
-                    main_condition_id = self.condition(
-                        main_requirement_condition, name=pkg_name, msg=msg  # type: ignore
-                    )
+                    main_condition_id = self.condition(rule.condition, name=pkg_name, msg=msg)
                 except Exception as e:
                     if rule.kind != RequirementKind.DEFAULT:
                         raise RuntimeError(
diff --git a/lib/spack/spack/test/directives.py b/lib/spack/spack/test/directives.py
index 5b34e40793..a878cea6f2 100644
--- a/lib/spack/spack/test/directives.py
+++ b/lib/spack/spack/test/directives.py
@@ -101,7 +101,7 @@ def test_license_directive(config, mock_packages, package_name, expected_license
 
 def test_duplicate_exact_range_license():
     package = namedtuple("package", ["licenses", "name"])
-    package.licenses = {spack.directives.make_when_spec("+foo"): "Apache-2.0"}
+    package.licenses = {spack.spec.Spec("+foo"): "Apache-2.0"}
     package.name = "test_package"
 
     msg = (
@@ -115,7 +115,7 @@ def test_duplicate_exact_range_license():
 
 def test_overlapping_duplicate_licenses():
     package = namedtuple("package", ["licenses", "name"])
-    package.licenses = {spack.directives.make_when_spec("+foo"): "Apache-2.0"}
+    package.licenses = {spack.spec.Spec("+foo"): "Apache-2.0"}
     package.name = "test_package"
 
     msg = (