concretizer: unify logic for spec conditionals

This builds on #20638 by unifying all the places in the concretizer where
things are conditional on specs. Previously, we duplicated a common spec
conditional pattern for dependencies, virtual providers, conflicts, and
externals. That was introduced in #20423 and refined in #20507, and
roughly looked as follows.

Given some directives in a package like:

```python
depends_on("foo@1.0+bar", when="@2.0+variant")
provides("mpi@2:", when="@1.9:")
```

We handled the `@2.0+variant` and `@1.9:` parts by generating generated
`dependency_condition()`, `required_dependency_condition()`, and
`imposed_dependency_condition()` facts to trigger rules like this:

```prolog
dependency_conditions_hold(ID, Parent, Dependency) :-
  attr(Name, Arg1)             : required_dependency_condition(ID, Name, Arg1);
  attr(Name, Arg1, Arg2)       : required_dependency_condition(ID, Name, Arg1, Arg2);
  attr(Name, Arg1, Arg2, Arg3) : required_dependency_condition(ID, Name, Arg1, Arg2, Arg3);
  dependency_condition(ID, Parent, Dependency);
  node(Parent).
```

And we handled `foo@1.0+bar` and `mpi@2:` parts ("imposed constraints")
like this:

```prolog
attr(Name, Arg1, Arg2) :-
  dependency_conditions_hold(ID, Package, Dependency),
  imposed_dependency_condition(ID, Name, Arg1, Arg2).

attr(Name, Arg1, Arg2, Arg3) :-
  dependency_conditions_hold(ID, Package, Dependency),
  imposed_dependency_condition(ID, Name, Arg1, Arg2, Arg3).
```

These rules were repeated with different input predicates for
requirements (e.g., `required_dependency_condition`) and imposed
constraints (e.g., `imposed_dependency_condition`) throughout
`concretize.lp`. In #20638 it got to be a bit confusing, because we used
the same `dependency_condition_holds` predicate to impose constraints on
conditional dependencies and virtual providers. So, even though the
pattern was repeated, some of the conditional rules were conjoined in a
weird way.

Instead of repeating this pattern everywhere, we now have *one* set of
consolidated rules for conditions:

```prolog
condition_holds(ID) :-
  condition(ID);
  attr(Name, A1)         : condition_requirement(ID, Name, A1);
  attr(Name, A1, A2)     : condition_requirement(ID, Name, A1, A2);
  attr(Name, A1, A2, A3) : condition_requirement(ID, Name, A1, A2, A3).

attr(Name, A1)         :- condition_holds(ID), imposed_constraint(ID, Name, A1).
attr(Name, A1, A2)     :- condition_holds(ID), imposed_constraint(ID, Name, A1, A2).
attr(Name, A1, A2, A3) :- condition_holds(ID), imposed_constraint(ID, Name, A1, A2, A3).
```

this allows us to use `condition(ID)` and `condition_holds(ID)` to
encapsulate the conditional logic on specs in all the scenarios where we
need it. Instead of defining predicates for the requirements and imposed
constraints, we generate the condition inputs with generic facts, and
define predicates to associate the condition ID with a particular
scenario. So, now, the generated facts for a condition look like this:

```prolog
condition(121).
condition_requirement(121,"node","cairo").
condition_requirement(121,"variant_value","cairo","fc","True").
imposed_constraint(121,"version_satisfies","fontconfig","2.10.91:").
dependency_condition(121,"cairo","fontconfig").
dependency_type(121,"build").
dependency_type(121,"link").
```

The requirements and imposed constraints are generic, and we associate
them with their meaning via the id. Here, `dependency_condition(121,
"cairo", "fontconfig")` tells us that condition 121 has to do with the
dependency of `cairo` on `fontconfig`, and the conditional dependency
rules just become:

```prolog
dependency_holds(Package, Dependency, Type) :-
  dependency_condition(ID, Package, Dependency),
  dependency_type(ID, Type),
  condition_holds(ID).
```

