Avoid best-effort expansion of stacks (#40792)

fixes #40791

Currently stacks behave differently if used in unify:false
environments, which leads to inconsistencies during concretization.

For instance, we might have two abstract user specs that do not
intersect with each other map to the same concrete spec in the
environment. This is clearly wrong.

This PR removes the best effort expansion, so that user specs
are always applied strictly.

3 files changed, 35 insertions, 144 deletions

diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index 877e294b74..f6f4ffa0ad 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -58,9 +58,8 @@ from spack import traverse
 from spack.installer import PackageInstaller
 from spack.schema.env import TOP_LEVEL_KEY
 from spack.spec import Spec
-from spack.spec_list import InvalidSpecConstraintError, SpecList
+from spack.spec_list import SpecList
 from spack.util.path import substitute_path_variables
-from spack.variant import UnknownVariantError
 
 #: environment variable used to indicate the active environment
 spack_env_var = "SPACK_ENV"
@@ -1625,10 +1624,10 @@ class Environment:
 
         # Concretize any new user specs that we haven't concretized yet
         args, root_specs, i = [], [], 0
-        for uspec, uspec_constraints in zip(self.user_specs, self.user_specs.specs_as_constraints):
+        for uspec in self.user_specs:
             if uspec not in old_concretized_user_specs:
                 root_specs.append(uspec)
-                args.append((i, [str(x) for x in uspec_constraints], tests))
+                args.append((i, str(uspec), tests))
                 i += 1
 
         # Ensure we don't try to bootstrap clingo in parallel
@@ -2508,52 +2507,11 @@ def display_specs(specs):
     print(tree_string)
 
 
-def _concretize_from_constraints(spec_constraints, tests=False):
-    # Accept only valid constraints from list and concretize spec
-    # Get the named spec even if out of order
-    root_spec = [s for s in spec_constraints if s.name]
-    if len(root_spec) != 1:
-        m = "The constraints %s are not a valid spec " % spec_constraints
-        m += "concretization target. all specs must have a single name "
-        m += "constraint for concretization."
-        raise InvalidSpecConstraintError(m)
-    spec_constraints.remove(root_spec[0])
-
-    invalid_constraints = []
-    while True:
-        # Attach all anonymous constraints to one named spec
-        s = root_spec[0].copy()
-        for c in spec_constraints:
-            if c not in invalid_constraints:
-                s.constrain(c)
-        try:
-            return s.concretized(tests=tests)
-        except spack.spec.InvalidDependencyError as e:
-            invalid_deps_string = ["^" + d for d in e.invalid_deps]
-            invalid_deps = [
-                c
-                for c in spec_constraints
-                if any(c.satisfies(invd) for invd in invalid_deps_string)
-            ]
-            if len(invalid_deps) != len(invalid_deps_string):
-                raise e
-            invalid_constraints.extend(invalid_deps)
-        except UnknownVariantError as e:
-            invalid_variants = e.unknown_variants
-            inv_variant_constraints = [
-                c for c in spec_constraints if any(name in c.variants for name in invalid_variants)
-            ]
-            if len(inv_variant_constraints) != len(invalid_variants):
-                raise e
-            invalid_constraints.extend(inv_variant_constraints)
-
-
 def _concretize_task(packed_arguments) -> Tuple[int, Spec, float]:
-    index, spec_constraints, tests = packed_arguments
-    spec_constraints = [Spec(x) for x in spec_constraints]
+    index, spec_str, tests = packed_arguments
     with tty.SuppressOutput(msg_enabled=False):
         start = time.time()
-        spec = _concretize_from_constraints(spec_constraints, tests)
+        spec = Spec(spec_str).concretized(tests=tests)
         return index, spec, time.time() - start
 
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 9c32ab7610..ee725770cb 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -2336,103 +2336,6 @@ spack:
             assert mpileaks_spec not in after_conc
 
 
-def test_stack_concretize_extraneous_deps(tmpdir, mock_packages):
-    # FIXME: The new concretizer doesn't handle yet soft
-    # FIXME: constraints for stacks
-    # FIXME: This now works for statically-determinable invalid deps
-    # FIXME: But it still does not work for dynamically determined invalid deps
-
-    filename = str(tmpdir.join("spack.yaml"))
-    with open(filename, "w") as f:
-        f.write(
-            """\
-spack:
-  definitions:
-    - packages: [libelf, mpileaks]
-    - install:
-        - matrix:
-            - [$packages]
-            - ['^zmpi', '^mpich']
-  specs:
-    - $install
-"""
-        )
-    with tmpdir.as_cwd():
-        env("create", "test", "./spack.yaml")
-        with ev.read("test"):
-            concretize()
-
-        test = ev.read("test")
-
-        for user, concrete in test.concretized_specs():
-            assert concrete.concrete
-            assert not user.concrete
-            if user.name == "libelf":
-                assert not concrete.satisfies("^mpi")
-            elif user.name == "mpileaks":
-                assert concrete.satisfies("^mpi")
-
-
-def test_stack_concretize_extraneous_variants(tmpdir, mock_packages):
-    filename = str(tmpdir.join("spack.yaml"))
-    with open(filename, "w") as f:
-        f.write(
-            """\
-spack:
-  definitions:
-    - packages: [libelf, mpileaks]
-    - install:
-        - matrix:
-            - [$packages]
-            - ['~shared', '+shared']
-  specs:
-    - $install
-"""
-        )
-    with tmpdir.as_cwd():
-        env("create", "test", "./spack.yaml")
-        with ev.read("test"):
-            concretize()
-
-        test = ev.read("test")
-
-        for user, concrete in test.concretized_specs():
-            assert concrete.concrete
-            assert not user.concrete
-            if user.name == "libelf":
-                assert "shared" not in concrete.variants
-            if user.name == "mpileaks":
-                assert concrete.variants["shared"].value == user.variants["shared"].value
-
-
-def test_stack_concretize_extraneous_variants_with_dash(tmpdir, mock_packages):
-    filename = str(tmpdir.join("spack.yaml"))
-    with open(filename, "w") as f:
-        f.write(
-            """\
-spack:
-  definitions:
-    - packages: [libelf, mpileaks]
-    - install:
-        - matrix:
-            - [$packages]
-            - ['shared=False', '+shared-libs']
-  specs:
-    - $install
-"""
-        )
-    with tmpdir.as_cwd():
-        env("create", "test", "./spack.yaml")
-        with ev.read("test"):
-            concretize()
-
-        ev.read("test")
-
-        # Regression test for handling of variants with dashes in them
-        # will fail before this point if code regresses
-        assert True
-
-
 def test_stack_definition_extension(tmpdir):
     filename = str(tmpdir.join("spack.yaml"))
     with open(filename, "w") as f:
diff --git a/lib/spack/spack/test/env.py b/lib/spack/spack/test/env.py
index 2452cd937a..b239301680 100644
--- a/lib/spack/spack/test/env.py
+++ b/lib/spack/spack/test/env.py
@@ -860,3 +860,33 @@ def test_env_view_on_non_empty_dir_errors(tmp_path, config, mock_packages, tempo
     env.install_all(fake=True)
     with pytest.raises(ev.SpackEnvironmentError, match="because it is a non-empty dir"):
         env.regenerate_views()
+
+
+@pytest.mark.parametrize(
+    "matrix_line", [("^zmpi", "^mpich"), ("~shared", "+shared"), ("shared=False", "+shared-libs")]
+)
+@pytest.mark.regression("40791")
+def test_stack_enforcement_is_strict(tmp_path, matrix_line, config, mock_packages):
+    """Ensure that constraints in matrices are applied strictly after expansion, to avoid
+    inconsistencies between abstract user specs and concrete specs.
+    """
+    manifest = tmp_path / "spack.yaml"
+    manifest.write_text(
+        f"""\
+spack:
+  definitions:
+    - packages: [libelf, mpileaks]
+    - install:
+        - matrix:
+            - [$packages]
+            - [{", ".join(item for item in matrix_line)}]
+  specs:
+    - $install
+  concretizer:
+    unify: false
+"""
+    )
+    # Here we raise different exceptions depending on whether we solve serially or not
+    with pytest.raises(Exception):
+        with ev.Environment(tmp_path) as e:
+            e.concretize()