add mypy to style checks; rename spack flake8 to spack style (#20384)

I lost my mind a bit after getting the completion stuff working and
decided to get Mypy working for spack as well. This adds a 
`.mypy.ini` that checks all of the spack and llnl modules, though
not yet packages, and fixes all of the identified missing types and
type issues for the spack library.

In addition to these changes, this includes:

* rename `spack flake8` to `spack style`

Aliases flake8 to style, and just runs flake8 as before, but with
a warning.  The style command runs both `flake8` and `mypy`,
in sequence. Added --no-<tool> options to turn off one or the
other, they are on by default.  Fixed two issues caught by the tools.

* stub typing module for python2.x

We don't support typing in Spack for python 2.x. To allow 2.x to
support `import typing` and `from typing import ...` without a
try/except dance to support old versions, this adds a stub module
*just* for python 2.x.  Doing it this way means we can only reliably
use all type hints in python3.7+, and mypi.ini has been updated to
reflect that.

* add non-default black check to spack style

This is a first step to requiring black.  It doesn't enforce it by
default, but it will check it if requested.  Currently enforcing the
line length of 79 since that's what flake8 requires, but it's a bit odd
for a black formatted project to be quite that narrow.  All settings are
in the style command since spack has no pyproject.toml and I don't
want to add one until more discussion happens. Also re-format
`style.py` since it no longer passed the black style check
with the new length.

* use style check in github action

Update the style and docs action to use `spack style`, adding in mypy
and black to the action even if it isn't running black right now.

.flake8

@@ -27,9 +27,17 @@
 # - N813: camelcase imported as lowercase
 # - N814: camelcase imported as constant
 #
+# F4: pyflakes import checks, these are now checked by mypy more precisely
+# - F403: from module import *
+# - F405: undefined name or from *
+#
+# Black ignores, these are incompatible with black style and do not follow PEP-8
+# - E203: white space around slice operators can be required, ignore : warn
+# - W503: see above, already ignored for line-breaks
+#
 [flake8]
-ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814
+ignore = E129,E221,E241,E272,E731,W503,W504,F999,N801,N813,N814,F403,F405
-max-line-length = 79
+max-line-length = 88
 
 # F4: Import
 # - F405: `name` may be undefined, or undefined from star imports: `module`

.github/workflows/macos_unit_tests.yaml

@@ -26,7 +26,7 @@ jobs:
       run: |
           pip install --upgrade pip six setuptools
           pip install --upgrade codecov coverage
-          pip install --upgrade flake8 pep8-naming
+          pip install --upgrade flake8 pep8-naming mypy
     - name: Setup Homebrew packages
       run: |
         brew install dash fish gcc gnupg2 kcov

.github/workflows/style_and_docs.yaml

@@ -22,10 +22,10 @@ jobs:
         pip install --upgrade pip
         pip install --upgrade vermin
     - name: Minimum Version (Spack's Core)
-      run: vermin --backport argparse -t=2.6- -t=3.5- -v lib/spack/spack/ lib/spack/llnl/ bin/
+      run: vermin --backport argparse --backport typing -t=2.6- -t=3.5- -v lib/spack/spack/ lib/spack/llnl/ bin/
     - name: Minimum Version (Repositories)
-      run: vermin --backport argparse -t=2.6- -t=3.5- -v var/spack/repos
+      run: vermin --backport argparse --backport typing -t=2.6- -t=3.5- -v var/spack/repos
-  flake8:
+  style:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
@@ -36,15 +36,15 @@ jobs:
         python-version: 3.9
     - name: Install Python packages
       run: |
-        pip install --upgrade pip six setuptools flake8
+        pip install --upgrade pip six setuptools flake8 mypy black
     - name: Setup git configuration
       run: |
         # Need this for the git tests to succeed.
         git --version
         . .github/workflows/setup_git.sh
-    - name: Run flake8 tests
+    - name: Run style tests
       run: |
-          share/spack/qa/run-flake8-tests
+          share/spack/qa/run-style-tests
   documentation:
     runs-on: ubuntu-latest
     steps:

.mypy.ini

@@ -0,0 +1,35 @@
+[mypy]
+python_version = 3.7
+files=lib/spack/llnl/**/*.py,lib/spack/spack/**/*.py
+mypy_path=bin,lib/spack,lib/spack/external,var/spack/repos/builtin
+# This and a generated import file allows supporting packages
+namespace_packages=True
+# To avoid re-factoring all the externals, ignore errors and missing imports
+# globally, then turn back on in spack and spack submodules
+ignore_errors=True
+ignore_missing_imports=True
+
+[mypy-spack.*]
+ignore_errors=False
+ignore_missing_imports=False
+
+[mypy-packages.*]
+ignore_errors=False
+ignore_missing_imports=False
+
+[mypy-llnl.*]
+ignore_errors=False
+ignore_missing_imports=False
+
+[mypy-spack.test.packages]
+ignore_errors=True
+
+# ignore errors in fake import path for packages
+[mypy-spack.pkg.*]
+ignore_errors=True
+ignore_missing_imports=True
+
+# jinja has syntax in it that requires python3 and causes a parse error
+# skip importing it
+[mypy-jinja2]
+follow_imports=skip

bin/spack

@@ -42,6 +42,8 @@ sys.path.insert(0, spack_lib_path)
 # Add external libs
 spack_external_libs = os.path.join(spack_lib_path, "external")
 
+if sys.version_info[:2] <= (2, 7):
+    sys.path.insert(0, os.path.join(spack_external_libs, 'py2'))
 if sys.version_info[:2] == (2, 6):
     sys.path.insert(0, os.path.join(spack_external_libs, 'py26'))
 

lib/spack/docs/contribution_guide.rst

@@ -179,24 +179,26 @@ how to write tests!
    run the unit tests yourself, we suggest you use ``spack unit-test``.
 
 ^^^^^^^^^^^^
-Flake8 Tests
+Style Tests
 ^^^^^^^^^^^^
 
 Spack uses `Flake8 <http://flake8.pycqa.org/en/latest/>`_ to test for
-`PEP 8 <https://www.python.org/dev/peps/pep-0008/>`_ conformance. PEP 8 is
+`PEP 8 <https://www.python.org/dev/peps/pep-0008/>`_ conformance and
+`mypy <https://mypy.readthedocs.io/en/stable/>` for type checking. PEP 8 is
 a series of style guides for Python that provide suggestions for everything
 from variable naming to indentation. In order to limit the number of PRs that
 were mostly style changes, we decided to enforce PEP 8 conformance. Your PR
-needs to comply with PEP 8 in order to be accepted.
+needs to comply with PEP 8 in order to be accepted, and if it modifies the
+spack library it needs to successfully type-check with mypy as well.
 
-Testing for PEP 8 compliance is easy. Simply run the ``spack flake8``
+Testing for compliance with spack's style is easy. Simply run the ``spack style``
 command:
 
 .. code-block:: console
 
-   $ spack flake8
+   $ spack style
 
-``spack flake8`` has a couple advantages over running ``flake8`` by hand:
+``spack style`` has a couple advantages over running the tools by hand:
 
 #. It only tests files that you have modified since branching off of
    ``develop``.
@@ -207,7 +209,9 @@ command:
    checks. For example, URLs are often longer than 80 characters, so we
    exempt them from line length checks. We also exempt lines that start
    with "homepage", "url", "version", "variant", "depends_on", and
-   "extends" in ``package.py`` files.
+   "extends" in ``package.py`` files.  This is now also possible when directly
+   running flake8 if you can use the ``spack`` formatter plugin included with
+   spack.
 
 More approved flake8 exemptions can be found
 `here <https://github.com/spack/spack/blob/develop/.flake8>`_.
@@ -240,13 +244,13 @@ However, if you aren't compliant with PEP 8, flake8 will complain:
 
 Most of the error messages are straightforward, but if you don't understand what
 they mean, just ask questions about them when you submit your PR. The line numbers
-will change if you add or delete lines, so simply run ``spack flake8`` again
+will change if you add or delete lines, so simply run ``spack style`` again
 to update them.
 
 .. tip::
 
    Try fixing flake8 errors in reverse order. This eliminates the need for
-   multiple runs of ``spack flake8`` just to re-compute line numbers and
+   multiple runs of ``spack style`` just to re-compute line numbers and
    makes it much easier to fix errors directly off of the CI output.
 
 .. warning::

lib/spack/external/py2/typing.py

@@ -0,0 +1,84 @@
+# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+"""
+This is a fake set of symbols to allow spack to import typing in python
+versions where we do not support type checking (<3)
+"""
+Annotated = None
+Any = None
+Callable = None
+ForwardRef = None
+Generic = None
+Literal = None
+Optional = None
+Tuple = None
+TypeVar = None
+Union = None
+AbstractSet = None
+ByteString = None
+Container = None
+Hashable = None
+ItemsView = None
+Iterable = None
+Iterator = None
+KeysView = None
+Mapping = None
+MappingView = None
+MutableMapping = None
+MutableSequence = None
+MutableSet = None
+Sequence = None
+Sized = None
+ValuesView = None
+Awaitable = None
+AsyncIterator = None
+AsyncIterable = None
+Coroutine = None
+Collection = None
+AsyncGenerator = None
+AsyncContextManager = None
+Reversible = None
+SupportsAbs = None
+SupportsBytes = None
+SupportsComplex = None
+SupportsFloat = None
+SupportsInt = None
+SupportsRound = None
+ChainMap = None
+Dict = None
+List = None
+OrderedDict = None
+Set = None
+FrozenSet = None
+NamedTuple = None
+Generator = None
+AnyStr = None
+cast = None
+get_args = None
+get_origin = None
+get_type_hints = None
+no_type_check = None
+no_type_check_decorator = None
+NoReturn = None
+
+# these are the typing extension symbols
+ClassVar = None
+Final = None
+Protocol = None
+Type = None
+TypedDict = None
+ContextManager = None
+Counter = None
+Deque = None
+DefaultDict = None
+SupportsIndex = None
+final = None
+IntVar = None
+Literal = None
+NewType = None
+overload = None
+runtime_checkable = None
+Text = None
+TYPE_CHECKING = None

lib/spack/llnl/util/tty/log.py

@@ -20,12 +20,17 @@
 from six import string_types
 from six import StringIO
 
+from typing import Optional  # novm
+from types import ModuleType  # novm
+
 import llnl.util.tty as tty
 
+termios = None  # type: Optional[ModuleType]
 try:
-    import termios
+    import termios as term_mod
+    termios = term_mod
 except ImportError:
-    termios = None
+    pass
 
 
 # Use this to strip escape sequences

lib/spack/spack/architecture.py

@@ -235,18 +235,19 @@ class Platform(object):
         Will return a instance of it once it is returned.
     """
 
-    priority        = None   # Subclass sets number. Controls detection order
+    # Subclass sets number. Controls detection order
+    priority        = None   # type: int
 
     #: binary formats used on this platform; used by relocation logic
     binary_formats  = ['elf']
 
-    front_end       = None
+    front_end       = None   # type: str
-    back_end        = None
+    back_end        = None   # type: str
-    default         = None   # The default back end target.
+    default         = None   # type: str # The default back end target.
 
-    front_os        = None
+    front_os        = None   # type: str
-    back_os         = None
+    back_os         = None   # type: str
-    default_os      = None
+    default_os      = None   # type: str
 
     reserved_targets = ['default_target', 'frontend', 'fe', 'backend', 'be']
     reserved_oss = ['default_os', 'frontend', 'fe', 'backend', 'be']

lib/spack/spack/build_systems/autotools.py

@@ -8,6 +8,7 @@
 import os.path
 from subprocess import PIPE
 from subprocess import check_call
+from typing import List  # novm
 
 import llnl.util.tty as tty
 import llnl.util.filesystem as fs
@@ -61,7 +62,7 @@ class AutotoolsPackage(PackageBase):
 
     #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.build`
     #: phase
-    build_targets = []
+    build_targets = []  # type: List[str]
     #: Targets for ``make`` during the :py:meth:`~.AutotoolsPackage.install`
     #: phase
     install_targets = ['install']
@@ -75,7 +76,7 @@ class AutotoolsPackage(PackageBase):
     #: Set to true to force the autoreconf step even if configure is present
     force_autoreconf = False
     #: Options to be passed to autoreconf when using the default implementation
-    autoreconf_extra_args = []
+    autoreconf_extra_args = []  # type: List[str]
 
     #: If False deletes all the .la files in the prefix folder
     #: after the installation. If True instead it installs them.

lib/spack/spack/build_systems/cmake.py

@@ -8,6 +8,7 @@
 import os
 import platform
 import re
+from typing import List  # novm
 
 import spack.build_environment
 from llnl.util.filesystem import working_dir
@@ -74,7 +75,7 @@ class CMakePackage(PackageBase):
     #: system base class
     build_system_class = 'CMakePackage'
 
-    build_targets = []
+    build_targets = []  # type: List[str]
     install_targets = ['install']
 
     build_time_test_callbacks = ['check']

lib/spack/spack/build_systems/makefile.py

@@ -5,6 +5,7 @@
 
 
 import inspect
+from typing import List  # novm
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import working_dir
@@ -48,7 +49,7 @@ class MakefilePackage(PackageBase):
 
     #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.build`
     #: phase
-    build_targets = []
+    build_targets = []  # type: List[str]
     #: Targets for ``make`` during the :py:meth:`~.MakefilePackage.install`
     #: phase
     install_targets = ['install']

lib/spack/spack/build_systems/meson.py

@@ -6,6 +6,7 @@
 
 import inspect
 import os
+from typing import List  # novm
 
 from llnl.util.filesystem import working_dir
 from spack.directives import depends_on, variant
@@ -46,7 +47,7 @@ class MesonPackage(PackageBase):
     #: system base class
     build_system_class = 'MesonPackage'
 
-    build_targets = []
+    build_targets = []  # type: List[str]
     install_targets = ['install']
 
     build_time_test_callbacks = ['check']

lib/spack/spack/cmd/flake8.py

@@ -5,200 +5,22 @@
 
 from __future__ import print_function
 
-try:
+import llnl.util.tty as tty
-    from itertools import zip_longest  # novm
-except ImportError:
-    from itertools import izip_longest  # novm
 
-    zip_longest = izip_longest
+import spack.cmd.style
-
-import re
-import os
-import sys
-import argparse
-
-from llnl.util.filesystem import working_dir
-
-import spack.paths
-from spack.util.executable import which
 
 
-description = "runs source code style checks on Spack. requires flake8"
+description = "alias for spack style (deprecated)"
-section = "developer"
+section = spack.cmd.style.section
-level = "long"
+level = spack.cmd.style.level
-
-
-def grouper(iterable, n, fillvalue=None):
-    "Collect data into fixed-length chunks or blocks"
-    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
-    args = [iter(iterable)] * n
-    return zip_longest(*args, fillvalue=fillvalue)
-
-
-def is_package(f):
-    """Whether flake8 should consider a file as a core file or a package.
-
-    We run flake8 with different exceptions for the core and for
-    packages, since we allow `from spack import *` and poking globals
-    into packages.
-    """
-    return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f
-
-
-#: List of directories to exclude from checks.
-exclude_directories = [spack.paths.external_path]
-
-#: max line length we're enforcing (note: this duplicates what's in .flake8)
-max_line_length = 79
-
-
-def changed_files(base=None, untracked=True, all_files=False):
-    """Get list of changed files in the Spack repository."""
-
-    git = which("git", required=True)
-
-    if base is None:
-        base = os.environ.get("TRAVIS_BRANCH", "develop")
-
-    range = "{0}...".format(base)
-
-    git_args = [
-        # Add changed files committed since branching off of develop
-        ["diff", "--name-only", "--diff-filter=ACMR", range],
-        # Add changed files that have been staged but not yet committed
-        ["diff", "--name-only", "--diff-filter=ACMR", "--cached"],
-        # Add changed files that are unstaged
-        ["diff", "--name-only", "--diff-filter=ACMR"],
-    ]
-
-    # Add new files that are untracked
-    if untracked:
-        git_args.append(["ls-files", "--exclude-standard", "--other"])
-
-    # add everything if the user asked for it
-    if all_files:
-        git_args.append(["ls-files", "--exclude-standard"])
-
-    excludes = [os.path.realpath(f) for f in exclude_directories]
-    changed = set()
-
-    for arg_list in git_args:
-        files = git(*arg_list, output=str).split("\n")
-
-        for f in files:
-            # Ignore non-Python files
-            if not (f.endswith(".py") or f == "bin/spack"):
-                continue
-
-            # Ignore files in the exclude locations
-            if any(os.path.realpath(f).startswith(e) for e in excludes):
-                continue
-
-            changed.add(f)
-
-    return sorted(changed)
 
 
 def setup_parser(subparser):
-    subparser.add_argument(
+    spack.cmd.style.setup_parser(subparser)
-        "-b",
-        "--base",
-        action="store",
-        default=None,
-        help="select base branch for collecting list of modified files",
-    )
-    subparser.add_argument(
-        "-a",
-        "--all",
-        action="store_true",
-        help="check all files, not just changed files",
-    )
-    subparser.add_argument(
-        "-o",
-        "--output",
-        action="store_true",
-        help="send filtered files to stdout as well as temp files",
-    )
-    subparser.add_argument(
-        "-r",
-        "--root-relative",
-        action="store_true",
-        default=False,
-        help="print root-relative paths (default: cwd-relative)",
-    )
-    subparser.add_argument(
-        "-U",
-        "--no-untracked",
-        dest="untracked",
-        action="store_false",
-        default=True,
-        help="exclude untracked files from checks",
-    )
-    subparser.add_argument(
-        "files", nargs=argparse.REMAINDER, help="specific files to check"
-    )
 
 
 def flake8(parser, args):
-    file_list = args.files
+    tty.warn(
-    if file_list:
+        "spack flake8 is deprecated", "please use `spack style` to run style checks"
-
-        def prefix_relative(path):
-            return os.path.relpath(
-                os.path.abspath(os.path.realpath(path)), spack.paths.prefix
     )
-
+    return spack.cmd.style.style(parser, args)
-        file_list = [prefix_relative(p) for p in file_list]
-
-    returncode = 0
-    with working_dir(spack.paths.prefix):
-        if not file_list:
-            file_list = changed_files(args.base, args.untracked, args.all)
-
-        print("=======================================================")
-        print("flake8: running flake8 code checks on spack.")
-        print()
-        print("Modified files:")
-        for filename in file_list:
-            print("  {0}".format(filename.strip()))
-        print("=======================================================")
-
-        output = ""
-        # run in chunks of 100 at a time to avoid line length limit
-        # filename parameter in config *does not work* for this reliably
-        for chunk in grouper(file_list, 100):
-            flake8_cmd = which("flake8", required=True)
-            chunk = filter(lambda e: e is not None, chunk)
-
-            output = flake8_cmd(
-                # use .flake8 implicitly to work around bug in flake8 upstream
-                # append-config is ignored if `--config` is explicitly listed
-                # see: https://gitlab.com/pycqa/flake8/-/issues/455
-                # "--config=.flake8",
-                *chunk,
-                fail_on_error=False,
-                output=str
-            )
-            returncode |= flake8_cmd.returncode
-
-            if args.root_relative:
-                # print results relative to repo root.
-                print(output)
-            else:
-                # print results relative to current working directory
-                def cwd_relative(path):
-                    return "{0}: [".format(
-                        os.path.relpath(
-                            os.path.join(spack.paths.prefix, path.group(1)),
-                            os.getcwd(),
-                        )
-                    )
-
-                for line in output.split("\n"):
-                    print(re.sub(r"^(.*): \[", cwd_relative, line))
-
-    if returncode != 0:
-        print("Flake8 found errors.")
-        sys.exit(1)
-    else:
-        print("Flake8 checks were clean.")

lib/spack/spack/cmd/module.py

@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import argparse
+from typing import Dict, Callable  # novm
 
 import llnl.util.tty as tty
 
@@ -15,7 +16,7 @@
 level = "short"
 
 
-_subcommands = {}
+_subcommands = {}  # type: Dict[str, Callable]
 
 _deprecated_commands = ('refresh', 'find', 'rm', 'loads')
 

lib/spack/spack/cmd/style.py

@@ -0,0 +1,330 @@
+# Copyright 2013-2020 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from __future__ import print_function
+
+import re
+import os
+import sys
+import argparse
+
+if sys.version_info < (3, 0):
+    from itertools import izip_longest  # novm
+
+    zip_longest = izip_longest
+else:
+    from itertools import zip_longest  # novm
+
+from llnl.util.filesystem import working_dir
+import llnl.util.tty as tty
+
+import spack.paths
+from spack.util.executable import which
+
+
+description = (
+    "runs source code style checks on Spack. Requires flake8, mypy, black for "
+    + "their respective checks"
+)
+section = "developer"
+level = "long"
+
+
+def grouper(iterable, n, fillvalue=None):
+    "Collect data into fixed-length chunks or blocks"
+    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
+    args = [iter(iterable)] * n
+    return zip_longest(*args, fillvalue=fillvalue)
+
+
+#: List of directories to exclude from checks.
+exclude_directories = [spack.paths.external_path]
+
+#: max line length we're enforcing (note: this duplicates what's in .flake8)
+max_line_length = 79
+
+
+def is_package(f):
+    """Whether flake8 should consider a file as a core file or a package.
+
+    We run flake8 with different exceptions for the core and for
+    packages, since we allow `from spack import *` and poking globals
+    into packages.
+    """
+    return f.startswith("var/spack/repos/") or "docs/tutorial/examples" in f
+
+
+def changed_files(base=None, untracked=True, all_files=False):
+    """Get list of changed files in the Spack repository."""
+
+    git = which("git", required=True)
+
+    if base is None:
+        base = os.environ.get("TRAVIS_BRANCH", "develop")
+
+    range = "{0}...".format(base)
+
+    git_args = [
+        # Add changed files committed since branching off of develop
+        ["diff", "--name-only", "--diff-filter=ACMR", range],
+        # Add changed files that have been staged but not yet committed
+        ["diff", "--name-only", "--diff-filter=ACMR", "--cached"],
+        # Add changed files that are unstaged
+        ["diff", "--name-only", "--diff-filter=ACMR"],
+    ]
+
+    # Add new files that are untracked
+    if untracked:
+        git_args.append(["ls-files", "--exclude-standard", "--other"])
+
+    # add everything if the user asked for it
+    if all_files:
+        git_args.append(["ls-files", "--exclude-standard"])
+
+    excludes = [os.path.realpath(f) for f in exclude_directories]
+    changed = set()
+
+    for arg_list in git_args:
+        files = git(*arg_list, output=str).split("\n")
+
+        for f in files:
+            # Ignore non-Python files
+            if not (f.endswith(".py") or f == "bin/spack"):
+                continue
+
+            # Ignore files in the exclude locations
+            if any(os.path.realpath(f).startswith(e) for e in excludes):
+                continue
+
+            changed.add(f)
+
+    return sorted(changed)
+
+
+def setup_parser(subparser):
+    subparser.add_argument(
+        "-b",
+        "--base",
+        action="store",
+        default=None,
+        help="select base branch for collecting list of modified files",
+    )
+    subparser.add_argument(
+        "-a",
+        "--all",
+        action="store_true",
+        help="check all files, not just changed files",
+    )
+    subparser.add_argument(
+        "-o",
+        "--output",
+        action="store_true",
+        help="send filtered files to stdout as well as temp files",
+    )
+    subparser.add_argument(
+        "-r",
+        "--root-relative",
+        action="store_true",
+        default=False,
+        help="print root-relative paths (default: cwd-relative)",
+    )
+    subparser.add_argument(
+        "-U",
+        "--no-untracked",
+        dest="untracked",
+        action="store_false",
+        default=True,
+        help="exclude untracked files from checks",
+    )
+    subparser.add_argument(
+        "--no-flake8",
+        dest="flake8",
+        action="store_false",
+        help="Do not run flake8, default is run flake8",
+    )
+    subparser.add_argument(
+        "--no-mypy",
+        dest="mypy",
+        action="store_false",
+        help="Do not run mypy, default is run mypy if available",
+    )
+    subparser.add_argument(
+        "--black",
+        dest="black",
+        action="store_true",
+        help="Run black checks, default is skip",
+    )
+    subparser.add_argument(
+        "files", nargs=argparse.REMAINDER, help="specific files to check"
+    )
+
+
+def rewrite_and_print_output(
+    output,
+    args,
+    re_obj=re.compile(r"^(.+):([0-9]+):"),
+    replacement=r"{0}:{1}:",
+):
+    """rewrite ouput with <file>:<line>: format to respect path args"""
+    if args.root_relative or re_obj is None:
+        # print results relative to repo root.
+        print(output)
+    else:
+        # print results relative to current working directory
+        def cwd_relative(path):
+            return replacement.format(
+                os.path.relpath(
+                    os.path.join(spack.paths.prefix, path.group(1)),
+                    os.getcwd(),
+                ),
+                *list(path.groups()[1:])
+            )
+
+        for line in output.split("\n"):
+            print(
+                re_obj.sub(
+                    cwd_relative,
+                    line,
+                )
+            )
+
+
+def print_style_header(file_list, args):
+    tty.msg("style: running code checks on spack.")
+    tools = []
+    if args.flake8:
+        tools.append("flake8")
+    if args.mypy:
+        tools.append("mypy")
+    if args.black:
+        tools.append("black")
+    tty.msg("style: tools selected: " + ", ".join(tools))
+    tty.msg("Modified files:", *[filename.strip() for filename in file_list])
+    sys.stdout.flush()
+
+
+def print_tool_header(tool):
+    sys.stdout.flush()
+    tty.msg("style: running %s checks on spack." % tool)
+    sys.stdout.flush()
+
+
+def run_flake8(file_list, args):
+    returncode = 0
+    print_tool_header("flake8")
+    flake8_cmd = which("flake8", required=True)
+
+    output = ""
+    # run in chunks of 100 at a time to avoid line length limit
+    # filename parameter in config *does not work* for this reliably
+    for chunk in grouper(file_list, 100):
+        chunk = filter(lambda e: e is not None, chunk)
+
+        output = flake8_cmd(
+            # use .flake8 implicitly to work around bug in flake8 upstream
+            # append-config is ignored if `--config` is explicitly listed
+            # see: https://gitlab.com/pycqa/flake8/-/issues/455
+            # "--config=.flake8",
+            *chunk,
+            fail_on_error=False,
+            output=str
+        )
+        returncode |= flake8_cmd.returncode
+
+        rewrite_and_print_output(output, args)
+
+    if returncode == 0:
+        tty.msg("Flake8 style checks were clean")
+    else:
+        tty.error("Flake8 style checks found errors")
+    return returncode
+
+
+def run_mypy(file_list, args):
+    mypy_cmd = which("mypy")
+    if mypy_cmd is None:
+        tty.error("style: mypy is not available in path, skipping")
+        return 1
+
+    print_tool_header("mypy")
+
+    returncode = 0
+    output = ""
+    # run in chunks of 100 at a time to avoid line length limit
+    # filename parameter in config *does not work* for this reliably
+    for chunk in grouper(file_list, 100):
+        chunk = filter(lambda e: e is not None, chunk)
+
+        output = mypy_cmd(*chunk, fail_on_error=False, output=str)
+        returncode |= mypy_cmd.returncode
+
+        rewrite_and_print_output(output, args)
+
+    if returncode == 0:
+        tty.msg("mypy checks were clean")
+    else:
+        tty.error("mypy checks found errors")
+    return returncode
+
+
+def run_black(file_list, args):
+    black_cmd = which("black")
+    if black_cmd is None:
+        tty.error("style: black is not available in path, skipping")
+        return 1
+
+    print_tool_header("black")
+
+    pat = re.compile("would reformat +(.*)")
+    replacement = "would reformat {0}"
+    returncode = 0
+    output = ""
+    # run in chunks of 100 at a time to avoid line length limit
+    # filename parameter in config *does not work* for this reliably
+    for chunk in grouper(file_list, 100):
+        chunk = filter(lambda e: e is not None, chunk)
+
+        output = black_cmd(
+            "--check", "--diff", *chunk, fail_on_error=False, output=str, error=str
+        )
+        returncode |= black_cmd.returncode
+
+        rewrite_and_print_output(output, args, pat, replacement)
+
+    if returncode == 0:
+        tty.msg("black style checks were clean")
+    else:
+        tty.error("black checks found errors")
+    return returncode
+
+
+def style(parser, args):
+    file_list = args.files
+    if file_list:
+
+        def prefix_relative(path):
+            return os.path.relpath(
+                os.path.abspath(os.path.realpath(path)), spack.paths.prefix
+            )
+
+        file_list = [prefix_relative(p) for p in file_list]
+
+    returncode = 0
+    with working_dir(spack.paths.prefix):
+        if not file_list:
+            file_list = changed_files(args.base, args.untracked, args.all)
+        print_style_header(file_list, args)
+        if args.flake8:
+            returncode = run_flake8(file_list, args)
+        if args.mypy:
+            returncode |= run_mypy(file_list, args)
+        if args.black:
+            returncode |= run_black(file_list, args)
+
+    if returncode != 0:
+        print("spack style found errors.")
+        sys.exit(1)
+    else:
+        print("spack style checks were clean.")

lib/spack/spack/compiler.py

@@ -10,6 +10,7 @@
 import itertools
 import shutil
 import tempfile
+from typing import Sequence, List  # novm
 
 import llnl.util.lang
 from llnl.util.filesystem import (
@@ -190,20 +191,20 @@ class Compiler(object):
        and how to identify the particular type of compiler."""
 
     # Subclasses use possible names of C compiler
-    cc_names = []
+    cc_names = []  # type: List[str]
 
     # Subclasses use possible names of C++ compiler
-    cxx_names = []
+    cxx_names = []  # type: List[str]
 
     # Subclasses use possible names of Fortran 77 compiler
-    f77_names = []
+    f77_names = []  # type: List[str]
 
     # Subclasses use possible names of Fortran 90 compiler
-    fc_names = []
+    fc_names = []  # type: List[str]
 
     # Optional prefix regexes for searching for this type of compiler.
     # Prefixes are sometimes used for toolchains
-    prefixes = []
+    prefixes = []  # type: List[str]
 
     # Optional suffix regexes for searching for this type of compiler.
     # Suffixes are used by some frameworks, e.g. macports uses an '-mp-X.Y'
@@ -214,7 +215,7 @@ class Compiler(object):
     version_argument = '-dumpversion'
 
     #: Return values to ignore when invoking the compiler to get its version
-    ignore_version_errors = ()
+    ignore_version_errors = ()  # type: Sequence[int]
 
     #: Regex used to extract version from compiler's output
     version_regex = '(.*)'
@@ -266,9 +267,9 @@ def opt_flags(self):
         return ['-O', '-O0', '-O1', '-O2', '-O3']
 
     # Cray PrgEnv name that can be used to load this compiler
-    PrgEnv = None
+    PrgEnv = None  # type: str
     # Name of module used to switch versions of this compiler
-    PrgEnv_compiler = None
+    PrgEnv_compiler = None  # type: str
 
     def __init__(self, cspec, operating_system, target,
                  paths, modules=None, alias=None, environment=None,

lib/spack/spack/compilers/__init__.py

@@ -11,6 +11,7 @@
 import multiprocessing.pool
 import os
 import six
+from typing import Dict  # novm
 
 import llnl.util.lang
 import llnl.util.filesystem as fs
@@ -21,6 +22,7 @@
 import spack.error
 import spack.spec
 import spack.config
+import spack.compiler
 import spack.architecture
 import spack.util.imp as simp
 from spack.util.environment import get_path
@@ -36,7 +38,7 @@
 # TODO: Caches at module level make it difficult to mock configurations in
 # TODO: unit tests. It might be worth reworking their implementation.
 #: cache of compilers constructed from config data, keyed by config entry id.
-_compiler_cache = {}
+_compiler_cache = {}  # type: Dict[str, spack.compiler.Compiler]
 
 _compiler_to_pkg = {
     'clang': 'llvm+clang'

lib/spack/spack/compilers/nag.py

@@ -3,15 +3,17 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+from typing import List  # novm
+
 import spack.compiler
 
 
 class Nag(spack.compiler.Compiler):
     # Subclasses use possible names of C compiler
-    cc_names = []
+    cc_names = []  # type: List[str]
 
     # Subclasses use possible names of C++ compiler
-    cxx_names = []
+    cxx_names = []  # type: List[str]
 
     # Subclasses use possible names of Fortran 77 compiler
     f77_names = ['nagfor']

lib/spack/spack/config.py

@@ -38,6 +38,7 @@
 from contextlib import contextmanager
 from six import iteritems
 from ordereddict_backport import OrderedDict
+from typing import List  # novm
 
 import ruamel.yaml as yaml
 from ruamel.yaml.error import MarkedYAMLError
@@ -735,7 +736,7 @@ def override(path_or_scope, value=None):
 
 #: configuration scopes added on the command line
 #: set by ``spack.main.main()``.
-command_line_scopes = []
+command_line_scopes = []  # type: List[str]
 
 
 def _add_platform_scope(cfg, scope_type, name, path):

lib/spack/spack/database.py

@@ -27,6 +27,8 @@
 import socket
 import sys
 import time
+from typing import Dict  # novm
+
 try:
     import uuid
     _use_uuid = True
@@ -289,10 +291,10 @@ def __getattribute__(self, name):
 class Database(object):
 
     """Per-process lock objects for each install prefix."""
-    _prefix_locks = {}
+    _prefix_locks = {}  # type: Dict[str, lk.Lock]
 
     """Per-process failure (lock) objects for each install prefix."""
-    _prefix_failures = {}
+    _prefix_failures = {}  # type: Dict[str, lk.Lock]
 
     def __init__(self, root, db_dir=None, upstream_dbs=None,
                  is_upstream=False, enable_transaction_locking=True,
@@ -1464,6 +1466,8 @@ def _query(
 
         return results
 
+    if _query.__doc__ is None:
+        _query.__doc__ = ""
     _query.__doc__ += _query_docstring
 
     def query_local(self, *args, **kwargs):
@@ -1471,6 +1475,8 @@ def query_local(self, *args, **kwargs):
         with self.read_transaction():
             return sorted(self._query(*args, **kwargs))
 
+    if query_local.__doc__ is None:
+        query_local.__doc__ = ""
     query_local.__doc__ += _query_docstring
 
     def query(self, *args, **kwargs):
@@ -1489,6 +1495,8 @@ def query(self, *args, **kwargs):
 
         return sorted(results)
 
+    if query.__doc__ is None:
+        query.__doc__ = ""
     query.__doc__ += _query_docstring
 
     def query_one(self, query_spec, known=any, installed=True):

lib/spack/spack/directives.py

@@ -33,6 +33,7 @@ class OpenMpi(Package):
 import os.path
 import re
 from six import string_types
+from typing import Set, List  # novm
 
 import llnl.util.lang
 import llnl.util.tty.color
@@ -103,8 +104,8 @@ class DirectiveMeta(type):
     """
 
     # Set of all known directives
-    _directive_names = set()
+    _directive_names = set()  # type: Set[str]
-    _directives_to_be_executed = []
+    _directives_to_be_executed = []  # type: List[str]
 
     def __new__(cls, name, bases, attr_dict):
         # Initialize the attribute containing the list of directives

lib/spack/spack/fetch_strategy.py

@@ -29,6 +29,7 @@
 import re
 import shutil
 import sys
+from typing import Optional, List  # novm
 
 import llnl.util.tty as tty
 import six
@@ -92,11 +93,12 @@ class FetchStrategy(object):
     #: The URL attribute must be specified either at the package class
     #: level, or as a keyword argument to ``version()``.  It is used to
     #: distinguish fetchers for different versions in the package DSL.
-    url_attr = None
+    url_attr = None  # type: Optional[str]
 
     #: Optional attributes can be used to distinguish fetchers when :
     #: classes have multiple ``url_attrs`` at the top-level.
-    optional_attrs = []  # optional attributes in version() args.
+    # optional attributes in version() args.
+    optional_attrs = []  # type: List[str]
 
     def __init__(self, **kwargs):
         # The stage is initialized late, so that fetch strategies can be
@@ -420,7 +422,7 @@ def _fetch_from_url(self, url):
             warn_content_type_mismatch(self.archive_file or "the archive")
         return partial_file, save_file
 
-    @property
+    @property  # type: ignore # decorated properties unsupported in mypy
     @_needs_stage
     def archive_file(self):
         """Path to the source archive within this stage directory."""

lib/spack/spack/installer.py

@@ -1542,7 +1542,7 @@ def install(self):
                     (stop_before_phase is None and last_phase is None)
 
             except spack.directory_layout.InstallDirectoryAlreadyExistsError \
-                    as err:
+                    as exc:
                 tty.debug('Install prefix for {0} exists, keeping {1} in '
                           'place.'.format(pkg.name, pkg.prefix))
                 self._update_installed(task)
@@ -1553,7 +1553,7 @@ def install(self):
                     raise
 
                 if task.explicit:
-                    exists_errors.append((pkg_id, str(err)))
+                    exists_errors.append((pkg_id, str(exc)))
 
             except KeyboardInterrupt as exc:
                 # The build has been terminated with a Ctrl-C so terminate

lib/spack/spack/mirror.py

@@ -23,9 +23,9 @@
 
 from ordereddict_backport import OrderedDict
 
-try:
+if sys.version_info >= (3, 5):
     from collections.abc import Mapping  # novm
-except ImportError:
+else:
     from collections import Mapping
 
 import llnl.util.tty as tty

lib/spack/spack/mixins.py

@@ -8,6 +8,12 @@
 """
 import collections
 import os
+import sys
+from typing import Callable, DefaultDict, Dict, List  # novm
+if sys.version_info >= (3, 5):
+    CallbackDict = DefaultDict[str, List[Callable]]
+else:
+    CallbackDict = None
 
 import llnl.util.filesystem
 
@@ -26,12 +32,12 @@ class PackageMixinsMeta(type):
     gets implicitly attached to the package class by calling the mixin.
     """
 
-    _methods_to_be_added = {}
+    _methods_to_be_added = {}  # type: Dict[str, Callable]
-    _add_method_before = collections.defaultdict(list)
+    _add_method_before = collections.defaultdict(list)  # type: CallbackDict
-    _add_method_after = collections.defaultdict(list)
+    _add_method_after = collections.defaultdict(list)  # type: CallbackDict
 
     @staticmethod
-    def register_method_before(fn, phase):
+    def register_method_before(fn, phase):  # type: (Callable, str) -> None
         """Registers a method to be run before a certain phase.
 
         Args:
@@ -42,7 +48,7 @@ def register_method_before(fn, phase):
         PackageMixinsMeta._add_method_before[phase].append(fn)
 
     @staticmethod
-    def register_method_after(fn, phase):
+    def register_method_after(fn, phase):  # type: (Callable, str) -> None
         """Registers a method to be run after a certain phase.
 
         Args:

lib/spack/spack/modules/common.py

@@ -34,6 +34,7 @@
 import inspect
 import os.path
 import re
+from typing import Optional  # novm
 
 import llnl.util.filesystem
 from llnl.util.lang import dedupe
@@ -540,7 +541,7 @@ class BaseFileLayout(object):
     """
 
     #: This needs to be redefined
-    extension = None
+    extension = None  # type: Optional[str]
 
     def __init__(self, configuration):
         self.conf = configuration

lib/spack/spack/modules/lmod.py

@@ -8,6 +8,7 @@
 import llnl.util.lang as lang
 import itertools
 import collections
+from typing import Dict, Any  # novm
 
 import spack.config
 import spack.compilers
@@ -26,7 +27,7 @@ def configuration():
 
 
 #: Caches the configuration {spec_hash: configuration}
-configuration_registry = {}
+configuration_registry = {}  # type: Dict[str, Any]
 
 
 def make_configuration(spec):

lib/spack/spack/modules/tcl.py

@@ -8,6 +8,7 @@
 """
 import os.path
 import string
+from typing import Dict, Any  # novm
 
 import llnl.util.tty as tty
 
@@ -24,7 +25,7 @@ def configuration():
 
 
 #: Caches the configuration {spec_hash: configuration}
-configuration_registry = {}
+configuration_registry = {}  # type: Dict[str, Any]
 
 
 def make_configuration(spec):

lib/spack/spack/package.py

@@ -26,6 +26,7 @@
 import traceback
 import six
 import types
+from typing import Optional, List, Dict, Any, Callable  # novm
 
 import llnl.util.filesystem as fsys
 import llnl.util.tty as tty
@@ -253,8 +254,9 @@ class PackageMeta(
     """
     phase_fmt = '_InstallPhase_{0}'
 
-    _InstallPhase_run_before = {}
+    # These are accessed only through getattr, by name
-    _InstallPhase_run_after = {}
+    _InstallPhase_run_before = {}  # type: Dict[str, List[Callable]]
+    _InstallPhase_run_after = {}  # type: Dict[str, List[Callable]]
 
     def __new__(cls, name, bases, attr_dict):
         """
@@ -555,7 +557,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     #: A list or set of build time test functions to be called when tests
     #: are executed or 'None' if there are no such test functions.
-    build_time_test_callbacks = None
+    build_time_test_callbacks = None  # type: Optional[List[str]]
 
     #: By default, packages are not virtual
     #: Virtual packages override this attribute
@@ -567,7 +569,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     #: A list or set of install time test functions to be called when tests
     #: are executed or 'None' if there are no such test functions.
-    install_time_test_callbacks = None
+    install_time_test_callbacks = None  # type: Optional[List[str]]
 
     #: By default we build in parallel.  Subclasses can override this.
     parallel = True
@@ -589,19 +591,19 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
     #: List of prefix-relative file paths (or a single path). If these do
     #: not exist after install, or if they exist but are not files,
     #: sanity checks fail.
-    sanity_check_is_file = []
+    sanity_check_is_file = []  # type: List[str]
 
     #: List of prefix-relative directory paths (or a single path). If
     #: these do not exist after install, or if they exist but are not
     #: directories, sanity checks will fail.
-    sanity_check_is_dir = []
+    sanity_check_is_dir = []  # type: List[str]
 
     #: List of glob expressions. Each expression must either be
     #: absolute or relative to the package source path.
     #: Matching artifacts found at the end of the build process will be
     #: copied in the same directory tree as _spack_build_logfile and
     #: _spack_build_envfile.
-    archive_files = []
+    archive_files = []  # type: List[str]
 
     #: Boolean. Set to ``True`` for packages that require a manual download.
     #: This is currently used by package sanity tests and generation of a
@@ -609,7 +611,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
     manual_download = False
 
     #: Set of additional options used when fetching package versions.
-    fetch_options = {}
+    fetch_options = {}  # type: Dict[str, Any]
 
     #
     # Set default licensing information
@@ -627,12 +629,12 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
     #: looking for a license. All file paths must be relative to the
     #: installation directory. More complex packages like Intel may require
     #: multiple licenses for individual components. Defaults to the empty list.
-    license_files = []
+    license_files = []  # type: List[str]
 
     #: List of strings. Environment variables that can be set to tell the
     #: software where to look for a license if it is not in the usual location.
     #: Defaults to the empty list.
-    license_vars = []
+    license_vars = []  # type: List[str]
 
     #: String. A URL pointing to license setup instructions for the software.
     #: Defaults to the empty string.
@@ -646,7 +648,7 @@ class PackageBase(six.with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     #: List of strings which contains GitHub usernames of package maintainers.
     #: Do not include @ here in order not to unnecessarily ping the users.
-    maintainers = []
+    maintainers = []  # type: List[str]
 
     #: List of attributes to be excluded from a package's hash.
     metadata_attrs = ['homepage', 'url', 'urls', 'list_url', 'extendable',
@@ -2591,7 +2593,7 @@ class BundlePackage(PackageBase):
     """General purpose bundle, or no-code, package class."""
     #: There are no phases by default but the property is required to support
     #: post-install hooks (e.g., for module generation).
-    phases = []
+    phases = []  # type: List[str]
     #: This attribute is used in UI queries that require to know which
     #: build-system class we are using
     build_system_class = 'BundlePackage'

lib/spack/spack/repo.py

@@ -13,18 +13,18 @@
 import os
 import re
 import shutil
+import six
 import stat
 import sys
 import traceback
 import types
+from typing import Dict  # novm
 
-try:
+if sys.version_info >= (3, 5):
     from collections.abc import Mapping  # novm
-except ImportError:
+else:
     from collections import Mapping
 
-import six
-
 import ruamel.yaml as yaml
 
 import llnl.util.lang
@@ -131,7 +131,7 @@ class FastPackageChecker(Mapping):
     during instance initialization.
     """
     #: Global cache, reused by every instance
-    _paths_cache = {}
+    _paths_cache = {}  # type: Dict[str, Dict[str, os.stat_result]]
 
     def __init__(self, packages_path):
         # The path of the repository managed by this instance
@@ -149,7 +149,7 @@ def invalidate(self):
         self._paths_cache[self.packages_path] = self._create_new_cache()
         self._packages_to_stats = self._paths_cache[self.packages_path]
 
-    def _create_new_cache(self):
+    def _create_new_cache(self):  # type: () -> Dict[str, os.stat_result]
         """Create a new cache for packages in a repo.
 
         The implementation here should try to minimize filesystem
@@ -159,7 +159,7 @@ def _create_new_cache(self):
         """
         # Create a dictionary that will store the mapping between a
         # package name and its stat info
-        cache = {}
+        cache = {}  # type: Dict[str, os.stat_result]
         for pkg_name in os.listdir(self.packages_path):
             # Skip non-directories in the package root.
             pkg_dir = os.path.join(self.packages_path, pkg_name)
@@ -341,7 +341,7 @@ class PatchIndexer(Indexer):
     def _create(self):
         return spack.patch.PatchCache()
 
-    def needs_update():
+    def needs_update(self):
         # TODO: patches can change under a package and we should handle
         # TODO: it, but we currently punt. This should be refactored to
         # TODO: check whether patches changed each time a package loads,

lib/spack/spack/schema/environment.py

@@ -38,11 +38,12 @@ def parse(config_obj):
         config_obj: a configuration dictionary conforming to the
             schema definition for environment modifications
     """
+    import sys
     import spack.util.environment as ev
-    try:
+    if sys.version_info >= (3, 5):
-        from collections import Sequence  # novm
-    except ImportError:
         from collections.abc import Sequence  # novm
+    else:
+        from collections import Sequence  # novm
 
     env = ev.EnvironmentModifications()
     for command, variable in config_obj.items():

lib/spack/spack/solver/asp.py

@@ -19,7 +19,7 @@
 try:
     import clingo
 except ImportError:
-    clingo = None
+    clingo = None  # type: ignore
 
 import llnl.util.lang
 import llnl.util.tty as tty

lib/spack/spack/spec.py

@@ -3269,7 +3269,7 @@ def virtual_dependencies(self):
         """Return list of any virtual deps in this spec."""
         return [spec for spec in self.traverse() if spec.virtual]
 
-    @property
+    @property  # type: ignore[misc] # decorated prop not supported in mypy
     @lang.memoized
     def patches(self):
         """Return patch objects for any patch sha256 sums on this Spec.

lib/spack/spack/stage.py

@@ -16,6 +16,7 @@
 import tempfile
 from six import string_types
 from six import iteritems
+from typing import Dict  # novm
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp, can_access, install, install_tree
@@ -221,7 +222,7 @@ class Stage(object):
     """
 
     """Shared dict of all stage locks."""
-    stage_locks = {}
+    stage_locks = {}  # type: Dict[str, spack.util.lock.Lock]
 
     """Most staging is managed by Spack.  DIYStage is one exception."""
     managed_by_spack = True

lib/spack/spack/tengine.py

@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 import itertools
 import textwrap
+from typing import List  # novm
 
 import llnl.util.lang
 import six
@@ -18,7 +19,7 @@ class ContextMeta(type):
     """
     #: Keeps track of the context properties that have been added
     #: by the class that is being defined
-    _new_context_properties = []
+    _new_context_properties = []  # type: List[str]
 
     def __new__(cls, name, bases, attr_dict):
         # Merge all the context properties that are coming from base classes

lib/spack/spack/test/cmd/flake8.py

@@ -11,12 +11,12 @@
 from llnl.util.filesystem import FileFilter
 
 import spack.paths
-from spack.cmd.flake8 import flake8, setup_parser, changed_files
+from spack.cmd.style import style, setup_parser, changed_files
 from spack.repo import Repo
 from spack.util.executable import which
 
 
-@pytest.fixture(scope='module')
+@pytest.fixture(scope="module")
 def parser():
     """Returns the parser for the ``flake8`` command"""
     parser = argparse.ArgumentParser()
@@ -24,7 +24,7 @@ def parser():
     return parser
 
 
-@pytest.fixture(scope='module')
+@pytest.fixture(scope="module")
 def flake8_package():
     """Flake8 only checks files that have been modified.
     This fixture makes a small change to the ``flake8``
@@ -32,7 +32,7 @@ def flake8_package():
     change on cleanup.
     """
     repo = Repo(spack.paths.mock_packages_path)
-    filename = repo.filename_for_package_name('flake8')
+    filename = repo.filename_for_package_name("flake8")
     package = FileFilter(filename)
 
     # Make the change
@@ -60,16 +60,18 @@ def test_changed_files(parser, flake8_package):
 # As of flake8 3.0.0, Python 2.6 and 3.3 are no longer supported
 # http://flake8.pycqa.org/en/latest/release-notes/3.0.0.html
 @pytest.mark.skipif(
-    sys.version_info[:2] <= (2, 6) or
+    sys.version_info[:2] <= (2, 6) or (3, 0) <= sys.version_info[:2] <= (3, 3),
-    (3, 0) <= sys.version_info[:2] <= (3, 3),
+    reason="flake8 no longer supports Python 2.6 or 3.3 and older",
-    reason='flake8 no longer supports Python 2.6 or 3.3 and older')
+)
-@pytest.mark.skipif(not which('flake8'), reason='flake8 is not installed.')
+@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
 def test_flake8(parser, flake8_package):
     # Only test the flake8_package that we modified
     # Otherwise, the unit tests would fail every time
     # the flake8 tests fail
-    args = parser.parse_args([flake8_package])
+    args = parser.parse_args(["--no-mypy", flake8_package])
-    flake8(parser, args)
+    style(parser, args)
     # Get even more coverage
-    args = parser.parse_args(['--output', '--root-relative', flake8_package])
+    args = parser.parse_args(
-    flake8(parser, args)
+        ["--no-mypy", "--output", "--root-relative", flake8_package]
+    )
+    style(parser, args)

lib/spack/spack/test/conftest.py

@@ -318,7 +318,7 @@ def _skip_if_missing_executables(request):
 # FIXME: The lines below should better be added to a fixture with
 # FIXME: session-scope. Anyhow doing it is not easy, as it seems
 # FIXME: there's some weird interaction with compilers during concretization.
-spack.architecture.real_platform = spack.architecture.platform
+spack.architecture.real_platform = spack.architecture.platform  # type: ignore
 
 
 def test_platform():
@@ -1238,7 +1238,7 @@ def mock_test_repo(tmpdir_factory):
 class MockBundle(object):
     has_code = False
     name = 'mock-bundle'
-    versions = {}
+    versions = {}  # type: ignore
 
 
 @pytest.fixture

lib/spack/spack/test/package_sanity.py

@@ -16,7 +16,7 @@
 import spack.variant
 # A few functions from this module are used to
 # do sanity checks only on packagess modified by a PR
-import spack.cmd.flake8 as flake8
+import spack.cmd.style as style
 import spack.util.crypto as crypto
 import pickle
 
@@ -207,7 +207,7 @@ def test_prs_update_old_api():
     deprecated calls to any method.
     """
     changed_package_files = [
-        x for x in flake8.changed_files() if flake8.is_package(x)
+        x for x in style.changed_files() if style.is_package(x)
     ]
     failing = []
     for file in changed_package_files:

lib/spack/spack/test/sbang.py

@@ -44,7 +44,7 @@
 last_line  = "last!\n"
 
 
-@pytest.fixture
+@pytest.fixture  # type: ignore[no-redef]
 def sbang_line():
     yield '#!/bin/sh %s/bin/sbang\n' % spack.store.layout.root
 

lib/spack/spack/util/crypto.py

@@ -5,10 +5,10 @@
 
 import sys
 import hashlib
+from typing import Dict, Callable, Any  # novm
 
 import llnl.util.tty as tty
 
-
 #: Set of hash algorithms that Spack can use, mapped to digest size in bytes
 hashes = {
     'md5': 16,
@@ -30,7 +30,7 @@
 
 
 #: cache of hash functions generated
-_hash_functions = {}
+_hash_functions = {}  # type: Dict[str, Callable[[], Any]]
 
 
 class DeprecatedHash(object):

lib/spack/spack/util/gpg.py

@@ -115,7 +115,7 @@ def result(self):
         return result
 
 
-class GpgConstants(object):
+class _GpgConstants(object):
     @cached_property
     def target_version(self):
         return spack.version.Version('2')
@@ -189,7 +189,7 @@ def user_run_dir(self):
         #                "file-name-too-long" errors in gpg.
 
         try:
-            has_suitable_gpgconf = bool(GpgConstants.gpgconf_string)
+            has_suitable_gpgconf = bool(_GpgConstants.gpgconf_string)
         except SpackGPGError:
             has_suitable_gpgconf = False
 
@@ -244,7 +244,7 @@ def clear(self):
                 pass
 
 
-GpgConstants = GpgConstants()
+GpgConstants = _GpgConstants()
 
 
 def ensure_gpg(reevaluate=False):
@@ -382,7 +382,7 @@ def gpg(*args, **kwargs):
     return get_global_gpg_instance()(*args, **kwargs)
 
 
-gpg.name = 'gpg'
+gpg.name = 'gpg'  # type: ignore[attr-defined]
 
 
 @functools.wraps(Gpg.create)

lib/spack/spack/util/lock.py

@@ -15,7 +15,7 @@
 import spack.paths
 
 
-class Lock(llnl.util.lock.Lock):
+class Lock(llnl.util.lock.Lock):  # type: ignore[no-redef]
     """Lock that can be disabled.
 
     This overrides the ``_lock()`` and ``_unlock()`` methods from

lib/spack/spack/util/log_parse.py

@@ -43,7 +43,7 @@ def parse_log_events(stream, context=6, jobs=None, profile=False):
 
 
 #: lazily constructed CTest log parser
-parse_log_events.ctest_parser = None
+parse_log_events.ctest_parser = None  # type: ignore[attr-defined]
 
 
 def _wrap(text, width):
@@ -107,7 +107,7 @@ def make_log_context(log_events, width=None):
         for i in range(start, event.end):
             # wrap to width
             lines = _wrap(event[i], wrap_width)
-            lines[1:] = [indent + l for l in lines[1:]]
+            lines[1:] = [indent + ln for ln in lines[1:]]
             wrapped_line = line_fmt % (i, '\n'.join(lines))
 
             if i in error_lines:

lib/spack/spack/util/spack_yaml.py

@@ -14,6 +14,7 @@
 """
 import ctypes
 import collections
+from typing import List  # novm
 
 from ordereddict_backport import OrderedDict
 from six import string_types, StringIO
@@ -219,7 +220,7 @@ def file_line(mark):
 #: This is nasty but YAML doesn't give us many ways to pass arguments --
 #: yaml.dump() takes a class (not an instance) and instantiates the dumper
 #: itself, so we can't just pass an instance
-_annotations = []
+_annotations = []  # type: List[str]
 
 
 class LineAnnotationDumper(OrderedLineDumper):

lib/spack/spack/util/web.py

@@ -20,10 +20,10 @@
 from six.moves.urllib.error import URLError
 from six.moves.urllib.request import urlopen, Request
 
-try:
+if sys.version_info < (3, 0):
     # Python 2 had these in the HTMLParser package.
     from HTMLParser import HTMLParser, HTMLParseError  # novm
-except ImportError:
+else:
     # In Python 3, things moved to html.parser
     from html.parser import HTMLParser
 

lib/spack/spack/variant.py

@@ -12,6 +12,12 @@
 import itertools
 import re
 from six import StringIO
+import sys
+
+if sys.version_info >= (3, 5):
+    from collections.abc import Sequence  # novm
+else:
+    from collections import Sequence
 
 import llnl.util.tty.color
 import llnl.util.lang as lang
@@ -20,11 +26,6 @@
 import spack.directives
 import spack.error as error
 
-try:
-    from collections.abc import Sequence  # novm
-except ImportError:
-    from collections import Sequence
-
 special_variant_values = [None, 'none', '*']
 
 

share/spack/qa/run-style-tests

@@ -15,10 +15,10 @@
 #     run-flake8-tests
 #
 . "$(dirname $0)/setup.sh"
-check_dependencies flake8
+check_dependencies flake8 mypy
 
 # verify that the code style is correct
-spack flake8
+spack style
 
 # verify that the license headers are present
 spack license verify

share/spack/qa/setup.sh

@@ -66,6 +66,10 @@ check_dependencies() {
                     spack_package=py-flake8
                     pip_package=flake8
                     ;;
+                mypy)
+                    spack_package=py-mypy
+                    pip_package=mypy
+                    ;;
                 dot)
                     spack_package=graphviz
                     ;;

share/spack/spack-completion.bash

@@ -333,7 +333,7 @@ _spack() {
     then
         SPACK_COMPREPLY="-h --help -H --all-help --color -C --config-scope -d --debug --timestamp --pdb -e --env -D --env-dir -E --no-env --use-env-repo -k --insecure -l --enable-locks -L --disable-locks -m --mock -p --profile --sorted-profile --lines -v --verbose --stacktrace -V --version --print-shell-vars"
     else
-        SPACK_COMPREPLY="activate add arch blame build-env buildcache cd checksum ci clean clone commands compiler compilers concretize config containerize create deactivate debug dependencies dependents deprecate dev-build develop docs edit env extensions external fetch find flake8 gc gpg graph help info install license list load location log-parse maintainers mark mirror module patch pkg providers pydoc python reindex remove rm repo resource restage setup solve spec stage test test-env tutorial undevelop uninstall unit-test unload url verify versions view"
+        SPACK_COMPREPLY="activate add arch blame build-env buildcache cd checksum ci clean clone commands compiler compilers concretize config containerize create deactivate debug dependencies dependents deprecate dev-build develop docs edit env extensions external fetch find flake8 gc gpg graph help info install license list load location log-parse maintainers mark mirror module patch pkg providers pydoc python reindex remove rm repo resource restage setup solve spec stage style test test-env tutorial undevelop uninstall unit-test unload url verify versions view"
     fi
 }
 
@@ -913,7 +913,7 @@ _spack_find() {
 _spack_flake8() {
     if $list_options
     then
-        SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked"
+        SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked --no-flake8 --no-mypy --black"
     else
         SPACK_COMPREPLY=""
     fi
@@ -1513,6 +1513,15 @@ _spack_stage() {
     fi
 }
 
+_spack_style() {
+    if $list_options
+    then
+        SPACK_COMPREPLY="-h --help -b --base -a --all -o --output -r --root-relative -U --no-untracked --no-flake8 --no-mypy --black"
+    else
+        SPACK_COMPREPLY=""
+    fi
+}
+
 _spack_test() {
     if $list_options
     then