Make GoalSubsystem subclass Subsystem. (#10483)

Previously it subclassed the same bases as Subsystem,
and reimplemented some of its functionality, which
was a weird quirk: you'd expect something called GoalSubsystem,
which acts like a Subsystem, to actually be one.

This change folds one of those bases (SubsystemClientMixin)
into Subsystem, and gets rid of it.

A future change will similarly merge Subsystem and Optionable,
since there is now only one kind of Optionable.

[ci skip-rust]

[ci skip-build-wheels]

src/python/pants/engine/goal.py

@@ -8,16 +8,14 @@
 
 from typing_extensions import final
 
-from pants.option.option_value_container import OptionValueContainer
-from pants.option.optionable import Optionable
-from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin
+from pants.subsystem.subsystem import Subsystem
 from pants.util.meta import classproperty
 
 if TYPE_CHECKING:
     from pants.engine.console import Console
 
 
-class GoalSubsystem(SubsystemClientMixin, Optionable):
+class GoalSubsystem(Subsystem):
     """The Subsystem used by `Goal`s to register the external API, meaning the goal name, the help
     message, and any options.
 
@@ -49,19 +47,6 @@ def name(cls):
     def options_scope(cls) -> str:  # type: ignore[override]
         return cast(str, cls.name)
 
-    @final
-    def __init__(self, scope: str, scoped_options: OptionValueContainer) -> None:
-        # NB: This constructor is shaped to meet the contract of `Optionable(Factory).signature`.
-        super().__init__()
-        self._scope = scope
-        self._scoped_options = scoped_options
-
-    @final
-    @property
-    def options(self) -> OptionValueContainer:
-        """Returns the option values."""
-        return self._scoped_options
-
 
 @dataclass(frozen=True)
 class Goal:

src/python/pants/option/options_test.py

@@ -1740,7 +1740,7 @@ class DummyOptionable1(Optionable):
                 global_scope(),
                 DummyOptionable1.get_scope_info(),
                 # A deprecated, scoped dependency on `DummyOptionable1`. This
-                # imitates the construction of SubsystemClientMixin.known_scope_infos.
+                # imitates the construction of Subsystem.known_scope_infos.
                 ScopeInfo(
                     DummyOptionable1.subscope("sub"),
                     DummyOptionable1,

src/python/pants/subsystem/subsystem.py

@@ -4,13 +4,14 @@
 import importlib
 import inspect
 import logging
-from typing import Callable, Dict, Iterable, List, Optional, Tuple, Type, TypeVar, Union, cast
+from dataclasses import dataclass
+from typing import Any, Callable, Dict, Iterable, List, Optional, Tuple, Type, TypeVar, Union, cast
 
 from pants.option.option_value_container import OptionValueContainer
-from pants.option.optionable import Optionable
+from pants.option.optionable import Optionable, OptionableFactory
 from pants.option.options import Options
-from pants.option.scope import ScopeInfo
-from pants.subsystem.subsystem_client_mixin import SubsystemClientMixin, SubsystemDependency
+from pants.option.scope import GLOBAL_SCOPE, ScopeInfo
+from pants.util.ordered_set import OrderedSet
 
 logger = logging.getLogger(__name__)
 
@@ -19,10 +20,44 @@ class SubsystemError(Exception):
     """An error in a subsystem."""
 
 
+class SubsystemClientError(Exception):
+    pass
+
+
+@dataclass(frozen=True)
+class SubsystemDependency(OptionableFactory):
+    """Indicates intent to use an instance of `subsystem_cls` scoped to `scope`."""
+
+    subsystem_cls: Any
+    scope: Any
+    removal_version: Optional[Any] = None
+    removal_hint: Optional[Any] = None
+
+    def is_global(self):
+        return self.scope == GLOBAL_SCOPE
+
+    @property
+    def optionable_cls(self):
+        # Fills the OptionableFactory contract.
+        return self.subsystem_cls
+
+    @property
+    def options_scope(self):
+        """The subscope for options of `subsystem_cls` scoped to `scope`.
+
+        This is the scope that option values are read from when initializing the instance indicated
+        by this dependency.
+        """
+        if self.is_global():
+            return self.subsystem_cls.options_scope
+        else:
+            return self.subsystem_cls.subscope(self.scope)
+
+
 _S = TypeVar("_S", bound="Subsystem")
 
 
-class Subsystem(SubsystemClientMixin, Optionable):
+class Subsystem(Optionable):
     """A separable piece of functionality that may be reused across multiple tasks or other code.
 
     Subsystems encapsulate the configuration and initialization of things like JVMs,
@@ -40,7 +75,7 @@ class Subsystem(SubsystemClientMixin, Optionable):
     For example, the global artifact cache options would be in scope `cache`, but the
     compile.java task can override those options in scope `cache.compile.java`.
 
-    Subsystems may depend on other subsystems, and therefore mix in SubsystemClientMixin.
+    Subsystems may depend on other subsystems.
 
     :API: public
     """
@@ -60,7 +95,7 @@ def scoped(cls, optionable, removal_version=None, removal_hint=None):
         :param removal_version: An optional deprecation version for this scoped Subsystem dependency.
         :param removal_hint: An optional hint to accompany a deprecation removal_version.
 
-        Return value is suitable for use in SubsystemClientMixin.subsystem_dependencies().
+        Return value is suitable for use in Subsystem.subsystem_dependencies().
         """
         return SubsystemDependency(cls, optionable.options_scope, removal_version, removal_hint)
 
@@ -143,8 +178,6 @@ def reset(cls, reset_options: bool = True) -> None:
     def __init__(self, scope: str, scoped_options: OptionValueContainer) -> None:
         """Note: A subsystem has no access to options in scopes other than its own.
 
-    TODO: We'd like that to be true of Tasks some day. Subsystems will help with that.
-
     Code should call scoped_instance() or global_instance() to get a subsystem instance.
     It should not invoke this constructor directly.
 
@@ -228,3 +261,122 @@ def get_streaming_workunit_callbacks(subsystem_names: Iterable[str]) -> List[Cal
                 continue
 
         return callables
+
+    @classmethod
+    def subsystem_dependencies(cls):
+        """The subsystems this object uses.
+
+        Override to specify your subsystem dependencies. Always add them to your superclass's value.
+
+        Note: Do not call this directly to retrieve dependencies. See subsystem_dependencies_iter().
+
+        :return: A tuple of SubsystemDependency instances.
+                 In the common case where you're an optionable and you want to get an instance scoped
+                 to you, call subsystem_cls.scoped(cls) to get an appropriate SubsystemDependency.
+                 As a convenience, you may also provide just a subsystem_cls, which is shorthand for
+                 SubsystemDependency(subsystem_cls, GLOBAL SCOPE) and indicates that we want to use
+                 the global instance of that subsystem.
+        """
+        return tuple()
+
+    @classmethod
+    def subsystem_dependencies_iter(cls):
+        """Iterate over the direct subsystem dependencies of this Optionable."""
+        for dep in cls.subsystem_dependencies():
+            if isinstance(dep, SubsystemDependency):
+                yield dep
+            else:
+                yield SubsystemDependency(
+                    dep, GLOBAL_SCOPE, removal_version=None, removal_hint=None
+                )
+
+    @classmethod
+    def subsystem_closure_iter(cls):
+        """Iterate over the transitive closure of subsystem dependencies of this Optionable.
+
+        :rtype: :class:`collections.Iterator` of :class:`SubsystemDependency`
+        :raises: :class:`Subsystem.CycleException`
+                 if a dependency cycle is detected.
+        """
+        seen = set()
+        dep_path = OrderedSet()
+
+        def iter_subsystem_closure(subsystem_cls):
+            if subsystem_cls in dep_path:
+                raise cls.CycleException(list(dep_path) + [subsystem_cls])
+            dep_path.add(subsystem_cls)
+
+            for dep in subsystem_cls.subsystem_dependencies_iter():
+                if dep not in seen:
+                    seen.add(dep)
+                    yield dep
+                    for d in iter_subsystem_closure(dep.subsystem_cls):
+                        yield d
+
+            dep_path.remove(subsystem_cls)
+
+        for dep in iter_subsystem_closure(cls):
+            yield dep
+
+    class CycleException(Exception):
+        """Thrown when a circular subsystem dependency is detected."""
+
+        def __init__(self, cycle):
+            message = "Cycle detected:\n\t{}".format(
+                " ->\n\t".join(
+                    "{} scope: {}".format(optionable_cls, optionable_cls.options_scope)
+                    for optionable_cls in cycle
+                )
+            )
+            super().__init__(message)
+
+    @classmethod
+    def known_scope_infos(cls):
+        """Yield ScopeInfo for all known scopes for this optionable, in no particular order.
+
+        :rtype: set of :class:`pants.option.scope.ScopeInfo`
+        :raises: :class:`Subsystem.CycleException`
+                 if a dependency cycle is detected.
+        """
+        known_scope_infos = set()
+        optionables_path = (
+            OrderedSet()
+        )  # To check for cycles at the Optionable level, ignoring scope.
+
+        def collect_scope_infos(optionable_cls, scoped_to, removal_version=None, removal_hint=None):
+            if optionable_cls in optionables_path:
+                raise cls.CycleException(list(optionables_path) + [optionable_cls])
+            optionables_path.add(optionable_cls)
+
+            scope = (
+                optionable_cls.options_scope
+                if scoped_to == GLOBAL_SCOPE
+                else optionable_cls.subscope(scoped_to)
+            )
+            scope_info = ScopeInfo(
+                scope, optionable_cls, removal_version=removal_version, removal_hint=removal_hint,
+            )
+
+            if scope_info not in known_scope_infos:
+                known_scope_infos.add(scope_info)
+                for dep in scope_info.optionable_cls.subsystem_dependencies_iter():
+                    # A subsystem always exists at its global scope (for the purpose of options
+                    # registration and specification), even if in practice we only use it scoped to
+                    # some other scope.
+                    #
+                    # NB: We do not apply deprecations to this implicit global copy of the scope, because if
+                    # the intention was to deprecate the entire scope, that could be accomplished by
+                    # deprecating all options in the scope.
+                    collect_scope_infos(dep.subsystem_cls, GLOBAL_SCOPE)
+                    if not dep.is_global():
+                        collect_scope_infos(
+                            dep.subsystem_cls,
+                            scope,
+                            removal_version=dep.removal_version,
+                            removal_hint=dep.removal_hint,
+                        )
+
+            optionables_path.remove(scope_info.optionable_cls)
+
+        collect_scope_infos(cls, GLOBAL_SCOPE)
+        return known_scope_infos