Spec: Remove _normal attribute and unused constructor arguments (sp…

…ack#48119)

The `_normal` attribute on specs is no longer used and has no meaning.
It's left over from part of the original concretizer.

The `concrete` constructor argument is also not used by any part of core.

- [x] remove `_normal` attribute from `Spec`
- [x] remove `concrete` argument from `Spec.__init__`
- [x] remove unused `check_diamond_normalized_dag` function in tests
- [x] simplify `Spec` constructor and docstrings

I tried to add typing to `Spec` here, but it creates a huge number of type issues
because *most* things on `Spec` are optional. We probably need separate `Spec` and
`ConcreteSpec` classes before attempting that.

Signed-off-by: Todd Gamblin <[email protected]>

lib/spack/spack/spec.py

@@ -1433,38 +1433,24 @@ def tree(
 
 @lang.lazy_lexicographic_ordering(set_hash=False)
 class Spec:
-    #: Cache for spec's prefix, computed lazily in the corresponding property
-    _prefix = None
-    abstract_hash = None
-
     @staticmethod
     def default_arch():
         """Return an anonymous spec for the default architecture"""
         s = Spec()
         s.architecture = ArchSpec.default_arch()
         return s
 
-    def __init__(
-        self,
-        spec_like=None,
-        normal=False,
-        concrete=False,
-        external_path=None,
-        external_modules=None,
-    ):
+    def __init__(self, spec_like=None, *, external_path=None, external_modules=None):
         """Create a new Spec.
 
         Arguments:
-            spec_like (optional string): if not provided, we initialize
-                an anonymous Spec that matches any Spec object; if
-                provided we parse this as a Spec string.
+            spec_like: if not provided, we initialize an anonymous Spec that matches any Spec;
+                if provided we parse this as a Spec string, or we copy the provided Spec.
 
         Keyword arguments:
-        # assign special fields from constructor
-        self._normal = normal
-        self._concrete = concrete
-        self.external_path = external_path
-        self.external_module = external_module
+            external_path: prefix, if this is a spec for an external package
+            external_modules: list of external modules, if this is an external package
+                using modules.
         """
         # Copy if spec_like is a Spec.
         if isinstance(spec_like, Spec):
@@ -1481,26 +1467,26 @@ def __init__(
         self._dependents = _EdgeMap(store_by_child=False)
         self._dependencies = _EdgeMap(store_by_child=True)
         self.namespace = None
+        self.abstract_hash = None
 
         # initial values for all spec hash types
         for h in ht.hashes:
             setattr(self, h.attr, None)
 
+        # cache for spec's prefix, computed lazily by prefix property
+        self._prefix = None
+
         # Python __hash__ is handled separately from the cached spec hashes
         self._dunder_hash = None
 
         # cache of package for this spec
         self._package = None
 
-        # Most of these are internal implementation details that can be
-        # set by internal Spack calls in the constructor.
-        #
-        # For example, Specs are by default not assumed to be normal, but
-        # in some cases we've read them from a file want to assume
-        # normal.  This allows us to manipulate specs that Spack doesn't
-        # have package.py files for.
-        self._normal = normal
-        self._concrete = concrete
+        # whether the spec is concrete or not; set at the end of concretization
+        self._concrete = False
+
+        # External detection details that can be set by internal Spack calls
+        # in the constructor.
         self._external_path = external_path
         self.external_modules = Spec._format_module_list(external_modules)
 
@@ -2876,7 +2862,6 @@ def _mark_root_concrete(self, value=True):
         """Mark just this spec (not dependencies) concrete."""
         if (not value) and self.concrete and self.installed:
             return
-        self._normal = value
         self._concrete = value
         self._validate_version()
 
@@ -3538,7 +3523,6 @@ def _dup(self, other, deps: Union[bool, dt.DepTypes, dt.DepFlag] = True, clearde
                 and self.architecture != other.architecture
                 and self.compiler != other.compiler
                 and self.variants != other.variants
-                and self._normal != other._normal
                 and self.concrete != other.concrete
                 and self.external_path != other.external_path
                 and self.external_modules != other.external_modules
@@ -3584,20 +3568,17 @@ def _dup(self, other, deps: Union[bool, dt.DepTypes, dt.DepFlag] = True, clearde
                 depflag = dt.canonicalize(deps)
             self._dup_deps(other, depflag)
 
+        self._prefix = other._prefix
         self._concrete = other._concrete
 
         self.abstract_hash = other.abstract_hash
 
         if self._concrete:
             self._dunder_hash = other._dunder_hash
-            self._normal = other._normal
             for h in ht.hashes:
                 setattr(self, h.attr, getattr(other, h.attr, None))
         else:
             self._dunder_hash = None
-            # Note, we could use other._normal if we are copying all deps, but
-            # always set it False here to avoid the complexity of checking
-            self._normal = False
             for h in ht.hashes:
                 setattr(self, h.attr, None)
 

lib/spack/spack/test/spec_dag.py

@@ -391,7 +391,6 @@ def test_copy_simple(self):
 
         assert orig == copy
         assert orig.eq_dag(copy)
-        assert orig._normal == copy._normal
         assert orig._concrete == copy._concrete
 
         # ensure no shared nodes bt/w orig and copy.
@@ -408,7 +407,6 @@ def test_copy_concretized(self):
 
         assert orig == copy
         assert orig.eq_dag(copy)
-        assert orig._normal == copy._normal
         assert orig._concrete == copy._concrete
 
         # ensure no shared nodes bt/w orig and copy.
@@ -621,18 +619,6 @@ def check_diamond_deptypes(self, spec):
             == dt.BUILD | dt.LINK | dt.RUN
         )
 
-    def check_diamond_normalized_dag(self, spec):
-        dag = Spec.from_literal(
-            {
-                "dt-diamond": {
-                    "dt-diamond-left:build,link": {"dt-diamond-bottom:build": None},
-                    "dt-diamond-right:build,link": {"dt-diamond-bottom:build,link,run": None},
-                }
-            }
-        )
-
-        assert spec.eq_dag(dag)
-
     def test_concretize_deptypes(self):
         """Ensure that dependency types are preserved after concretization."""
         s = Spec("dt-diamond")