Fixes for parsing specs with hashes (#2889)

- Allows hashes to be specified after other parts of the spec
- Does not allow other parts of the spec to be specified after the hash
- The hash must either end input or be followed by another separate spec
- The next spec cannot be an anonymous spec (it must start with a package name or a hash)

See #2769 (after it was merged) for further discussion of this interface addition. That discussion resulted in these requirements:

```
python                     # 1 spec
/abc123                    # 1 spec
python /abc123             # 1 spec
/456789                    # 1 spec
python /abc123 /456789     # 2 specs
python /456789 /abc123     # 2 specs
/abc123 /456789            # 2 specs
/456789 /abc123            # 2 specs
/456789 /abc123 python     # 3 specs
```

assuming `abc123` and `456789` are both hashes of different python specs.

lib/spack/spack/spec.py

@@ -152,7 +152,9 @@
     'UnsatisfiableArchitectureSpecError',
     'UnsatisfiableProviderSpecError',
     'UnsatisfiableDependencySpecError',
-    'AmbiguousHashError']
+    'AmbiguousHashError',
+    'InvalidHashError',
+    'RedundantSpecError']
 
 # Valid pattern for an identifier in Spack
 identifier_re = r'\w[\w-]*'
@@ -1993,7 +1995,7 @@ def _autospec(self, spec_like):
         except SpecError:
             return parse_anonymous_spec(spec_like, self.name)
 
-    def satisfies(self, other, deps=True, strict=False):
+    def satisfies(self, other, deps=True, strict=False, strict_deps=False):
         """Determine if this spec satisfies all constraints of another.
 
         There are two senses for satisfies:
@@ -2067,7 +2069,8 @@ def satisfies(self, other, deps=True, strict=False):
         # If we need to descend into dependencies, do it, otherwise we're done.
         if deps:
             deps_strict = strict
-            if not (self.name and other.name):
+            if self.concrete and not other.name:
+                # We're dealing with existing specs
                 deps_strict = True
             return self.satisfies_dependencies(other, strict=deps_strict)
         else:
@@ -2083,9 +2086,10 @@ def satisfies_dependencies(self, other, strict=False):
             if other._dependencies and not self._dependencies:
                 return False
 
-            alldeps = set(d.name for d in self.traverse(root=False))
+            selfdeps = self.traverse(root=False)
-            if not all(dep.name in alldeps
+            otherdeps = other.traverse(root=False)
-                       for dep in other.traverse(root=False)):
+            if not all(any(d.satisfies(dep) for d in selfdeps)
+                       for dep in otherdeps):
                 return False
 
         elif not self._dependencies or not other._dependencies:
@@ -2697,30 +2701,28 @@ def do_parse(self):
         specs = []
 
         try:
-            while self.next or self.previous:
+            while self.next:
                 # TODO: clean this parsing up a bit
-                if self.previous:
+                if self.accept(ID):
-                    # We picked up the name of this spec while finishing the
-                    # previous spec
-                    specs.append(self.spec(self.previous.value))
-                    self.previous = None
-                elif self.accept(ID):
                     self.previous = self.token
                     if self.accept(EQ):
-                        # We're either parsing an anonymous spec beginning
+                        # We're parsing an anonymous spec beginning with a
-                        # with a key-value pair or adding a key-value pair
+                        # key-value pair.
-                        # to the last spec
                         if not specs:
                             specs.append(self.spec(None))
                         self.expect(VAL)
+                        # Raise an error if the previous spec is already
+                        # concrete (assigned by hash)
+                        if specs[-1]._hash:
+                            raise RedundantSpecError(specs[-1],
+                                                     'key-value pair')
                         specs[-1]._add_flag(
                             self.previous.value, self.token.value)
                         self.previous = None
                     else:
                         # We're parsing a new spec by name
-                        value = self.previous.value
                         self.previous = None
-                        specs.append(self.spec(value))
+                        specs.append(self.spec(self.token.value))
                 elif self.accept(HASH):
                     # We're finding a spec by hash
                     specs.append(self.spec_by_hash())
@@ -2728,19 +2730,24 @@ def do_parse(self):
                 elif self.accept(DEP):
                     if not specs:
                         # We're parsing an anonymous spec beginning with a
-                        # dependency
+                        # dependency. Push the token to recover after creating
-                        self.previous = self.token
+                        # anonymous spec
+                        self.push_tokens([self.token])
                         specs.append(self.spec(None))
-                        self.previous = None
+                    else:
                         if self.accept(HASH):
-                        # We're finding a dependency by hash for an anonymous
+                            # We're finding a dependency by hash for an
-                        # spec
+                            # anonymous spec
                             dep = self.spec_by_hash()
                         else:
                             # We're adding a dependency to the last spec
                             self.expect(ID)
                             dep = self.spec(self.token.value)
 
+                        # Raise an error if the previous spec is already
+                        # concrete (assigned by hash)
+                        if specs[-1]._hash:
+                            raise RedundantSpecError(specs[-1], 'dependency')
                         # command line deps get empty deptypes now.
                         # Real deptypes are assigned later per packages.
                         specs[-1]._add_dependency(dep, ())
@@ -2749,6 +2756,12 @@ def do_parse(self):
                     # If the next token can be part of a valid anonymous spec,
                     # create the anonymous spec
                     if self.next.type in (AT, ON, OFF, PCT):
+                        # Raise an error if the previous spec is already
+                        # concrete (assigned by hash)
+                        if specs and specs[-1]._hash:
+                            raise RedundantSpecError(specs[-1],
+                                                     'compiler, version, '
+                                                     'or variant')
                         specs.append(self.spec(None))
                     else:
                         self.unexpected_token()
@@ -2783,13 +2796,13 @@ def spec_by_hash(self):
 
         if len(matches) != 1:
             raise AmbiguousHashError(
-                "Multiple packages specify hash %s." % self.token.value,
+                "Multiple packages specify hash beginning %s."
-                *matches)
+                % self.token.value, *matches)
 
         return matches[0]
 
     def spec(self, name):
-        """Parse a spec out of the input.  If a spec is supplied, then initialize
+        """Parse a spec out of the input. If a spec is supplied, initialize
            and return it instead of creating a new one."""
         if name:
             spec_namespace, dot, spec_name = name.rpartition('.')
