diff --git a/build-support/bin/mypy.py b/build-support/bin/mypy.py index f5c5ac6806a4..efd1f339c1ac 100755 --- a/build-support/bin/mypy.py +++ b/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__': diff --git a/build-support/githooks/pre-commit b/build-support/githooks/pre-commit index 776ea1172d43..446d29020aa8 100755 --- a/build-support/githooks/pre-commit +++ b/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 diff --git a/contrib/mypy/src/python/pants/contrib/mypy/register.py b/contrib/mypy/src/python/pants/contrib/mypy/register.py index 0ab831381936..7e26bfd2a4e7 100644 --- a/contrib/mypy/src/python/pants/contrib/mypy/register.py +++ b/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') diff --git a/contrib/mypy/src/python/pants/contrib/mypy/tasks/BUILD b/contrib/mypy/src/python/pants/contrib/mypy/tasks/BUILD index 6f74ff382ea3..985e61393418 100644 --- a/contrib/mypy/src/python/pants/contrib/mypy/tasks/BUILD +++ b/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' ], ) diff --git a/contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py b/contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py index ec5408b806f3..7e35e22bbc38 100644 --- a/contrib/mypy/src/python/pants/contrib/mypy/tasks/mypy_task.py +++ b/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,6 +14,8 @@ 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 @@ -20,12 +23,30 @@ 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}') diff --git a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD b/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD index c3a7e88239b9..dbb2e4f5be18 100644 --- a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/BUILD +++ b/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'}, -) \ No newline at end of file + 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'}, +) diff --git a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy.py b/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy.py new file mode 100644 index 000000000000..a5d8ddb0a589 --- /dev/null +++ b/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() diff --git a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py b/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py index 532f38a3eb9c..0ff099f2daef 100644 --- a/contrib/mypy/tests/python/pants_test/contrib/mypy/tasks/test_mypy_integration.py +++ b/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' diff --git a/pants.ini b/pants.ini index 331018df422b..68f1593299a7 100644 --- a/pants.ini +++ b/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 diff --git a/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/BUILD b/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/BUILD index d8224ddb9b41..90335475d4a3 100644 --- a/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/BUILD +++ b/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(