Add a test, and switch to using UnexpandedTargets to compute Coarsene…

…dTargets.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

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

@@ -241,6 +241,12 @@ def visit(address: Address):
             )
 
 
+@dataclass(frozen=True)
+class _DependencyMappingRequest:
+    tt_request: TransitiveTargetsRequest
+    expanded_targets: bool
+
+
 @dataclass(frozen=True)
 class _DependencyMapping:
     mapping: FrozenDict[Address, Tuple[Address, ...]]
@@ -249,27 +255,44 @@ class _DependencyMapping:
 
 
 @rule
-async def transitive_dependency_mapping(request: TransitiveTargetsRequest) -> _DependencyMapping:
+async def transitive_dependency_mapping(request: _DependencyMappingRequest) -> _DependencyMapping:
     """This uses iteration, rather than recursion, so that we can tolerate dependency cycles.
 
     Unlike a traditional BFS algorithm, we batch each round of traversals via `MultiGet` for
     improved performance / concurrency.
     """
-    roots_as_targets = await Get(Targets, Addresses(request.roots))
+    roots_as_targets: Collection[Target]
+    if request.expanded_targets:
+        roots_as_targets = await Get(Targets, Addresses(request.tt_request.roots))
+    else:
+        roots_as_targets = await Get(UnexpandedTargets, Addresses(request.tt_request.roots))
     visited: OrderedSet[Target] = OrderedSet()
     queued = FrozenOrderedSet(roots_as_targets)
     dependency_mapping: Dict[Address, Tuple[Address, ...]] = {}
     while queued:
-        direct_dependencies = await MultiGet(
-            Get(
-                Targets,
-                DependenciesRequest(
-                    tgt.get(Dependencies),
-                    include_special_cased_deps=request.include_special_cased_deps,
-                ),
+        direct_dependencies: Tuple[Collection[Target], ...]
+        if request.expanded_targets:
+            direct_dependencies = await MultiGet(
+                Get(
+                    Targets,
+                    DependenciesRequest(
+                        tgt.get(Dependencies),
+                        include_special_cased_deps=request.tt_request.include_special_cased_deps,
+                    ),
+                )
+                for tgt in queued
+            )
+        else:
+            direct_dependencies = await MultiGet(
+                Get(
+                    UnexpandedTargets,
+                    DependenciesRequest(
+                        tgt.get(Dependencies),
+                        include_special_cased_deps=request.tt_request.include_special_cased_deps,
+                    ),
+                )
+                for tgt in queued
             )
-            for tgt in queued
-        )
 
         dependency_mapping.update(
             zip(
@@ -287,13 +310,17 @@ async def transitive_dependency_mapping(request: TransitiveTargetsRequest) -> _D
     # is because expanding from the `Addresses` -> `Targets` may have resulted in generated
     # subtargets being used, so we need to use `roots_as_targets` to have this expansion.
     _detect_cycles(tuple(t.address for t in roots_as_targets), dependency_mapping)
-    return _DependencyMapping(dependency_mapping, FrozenOrderedSet(visited), roots_as_targets)
+    return _DependencyMapping(
+        FrozenDict(dependency_mapping), FrozenOrderedSet(visited), roots_as_targets
+    )
 
 
 @rule(desc="Resolve transitive targets")
-async def transitive_targets(dependency_mapping: _DependencyMapping) -> TransitiveTargets:
+async def transitive_targets(request: TransitiveTargetsRequest) -> TransitiveTargets:
     """Find all the targets transitively depended upon by the target roots."""
 
+    dependency_mapping = await Get(_DependencyMapping, _DependencyMappingRequest(request, True))
+
     # Apply any transitive excludes (`!!` ignores).
     transitive_excludes: FrozenOrderedSet[Target] = FrozenOrderedSet()
     unevaluated_transitive_excludes = []
@@ -323,16 +350,25 @@ async def transitive_targets(dependency_mapping: _DependencyMapping) -> Transiti
 
 @rule
 async def coarsened_targets(address: Address) -> CoarsenedTargets:
-    dependency_mapping = await Get(_DependencyMapping, TransitiveTargetsRequest([address]))
-    components = native_engine.strongly_connected_components(list(dependency_mapping))
+    dependency_mapping = await Get(
+        _DependencyMapping,
+        _DependencyMappingRequest(TransitiveTargetsRequest([address]), expanded_targets=False),
+    )
+    components = native_engine.strongly_connected_components(
+        list(dependency_mapping.mapping.items())
+    )
 
     # Return the relevant component.
     # TODO: This is very inefficient. Before landing, this should apply a strategy similar to
     # https://github.com/pantsbuild/pants/issues/11270 to share subgraphs which have already been
     # computed.
+    addresses_to_targets = {
+        t.address: t for t in [*dependency_mapping.visited, *dependency_mapping.roots_as_targets]
+    }
     for component in components:
-        if address in component:
-            return CoarsenedTargets(component)
+        if address not in component:
+            continue
+        return CoarsenedTargets([addresses_to_targets[a] for a in component])
     raise AssertionError("No component contained the relevant Address.")
 
 

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

@@ -43,6 +43,7 @@
 from pants.engine.rules import Get, MultiGet, rule
 from pants.engine.target import (
     AsyncFieldMixin,
+    CoarsenedTargets,
     Dependencies,
     DependenciesRequest,
     ExplicitlyProvidedDependencies,
@@ -97,6 +98,7 @@ class MockTarget(Target):
 def transitive_targets_rule_runner() -> RuleRunner:
     return RuleRunner(
         rules=[
+            QueryRule(CoarsenedTargets, (Address,)),
             QueryRule(Targets, (DependenciesRequest,)),
             QueryRule(TransitiveTargets, (TransitiveTargetsRequest,)),
         ],
@@ -306,6 +308,53 @@ def test_transitive_targets_tolerates_subtarget_cycles(
     ]
 
 
+def test_coarsened_targets(transitive_targets_rule_runner: RuleRunner) -> None:
+    """CoarsenedTargets should "coarsen" a cycle into a single CoarsenedTarget instance.
+
+    Cycles are only allowed for file targets, so we use explicit file dependencies in this test.
+    """
+    transitive_targets_rule_runner.create_files("", ["dep.txt", "t1.txt", "t2.txt"])
+    transitive_targets_rule_runner.add_to_build_file(
+        "",
+        dedent(
+            """\
+            target(name='dep', sources=['dep.txt'])
+            target(name='t1', sources=['t1.txt'], dependencies=['dep.txt:dep', 't2.txt:t2'])
+            target(name='t2', sources=['t2.txt'], dependencies=['t1.txt:t1'])
+            """
+        ),
+    )
+
+    def coarsened(a: Address) -> List[Address]:
+        return list(
+            sorted(
+                t.address
+                for t in transitive_targets_rule_runner.request(
+                    CoarsenedTargets,
+                    [a],
+                )
+            )
+        )
+
+    # BUILD targets are never involved in cycles, and so always coarsen to themselves.
+    assert coarsened(Address("", target_name="dep")) == [Address("", target_name="dep")]
+    assert coarsened(Address("", target_name="t1")) == [Address("", target_name="t1")]
+    assert coarsened(Address("", target_name="t2")) == [Address("", target_name="t2")]
+
+    # As will file targets not involved in cycles.
+    assert coarsened(Address("", relative_file_path="dep.txt", target_name="dep")) == [
+        Address("", relative_file_path="dep.txt", target_name="dep")
+    ]
+
+    # But file targets involved in cycles will coarsen to the cycle.
+    cycle_files = [
+        Address("", relative_file_path="t1.txt", target_name="t1"),
+        Address("", relative_file_path="t2.txt", target_name="t2"),
+    ]
+    assert coarsened(Address("", relative_file_path="t1.txt", target_name="t1")) == cycle_files
+    assert coarsened(Address("", relative_file_path="t2.txt", target_name="t2")) == cycle_files
+
+
 def assert_failed_cycle(
     rule_runner: RuleRunner,
     *,

src/rust/engine/src/externs/interface.rs

@@ -841,10 +841,9 @@ fn strongly_connected_components(
 
   for (node, adjacency_list) in adjacency_lists {
     let node_key = externs::key_for(node.clone_ref(py).into())?;
-    let node_id = node_ids
+    let node_id = *node_ids
       .entry(node_key)
-      .or_insert_with(|| graph.add_node(node_key))
-      .clone();
+      .or_insert_with(|| graph.add_node(node_key));
     for dependency in adjacency_list {
       let dependency_key = externs::key_for(dependency.clone_ref(py).into())?;
       let dependency_id = node_ids