Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/python/pants/ng/subsystem_mypy_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@
ClassDef,
Decorator,
FuncDef,
MypyFile,
Statement,
)
from mypy.plugin import Plugin
from mypy.plugin import ClassDefContext, Plugin


def _has_option_decorator(node: Decorator) -> bool:
Expand Down Expand Up @@ -42,13 +41,28 @@ def _process_defns(defns: list[Statement]) -> None:
_process_defns(defn.func.body.body)


def _process_subclass_hook(ctx: ClassDefContext) -> None:
_process_defns(ctx.cls.defs.body)


# NB: If we create more intermediate subclasses, they must be added here.
_SUBSYSTEM_BASE_CLASSES = frozenset(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be discovered by the plugin from semantic analysis in a performant manner?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that can be done reliably, let alone performantly?

Copy link
Copy Markdown
Contributor

@tdyas tdyas May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked Codex and it suggests looking at ctx.cls.info.mro to access the MRO for the class:

def _is_subsystem_ng(ctx: ClassDefContext) -> bool:
      return any(info.fullname == _SUBSYSTEM_ROOT for info in ctx.cls.info.mro)

Open question whether that is performant enough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but not with the current get_base_class_hook(fullname) shape alone.

get_base_class_hook() only receives the direct base class fullname being analyzed, so for:

class FooSubsystem(GoalSubsystemNg):
...

mypy asks the plugin about pants.ng.goal.GoalSubsystemNg, not about its transitive base
SubsystemNg. That is why _SUBSYSTEM_BASE_CLASSES currently has to include intermediate
classes like GoalSubsystemNg, UniversalSubsystem, and ContextualSubsystem.

A dynamic version is possible by making the hook run more broadly, then filtering inside the
callback using the class MRO:

  _SUBSYSTEM_ROOT = "pants.ng.subsystem.SubsystemNg"

  def _is_subsystem_ng(ctx: ClassDefContext) -> bool:
      return any(info.fullname == _SUBSYSTEM_ROOT for info in ctx.cls.info.mro)

  def _process_subclass_hook(ctx: ClassDefContext) -> None:
      if _is_subsystem_ng(ctx):
          _process_defns(ctx.cls.defs.body)

  class SubsystemPlugin(Plugin):
      def get_base_class_hook(self, fullname: str):
          return _process_subclass_hook

That would infer intermediate subclasses dynamically from mypy’s semantic model, avoiding
the manually maintained _SUBSYSTEM_BASE_CLASSES.

Tradeoff: returning a hook for every base class means the callback runs for many non-
subsystem classes, and possibly more than once for classes with multiple bases. The callback
is cheap and idempotent enough here, but I’d add a guard such as metadata on ctx.cls.info if
duplicate processing becomes a concern.

Copy link
Copy Markdown
Contributor

@tdyas tdyas May 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a conformance check in the plugin for all direct subclasses of pants.ng.subsystem.SubsystemNg that they are listed in _SUBSYSTEM_BASE_CLASSES?

Basically, it would be better if the plugin either discovers the contents of _SUBSYSTEM_BASE_CLASSES or otherwise flags an error when items are missing from it.

{
"pants.ng.subsystem.SubsystemNg",
"pants.ng.subsystem.UniversalSubsystem",
"pants.ng.subsystem.ContextualSubsystem",
"pants.ng.goal.GoalSubsystemNg",
}
)


class SubsystemPlugin(Plugin):
"""Mypy plugin that processes files to find methods decorated with @option."""
"""Mypy plugin that processes SubsystemNg subclasses to find methods decorated with @option."""

def get_additional_deps(self, file: MypyFile) -> list[tuple[int, str, int]]:
"""A file-level hook that we piggyback on to process the parsed file."""
_process_defns(file.defs)
return []
def get_base_class_hook(self, fullname: str):
if fullname in _SUBSYSTEM_BASE_CLASSES:
return _process_subclass_hook
return None


def plugin(version: str) -> Type[Plugin]:
Expand Down
91 changes: 91 additions & 0 deletions src/python/pants/ng/subsystem_mypy_plugin_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# Copyright 2026 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

import tempfile
import textwrap
from pathlib import Path

import mypy.api

from pants.util.contextutil import environment_as

_FOO_GOAL_SUBSYSTEM_SOURCE = textwrap.dedent("""\
from pants.ng.goal import GoalSubsystemNg
from pants.ng.subsystem import option

class FooSubsystem(GoalSubsystemNg):
options_scope = "foo"
help = "Run foo"

@option(default=0, help="How much to foo")
def level(self) -> int: ...
""")


_BAR_SUBSYSTEM_SOURCE = textwrap.dedent("""\
from pants.ng.subsystem import ContextualSubsystem, option

class Bar(ContextualSubsystem):
options_scope = "bar"
help = "Options for bar."

@option(default="A", help="how to bar")
def how_to(self) -> str: ...

@option(default=false, help="whether to do the bar")
def do_it(self) -> bool: ...
""")


def _run_mypy(source: str, *, with_plugin: bool) -> tuple[str, str]:
"""Run mypy on the given source string and return stdout."""
# SubsystemNg and its subclasses depend on various code from the
# codebase, so we must point mypy at it.
import pants

pants_src = str(Path(pants.__file__).parents[1])
plugin_path = str(Path(__file__).parent / "subsystem_mypy_plugin.py")

with (
tempfile.NamedTemporaryFile(suffix=".py", mode="w") as src_f,
tempfile.NamedTemporaryFile(suffix=".ini", mode="w") as cfg_f,
):
src_f.write(source)
src_f.flush()
cfg_f.write("[mypy]\n")
cfg_f.flush()
with environment_as(MYPYPATH=pants_src):
args = [
src_f.name,
"--no-error-summary",
"--no-incremental",
f"--config-file={cfg_f.name}",
]
if with_plugin:
args.extend(["--plugin", plugin_path])
stdout, stderr, _code = mypy.api.run(args)
return stdout, stderr


def test_plugin_suppresses_empty_body_on_lint_subsystem() -> None:
stdout_noplugin, stderr_noplugin = _run_mypy(_FOO_GOAL_SUBSYSTEM_SOURCE, with_plugin=False)
assert "empty-body" in stdout_noplugin, (
f"Expected [empty-body] error without plugin:\n{stdout_noplugin} (err: {stderr_noplugin})"
)

stdout_plugin, stderr_plugin = _run_mypy(_FOO_GOAL_SUBSYSTEM_SOURCE, with_plugin=True)
assert "empty-body" not in stdout_plugin, (
f"Plugin did not suppress [empty-body] error:\n{stdout_plugin} (err: {stderr_plugin})"
)


def test_plugin_suppresses_empty_body_on_contextual_subsystem() -> None:
stdout_noplugin, stderr_noplugin = _run_mypy(_BAR_SUBSYSTEM_SOURCE, with_plugin=False)
assert "empty-body" in stdout_noplugin, (
f"Expected [empty-body] error without plugin:\n{stdout_noplugin} (err: {stderr_noplugin})"
)

stdout_plugin, stderr_plugin = _run_mypy(_BAR_SUBSYSTEM_SOURCE, with_plugin=True)
assert "empty-body" not in stdout_plugin, (
f"Plugin did not suppress [empty-body] error:\n{stdout_plugin} (err: {stderr_plugin})"
)
Loading