Dependencies, virtuals, conflicts, and externals all now use similar
patterns, and the logic for generating condition facts is common to all
of them on the python side, as well. The more specific routines like
`package_dependencies_rules` just call `self.condition(...)` to get an id
and generate requirements and imposed constraints, then they generate
their extra facts with the returned id, like this:

```python
    def package_dependencies_rules(self, pkg, tests):
        """Translate 'depends_on' directives into ASP logic."""
        for _, conditions in sorted(pkg.dependencies.items()):
            for cond, dep in sorted(conditions.items()):
                condition_id = self.condition(cond, dep.spec, pkg.name)  # create a condition and get its id
                self.gen.fact(fn.dependency_condition(  # associate specifics about the dependency w/the id
                    condition_id, pkg.name, dep.spec.name
                ))
        # etc.
```

- [x] unify generation and logic for conditions
- [x] use unified logic for dependencies
- [x] use unified logic for virtuals
- [x] use unified logic for conflicts
- [x] use unified logic for externals

LocalWords:  concretizer mpi attr Arg concretize lp cairo fc fontconfig
LocalWords:  virtuals def pkg cond dep fn refactor github py

3 files changed, 119 insertions, 183 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index e61ebc74fd..c2dd241477 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -483,28 +483,12 @@ class SpackSolverSetup(object):
 
     def conflict_rules(self, pkg):
         for trigger, constraints in pkg.conflicts.items():
+            trigger_id = self.condition(spack.spec.Spec(trigger), name=pkg.name)
+            self.gen.fact(fn.conflict_trigger(trigger_id))
+
             for constraint, _ in constraints:
-                constraint_body = spack.spec.Spec(pkg.name)
-                constraint_body.constrain(constraint)
-                constraint_body.constrain(trigger)
-
-                clauses = []
-                for s in constraint_body.traverse():
-                    clauses += self.spec_clauses(s, body=True)
-
-                # TODO: find a better way to generate clauses for integrity
-                # TODO: constraints, instead of generating them for the body
-                # TODO: of a rule and filter unwanted functions.
-                to_be_filtered = ['node_compiler_hard']
-                clauses = [x for x in clauses if x.name not in to_be_filtered]
-
-                # Emit facts based on clauses
-                condition_id = next(self._condition_id_counter)
-                self.gen.fact(fn.conflict(condition_id, pkg.name))
-                for clause in clauses:
-                    self.gen.fact(fn.conflict_condition(
-                        condition_id, clause.name, *clause.args
-                    ))
+                constraint_id = self.condition(constraint, name=pkg.name)
+                self.gen.fact(fn.conflict(pkg.name, trigger_id, constraint_id))
                 self.gen.newline()
 
     def available_compilers(self):
@@ -642,50 +626,44 @@ class SpackSolverSetup(object):
             )
         )
 
-    def _condition_facts(
-            self, pkg_name, cond_spec, dep_spec,
-            cond_fn, require_fn, impose_fn
-    ):
+    def condition(self, required_spec, imposed_spec=None, name=None):
         """Generate facts for a dependency or virtual provider condition.
 
         Arguments:
-            pkg_name (str): name of the package that triggers the
-                condition (e.g., the dependent or the provider)
-            cond_spec (Spec): the dependency spec representing the
-                condition that needs to be True (can be anonymous)
-            dep_spec (Spec): the sepc of the dependency or provider
-                to be depended on/provided if the condition holds.
-            cond_fn (AspFunction): function to use to declare the condition;
-                will be called with the cond id, pkg_name, an dep_spec.name
-            require_fn (AspFunction): function to use to declare the conditions
-                required of the dependent/provider to trigger
-            impose_fn (AspFunction): function to use for constraints imposed
-                on the dependency/virtual
+            required_spec (Spec): the spec that triggers this condition
+            imposed_spec (optional, Spec): the sepc with constraints that
+                are imposed when this condition is triggered
+            name (optional, str): name for `required_spec` (required if
+                required_spec is anonymous, ignored if not)
 
         Returns:
             (int): id of the condition created by this function
         """
