tests: test consistency of solver and spec_clauses

Previously we didn't check whether all facts on a specs were represented in the models
returned by the solver. This is an important check as it ensures that we can have
conditions (dependencies, conflicts, etc.) on any property modeled by spec_clauses.

This caught the issue reported in #32497 (https://github.com/spack/spack/issues/32497),
specifically that we don't currently model patch facts. We can fix this in a follow-on
but it is marked XFAIL for now.

lib/spack/spack/cmd/solve.py

@@ -97,7 +97,7 @@ def setup_parser(subparser):
 
 def _process_result(result, show, required_format, kwargs):
     result.raise_if_unsat()
-    opt, _, _ = min(result.answers)
+    opt, *_ = min(result.answers)
     if ("opt" in show) and (not required_format):
         tty.msg("Best of %d considered solutions." % result.nmodels)
         tty.msg("Optimization Criteria:")

lib/spack/spack/solver/asp.py

@@ -13,7 +13,7 @@
 import re
 import types
 import warnings
-from typing import Tuple
+from typing import Tuple, Union
 
 import archspec.cpu
 
@@ -240,7 +240,11 @@ def _id(thing):
 class AspFunction(AspObject):
     def __init__(self, name, args=None):
         self.name = name
-        self.args = () if args is None else tuple(args)
+
+        def simplify(arg):
+            return arg if isinstance(arg, (str, bool, int)) else str(arg)
+
+        self.args = () if args is None else tuple(simplify(arg) for arg in args)
 
     def _cmp_key(self):
         return (self.name, self.args)
@@ -275,7 +279,7 @@ def argify(arg):
             elif isinstance(arg, int):
                 return clingo.Number(arg)
             else:
-                return clingo.String(str(arg))
+                return clingo.String(arg)
 
         return clingo.Function(self.name, [argify(arg) for arg in self.args], positive=positive)
 
@@ -502,7 +506,8 @@ def _compute_specs_from_answer_set(self):
         self._concrete_specs, self._unsolved_specs = [], []
         self._concrete_specs_by_input = {}
         best = min(self.answers)
-        opt, _, answer = best
+
+        opt, _, answer, _ = best
         for input_spec in self.abstract_specs:
             key = input_spec.name
             if input_spec.virtual:
@@ -773,7 +778,7 @@ def on_model(model):
             answers = builder.build_specs(spec_attrs)
 
             # add best spec to the results
-            result.answers.append((list(min_cost), 0, answers))
+            result.answers.append((list(min_cost), 0, answers, spec_attrs))
 
             # get optimization criteria
             criteria = extract_functions(best_model, "opt_criterion")
