From 93a6c51d8819a2f0003ed4f788f621717cbece81 Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Thu, 23 Dec 2021 00:29:06 -0800
Subject: package_hash: rework `RemoveDirectives` and add a test

Previously we used `directives.__all__` to get directive names, but it wasn't
quite right -- it included `DirectiveMeta`, etc.  It's not wrong, but it's also
not the clearest way to do this.

- [x] Refactor `@directive` to track names in `directive_names` global
- [x] Rename `_directive_names` to `_directive_dict_names` in `DirectiveMeta`
- [x] Add a test for `RemoveDirectives`

Co-authored-by: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com>
---
 lib/spack/spack/directives.py             | 18 +++++++++++++-----
 lib/spack/spack/test/util/package_hash.py | 27 +++++++++++++++++++++++++++
 lib/spack/spack/util/package_hash.py      |  2 +-
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index c23146125a..060465184d 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -59,6 +59,10 @@ __all__ = ['DirectiveError', 'DirectiveMeta']
 #: These are variant names used by Spack internally; packages can't use them
 reserved_names = ['patches', 'dev_path']
 
+#: Names of possible directives. This list is populated elsewhere in the file and then
+#: added to `__all__` at the bottom.
+directive_names = []
+
 _patch_order_index = 0
 
 
@@ -113,7 +117,7 @@ class DirectiveMeta(type):
     """
 
     # Set of all known directives
-    _directive_names = set()  # type: Set[str]
+    _directive_dict_names = set()  # type: Set[str]
     _directives_to_be_executed = []  # type: List[str]
     _when_constraints_from_context = []  # type: List[str]
 
@@ -156,7 +160,7 @@ class DirectiveMeta(type):
         if 'spack.pkg' in cls.__module__:
             # Ensure the presence of the dictionaries associated
             # with the directives
-            for d in DirectiveMeta._directive_names:
+            for d in DirectiveMeta._directive_dict_names:
                 setattr(cls, d, {})
 
             # Lazily execute directives
@@ -222,7 +226,7 @@ class DirectiveMeta(type):
         Package class, and it's how Spack gets information from the
         packages to the core.
         """
-        global __all__
+        global directive_names
 
         if isinstance(dicts, six.string_types):
             dicts = (dicts, )
@@ -232,11 +236,11 @@ class DirectiveMeta(type):
             raise TypeError(message.format(type(dicts)))
 
         # Add the dictionary names if not already there
-        DirectiveMeta._directive_names |= set(dicts)
+        DirectiveMeta._directive_dict_names |= set(dicts)
 
         # This decorator just returns the directive functions
         def _decorator(decorated_function):
-            __all__.append(decorated_function.__name__)
+            directive_names.append(decorated_function.__name__)
 
             @functools.wraps(decorated_function)
             def _wrapper(*args, **kwargs):
@@ -730,3 +734,7 @@ class DependencyPatchError(DirectiveError):
 
 class UnsupportedPackageDirective(DirectiveError):
     """Raised when an invalid or unsupported package directive is specified."""
+
+
+#: add all directive names to __all__
+__all__.extend(directive_names)
diff --git a/lib/spack/spack/test/util/package_hash.py b/lib/spack/spack/test/util/package_hash.py
index 16829586e4..aedb42e20d 100644
--- a/lib/spack/spack/test/util/package_hash.py
+++ b/lib/spack/spack/test/util/package_hash.py
@@ -5,6 +5,7 @@
 
 import ast
 
+import spack.directives
 import spack.paths
 import spack.util.package_hash as ph
 from spack.spec import Spec
@@ -123,3 +124,29 @@ def test_remove_docstrings():
     assert "SEVEN" in unparsed
     assert "NINE" in unparsed
     assert "TWELVE" in unparsed
+
+
+many_directives = """\
+
+class HasManyDirectives:
+{directives}
+
+    def foo():
+        # just a method to get in the way
+        pass
+
+{directives}
+""".format(directives="\n".join(
+    "    %s()" % name for name in spack.directives.directive_names
+))
+
+
+def test_remove_directives():
+    """Ensure all directives are removed from packages before hashing."""
+    tree = ast.parse(many_directives)
+    spec = Spec("has-many-directives")
+    tree = ph.RemoveDirectives(spec).visit(tree)
+    unparsed = unparse(tree)
+
+    for name in spack.directives.directive_names:
+        assert name not in unparsed
diff --git a/lib/spack/spack/util/package_hash.py b/lib/spack/spack/util/package_hash.py
index d1412e16fd..3421d90d23 100644
--- a/lib/spack/spack/util/package_hash.py
+++ b/lib/spack/spack/util/package_hash.py
@@ -66,7 +66,7 @@ class RemoveDirectives(ast.NodeTransformer):
         return (isinstance(node, ast.Expr) and
                 node.value and isinstance(node.value, ast.Call) and
                 isinstance(node.value.func, ast.Name) and
-                node.value.func.id in spack.directives.__all__)
+                node.value.func.id in spack.directives.directive_names)
 
     def is_spack_attr(self, node):
         return (isinstance(node, ast.Assign) and
-- 
cgit v1.2.3-70-g09d2