-        condition_id = next(self._condition_id_counter)
-        named_cond = cond_spec.copy()
-        named_cond.name = named_cond.name or pkg_name
+        named_cond = required_spec.copy()
+        named_cond.name = named_cond.name or name
+        assert named_cond.name, "must provide name for anonymous condtions!"
 
-        self.gen.fact(cond_fn(condition_id, pkg_name, dep_spec.name))
+        condition_id = next(self._condition_id_counter)
+        self.gen.fact(fn.condition(condition_id))
 
-        # conditions that trigger the condition
-        conditions = self.checked_spec_clauses(
-            named_cond, body=True, required_from=pkg_name
-        )
-        for pred in conditions:
-            self.gen.fact(require_fn(condition_id, pred.name, *pred.args))
+        # requirements trigger the condition
+        requirements = self.checked_spec_clauses(
+            named_cond, body=True, required_from=name)
+        for pred in requirements:
+            self.gen.fact(
+                fn.condition_requirement(condition_id, pred.name, *pred.args)
+            )
 
-        imposed_constraints = self.checked_spec_clauses(
-            dep_spec, required_from=pkg_name
-        )
-        for pred in imposed_constraints:
-            # imposed "node"-like conditions are no-ops
-            if pred.name in ("node", "virtual_node"):
-                continue
-            self.gen.fact(impose_fn(condition_id, pred.name, *pred.args))
+        if imposed_spec:
+            imposed_constraints = self.checked_spec_clauses(
+                imposed_spec, body=False, required_from=name)
+            for pred in imposed_constraints:
+                # imposed "node"-like conditions are no-ops
+                if pred.name in ("node", "virtual_node"):
+                    continue
+                self.gen.fact(
+                    fn.imposed_constraint(condition_id, pred.name, *pred.args)
+                )
 
         return condition_id
 
@@ -695,25 +673,20 @@ class SpackSolverSetup(object):
 
         for provided, whens in pkg.provided.items():
             for when in whens:
-                self._condition_facts(
-                    pkg.name, when, provided,
-                    fn.provider_condition,
-                    fn.required_provider_condition,
-                    fn.imposed_dependency_condition
-                )
-
+                condition_id = self.condition(when, provided, pkg.name)
+                self.gen.fact(fn.provider_condition(
+                    condition_id, when.name, provided.name
+                ))
             self.gen.newline()
 
     def package_dependencies_rules(self, pkg, tests):
         """Translate 'depends_on' directives into ASP logic."""
         for _, conditions in sorted(pkg.dependencies.items()):
             for cond, dep in sorted(conditions.items()):
-                condition_id = self._condition_facts(
-                    pkg.name, cond, dep.spec,
-                    fn.dependency_condition,
-                    fn.required_dependency_condition,
-                    fn.imposed_dependency_condition
-                )
+                condition_id = self.condition(cond, dep.spec, pkg.name)
+                self.gen.fact(fn.dependency_condition(
+                    condition_id, pkg.name, dep.spec.name
+                ))
 
                 for t in sorted(dep.type):
                     # Skip test dependencies if they're not requested at all
@@ -794,24 +767,13 @@ class SpackSolverSetup(object):
                     pkg_name, str(version), weight, id
                 ))
 
+            # Declare external conditions with a local index into packages.yaml
             for local_idx, spec in enumerate(external_specs):
-                condition_id = next(self._condition_id_counter)
-
-                # Declare the global ID associated with this external spec
-                self.gen.fact(fn.external_spec(condition_id, pkg_name))
-
-                # Local index into packages.yaml
+                condition_id = self.condition(spec)
                 self.gen.fact(
-                    fn.external_spec_index(condition_id, pkg_name, local_idx))
-
-                # Add conditions to be satisfied for this external
+                    fn.possible_external(condition_id, pkg_name, local_idx)
+                )
                 self.possible_versions[spec.name].add(spec.version)
-                clauses = self.spec_clauses(spec, body=True)
-                for clause in clauses:
-                    self.gen.fact(
-                        fn.external_spec_condition(
-                            condition_id, clause.name, *clause.args)
-                    )
                 self.gen.newline()
 
     def preferred_variants(self, pkg_name):
