ASP-based solver: use edge properties for reused specs (#39508)

Since #34821 we are annotating virtual dependencies on
DAG edges, and reconstructing virtuals in memory when
we read a concrete spec from previous formats.

Therefore, we can remove a TODO in asp.py, and rely on
"virtual_on_edge" facts to be imposed.

3 files changed, 33 insertions, 31 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 7710641cb5..8ce04cdc5b 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1827,37 +1827,11 @@ class SpackSolverSetup:
                         # skip build dependencies of already-installed specs
                         if concrete_build_deps or dtype != "build":
                             clauses.append(fn.attr("depends_on", spec.name, dep.name, dtype))
-
-                            # TODO: We have to look up info from package.py here, but we'd
-                            # TODO: like to avoid this entirely. We should not need to look
-                            # TODO: up potentially wrong info if we have virtual edge info.
-                            try:
-                                try:
-                                    pkg = dep.package
-
-                                except spack.repo.UnknownNamespaceError:
-                                    # Try to look up the package of the same name and use its
-                                    # providers. This is as good as we can do without edge info.
-                                    pkg_class = spack.repo.PATH.get_pkg_class(dep.name)
-                                    spec = spack.spec.Spec(f"{dep.name}@{dep.version}")
-                                    pkg = pkg_class(spec)
-
-                                virtuals = pkg.virtuals_provided
-
-                            except spack.repo.UnknownPackageError:
-                                # Skip virtual node constriants for renamed/deleted packages,
-                                # so their binaries can still be installed.
-                                # NOTE: with current specs (which lack edge attributes) this
-                                # can allow concretizations with two providers, but it's unlikely.
-                                continue
-
-                            # Don't concretize with two providers of the same virtual.
-                            # See above for exception for unknown packages.
-                            # TODO: we will eventually record provider information on edges,
-                            # TODO: which avoids the need for the package lookup above.
-                            for virtual in virtuals:
-                                clauses.append(fn.attr("virtual_node", virtual.name))
-                                clauses.append(fn.provider(dep.name, virtual.name))
+                            for virtual_name in dspec.virtuals:
+                                clauses.append(
+                                    fn.attr("virtual_on_edge", spec.name, dep.name, virtual_name)
+                                )
+                                clauses.append(fn.attr("virtual_node", virtual_name))
 
                     # imposing hash constraints for all but pure build deps of
                     # already-installed concrete specs.
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 90b86cd160..9d1b48b379 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -113,6 +113,7 @@ unification_set(SetID, VirtualNode)
 % Node attributes that have multiple node arguments (usually, only the first argument is a node)
 multiple_nodes_attribute("node_flag_source").
 multiple_nodes_attribute("depends_on").
+multiple_nodes_attribute("virtual_on_edge").
 
 % Map constraint on the literal ID to facts on the node
 attr(Name, node(min_dupe_id, A1))             :- literal(LiteralID, Name, A1), solve_literal(LiteralID).
@@ -367,6 +368,18 @@ attr("node_flag_source", node(X, A1), A2, node(Y, A3))
   :- impose(ID, node(X, A1)),
      imposed_constraint(ID, "depends_on", A1, A2, A3).
 
+% Reconstruct virtual dependencies for reused specs
+attr("virtual_on_edge", node(X, A1), node(Y, A2), Virtual)
+  :- impose(ID, node(X, A1)),
+     depends_on(node(X, A1), node(Y, A2)),
+     imposed_constraint(ID, "virtual_on_edge", A1, A2, Virtual),
+     not build(node(X, A1)).
+
+virtual_condition_holds(node(Y, A2), Virtual)
+  :- impose(ID, node(X, A1)),
+     attr("virtual_on_edge", node(X, A1), node(Y, A2), Virtual),
+     not build(node(X, A1)).
+
 % we cannot have additional variant values when we are working with concrete specs
 :- attr("node", node(ID, Package)),
    attr("hash", node(ID, Package), Hash),
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index ba84619d04..4cd22249ca 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -2099,6 +2099,21 @@ class TestConcretize:
         edges = spec.edges_to_dependencies(name="callpath")
         assert len(edges) == 1 and edges[0].virtuals == ()
 
+    @pytest.mark.only_clingo("Use case not supported by the original concretizer")
+    @pytest.mark.db
+    @pytest.mark.parametrize(
+        "spec_str,mpi_name",
+        [("mpileaks", "mpich"), ("mpileaks ^mpich2", "mpich2"), ("mpileaks ^zmpi", "zmpi")],
+    )
+    def test_virtuals_are_reconstructed_on_reuse(self, spec_str, mpi_name, database):
+        """Tests that when we reuse a spec, virtual on edges are reconstructed correctly"""
+        with spack.config.override("concretizer:reuse", True):
+            spec = Spec(spec_str).concretized()
+            assert spec.installed
+            mpi_edges = spec.edges_to_dependencies(mpi_name)
+            assert len(mpi_edges) == 1
+            assert "mpi" in mpi_edges[0].virtuals
+
 
 @pytest.fixture()
 def duplicates_test_repository():