Type hints that satisfy `mypy --py2 --silent-imports --strict-optional utils/lit/lit/main.py`

typehints-lit-main.diff

diff --git a/utils/lit/lit/ProgressBar.py b/utils/lit/lit/ProgressBar.py
index 3ad704d..e05bd7c 100644
--- a/utils/lit/lit/ProgressBar.py
+++ b/utils/lit/lit/ProgressBar.py
@@ -167,7 +167,17 @@ class TerminalController:
 # Example use case: progress bar
 #######################################################################
 
-class SimpleProgressBar:
+class ProgressBarBase(object):
+    def update(self, percent, message):
+        # type: (float, str) -> None
+        raise NotImplementedError()
+
+    def clear(self):
+        # type: () -> None
+        raise NotImplementedError()
+
+
+class SimpleProgressBar(ProgressBarBase):
     """
     A simple progress bar which doesn't need any terminal support.
 
@@ -207,7 +217,7 @@ class SimpleProgressBar:
             sys.stdout.flush()
             self.atIndex = None
 
-class ProgressBar:
+class ProgressBar(ProgressBarBase):
     """
     A 3-line progress bar, which looks like::
     
diff --git a/utils/lit/lit/main.py b/utils/lit/lit/main.py
index ac3066e..3d0e5d1 100755
--- a/utils/lit/lit/main.py
+++ b/utils/lit/lit/main.py
@@ -6,7 +6,6 @@ lit - LLVM Integrated Tester.
 See lit.pod for more information.
 """
 
-from __future__ import absolute_import
 import os
 import platform
 import random
@@ -24,14 +23,25 @@ import lit.run
 import lit.util
 import lit.discovery
 
+# Used for static analysis. This is available in Python 3
+# and when running static analysis tools such as mypy.
+try:
+    from typing import Dict, List, Optional
+except ImportError:
+    pass
+
+
+
 class TestingProgressDisplay(object):
     def __init__(self, opts, numTests, progressBar=None):
+        # type: (argparse.Namespace, int, Optional[lit.ProgressBar.ProgressBarBase]) -> None
         self.opts = opts
         self.numTests = numTests
         self.progressBar = progressBar
         self.completed = 0
 
     def finish(self):
+        # type: () -> None
         if self.progressBar:
             self.progressBar.clear()
         elif self.opts.quiet:
@@ -40,6 +50,7 @@ class TestingProgressDisplay(object):
             sys.stdout.write('\n')
 
     def update(self, test):
+        # type: (lit.Test.Test) -> None
         self.completed += 1
 
         if self.opts.incremental:
@@ -85,6 +96,7 @@ class TestingProgressDisplay(object):
         sys.stdout.flush()
 
 def write_test_results(run, lit_config, testing_time, output_path):
+    # type: (lit.run.Run, lit.LitConfig.LitConfig, float, str) -> None
     try:
         import json
     except ImportError:
@@ -124,13 +136,16 @@ def write_test_results(run, lit_config, testing_time, output_path):
         f.close()
 
 def update_incremental_cache(test):
+    # type: (lit.Test.Test) -> None
     if not test.result.code.isFailure:
         return
     fname = test.getFilePath()
     os.utime(fname, None)
 
 def sort_by_incremental_cache(run):
+    # type: (lit.run.Run) -> None
     def sortIndex(test):
+        # type: (lit.Test.Test) -> float
         fname = test.getFilePath()
         try:
             return -os.path.getmtime(fname)
@@ -139,6 +154,7 @@ def sort_by_incremental_cache(run):
     run.tests.sort(key = lambda t: sortIndex(t))
 
 def main(builtinParameters = {}):
+    # type: (dict) -> None
     # Create a temp directory inside the normal temp directory so that we can
     # try to avoid temporary test file leaks. The user can avoid this behavior
     # by setting LIT_PRESERVES_TMP in the environment, so they can easily use
@@ -164,6 +180,7 @@ def main(builtinParameters = {}):
             shutil.rmtree(lit_tmp)
 
 def main_with_tmp(builtinParameters):
+    # type: (dict) -> None
     parser = argparse.ArgumentParser()
     parser.add_argument('test_paths',
                         nargs='*',
@@ -348,12 +365,12 @@ def main_with_tmp(builtinParameters):
 
     if opts.showSuites or opts.showTests:
         # Aggregate the tests by suite.
-        suitesAndTests = {}
+        suitesAndTestsMap = {}  # type: Dict[lit.Test.TestSuite, lit.Test.Test]
         for result_test in run.tests:
-            if result_test.suite not in suitesAndTests:
-                suitesAndTests[result_test.suite] = []
-            suitesAndTests[result_test.suite].append(result_test)
-        suitesAndTests = list(suitesAndTests.items())
+            if result_test.suite not in suitesAndTestsMap:
+                suitesAndTestsMap[result_test.suite] = []
+            suitesAndTestsMap[result_test.suite].append(result_test)
+        suitesAndTests = list(suitesAndTestsMap.items())
         suitesAndTests.sort(key = lambda item: item[0].name)
 
         # Show the suites, if requested.
@@ -404,7 +421,7 @@ def main_with_tmp(builtinParameters):
         run.tests = run.tests[:opts.maxTests]
 
     # Don't create more threads than tests.
-    opts.numThreads = min(len(run.tests), opts.numThreads)
+    opts.numThreads = min(len(run.tests), opts.numThreads)  # type: ignore
 
     # Because some tests use threads internally, and at least on Linux each
     # of these threads counts toward the current process limit, try to
@@ -464,7 +481,7 @@ def main_with_tmp(builtinParameters):
 
     # List test results organized by kind.
     hasFailures = False
-    byCode = {}
+    byCode = {}  # type: Dict[lit.Test.ResultCode, List[lit.Test.Test]]
     for test in run.tests:
         if test.result.code not in byCode:
             byCode[test.result.code] = []
@@ -514,7 +531,7 @@ def main_with_tmp(builtinParameters):
 
     if opts.xunit_output_file:
         # Collect the tests, indexed by test suite
-        by_suite = {}
+        by_suite = {}  # type: Dict[lit.Test.TestSuite, dict]
         for result_test in run.tests:
             suite = result_test.suite.config.name
             if suite not in by_suite:
@@ -558,3 +575,4 @@ def main_with_tmp(builtinParameters):
 
 if __name__=='__main__':
     main()
+

In response to https://twitter.com/daniel_dunbar/status/785271265992990720:

I'm not sure I see the value...

I'm on the fence as well.

Pros

• The signature for methods like write_test_results definitely helps me. I don't know the codebase 100%, so knowing that the function takes a Run and a LitConfig is helpful. Especially considering there are files like TestingConfig.py in the codebase -- since there are a lot of similar names, the types encode information that doesn't come across from variable names alone.
• Running mypy on the command line validates the correctness of the types. If we integrate this step into CI, we're essentially guaranteeing our "documentation" doesn't go stale.
• It doesn't come across in the diff alone, but mypy tells you when you try to invoke a method on a class that doesn't define that method. It's really nice to get that sort of feedback. It's even better when using an editor that displays those hints inline, like PyCharm, or using Vim plugins.

Cons

• Running mypy, even to type-check Python 2.7 code, requires Python 3. That might not be possible on the LLVM buildbots. (It should be possible on Travis.)
• Notice the sometimes awkward workarounds: defining a base class for SimpleProgressBar and ProgressBar, and avoiding a reassignment of a variable to a different type. Both are unnecessary in normal Python. I think the reassignment should be avoided anyway, but the base class definitely seems like a bad change.
• mypy is an additional dependency that contributors will need to install in order to perform type checking, or contribute changes that necessitate type hint changes.

I think you're thinking about it reasonably overall.

The nice thing about type hints is they're easy to ignore and that you can use them in some places without being forced to put them everywhere. In my project, they've 1) helped automatically document what my functions are doing and 2) helped PyCharm get its static analysis right enough that my code isn't cluttered with incorrect warnings (so I can take the warnings that are still there seriously instead of getting numb to them--that's saved me from quite a few bugs).

Likewise, running mypy is optional...if a new contributor doesn't feel like running it she's free to not do so and that won't cause any obvious problems (provided she doesn't make mistakes with her types that someone else catches running mypy later!).

Re: forcing the base class...I don't see that as necessarily a bad thing. Classes in Python are lightweight and pretty flexible...(they're really just fancy dictionaries that you can add magic methods to.) So it's not wrong to use classes in Python as roughly equivalent to protocols in Swift. If you're going to be reusing a single variable to hold either ProgressBar or SimpleProgressBar, then you're effectively telling your code 'whatever you get here will behave, at the very least, in a certain set of specified ways' which is exactly what a protocol does. And having the two objects that can be used in that spot have a common supertype is as close as you can come in Python to making that contract explicit. (I don't know how this code is being used, but maybe the fact that you're facing this issue in the first place is a hint that the ProgressBars could be refactored into a more natural hierarchy, e.g. making ProgressBar a subtype of SimpleProgressBar or something like that).

If you don't want to change the types themselves, there is probably a way to supress the warning from mypy in the one spot where that's happening. And if you find your self needing to supress a type warning, that's probably a good occasion anyway to drop in a comment explaining what you're doing with the variable reassignment and why you're doing it.

Hope (some or any of) that helps!