Move MyPy from isolated goal into 'lint' goal and add MyPy whitelist/…

…opt-in type checking (#8099)

### Problem

We want to move MyPy into the lint goal and allow for users to opt-in into type_checking
for the new whitelisting/opt-in logic, we can make use of the lint goal capabilities instead of rebuilding the wheel
See #7886 and #6742 for more context.

### Solution

Registers MyPy as a 'lint' goal, and implements opt-in 'whitelist' system for type checking.
Targets to be type checked are tagged according to the `--whitelisted-tag-name` option, defaults to `type_checked` as used from #7886. Add new mypy_tests for new logic, and update old tests and mypy instances to reflect lint goal change.

New mypy logic added to accommodate new opt-in strategy. Mypy will acts according to the following:
1. Filter out non-whitelisted target roots
2. Check if the transitive deps from the filtered roots are whitelisted, if not we throw a warning
3. Continue as normal, collecting python source files and running mypy

build-support/bin/mypy.py

@@ -6,7 +6,9 @@
 
 
 def main() -> None:
-  subprocess.run(["./pants", "--tag=+type_checked", "mypy", "::"], check=True)
+  subprocess.run(["./pants", "--backend-packages=pants.contrib.mypy", "--lint-mypy-verbose",
+                  "--lint-mypy-whitelist-tag-name=type_checked", "--tag=type_checked",
+                  "--lint-mypy-config-file=build-support/mypy/mypy.ini", "lint", "::"], check=True)
 
 
 if __name__ == '__main__':

build-support/githooks/pre-commit

@@ -62,7 +62,7 @@ if git rev-parse --verify "${MERGE_BASE}" &>/dev/null; then
   echo "* Checking lint"
   ./pants --exclude-target-regexp='testprojects/.*' --changed-parent="${MERGE_BASE}" lint || exit 1
 
-  echo "* Checking types for targets marked \`type_checked\`"
+  echo "* Checking types for targets marked \`type_checked\`"  
   ./build-support/bin/mypy.py || exit 1
 
   if git diff "${MERGE_BASE}" --name-only | grep '\.rs$' > /dev/null; then

contrib/mypy/src/python/pants/contrib/mypy/register.py

@@ -7,4 +7,4 @@
 
 
 def register_goals():
-  task(name='mypy', action=MypyTask).install('mypy')
+  task(name='mypy', action=MypyTask).install('lint')

contrib/mypy/src/python/pants/contrib/mypy/tasks/BUILD

@@ -15,5 +15,6 @@ python_library(
     'src/python/pants/task',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:memo',
+    'src/python/pants/build_graph'
   ],
 )

contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py

@@ -3,6 +3,7 @@
 
 import os
 import subprocess
+from typing import List
 
 from pants.backend.python.interpreter_cache import PythonInterpreterCache
 from pants.backend.python.targets.python_binary import PythonBinary
@@ -13,19 +14,39 @@
 from pants.base.build_environment import get_buildroot
 from pants.base.exceptions import TaskError
 from pants.base.workunit import WorkUnit, WorkUnitLabel
+from pants.build_graph.target import Target
+from pants.task.lint_task_mixin import LintTaskMixin
 from pants.util.contextutil import temporary_file_path
 from pants.util.memo import memoized_property
 from pex.interpreter import PythonInterpreter
 from pex.pex import PEX
 from pex.pex_info import PexInfo
 
 
-class MypyTask(ResolveRequirementsTaskBase):
-  """Invoke the mypy static type analyzer for Python."""
+class MypyTaskError(TaskError):
+  """Indicates a TaskError from a failing MyPy run."""
+
+
+class MypyTask(LintTaskMixin, ResolveRequirementsTaskBase):
+  """Invoke the mypy static type analyzer for Python.
+
+  Mypy lint task filters out target_roots that are not properly tagged according to
+  --whitelisted-tag-name (defaults to None, and no filtering occurs if this option is 'None'),
+  and executes MyPy on targets in context from whitelisted target roots.
+  (if any transitive targets from the filtered roots are not whitelisted, a warning
+  will be printed.)
+
+  'In context' meaning in the sub-graph where a whitelisted target is the root
+  """
 
   _MYPY_COMPATIBLE_INTERPETER_CONSTRAINT = '>=3.5'
   _PYTHON_SOURCE_EXTENSION = '.py'
 
+  deprecated_options_scope = 'mypy'
+  deprecated_options_scope_removal_version = '1.21.0.dev0'
+
+  WARNING_MESSAGE = "[WARNING]: Targets not currently whitelisted and may cause issues."
+
   @classmethod
   def prepare(cls, options, round_manager):
     super().prepare(options, round_manager)
@@ -36,6 +57,10 @@ def register_options(cls, register):
     register('--mypy-version', default='0.710', help='The version of mypy to use.')
     register('--config-file', default=None,
              help='Path mypy configuration file, relative to buildroot.')
+    register('--whitelist-tag-name', default=None,
+             help='Tag name to identify python targets to execute MyPy')
+    register('--verbose', type=bool, default=False,
+             help='Extra detail showing non-whitelisted targets')
 
   @classmethod
   def supports_passthru_args(cls):