@@ -1475,7 +1437,7 @@ class SpecBuilder(object):
     def no_flags(self, pkg, flag_type):
         self._specs[pkg].compiler_flags[flag_type] = []
 
-    def external_spec_selected(self, condition_id, pkg, idx):
+    def external_spec_selected(self, pkg, idx):
         """This means that the external spec and index idx
         has been selected for this package.
         """
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 22ba77142a..6e3f89a10d 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -37,15 +37,53 @@ version_satisfies(Package, Constraint)
 #defined version_satisfies/3.
 
 %-----------------------------------------------------------------------------
+% Spec conditions and imposed constraints
+%
+% Given Spack directives like these:
+%    depends_on("foo@1.0+bar", when="@2.0+variant")
+%    provides("mpi@2:", when="@1.9:")
+%
+% The conditions are `@2.0+variant` and `@1.9:`, and the imposed constraints
+% are `@1.0+bar` on `foo` and `@2:` on `mpi`.
+%-----------------------------------------------------------------------------
+% conditions are specified with `condition_requirement` and hold when
+% corresponding spec attributes hold.
+condition_holds(ID) :-
+  condition(ID);
+  attr(Name, A1)         : condition_requirement(ID, Name, A1);
+  attr(Name, A1, A2)     : condition_requirement(ID, Name, A1, A2);
+  attr(Name, A1, A2, A3) : condition_requirement(ID, Name, A1, A2, A3).
+
+% conditions that hold impose constraints on other specs
+attr(Name, A1)         :- condition_holds(ID), imposed_constraint(ID, Name, A1).
+attr(Name, A1, A2)     :- condition_holds(ID), imposed_constraint(ID, Name, A1, A2).
+attr(Name, A1, A2, A3) :- condition_holds(ID), imposed_constraint(ID, Name, A1, A2, A3).
+
+#defined condition/1.
+#defined condition_requirement/3.
+#defined condition_requirement/4.
+#defined condition_requirement/5.
+#defined imposed_constraint/3.
+#defined imposed_constraint/4.
+#defined imposed_constraint/5.
+
+%-----------------------------------------------------------------------------
 % Dependency semantics
 %-----------------------------------------------------------------------------
 % Dependencies of any type imply that one package "depends on" another
 depends_on(Package, Dependency) :- depends_on(Package, Dependency, _).
 
+% a dependency holds if its condition holds
+dependency_holds(Package, Dependency, Type) :-
+  dependency_condition(ID, Package, Dependency),
+  dependency_type(ID, Type),
+  condition_holds(ID).
+
 % declared dependencies are real if they're not virtual AND
-% the package is not an external
+% the package is not an external.
+% They're only triggered if the associated dependnecy condition holds.
 depends_on(Package, Dependency, Type)
- :- dependency_conditions(Package, Dependency, Type),
+ :- dependency_holds(Package, Dependency, Type),
     not virtual(Dependency),
     not external(Package).
 
@@ -69,74 +107,19 @@ path(Parent, Child) :- depends_on(Parent, Child).
 path(Parent, Descendant) :- path(Parent, A), depends_on(A, Descendant).
 :- path(A, B), path(B, A).
 
-%-----------------------------------------------------------------------------
-% Conditional dependencies
-%
-% This takes care of `when=SPEC` in `depends_on("foo@1.0+bar", when="SPEC")`.
-%-----------------------------------------------------------------------------
-% if any individual condition below is true, trigger the dependency.
-dependency_conditions(Package, Dependency, Type) :-
-  dependency_conditions_hold(ID, Package, Dependency),
-  dependency_type(ID, Type).
-
 #defined dependency_type/2.
