spec.py: prefer transitive link and direct build/run/test deps (#33498)

Due to reuse concretization, we may generate DAGs with two occurrences
of the same package corresponding to distinct specs. This happens when
build dependencies are reused, since their dependencies are ignored in
concretization.

This caused a regression, for example: `spec['openssl']` would take the
'openssl' of the build dep `cmake`, instead of the direct `openssl`
dependency, simply because the edge to `cmake` was traversed first and
we do depth first traversal.

One solution that was discussed is to limit `spec[name]` to just direct
deps, or direct deps + transitive link deps, but this is too breaking.
Instead, this PR simply prioritizes transitive link and direct
build/run/test deps, and then falls back to a full DAG traversal. So,
it's just about order of iteration.

2 files changed, 48 insertions, 2 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 4c1421e1e8..9256be5803 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -3970,12 +3970,23 @@ class Spec(object):
             csv = query_parameters.pop().strip()
             query_parameters = re.split(r"\s*,\s*", csv)
 
+        # In some cases a package appears multiple times in the same DAG for *distinct*
+        # specs. For example, a build-type dependency may itself depend on a package
+        # the current spec depends on, but their specs may differ. Therefore we iterate
+        # in an order here that prioritizes the build, test and runtime dependencies;
+        # only when we don't find the package do we consider the full DAG.
+        order = lambda: itertools.chain(
+            self.traverse(deptype="link"),
+            self.dependencies(deptype=("build", "run", "test")),
+            self.traverse(),  # fall back to a full search
+        )
+
         try:
             value = next(
                 itertools.chain(
                     # Regular specs
-                    (x for x in self.traverse() if x.name == name),
-                    (x for x in self.traverse() if (not x.virtual) and x.package.provides(name)),
+                    (x for x in order() if x.name == name),
+                    (x for x in order() if (not x.virtual) and x.package.provides(name)),
                 )
             )
         except StopIteration:
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 66e5a0bc2f..898ae15f74 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -1068,3 +1068,38 @@ def test_adding_same_deptype_with_the_same_name_raises(
     p.add_dependency_edge(c1, deptype=c1_deptypes)
     with pytest.raises(spack.error.SpackError):
         p.add_dependency_edge(c2, deptype=c2_deptypes)
+
+
+@pytest.mark.regression("33499")
+def test_indexing_prefers_direct_or_transitive_link_deps():
+    # Test whether spec indexing prefers direct/transitive link type deps over deps of
+    # build/run/test deps, and whether it does fall back to a full dag search.
+    root = Spec("root")
+
+    # Use a and z to since we typically traverse by edges sorted alphabetically.
+    a1 = Spec("a1")
+    a2 = Spec("a2")
+    z1 = Spec("z1")
+    z2 = Spec("z2")
+
+    # Same package, different spec.
+    z3_flavor_1 = Spec("z3 +through_a1")
+    z3_flavor_2 = Spec("z3 +through_z1")
+
+    root.add_dependency_edge(a1, deptype=("build", "run", "test"))
+
+    # unique package as a dep of a build/run/test type dep.
+    a1.add_dependency_edge(a2, deptype="all")
+    a1.add_dependency_edge(z3_flavor_1, deptype="all")
+
+    # chain of link type deps root -> z1 -> z2 -> z3
+    root.add_dependency_edge(z1, deptype="link")
+    z1.add_dependency_edge(z2, deptype="link")
+    z2.add_dependency_edge(z3_flavor_2, deptype="link")
+
+    # Indexing should prefer the link-type dep.
+    assert "through_z1" in root["z3"].variants
+    assert "through_a1" in a1["z3"].variants
+
+    # Ensure that the full DAG is still searched
+    assert root["a2"]