@@ -60,9 +85,43 @@ def is_non_synthetic_python_target(target):
   def is_python_target(target):
     return isinstance(target, PythonTarget)
 
-  def _calculate_python_sources(self, targets):
-    """Generate a set of source files from the given targets."""
-    python_eval_targets = filter(self.is_non_synthetic_python_target, targets)
+  def _is_tagged_target(self, target: Target) -> bool:
+    return self.get_options().whitelist_tag_name in target.tags
+
+  def _is_tagged_non_synthetic_python_target(self, target: Target) -> bool:
+    return (self.is_non_synthetic_python_target(target) and
+            self._is_tagged_target(target))
+
+  def _not_tagged_non_synthetic_python_target(self, target: Target) -> bool:
+    return (self.is_non_synthetic_python_target(target) and
+            not self._is_tagged_target(target))
+
+  def _all_targets_are_whitelisted(self, whitelisted_targets: List[Target], all_targets: List[Target]) -> bool:
+    return len(whitelisted_targets) == 0 or len(whitelisted_targets) == len(all_targets)
+
+  def _format_targets_not_whitelisted(self, targets: List[Target]) -> str:
+    output = ''
+    for target in targets:
+      output = output + f"{target.address.spec}\n"
+    return output
+
+  def _whitelist_warning(self, targets_not_whitelisted: List[Target]) -> None:
+    self.context.log.warn(self.WARNING_MESSAGE)
+    if self.get_options().verbose:
+      output = self._format_targets_not_whitelisted(targets_not_whitelisted)
+      self.context.log.warn(f"{output}")
+
+  def _calculate_python_sources(self, target_roots: List[Target]):
+    """Filter targets to generate a set of source files from the given targets."""
+    if self.get_options().whitelist_tag_name:
+      all_targets = self._filter_targets(Target.closure_for_targets([tgt for tgt in target_roots if self._is_tagged_target(tgt)]))
+      python_eval_targets = [tgt for tgt in all_targets if self._is_tagged_non_synthetic_python_target(tgt)]
+      if not self._all_targets_are_whitelisted(python_eval_targets, all_targets):
+        targets_not_whitelisted = [tgt for tgt in all_targets if self._not_tagged_non_synthetic_python_target(tgt)]
+        self._whitelist_warning(targets_not_whitelisted)
+    else:
+      python_eval_targets = self._filter_targets([tgt for tgt in Target.closure_for_targets(target_roots) if self.is_non_synthetic_python_target(tgt)])
+
     sources = set()
     for target in python_eval_targets:
       sources.update(
@@ -91,7 +150,7 @@ def _run_mypy(self, py3_interpreter, mypy_args, **kwargs):
     mypy_version = self.get_options().mypy_version
 
     mypy_requirement_pex = self.resolve_requirement_strings(
-      py3_interpreter, ['mypy=={}'.format(mypy_version)])
+      py3_interpreter, [f'mypy=={mypy_version}'])
 
     path = os.path.realpath(os.path.join(self.workdir, str(py3_interpreter.identity), mypy_version))
     if not os.path.isdir(path):
@@ -102,8 +161,8 @@ def _run_mypy(self, py3_interpreter, mypy_args, **kwargs):
   def execute(self):
     mypy_interpreter = self.find_mypy_interpreter()
     if not mypy_interpreter:
-      raise TaskError('Unable to find a Python {} interpreter (required for mypy).'
-                      .format(self._MYPY_COMPATIBLE_INTERPETER_CONSTRAINT))
+      raise TaskError(f'Unable to find a Python {self._MYPY_COMPATIBLE_INTERPETER_CONSTRAINT} '
+                      f'interpreter (required for mypy).')
 
     sources = self._calculate_python_sources(self.context.target_roots)
     if not sources:
@@ -120,22 +179,19 @@ def execute(self):
     with temporary_file_path() as sources_list_path:
       with open(sources_list_path, 'w') as f:
         for source in sources:
-          f.write('{}\n'.format(source))
-
+          f.write(f'{source}\n')
       # Construct the mypy command line.
-      cmd = ['--python-version={}'.format(interpreter_for_targets.identity.python)]
+      cmd = [f'--python-version={interpreter_for_targets.identity.python}']
       if self.get_options().config_file:
-        cmd.append('--config-file={}'.format(os.path.join(get_buildroot(),
-                                                          self.get_options().config_file)))
+        cmd.append(f'--config-file={os.path.join(get_buildroot(), self.get_options().config_file)}')
       cmd.extend(self.get_passthru_args())
-      cmd.append('@{}'.format(sources_list_path))
-      self.context.log.debug('mypy command: {}'.format(' '.join(cmd)))
+      cmd.append(f'@{sources_list_path}')
+      self.context.log.debug(f'mypy command: {" ".join(cmd)}')
 
       # Collect source roots for the targets being checked.
       source_roots = self._collect_source_roots()
 
       mypy_path = os.pathsep.join([os.path.join(get_buildroot(), root) for root in source_roots])