-
-% collect all the dependency conditions into a single conditional rule
-% distinguishing between Parent and Package (Arg1) is needed to account
-% for conditions like:
-%
-% depends_on('patchelf@0.9', when='@1.0:1.1 ^python@:2')
-%
-% that include dependencies
-dependency_conditions_hold(ID, Parent, Dependency) :-
-  attr(Name, Arg1)             : required_dependency_condition(ID, Name, Arg1);
-  attr(Name, Arg1, Arg2)       : required_dependency_condition(ID, Name, Arg1, Arg2);
-  attr(Name, Arg1, Arg2, Arg3) : required_dependency_condition(ID, Name, Arg1, Arg2, Arg3);
-  dependency_condition(ID, Parent, Dependency);
-  % There must be at least a dependency type declared,
-  % otherwise the dependency doesn't hold
-  dependency_type(ID, _);
-  node(Parent);
-  not external(Parent).
-
 #defined dependency_condition/3.
-#defined required_dependency_condition/3.
-#defined required_dependency_condition/4.
-#defined required_dependency_condition/5.
-
-%-----------------------------------------------------------------------------
-% Imposed constraints on dependencies
-%
-% This handles the `@1.0+bar` in `depends_on("foo@1.0+bar", when="SPEC")`, or
-% the `mpi@2:` in `provides("mpi@2:", when="@1.9:")`.
-%-----------------------------------------------------------------------------
-% NOTE: `attr(Name, Arg1)` is omitted here b/c the only single-arg attribute is
-% NOTE: `node()`, which is handled above under "Dependency Semantics"
-
-attr(Name, Arg1, Arg2) :-
-  dependency_conditions_hold(ID, Package, Dependency),
-  imposed_dependency_condition(ID, Name, Arg1, Arg2).
-
-attr(Name, Arg1, Arg2, Arg3) :-
-  dependency_conditions_hold(ID, Package, Dependency),
-  imposed_dependency_condition(ID, Name, Arg1, Arg2, Arg3).
-
-#defined imposed_dependency_condition/4.
-#defined imposed_dependency_condition/5.
 
 %-----------------------------------------------------------------------------
 % Conflicts
 %-----------------------------------------------------------------------------
-:- not external(Package)        : conflict_condition(ID, "node", Package);
-   attr(Name, Arg1)             : conflict_condition(ID, Name, Arg1);
-   attr(Name, Arg1, Arg2)       : conflict_condition(ID, Name, Arg1, Arg2);
-   attr(Name, Arg1, Arg2, Arg3) : conflict_condition(ID, Name, Arg1, Arg2, Arg3);
-   conflict(ID, Package).
+:- node(Package),
+   not external(Package),
+   conflict(Package, TriggerID, ConstraintID),
+   condition_holds(TriggerID),
+   condition_holds(ConstraintID).
 
-#defined conflict/2.
-#defined conflict_condition/3.
-#defined conflict_condition/4.
-#defined conflict_condition/5.
+#defined conflict/3.
 
 %-----------------------------------------------------------------------------
 % Virtual dependencies
@@ -145,10 +128,15 @@ attr(Name, Arg1, Arg2, Arg3) :-
 % if a package depends on a virtual, it's not external and we have a
 % provider for that virtual then it depends on the provider
 depends_on(Package, Provider, Type)
-  :- dependency_conditions(Package, Virtual, Type),
+  :- dependency_holds(Package, Virtual, Type),
      provides_virtual(Provider, Virtual),
      not external(Package).
 
+% dependencies on virtuals also imply that the virtual is a virtual node
+virtual_node(Virtual)
+  :- dependency_holds(Package, Virtual, Type),
+     virtual(Virtual), not external(Package).
+
 % if there's a virtual node, we must select one provider
 1 { provides_virtual(Package, Virtual) : possible_provider(Package, Virtual) } 1
  :- virtual_node(Virtual).
@@ -158,32 +146,21 @@ virtual_node(Virtual) :- virtual_root(Virtual).
 1 { root(Package) : provides_virtual(Package, Virtual) } 1
   :- virtual_root(Virtual).
 
