[internal] Refactor MyPy config file setup (#12593)

`mypy/rules.py` is really complex and I'm having a hard time understanding it while adding a tool lockfile. This is the first of a couple refactors.

Here, we better encapsulate the config file code by moving into `mypy/subsystem.py` with a dedicated rule. This allows us to make the unit test more comprehensive.

[ci skip-rust]
[ci skip-build-wheels]

src/python/pants/backend/python/typecheck/mypy/register.py

@@ -8,8 +8,8 @@
 """
 
 from pants.backend.python.typecheck.mypy import rules as mypy_rules
-from pants.backend.python.typecheck.mypy import skip_field
+from pants.backend.python.typecheck.mypy import skip_field, subsystem
 
 
 def rules():
-    return (*mypy_rules.rules(), *skip_field.rules())
+    return (*mypy_rules.rules(), *skip_field.rules(), *subsystem.rules())

src/python/pants/backend/python/typecheck/mypy/rules.py

@@ -2,14 +2,13 @@
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 import itertools
-import logging
 from collections import defaultdict
 from dataclasses import dataclass
 from typing import Iterable, Optional, Tuple
 
 from pants.backend.python.target_types import PythonRequirementsField, PythonSources
 from pants.backend.python.typecheck.mypy.skip_field import SkipMyPyField
-from pants.backend.python.typecheck.mypy.subsystem import MyPy
+from pants.backend.python.typecheck.mypy.subsystem import MyPy, MyPyConfigFile
 from pants.backend.python.util_rules import pex_from_targets
 from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
 from pants.backend.python.util_rules.pex import (
@@ -30,29 +29,18 @@
     TypecheckResult,
     TypecheckResults,
 )
-from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest
 from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
 from pants.engine.addresses import Addresses, UnparsedAddressInputs
-from pants.engine.fs import (
-    CreateDigest,
-    Digest,
-    DigestContents,
-    FileContent,
-    MergeDigests,
-    RemovePrefix,
-)
+from pants.engine.fs import CreateDigest, Digest, FileContent, MergeDigests, RemovePrefix
 from pants.engine.process import FallibleProcessResult
 from pants.engine.rules import Get, MultiGet, collect_rules, rule
 from pants.engine.target import FieldSet, Target, TransitiveTargets, TransitiveTargetsRequest
 from pants.engine.unions import UnionRule
 from pants.python.python_setup import PythonSetup
-from pants.util.docutil import doc_url
 from pants.util.logging import LogLevel
 from pants.util.ordered_set import FrozenOrderedSet, OrderedSet
 from pants.util.strutil import pluralize
 
-logger = logging.getLogger(__name__)
-
 
 @dataclass(frozen=True)
 class MyPyFieldSet(FieldSet):
@@ -70,7 +58,6 @@ class MyPyPartition:
     root_targets: FrozenOrderedSet[Target]
     closure: FrozenOrderedSet[Target]
     interpreter_constraints: InterpreterConstraints
-    python_version_already_configured: bool
 
 
 class MyPyRequest(TypecheckRequest):
@@ -93,32 +80,6 @@ def generate_argv(
     return tuple(args)
 
 
-def check_and_warn_if_python_version_configured(
-    *, config: Optional[FileContent], args: Tuple[str, ...]
-) -> bool:
-    configured = []
-    if config and b"python_version" in config.content:
-        configured.append(
-            f"`python_version` in {config.path} (which is used because of either config "
-            "autodisocvery or the `[mypy].config` option)"
-        )
-    if "--py2" in args:
-        configured.append("`--py2` in the `--mypy-args` option")
-    if any(arg.startswith("--python-version") for arg in args):
-        configured.append("`--python-version` in the `--mypy-args` option")
-    if configured:
-        formatted_configured = " and you set ".join(configured)
-        logger.warning(
-            f"You set {formatted_configured}. Normally, Pants would automatically set this for you "
-            "based on your code's interpreter constraints "
-            f"({doc_url('python-interpreter-compatibility')}). Instead, it will "
-            "use what you set.\n\n(Automatically setting the option allows Pants to partition your "
-            "targets by their constraints, so that, for example, you can run MyPy on Python 2-only "
-            "code and Python 3-only code at the same time. This feature may no longer work.)"
-        )
-    return bool(configured)
-
-
 def determine_python_files(files: Iterable[str]) -> Tuple[str, ...]:
     """We run over all .py and .pyi files, but .pyi files take precedence.
 
@@ -140,7 +101,7 @@ def determine_python_files(files: Iterable[str]) -> Tuple[str, ...]:
 
 @rule
 async def mypy_typecheck_partition(
-    partition: MyPyPartition, mypy: MyPy, python_setup: PythonSetup
+    partition: MyPyPartition, config_file: MyPyConfigFile, mypy: MyPy, python_setup: PythonSetup
 ) -> TypecheckResult:
     plugin_target_addresses = await Get(Addresses, UnparsedAddressInputs, mypy.source_plugins)
     plugin_transitive_targets = await Get(
@@ -153,16 +114,6 @@ async def mypy_typecheck_partition(
         if plugin_tgt.has_field(PythonRequirementsField)
     )
 
-    # If the user did not set `--python-version` already, we set it ourselves based on their code's
-    # interpreter constraints. This determines what AST is used by MyPy.
-    python_version = (
-        None
-        if partition.python_version_already_configured
-        else partition.interpreter_constraints.minimum_python_version(
-            python_setup.interpreter_universe
-        )
-    )
-
     # MyPy requires 3.5+ to run, but uses the typed-ast library to work with 2.7, 3.4, 3.5, 3.6,
     # and 3.7. However, typed-ast does not understand 3.8+, so instead we must run MyPy with
     # Python 3.8+ when relevant. We only do this if <3.8 can't be used, as we don't want a
@@ -222,24 +173,20 @@ async def mypy_typecheck_partition(
         ),
     )
 
-    config_files_get = Get(ConfigFiles, ConfigFilesRequest, mypy.config_request)
-
     (
         plugin_sources,
         closure_sources,
         roots_sources,
         mypy_pex,
         requirements_pex,
         mypy_extra_requirements_pex,
-        config_files,
     ) = await MultiGet(
         plugin_sources_get,
         closure_sources_get,
         roots_sources_get,
         mypy_pex_get,
         requirements_pex_get,
         mypy_extra_requirements_pex_get,
-        config_files_get,
     )
 
     python_files = determine_python_files(roots_sources.snapshot.files)
@@ -271,7 +218,7 @@ async def mypy_typecheck_partition(
                 plugin_sources.source_files.snapshot.digest,
                 closure_sources.source_files.snapshot.digest,
                 typechecked_venv_pex.digest,
-                config_files.snapshot.digest,
+                config_file.digest,
             ]
         ),
     )