@@ -1417,23 +1422,23 @@ def spec_clauses(self, *args, **kwargs):
     def _spec_clauses(
         self,
         spec,
-        body=False,
+        body: bool = False,
-        transitive=True,
+        transitive: bool = True,
-        expand_hashes=False,
+        expand_hashes: bool = False,
-        concrete_build_deps=False,
+        concrete_build_deps: bool = False,
+        deptype: Union[str, Tuple[str, ...]] = "all",
     ):
         """Return a list of clauses for a spec mandates are true.
 
         Arguments:
-            spec (spack.spec.Spec): the spec to analyze
+            spec: the spec to analyze
-            body (bool): if True, generate clauses to be used in rule bodies
+            body: if True, generate clauses to be used in rule bodies
                 (final values) instead of rule heads (setters).
-            transitive (bool): if False, don't generate clauses from
+            transitive: if False, don't generate clauses from dependencies.
-                dependencies (default True)
+            expand_hashes: If transitive and True, descend into hashes of concrete specs.
-            expand_hashes (bool): if True, descend into hashes of concrete specs
+            concrete_build_deps: if False, do not include pure build deps
-                (default False)
-            concrete_build_deps (bool): if False, do not include pure build deps
                 of concrete specs (as they have no effect on runtime constraints)
+            deptype: dependency types to follow when transitive (default "all").
 
         Normally, if called with ``transitive=True``, ``spec_clauses()`` just generates
         hashes for the dependency requirements of concrete specs. If ``expand_hashes``
@@ -1561,7 +1566,7 @@ class Body(object):
         # add all clauses from dependencies
         if transitive:
             # TODO: Eventually distinguish 2 deps on the same pkg (build and link)
-            for dspec in spec.edges_to_dependencies():
+            for dspec in spec.edges_to_dependencies(deptype=deptype):
                 dep = dspec.spec
 
                 if spec.concrete:
@@ -1593,6 +1598,7 @@ class Body(object):
                             body=body,
                             expand_hashes=expand_hashes,
                             concrete_build_deps=concrete_build_deps,
+                            deptype=deptype,
                         )
                     )
 

lib/spack/spack/spec.py

@@ -2928,7 +2928,7 @@ def _new_concretize(self, tests=False):
         result.raise_if_unsat()
 
         # take the best answer
-        opt, i, answer = min(result.answers)
+        opt, i, answer, _ = min(result.answers)
         name = self.name
         # TODO: Consolidate this code with similar code in solve.py
         if self.virtual:

lib/spack/spack/test/solver/asp.py

@@ -0,0 +1,72 @@
+# Copyright 2013-2022 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import itertools
+import pytest
+
+import spack.spec
+import spack.solver.asp as asp
+import spack.store
+
+
+pytestmark = [
+    pytest.mark.skipif(
+        spack.config.get("config:concretizer") == "original", reason="requires new concretizer"
+    ),
+    pytest.mark.usefixtures("mutable_config", "mock_packages"),
+]
+
+
+@pytest.fixture
+def reusable_specs(mock_packages):
+    reusable_specs = []
+    for spec in ["mpich", "openmpi", "zmpi"]:
+        reusable_specs.extend(s for s in spack.spec.Spec(spec).concretized().traverse(root=True))
+    return list(sorted(set(reusable_specs)))
+
+
+@pytest.mark.parametrize(
+    "root,reuse",
+    itertools.product(
+        ("mpileaks ^mpich", "mpileaks ^openmpi", "mpileaks ^zmpi", "patch"),
+        (True, False),
+    ),
+)
+def test_all_facts_in_solve(database, root, reuse, reusable_specs):
+    reusable_specs = reusable_specs if reuse else []
+
+    solver = spack.solver.asp.Solver()
+    setup = spack.solver.asp.SpackSolverSetup()
+    result, _, _ = solver.driver.solve(setup, [spack.spec.Spec(root)], reuse=reusable_specs)
+
+    *_, result_attrs = result.answers[0]
+    result_attrs = set(result_attrs)
+
+    def remove_hashes(attrs):
+        return []
+
+    for spec in result.specs:
+        # check only link and run deps if reusing.
+        deptype = ("link", "run") if reuse else "all"
+
+        # get all facts about the spec and filter out just the "attr" ones.
+        attrs = setup.spec_clauses(spec, deptype=deptype, body=True, expand_hashes=True)
+
+        # only consider attr() functions, not other displayed atoms
+        # don't consider any DAG/package hashes, as they are added after solving
+        attrs = set(attr for attr in attrs if attr.name == "attr" and "hash" not in attr.args[0])
+
+        # make sure all facts from the solver are in the actual solution.
+        diff = attrs - result_attrs
+
+        # this is a current bug in the solver: we don't manage dependency patches
+        # properly, and with reuse it can grab something w/o the right patch.
+        # See https://github.com/spack/spack/issues/32497
+        # TODO: Remove this XFAIL when #32497 is fixed.
+        patches = [a for a in diff if a.args[0] == "variant_value" and a.args[2] == "patches"]
+        if diff and not (diff - set(patches)):
+            pytest.xfail("Bug in new concretizer with patch constraints. See #32497.")
+
+        assert not diff

var/spack/repos/builtin.mock/packages/openmpi/package.py

@@ -12,4 +12,6 @@ class Openmpi(Package):
     variant("internal-hwloc", default=False)
     variant("fabrics", values=any_combination_of("psm", "mxm"))
 
+    provides("mpi")
+
     depends_on("hwloc", when="~internal-hwloc")