-% all virtual providers come from provider conditions like this
-dependency_conditions_hold(ID, Provider, Virtual) :-
-  attr(Name, Arg1)             : required_provider_condition(ID, Name, Arg1);
-  attr(Name, Arg1, Arg2)       : required_provider_condition(ID, Name, Arg1, Arg2);
-  attr(Name, Arg1, Arg2, Arg3) : required_provider_condition(ID, Name, Arg1, Arg2, Arg3);
-  virtual(Virtual);
-  provider_condition(ID, Provider, Virtual).
-
 % The provider provides the virtual if some provider condition holds.
 provides_virtual(Provider, Virtual) :-
    provider_condition(ID, Provider, Virtual),
-   dependency_conditions_hold(ID, Provider, Virtual),
+   condition_holds(ID),
    virtual(Virtual).
 
 % a node that provides a virtual is a provider
 provider(Package, Virtual)
   :- node(Package), provides_virtual(Package, Virtual).
 
-% dependencies on virtuals also imply that the virtual is a virtual node
-virtual_node(Virtual)
-  :- dependency_conditions(Package, Virtual, Type),
-     virtual(Virtual), not external(Package).
-
 % for any virtual, there can be at most one provider in the DAG
 0 { node(Package) : provides_virtual(Package, Virtual) } 1 :- virtual(Virtual).
 
+#defined possible_provider/2.
+
 %-----------------------------------------------------------------------------
 % Virtual dependency weights
 %-----------------------------------------------------------------------------
@@ -308,32 +285,29 @@ external(Package) :- external_only(Package), node(Package).
 % a package is a real_node if it is not external
 real_node(Package) :- node(Package), not external(Package).
 
-% if an external version is selected, the package is external and
-% we are using the corresponding spec
-external(Package) :-
-    version(Package, Version), version_weight(Package, Weight),
-    external_version_declared(Package, Version, Weight, ID).
+% a package is external if we are using an external spec for it
+external(Package) :- external_spec_selected(Package, _).
+
+% we can't use the weight for an external version if we don't use the
+% corresponding external spec.
+:- version(Package, Version),
+   version_weight(Package, Weight),
+   external_version_declared(Package, Version, Weight, ID),
+   not external(Package).
 
 % determine if an external spec has been selected
-external_spec_selected(ID, Package, LocalIndex) :-
-    version(Package, Version), version_weight(Package, Weight),
-    external_spec_index(ID, Package, LocalIndex),
-    external_version_declared(Package, Version, Weight, LocalIndex),
-    external_spec_conditions_hold(ID, Package).
-
-% determine if all the conditions on an external spec hold. If they do
-% the spec can be selected.
-external_spec_conditions_hold(ID, Package) :-
-  attr(Name, Arg1)             : external_spec_condition(ID, Name, Arg1);
-  attr(Name, Arg1, Arg2)       : external_spec_condition(ID, Name, Arg1, Arg2);
-  attr(Name, Arg1, Arg2, Arg3) : external_spec_condition(ID, Name, Arg1, Arg2, Arg3);
-  external_spec(ID, Package);
-  node(Package).
+external_spec_selected(Package, LocalIndex) :-
+    external_conditions_hold(Package, LocalIndex),
+    node(Package).
+
+external_conditions_hold(Package, LocalIndex) :-
+    possible_external(ID, Package, LocalIndex), condition_holds(ID).
 
 % it cannot happen that a spec is external, but none of the external specs
 % conditions hold.
-:- external(Package), not external_spec_conditions_hold(_, Package).
+:- external(Package), not external_conditions_hold(Package, _).
 
+#defined possible_external/3.
 #defined external_spec_index/3.
 #defined external_spec_condition/3.
 #defined external_spec_condition/4.
diff --git a/lib/spack/spack/solver/display.lp b/lib/spack/spack/solver/display.lp
index ad4cfdd2ec..f67c4b6c3c 100644
--- a/lib/spack/spack/solver/display.lp
+++ b/lib/spack/spack/solver/display.lp
@@ -29,4 +29,4 @@
 #show compiler_weight/2.
 #show node_target_match/2.
 #show node_target_weight/2.
-#show external_spec_selected/3.
+#show external_spec_selected/2.