-
       # Execute mypy.
       with self.context.new_workunit(
         name='check',
@@ -146,4 +202,4 @@ def execute(self):
         returncode = self._run_mypy(mypy_interpreter, cmd,
           env={'MYPYPATH': mypy_path}, stdout=workunit.output('stdout'), stderr=subprocess.STDOUT)
         if returncode != 0:
-          raise TaskError('mypy failed: code={}'.format(returncode))
+          raise MypyTaskError(f'mypy failed: code={returncode}')

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD

@@ -8,5 +8,15 @@ python_tests(
     'tests/python/pants_test:int-test',
     'tests/python/pants_test:interpreter_selection_utils',
   ],
-  tags={'integration'},
-)
+  tags={'integration', 'integration_test'},
+)
+
+python_tests(
+  name='mypy',
+  source = 'test_mypy.py',
+  dependencies=[
+    'tests/python/pants_test:task_test_base',
+    'contrib/mypy/src/python/pants/contrib/mypy/tasks',
+  ],
+  tags = {'integration_test'},
+)

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy.py

@@ -0,0 +1,59 @@
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from unittest.mock import MagicMock
+
+from pants.backend.python.targets.python_library import PythonLibrary
+from pants_test.task_test_base import TaskTestBase
+
+from pants.contrib.mypy.tasks.mypy_task import MypyTask
+
+
+class MyPyTests(TaskTestBase):
+
+  @classmethod
+  def task_type(cls):
+    return MypyTask
+
+  def test_throws_no_warning_on_all_whitelisted_target_roots(self) -> None:
+    t1 = self.make_target('t1', PythonLibrary, tags=['type_checked'])
+    t2 = self.make_target('t2', PythonLibrary, tags=['type_checked'])
+    task = self.create_task(self.context(target_roots=[t1, t2]))
+    self.set_options(whitelist_tag_name='type_checked')
+    task.execute()
+
+  def test_throws_no_warning_on_some_whitelisted_target_roots_but_all_whitelisted_in_context(self) -> None:
+    t1 = self.make_target('t1', PythonLibrary)
+    t2 = self.make_target('t2', PythonLibrary, tags=['type_checked'])
+    t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
+    task = self.create_task(self.context(target_roots=[t1, t3]))
+    self.set_options(whitelist_tag_name='type_checked')
+    task.execute()
+
+  def test_throws_warning_on_some_whitelisted_target_roots_but_all_whitelisted_in_context(self) -> None:
+    t1 = self.make_target('t1', PythonLibrary)
+    t2 = self.make_target('t2', PythonLibrary, tags=['something_else'])
+    t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
+    self.set_options(whitelist_tag_name='type_checked')
+    task = self.create_task(self.context(target_roots=[t1, t3]))
+    task._whitelist_warning = MagicMock()
+    task.execute()
+    task._whitelist_warning.assert_called()
+
+  def test_throws_warning_on_all_whitelisted_target_roots_but_some_whitelisted_transitive_targets(self) -> None:
+    t1 = self.make_target('t1', PythonLibrary, tags=['type_checked'])
+    t2 = self.make_target('t2', PythonLibrary, tags=['something_else'])
+    t3 = self.make_target('t3', PythonLibrary, tags=['type_checked'], dependencies=[t2])
+    t4 = self.make_target('t4', PythonLibrary, tags=['type_checked'], dependencies=[t3])
+    self.set_options(whitelist_tag_name='type_checked')
+    task = self.create_task(self.context(target_roots=[t1, t4]))
+    task._whitelist_warning = MagicMock()
+    task.execute()
+    task._whitelist_warning.assert_called()
+
+  def test_throws_no_warning_if_no_whitelist_specified(self) -> None:
+    t1 = self.make_target('t1', PythonLibrary)
+    task = self.create_task(self.context(target_roots=[t1]))
+    task._whitelist_warning = MagicMock()
+    task.execute()
+    task._whitelist_warning.assert_not_called()

contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py

@@ -8,7 +8,11 @@
 class MypyIntegrationTest(PantsRunIntegrationTest):
   def test_mypy(self):
     cmd = [
-      'mypy',
+      '--backend-packages=pants.contrib.mypy',
+      '--tag=integration_test'
+      '--lint-skip',
+      '--no-lint-mypy-skip',
+      'lint',
       'contrib/mypy/tests/python/pants_test/contrib/mypy::',
       '--',
       '--follow-imports=silent'

pants.ini

@@ -62,7 +62,6 @@ backend_packages: +[
     "pants.contrib.googlejavaformat",
     "pants.contrib.jax_ws",
     "pants.contrib.scalajs",
-    "pants.contrib.mypy",
     "pants.contrib.node",
     "pants.contrib.python.checks",
     "pants.contrib.scrooge",
@@ -302,13 +301,9 @@ skip: True
 [pycheck-newstyle-classes]
 skip: True
 
-[mypy]
-config_file: build-support/mypy/mypy.ini
-
 [scala]
 version: 2.12
 
-
 [java]
 strict_deps: True
 

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/BUILD

@@ -40,7 +40,7 @@ python_tests(
     'tests/python/pants_test/cache:cache_server',
   ],
   tags = {'integration'},
-  timeout=180,
+  timeout=600,
 )
 
 python_tests(