@@ -292,7 +239,9 @@ async def mypy_typecheck_partition(
                 mypy,
                 typechecked_venv_pex,
                 file_list_path=file_list_path,
-                python_version=python_version,
+                python_version=config_file.python_version_to_autoset(
+                    partition.interpreter_constraints, python_setup.interpreter_universe
+                ),
             ),
             input_digest=merged_input_files,
             extra_env=env,
@@ -317,16 +266,6 @@ async def mypy_typecheck(
     if mypy.skip:
         return TypecheckResults([], typechecker_name="MyPy")
 
-    # We batch targets by their interpreter constraints to ensure, for example, that all Python 2
-    # targets run together and all Python 3 targets run together. We can only do this by setting
-    # the `--python-version` option, but we allow the user to set it as a safety valve. We warn if
-    # they've set the option.
-    config_files = await Get(ConfigFiles, ConfigFilesRequest, mypy.config_request)
-    config_content = await Get(DigestContents, Digest, config_files.snapshot.digest)
-    python_version_configured = check_and_warn_if_python_version_configured(
-        config=next(iter(config_content), None), args=mypy.args
-    )
-
     # When determining how to batch by interpreter constraints, we must consider the entire
     # transitive closure to get the final resulting constraints.
     # TODO(#10863): Improve the performance of this.
@@ -359,7 +298,6 @@ async def mypy_typecheck(
                 FrozenOrderedSet(combined_roots),
                 FrozenOrderedSet(combined_closure),
                 interpreter_constraints,
-                python_version_already_configured=python_version_configured,
             )
         )
 

src/python/pants/backend/python/typecheck/mypy/rules_integration_test.py

@@ -17,15 +17,15 @@
 from pants.backend.python.typecheck.mypy.rules import (
     MyPyFieldSet,
     MyPyRequest,
-    check_and_warn_if_python_version_configured,
     determine_python_files,
 )
 from pants.backend.python.typecheck.mypy.rules import rules as mypy_rules
 from pants.backend.python.typecheck.mypy.subsystem import MyPy
+from pants.backend.python.typecheck.mypy.subsystem import rules as mypy_subystem_rules
 from pants.core.goals.typecheck import TypecheckResult, TypecheckResults
 from pants.core.util_rules import config_files, pants_bin
 from pants.engine.addresses import Address