@@ -2823,16 +2836,6 @@ def spec(self, name):
         # unspecified or not.
         added_version = False
 
-        if self.previous and self.previous.value == DEP:
-            if self.accept(HASH):
-                spec.add_dependency(self.spec_by_hash())
-            else:
-                self.expect(ID)
-                if self.accept(EQ):
-                    raise SpecParseError(spack.parse.ParseError(
-                        "", "", "Expected dependency received anonymous spec"))
-                spec.add_dependency(self.spec(self.token.value))
-
         while self.next:
             if self.accept(AT):
                 vlist = self.version_list()
@@ -2858,13 +2861,25 @@ def spec(self, name):
                     self.previous = None
                 else:
                     # We've found the start of a new spec. Go back to do_parse
+                    # and read this token again.
+                    self.push_tokens([self.token])
+                    self.previous = None
                     break
 
+            elif self.accept(HASH):
+                # Get spec by hash and confirm it matches what we already have
+                hash_spec = self.spec_by_hash()
+                if hash_spec.satisfies(spec):
+                    spec = hash_spec
+                    break
+                else:
+                    raise InvalidHashError(spec, hash_spec.dag_hash())
+
             else:
                 break
 
         # If there was no version in the spec, consier it an open range
-        if not added_version:
+        if not added_version and not spec._hash:
             spec.versions = VersionList(':')
 
         return spec
@@ -3139,3 +3154,18 @@ def __init__(self, msg, *specs):
         super(AmbiguousHashError, self).__init__(msg)
         for spec in specs:
             print('    ', spec.format('$.$@$%@+$+$=$#'))
+
+
+class InvalidHashError(SpecError):
+    def __init__(self, spec, hash):
+        super(InvalidHashError, self).__init__(
+            "The spec specified by %s does not match provided spec %s"
+            % (hash, spec))
+
+
+class RedundantSpecError(SpecError):
+    def __init__(self, spec, addition):
+        super(RedundantSpecError, self).__init__(
+            "Attempting to add %s to spec %s which is already concrete."
+            " This is likely the result of adding to a spec specified by hash."
+            % (addition, spec))

lib/spack/spack/test/spec_syntax.py

@@ -132,6 +132,13 @@ def test_package_names(self):
         self.check_parse("mvapich_foo")
         self.check_parse("_mvapich_foo")
 
+    def test_anonymous_specs(self):
+        self.check_parse("%intel")
+        self.check_parse("@2.7")
+        self.check_parse("^zlib")
+        self.check_parse("+foo")
+        self.check_parse("arch=test-None-None", "platform=test")
+
     def test_simple_dependence(self):
         self.check_parse("openmpi^hwloc")
         self.check_parse("openmpi^hwloc^libunwind")
