Tolerate target cycles when using dependency inference (#10393)

Closes #10059.

It's common in many languages, including Python, to sometimes have import cycles. When using generated subtargets—meaning either dependency inference or explicit file dependencies—Pants will tolerate these cycles. For now, there is no other way to express that a cycle should be tolerated. In the future, we may consider adding a syntax to BUILD files to allow indicating that cycles should be tolerated; it's easier to loosen than it is to tighten.

[ci skip-rust-tests]

src/python/pants/base/specs.py

@@ -5,7 +5,18 @@
 import re
 from abc import ABC, ABCMeta, abstractmethod
 from dataclasses import dataclass
-from typing import Any, Dict, Iterable, Iterator, List, Optional, Sequence, Tuple, Union, cast
+from typing import (
+    TYPE_CHECKING,
+    Dict,
+    Iterable,
+    Iterator,
+    List,
+    Optional,
+    Sequence,
+    Tuple,
+    Union,
+    cast,
+)
 
 from pants.engine.collection import Collection
 from pants.engine.fs import PathGlobs
@@ -17,6 +28,9 @@
 from pants.util.memo import memoized_property
 from pants.util.meta import frozen_after_init
 
+if TYPE_CHECKING:
+    from pants.engine.internals.mapper import AddressFamily, AddressMapper
+
 
 class Spec(ABC):
     """A specification for what Pants should operate on."""
@@ -40,7 +54,9 @@ class AddressFamilyResolutionError(Exception):
         pass
 
     @abstractmethod
-    def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
+    def matching_address_families(
+        self, address_families_dict: Dict[str, "AddressFamily"]
+    ) -> List["AddressFamily"]:
         """Given a dict of (namespace path) -> AddressFamily, return the values matching this
         address spec.
 
@@ -49,8 +65,8 @@ def matching_address_families(self, address_families_dict: Dict[str, Any]) -> Li
 
     @classmethod
     def address_families_for_dir(
-        cls, address_families_dict: Dict[str, Any], spec_dir_path: str
-    ) -> List[Any]:
+        cls, address_families_dict: Dict[str, "AddressFamily"], spec_dir_path: str
+    ) -> List["AddressFamily"]:
         """Implementation of `matching_address_families()` for address specs matching at most one
         directory."""
         maybe_af = address_families_dict.get(spec_dir_path, None)
@@ -64,7 +80,7 @@ class AddressResolutionError(Exception):
         pass
 
     @abstractmethod
-    def address_target_pairs_from_address_families(self, address_families: List[Any]):
+    def address_target_pairs_from_address_families(self, address_families: List["AddressFamily"]):
         """Given a list of AddressFamily, return (address, target) pairs matching this address spec.
 
         :raises: :class:`SingleAddress._SingleAddressResolutionError` for resolution errors with a
@@ -84,11 +100,11 @@ def all_address_target_pairs(cls, address_families):
         return addr_tgt_pairs
 
     @abstractmethod
-    def make_glob_patterns(self, address_mapper: Any) -> List[str]:
+    def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
         """Generate glob patterns matching exactly all the BUILD files this address spec covers."""
 
     @classmethod
-    def globs_in_single_dir(cls, spec_dir_path: str, address_mapper: Any) -> List[str]:
+    def globs_in_single_dir(cls, spec_dir_path: str, address_mapper: "AddressMapper") -> List[str]:
         """Implementation of `make_glob_patterns()` which only allows a single base directory."""
         return [os.path.join(spec_dir_path, pat) for pat in address_mapper.build_patterns]
 
@@ -109,16 +125,20 @@ def __post_init__(self) -> None:
     def to_spec_string(self) -> str:
         return "{}:{}".format(self.directory, self.name)
 
-    def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
+    def matching_address_families(
+        self, address_families_dict: Dict[str, "AddressFamily"]
+    ) -> List["AddressFamily"]:
         return self.address_families_for_dir(address_families_dict, self.directory)
 
     class _SingleAddressResolutionError(Exception):
-        def __init__(self, single_address_family: Any, name: str) -> None:
+        def __init__(self, single_address_family: "AddressFamily", name: str) -> None:
             super().__init__()
             self.single_address_family = single_address_family
             self.name = name
 
-    def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
+    def address_target_pairs_from_address_families(
+        self, address_families: Sequence["AddressFamily"]
+    ):
         """Return the pair for the single target matching the single AddressFamily, or error.
 
         :raises: :class:`SingleAddress._SingleAddressResolutionError` if no targets could be found for a
@@ -137,7 +157,7 @@ def address_target_pairs_from_address_families(self, address_families: Sequence[
         assert len(addr_tgt_pairs) == 1
         return addr_tgt_pairs
 
-    def make_glob_patterns(self, address_mapper: Any) -> List[str]:
+    def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
         return self.globs_in_single_dir(self.directory, address_mapper)
 
 
@@ -150,13 +170,17 @@ class SiblingAddresses(AddressSpec):
     def to_spec_string(self) -> str:
         return f"{self.directory}:"
 
-    def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
+    def matching_address_families(
+        self, address_families_dict: Dict[str, "AddressFamily"]
+    ) -> List["AddressFamily"]:
         return self.address_families_for_dir(address_families_dict, self.directory)
 
-    def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
+    def address_target_pairs_from_address_families(
+        self, address_families: Sequence["AddressFamily"]
+    ):
         return self.all_address_target_pairs(address_families)
 
-    def make_glob_patterns(self, address_mapper: Any) -> List[str]:
+    def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
         return self.globs_in_single_dir(self.directory, address_mapper)
 
 
@@ -170,22 +194,26 @@ class DescendantAddresses(AddressSpec):
     def to_spec_string(self) -> str:
         return f"{self.directory}::"
 
-    def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
+    def matching_address_families(
+        self, address_families_dict: Dict[str, "AddressFamily"]
+    ) -> List["AddressFamily"]:
         return [
             af
             for ns, af in address_families_dict.items()
             if fast_relpath_optional(ns, self.directory) is not None
         ]
 
-    def address_target_pairs_from_address_families(self, address_families: Sequence[Any]):
+    def address_target_pairs_from_address_families(
+        self, address_families: Sequence["AddressFamily"]
+    ):
         addr_tgt_pairs = self.all_address_target_pairs(address_families)
         if self.error_if_no_matches and len(addr_tgt_pairs) == 0:
             raise self.AddressResolutionError(
                 f"Address spec {repr(self.to_spec_string())} does not match any targets."
             )
         return addr_tgt_pairs
 
-    def make_glob_patterns(self, address_mapper: Any) -> List[str]:
+    def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
         return [os.path.join(self.directory, "**", pat) for pat in address_mapper.build_patterns]
 
 
@@ -198,17 +226,21 @@ class AscendantAddresses(AddressSpec):
     def to_spec_string(self) -> str:
         return f"{self.directory}^"
 
-    def matching_address_families(self, address_families_dict: Dict[str, Any]) -> List[Any]:
+    def matching_address_families(
+        self, address_families_dict: Dict[str, "AddressFamily"]
+    ) -> List["AddressFamily"]:
         return [
             af
             for ns, af in address_families_dict.items()
             if fast_relpath_optional(self.directory, ns) is not None
         ]
 
-    def address_target_pairs_from_address_families(self, address_families):
+    def address_target_pairs_from_address_families(
+        self, address_families: Sequence["AddressFamily"]
+    ):
         return self.all_address_target_pairs(address_families)
 
-    def make_glob_patterns(self, address_mapper: Any) -> List[str]:
+    def make_glob_patterns(self, address_mapper: "AddressMapper") -> List[str]:
         return [
             os.path.join(f, pattern)
             for pattern in address_mapper.build_patterns

src/python/pants/engine/fs.py

@@ -4,7 +4,7 @@
 import os
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Any, Iterable, Optional, Tuple
+from typing import TYPE_CHECKING, Iterable, Optional, Tuple
 
 from pants.engine.collection import Collection
 from pants.engine.rules import RootRule, side_effecting
@@ -18,6 +18,9 @@
 )
 from pants.util.meta import frozen_after_init
 
+if TYPE_CHECKING:
+    from pants.engine.internals.scheduler import SchedulerSession
+
 
 @dataclass(frozen=True)
 class FileContent:
@@ -282,8 +285,7 @@ class UrlToFetch:
 class Workspace:
     """Abstract handle for operations that touch the real local filesystem."""
 
-    # TODO: SchedulerSession. Untyped because `fs.py` and `scheduler.py` have a cycle.
-    _scheduler: Any
+    _scheduler: "SchedulerSession"
 
     def materialize_directory(self, directory_to_materialize: DirectoryToMaterialize):
         """Materialize one single directory digest to disk.

src/python/pants/engine/interactive_process.py

@@ -3,13 +3,16 @@
 
 import itertools
 from dataclasses import dataclass
-from typing import Any, Iterable, Mapping, Optional, Tuple, cast
+from typing import TYPE_CHECKING, Iterable, Mapping, Optional, Tuple
 
 from pants.base.exception_sink import ExceptionSink
 from pants.engine.fs import EMPTY_DIGEST, Digest
 from pants.engine.rules import RootRule, side_effecting
 from pants.util.meta import frozen_after_init
 
+if TYPE_CHECKING:
+    from pants.engine.internals.scheduler import SchedulerSession
+
 
 @dataclass(frozen=True)
 class InteractiveProcessResult:
@@ -56,13 +59,11 @@ def __post_init__(self):
 @side_effecting
 @dataclass(frozen=True)
 class InteractiveRunner:
-    _scheduler: Any
+    _scheduler: "SchedulerSession"
 
     def run_process(self, request: InteractiveProcess) -> InteractiveProcessResult:
         ExceptionSink.toggle_ignoring_sigint_v2_engine(True)
-        return cast(
-            InteractiveProcessResult, self._scheduler.run_local_interactive_process(request)
-        )
+        return self._scheduler.run_local_interactive_process(request)
 
 
 def rules():

src/python/pants/engine/internals/graph.py

@@ -141,8 +141,32 @@ async def transitive_target(wrapped_root: WrappedTarget) -> TransitiveTarget:
     if not root.has_field(Dependencies):
         return TransitiveTarget(root, ())
     dependency_addresses = await Get(Addresses, DependenciesRequest(root[Dependencies]))
-    dependencies = await MultiGet(Get(TransitiveTarget, Address, d) for d in dependency_addresses)
-    return TransitiveTarget(root, dependencies)
+
+    # For generated subtargets, we use a weak Get, which means that any dependency cycles will
+    # return None, rather than TransitiveTarget.
+    transitive_dependencies = await MultiGet(
+        Get(TransitiveTarget, Address, addr, weak=bool(addr.generated_base_target_name))
+        for addr in dependency_addresses
+    )
+    non_cyclic_dependencies = []
+    cyclic_addresses = []
+    for transitive_dep, address in zip(transitive_dependencies, dependency_addresses):
+        if transitive_dep:
+            non_cyclic_dependencies.append(transitive_dep)
+        else:
+            cyclic_addresses.append(address)
+    cyclic_dependencies = await MultiGet(
+        Get(WrappedTarget, Address, addr) for addr in cyclic_addresses
+    )
+
+    all_dependencies = (
+        *non_cyclic_dependencies,
+        *(
+            TransitiveTarget(wrapped_tgt.target, dependencies=())
+            for wrapped_tgt in cyclic_dependencies
+        ),
+    )
+    return TransitiveTarget(root, tuple(all_dependencies))
 
 
 @rule

src/python/pants/engine/internals/graph_test.py

@@ -133,6 +133,35 @@ def get_target(name: str) -> Target:
         assert transitive_targets.dependencies == FrozenOrderedSet([d1, d2, d3, t2, t1])
         assert transitive_targets.closure == FrozenOrderedSet([root, d2, d1, d3, t2, t1])
 
+    def test_transitive_targets_tolerates_cycles(self) -> None:
+        """For generated subtargets, we should tolerate cycles between targets.
+
+        This only works with generated subtargets, so we use explicit file dependencies in this
+        test.
+        """
+        self.create_files("", ["dep.txt", "t1.txt", "t2.txt"])
+        self.add_to_build_file(
+            "",
+            dedent(
+                """\
+                target(name='dep', sources=['dep.txt'])
+                target(name='t1', sources=['t1.txt'], dependencies=['dep.txt', 't2.txt'])
+                target(name='t2', sources=['t2.txt'], dependencies=['t1.txt'])
+                """
+            ),
+        )
+        result = self.request_single_product(
+            TransitiveTargets,
+            Params(Addresses([Address.parse("//:t2")]), create_options_bootstrapper()),
+        )
+        assert len(result.roots) == 1
+        assert result.roots[0].address == Address.parse("//:t2")
+        assert [tgt.address for tgt in result.dependencies] == [
+            Address("", target_name="t1.txt", generated_base_target_name="t1"),
+            Address("", target_name="dep.txt", generated_base_target_name="dep"),
+            Address("", target_name="t2.txt", generated_base_target_name="t2"),
+        ]
+
     def test_resolve_generated_subtarget(self) -> None:
         self.add_to_build_file("demo", "target(sources=['f1.txt', 'f2.txt'])")
         generated_target_addresss = Address(