-from pants.engine.fs import EMPTY_DIGEST, DigestContents, FileContent
+from pants.engine.fs import EMPTY_DIGEST, DigestContents
 from pants.engine.rules import QueryRule
 from pants.engine.target import Target
 from pants.testutil.python_interpreter_selection import (
@@ -42,6 +42,7 @@ def rule_runner() -> RuleRunner:
     return RuleRunner(
         rules=[
             *mypy_rules(),
+            *mypy_subystem_rules(),
             *dependency_inference_rules.rules(),  # Used for import inference.
             *pants_bin.rules(),
             *config_files.rules(),
@@ -685,32 +686,3 @@ def test_determine_python_files() -> None:
     assert determine_python_files(["f.py", "f.pyi"]) == ("f.pyi",)
     assert determine_python_files(["f.pyi", "f.py"]) == ("f.pyi",)
     assert determine_python_files(["f.json"]) == ()
-
-
-def test_warn_if_python_version_configured(caplog) -> None:
-    def assert_is_configured(*, has_config: bool, args: list[str], warning: str) -> None:
-        config = FileContent("mypy.ini", b"[mypy]\npython_version = 3.6") if has_config else None
-        is_configured = check_and_warn_if_python_version_configured(config=config, args=tuple(args))
-        assert is_configured
-        assert len(caplog.records) == 1
-        assert warning in caplog.text
-        caplog.clear()
-
-    assert_is_configured(has_config=True, args=[], warning="You set `python_version` in mypy.ini")
-    assert_is_configured(
-        has_config=False, args=["--py2"], warning="You set `--py2` in the `--mypy-args` option"
-    )
-    assert_is_configured(
-        has_config=False,
-        args=["--python-version=3.6"],
-        warning="You set `--python-version` in the `--mypy-args` option",
-    )
-    assert_is_configured(
-        has_config=True,
-        args=["--py2", "--python-version=3.6"],
-        warning=(
-            "You set `python_version` in mypy.ini (which is used because of either config "
-            "autodisocvery or the `[mypy].config` option) and you set `--py2` in the `--mypy-args` "
-            "option and you set `--python-version` in the `--mypy-args` option."
-        ),
-    )

src/python/pants/backend/python/typecheck/mypy/subsystem.py

@@ -3,13 +3,26 @@
 
 from __future__ import annotations
 
-from typing import cast
+import logging
+from dataclasses import dataclass
+from typing import Iterable, cast
 
 from pants.backend.python.subsystems.python_tool_base import PythonToolBase
 from pants.backend.python.target_types import ConsoleScript
-from pants.core.util_rules.config_files import ConfigFilesRequest
+from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
+from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest
 from pants.engine.addresses import UnparsedAddressInputs
+from pants.engine.fs import Digest, DigestContents, FileContent
+from pants.engine.rules import Get, collect_rules, rule
 from pants.option.custom_types import file_option, shell_str, target_option
+from pants.python.python_setup import PythonSetup
+from pants.util.docutil import doc_url
+
+logger = logging.getLogger(__name__)
+
+# --------------------------------------------------------------------------------------
+# Subsystem
+# --------------------------------------------------------------------------------------
 
 
 class MyPy(PythonToolBase):
@@ -106,3 +119,62 @@ def config_request(self) -> ConfigFilesRequest:
     @property
     def source_plugins(self) -> UnparsedAddressInputs:
         return UnparsedAddressInputs(self.options.source_plugins, owning_address=None)
+
+    def check_and_warn_if_python_version_configured(self, config: FileContent | None) -> bool:
+        """Determine if we can dynamically set `--python-version` and warn if not."""
+        configured = []
+        if config and b"python_version" in config.content:
+            configured.append(
+                f"`python_version` in {config.path} (which is used because of either config "
+                "discovery or the `[mypy].config` option)"
+            )
+        if "--py2" in self.args:
+            configured.append("`--py2` in the `--mypy-args` option")
+        if any(arg.startswith("--python-version") for arg in self.args):
+            configured.append("`--python-version` in the `--mypy-args` option")
+        if configured:
+            formatted_configured = " and you set ".join(configured)
+            logger.warning(
+                f"You set {formatted_configured}. Normally, Pants would automatically set this "
+                "for you based on your code's interpreter constraints "
+                f"({doc_url('python-interpreter-compatibility')}). Instead, it will "
+                "use what you set.\n\n"
+                "(Automatically setting the option allows Pants to partition your targets by their "
+                "constraints, so that, for example, you can run MyPy on Python 2-only code and "
+                "Python 3-only code at the same time. This feature may no longer work.)"
+            )
+        return bool(configured)
+
+
+# --------------------------------------------------------------------------------------
+# Config files
+# --------------------------------------------------------------------------------------
+
+
+@dataclass(frozen=True)
+class MyPyConfigFile:
+    digest: Digest
+    _python_version_configured: bool
+
+    def python_version_to_autoset(
+        self, interpreter_constraints: InterpreterConstraints, interpreter_universe: Iterable[str]
+    ) -> str | None:
+        """If the user did not already set `--python-version`, return the major.minor version to
+        use."""
+        if self._python_version_configured:
+            return None
+        return interpreter_constraints.minimum_python_version(interpreter_universe)
+
+
+@rule
+async def setup_mypy_config(mypy: MyPy, python_setup: PythonSetup) -> MyPyConfigFile:
+    config_files = await Get(ConfigFiles, ConfigFilesRequest, mypy.config_request)
+    digest_contents = await Get(DigestContents, Digest, config_files.snapshot.digest)
+    python_version_configured = mypy.check_and_warn_if_python_version_configured(
+        digest_contents[0] if digest_contents else None
+    )
+    return MyPyConfigFile(config_files.snapshot.digest, python_version_configured)
+
+
+def rules():
+    return collect_rules()