@@ -218,6 +225,115 @@ def test_parse_errors(self):
         errors = ['x@@1.2', 'x ^y@@1.2', 'x@1.2::', 'x::']
         self._check_raises(SpecParseError, errors)
 
+    def test_spec_by_hash(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is still the shape we expect
+        assert len(specs) > 3
+
+        self.check_parse(str(specs[0]), '/' + hashes[0])
+        self.check_parse(str(specs[1]), '/ ' + hashes[1][:5])
+        self.check_parse(str(specs[2]), specs[2].name + '/' + hashes[2])
+        self.check_parse(str(specs[3]),
+                         specs[3].name + '@' + str(specs[3].version) +
+                         ' /' + hashes[3][:6])
+
+    def test_dep_spec_by_hash(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is still the shape we expect
+        assert len(specs) > 10
+        assert specs[4].name in specs[10]
+        assert specs[-1].name in specs[10]
+
+        spec1 = sp.Spec(specs[10].name + '^/' + hashes[4])
+        assert specs[4].name in spec1 and spec1[specs[4].name] == specs[4]
+        spec2 = sp.Spec(specs[10].name + '%' + str(specs[10].compiler) +
+                        ' ^ / ' + hashes[-1])
+        assert (specs[-1].name in spec2 and
+                spec2[specs[-1].name] == specs[-1] and
+                spec2.compiler == specs[10].compiler)
+        spec3 = sp.Spec(specs[10].name + '^/' + hashes[4][:4] +
+                        '^ / ' + hashes[-1][:5])
+        assert (specs[-1].name in spec3 and
+                spec3[specs[-1].name] == specs[-1] and
+                specs[4].name in spec3 and spec3[specs[4].name] == specs[4])
+
+    def test_multiple_specs_with_hash(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        assert len(specs) > 3
+
+        output = sp.parse(specs[0].name + '/' + hashes[0] + '/' + hashes[1])
+        assert len(output) == 2
+        output = sp.parse('/' + hashes[0] + '/' + hashes[1])
+        assert len(output) == 2
+        output = sp.parse('/' + hashes[0] + '/' + hashes[1] +
+                          ' ' + specs[2].name)
+        assert len(output) == 3
+        output = sp.parse('/' + hashes[0] +
+                          ' ' + specs[1].name + ' ' + specs[2].name)
+        assert len(output) == 3
+        output = sp.parse('/' + hashes[0] + ' ' +
+                          specs[1].name + ' / ' + hashes[1])
+        assert len(output) == 2
+
+    def test_ambiguous_hash(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is as expected
+        assert hashes[1][:1] == hashes[2][:1] == 'b'
+
+        ambiguous_hashes = ['/b',
+                            specs[1].name + '/b',
+                            specs[0].name + '^/b',
+                            specs[0].name + '^' + specs[1].name + '/b']
+        self._check_raises(AmbiguousHashError, ambiguous_hashes)
+
+    def test_invalid_hash(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is as expected
+        assert (hashes[0] != hashes[3] and
+                hashes[1] != hashes[4] and len(specs) > 4)
+
+        inputs = [specs[0].name + '/' + hashes[3],
+                  specs[1].name + '^' + specs[4].name + '/' + hashes[0],
+                  specs[1].name + '^' + specs[4].name + '/' + hashes[1]]
+        self._check_raises(InvalidHashError, inputs)
+
+    def test_nonexistent_hash(self, database):
+        # This test uses database to make sure we don't accidentally access
+        # real installs, however unlikely
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is as expected
+        assert 'abc123' not in [h[:6] for h in hashes]
+
+        nonexistant_hashes = ['/abc123',
+                              specs[0].name + '/abc123']
+        self._check_raises(SystemExit, nonexistant_hashes)
+
+    def test_redundant_spec(self, database):
+        specs = database.mock.db.query()
+        hashes = [s._hash for s in specs]  # Preserves order of elements
+
+        # Make sure the database is as expected
+        assert len(specs) > 3
+
+        redundant_specs = ['/' + hashes[0] + '%' + str(specs[0].compiler),
+                           specs[1].name + '/' + hashes[1] +
+                           '@' + str(specs[1].version),
+                           specs[2].name + '/' + hashes[2] + '^ libelf',
+                           '/' + hashes[3] + ' cflags="-O3 -fPIC"']
+        self._check_raises(RedundantSpecError, redundant_specs)
+
     def test_duplicate_variant(self):
         duplicates = [
             'x@1.2+debug+debug',