Automatically inject dependencies on sibling files when dependency in…

…ference is unused (#10582)

### Problem

As of #10511, we now use file addresses / generated subtargets everywhere, except for the graph introspection rules.

By default, generated subtargets do not depend on the sibling files in their base target, i.e. the other files in the original `sources` field. Those dependencies will only be included if dependency inference added it, or if the user explicitly added something like `./sibling.txt` to the base target's `dependencies` field.

We generally do not want to automatically depend on sibling files. This would mean that file addresses (and file dependencies) do not actually have any benefit, because `./pants dependencies $file` would resolve the same results as `./pants dependencies $sibling`, i.e. we would not get finer-grained caching. Further, the conceptual model of "targets as metadata applied to files"—rather than "a collection of files with metadata"—would imply that we should not depend on sibling files automatically.

However, for users who are not using dep inference and/or explicit file deps, many things will break if we don't automatically depend on sibling files. In Pants 1.x, it was a safe assumption that you could access any file in the target's `sources` field without having to add a dependency. It's majorly disruptive to break this assumption, and in effect, they would need to use dependency inference to be ergonomic.

### Solution

Unless `--python-infer-imports` is True, and the target in question has `PythonSources`, then we will automatically add dependencies on all sibling files for file addresses. 

This means that every non-Python target (e.g. `files`, `bash_library`) will depend on its siblings, regardless of using dependency inference.

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

build-support/bin/BUILD

@@ -6,7 +6,11 @@ files(
   sources = ['*.sh'],
 )
 
-python_tests(name="tests")
+python_tests(
+  name="tests",
+  # reversion_test.py times out occasionally.
+  timeout=90,
+)
 
 python_binary(
    name = 'bootstrap_and_deploy_ci_pants_pex',

src/python/pants/backend/python/dependency_inference/rules.py

@@ -87,7 +87,7 @@ async def infer_python_dependencies(
     request: InferPythonDependencies, python_inference: PythonInference
 ) -> InferredDependencies:
     if not python_inference.imports:
-        return InferredDependencies()
+        return InferredDependencies([], sibling_dependencies_inferrable=False)
 
     stripped_sources = await Get(StrippedSourceFiles, SourceFilesRequest([request.sources_field]))
     modules = tuple(
@@ -105,14 +105,15 @@ async def infer_python_dependencies(
         for imported_module in file_imports.explicit_imports
         if imported_module not in combined_stdlib
     )
-    return InferredDependencies(
+    result = (
         owner.address
         for owner in owner_per_import
         if (
             owner.address
             and owner.address.maybe_convert_to_base_target() != request.sources_field.address
         )
     )
+    return InferredDependencies(result, sibling_dependencies_inferrable=True)
 
 
 class InferInitDependencies(InferDependenciesRequest):
@@ -124,7 +125,7 @@ async def infer_python_init_dependencies(
     request: InferInitDependencies, python_inference: PythonInference
 ) -> InferredDependencies:
     if not python_inference.inits:
-        return InferredDependencies()
+        return InferredDependencies([], sibling_dependencies_inferrable=False)
 
     # Locate __init__.py files not already in the Snapshot.
     hydrated_sources = await Get(HydratedSources, HydrateSourcesRequest(request.sources_field))
@@ -139,7 +140,9 @@ async def infer_python_init_dependencies(
     owners = await MultiGet(
         Get(Owners, OwnersRequest((f,))) for f in extra_init_files.snapshot.files
     )
-    return InferredDependencies(itertools.chain.from_iterable(owners))
+    return InferredDependencies(
+        itertools.chain.from_iterable(owners), sibling_dependencies_inferrable=False
+    )
 
 
 class InferConftestDependencies(InferDependenciesRequest):
@@ -151,7 +154,7 @@ async def infer_python_conftest_dependencies(
     request: InferConftestDependencies, python_inference: PythonInference,
 ) -> InferredDependencies:
     if not python_inference.conftests:
-        return InferredDependencies()
+        return InferredDependencies([], sibling_dependencies_inferrable=False)
 
     # Locate conftest.py files not already in the Snapshot.
     hydrated_sources = await Get(HydratedSources, HydrateSourcesRequest(request.sources_field))
@@ -165,7 +168,9 @@ async def infer_python_conftest_dependencies(
         Get(Owners, OwnersRequest((f,), OwnersNotFoundBehavior.error))
         for f in extra_conftest_files.snapshot.files
     )
-    return InferredDependencies(itertools.chain.from_iterable(owners))
+    return InferredDependencies(
+        itertools.chain.from_iterable(owners), sibling_dependencies_inferrable=False
+    )
 
 
 def rules():

src/python/pants/backend/python/dependency_inference/rules_test.py

@@ -110,14 +110,16 @@ def run_dep_inference(address: Address) -> InferredDependencies:
             [
                 Address("3rdparty/python", target_name="Django"),
                 Address("src/python/util", relative_file_path="dep.py", target_name="util"),
-            ]
+            ],
+            sibling_dependencies_inferrable=True,
         )
 
         generated_subtarget_address = Address(
             "src/python", relative_file_path="f2.py", target_name="python"
         )
         assert run_dep_inference(generated_subtarget_address) == InferredDependencies(
-            [Address("src/python", relative_file_path="app.py", target_name="python")]
+            [Address("src/python", relative_file_path="app.py", target_name="python")],
+            sibling_dependencies_inferrable=True,
         )
 
     def test_infer_python_inits(self) -> None:
@@ -147,7 +149,8 @@ def run_dep_inference(address: Address) -> InferredDependencies:
             [
                 Address("src/python/root", relative_file_path="__init__.py", target_name="root"),
                 Address("src/python/root/mid", relative_file_path="__init__.py", target_name="mid"),
-            ]
+            ],
+            sibling_dependencies_inferrable=False,
         )
 
     def test_infer_python_conftests(self) -> None:
@@ -178,5 +181,6 @@ def run_dep_inference(address: Address) -> InferredDependencies:
             [
                 Address("src/python/root", relative_file_path="conftest.py", target_name="root"),
                 Address("src/python/root/mid", relative_file_path="conftest.py", target_name="mid"),
-            ]
+            ],
+            sibling_dependencies_inferrable=False,
         )

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

@@ -717,12 +717,10 @@ async def resolve_dependencies(
         spec_path=request.field.address.spec_path,
         subproject_roots=global_options.options.subproject_roots,
     )
-
-    # If this is a base target, inject dependencies on its subtargets.
-    subtarget_addresses: Tuple[Address, ...] = ()
-    if request.field.address.is_base_target:
-        subtargets = await Get(Subtargets, Address, request.field.address)
-        subtarget_addresses = tuple(t.address for t in subtargets.subtargets)
+    literal_addresses = await MultiGet(Get(Address, AddressInput, ai) for ai in provided.addresses)
+    literal_ignored_addresses = set(
+        await MultiGet(Get(Address, AddressInput, ai) for ai in provided.ignored_addresses)
+    )
 
     # Inject any dependencies. This is determined by the `request.field` class. For example, if
     # there is a rule to inject for FortranDependencies, then FortranDependencies and any subclass
@@ -755,10 +753,20 @@ async def resolve_dependencies(
             for inference_request_type in relevant_inference_request_types
         )
 
-    literal_addresses = await MultiGet(Get(Address, AddressInput, ai) for ai in provided.addresses)
-    literal_ignored_addresses = set(
-        await MultiGet(Get(Address, AddressInput, ai) for ai in provided.ignored_addresses)
+    # If this is a base target, or no dependency inference implementation can infer dependencies on
+    # a file address's sibling files, then we inject dependencies on all the base target's
+    # generated subtargets.
+    subtarget_addresses: Tuple[Address, ...] = ()
+    no_sibling_file_deps_inferrable = not inferred or all(
+        inferred_deps.sibling_dependencies_inferrable is False for inferred_deps in inferred
     )
+    if request.field.address.is_base_target or no_sibling_file_deps_inferrable:
+        subtargets = await Get(
+            Subtargets, Address, request.field.address.maybe_convert_to_base_target()
+        )
+        subtarget_addresses = tuple(
+            t.address for t in subtargets.subtargets if t.address != request.field.address
+        )
 
     addresses: Set[Address] = set()
     used_ignored_addresses: Set[Address] = set()

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

@@ -999,7 +999,9 @@ async def infer_smalltalk_dependencies(request: InferSmalltalkDependencies) -> I
     resolved = await MultiGet(
         Get(Address, AddressInput, AddressInput.parse(line)) for line in all_lines
     )
-    return InferredDependencies(resolved)
+    # NB: See `test_depends_on_subtargets` for why we set the field
+    # `sibling_dependencies_inferrable` this way.
+    return InferredDependencies(resolved, sibling_dependencies_inferrable=bool(resolved))
 
 
 class TestDependencies(TestBase):
@@ -1227,3 +1229,36 @@ def test_dependency_inference(self) -> None:
                 ),
             ],
         )
+
+    def test_depends_on_subtargets(self) -> None:
+        """If the address is a base target, or none of the dependency inference rules can infer
+        dependencies on sibling files, then we should depend on all the base target's subtargets."""
+        self.create_file("src/smalltalk/f1.st")
+        self.create_file("src/smalltalk/f2.st")
+        self.add_to_build_file("src/smalltalk", "smalltalk(sources=['*.st'])")
+
+        # Test that a base address depends on its subtargets.
+        self.assert_dependencies_resolved(
+            requested_address=Address("src/smalltalk"),
+            expected=[
+                Address("src/smalltalk", relative_file_path="f1.st"),
+                Address("src/smalltalk", relative_file_path="f2.st"),
+            ],
+        )
+
+        # Test that a file address depends on its siblings if it has no dependency inference rule,
+        # or those inference rules do not claim to infer dependencies on siblings.
+        self.assert_dependencies_resolved(
+            requested_address=Address("src/smalltalk", relative_file_path="f1.st"),
+            expected=[Address("src/smalltalk", relative_file_path="f2.st")],
+        )
+
+        # Now we recreate the files so that the mock dependency inference will have results, which
+        # will cause it to claim to be able to infer dependencies on sibling files.
+        self.add_to_build_file("src/smalltalk/util", "smalltalk()")
+        self.create_file("src/smalltalk/f1.st", "src/smalltalk/util")
+        self.assert_dependencies_resolved(
+            requested_address=Address("src/smalltalk", relative_file_path="f1.st"),
+            # We only expect the inferred address, not any dependencies on sibling files.
+            expected=[Address("src/smalltalk/util")],
+        )

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

@@ -154,7 +154,6 @@ class AddressMapper:
     prelude_glob_patterns: Tuple[str, ...]
     build_patterns: Tuple[str, ...]
     build_ignore_patterns: Tuple[str, ...]
-    subproject_roots: Tuple[str, ...]
 
     def __init__(
         self,
@@ -163,13 +162,11 @@ def __init__(
         prelude_glob_patterns: Optional[Iterable[str]] = None,
         build_patterns: Optional[Iterable[str]] = None,
         build_ignore_patterns: Optional[Iterable[str]] = None,
-        subproject_roots: Optional[Iterable[str]] = None,
     ) -> None:
         self.parser = parser
         self.prelude_glob_patterns = tuple(prelude_glob_patterns or [])
         self.build_patterns = tuple(build_patterns or ["BUILD", "BUILD.*"])
         self.build_ignore_patterns = tuple(build_ignore_patterns or [])
-        self.subproject_roots = tuple(subproject_roots or [])
 
     def __repr__(self):
         return f"AddressMapper(build_patterns={self.build_patterns})"

src/python/pants/engine/target.py

@@ -14,6 +14,7 @@
     Dict,
     Generic,
     Iterable,
+    Iterator,
     Mapping,
     Optional,
     Tuple,
@@ -1542,8 +1543,30 @@ def rules():
     infer_from: ClassVar[Type[Sources]]
 
 
-class InferredDependencies(DeduplicatedCollection[Address]):
-    sort_input = True
+@frozen_after_init
+@dataclass(unsafe_hash=True)
+class InferredDependencies:
+    dependencies: FrozenOrderedSet[Address]
+    sibling_dependencies_inferrable: bool
+
+    def __init__(
+        self, dependencies: Iterable[Address], *, sibling_dependencies_inferrable: bool
+    ) -> None:
+        """The result of inferring dependencies.
+
+        If the inference implementation is able to infer file-level dependencies on sibling files
+        belonging to the same target, set sibling_dependencies_inferrable=True. This allows for
+        finer-grained caching because the dependency rule will not automatically add a dependency on
+        all sibling files.
+        """
+        self.dependencies = FrozenOrderedSet(sorted(dependencies))
+        self.sibling_dependencies_inferrable = sibling_dependencies_inferrable
+
+    def __bool__(self) -> bool:
+        return bool(self.dependencies)
+
+    def __iter__(self) -> Iterator[Address]:
+        return iter(self.dependencies)
 
 
 # -----------------------------------------------------------------------------------------------

src/python/pants/init/engine_initializer.py

@@ -216,7 +216,6 @@ def setup_graph_extended(
             prelude_glob_patterns=bootstrap_options.build_file_prelude_globs,
             build_patterns=bootstrap_options.build_patterns,
             build_ignore_patterns=bootstrap_options.build_ignore,
-            subproject_roots=bootstrap_options.subproject_roots,
         )
 
         @rule

src/python/pants/option/global_options.py

@@ -823,7 +823,8 @@ def register_bootstrap_options(cls, register):
             advanced=True,
         )
 
-        # TODO(#10569): move these out of bootstrap options into normal global options.
+        # TODO(#10569): move these out of bootstrap options into normal global options, near the
+        #  --subproject-roots option.
         register(
             "--build-patterns",
             advanced=True,
@@ -856,14 +857,6 @@ def register_bootstrap_options(cls, register):
                 "See https://www.pantsbuild.org/docs/macros."
             ),
         )
-        register(
-            "--subproject-roots",
-            type=list,
-            advanced=True,
-            default=[],
-            help="Paths that correspond with build roots for any subproject that this "
-            "project depends on.",
-        )
 
         # TODO(#10569): move this out of bootstrap options into normal global options.
         register(
@@ -929,6 +922,15 @@ def register_options(cls, register):
             help="Exclude target roots that match these regexes.",
         )
 
+        register(
+            "--subproject-roots",
+            type=list,
+            advanced=True,
+            default=[],
+            help="Paths that correspond with build roots for any subproject that this "
+            "project depends on.",
+        )
+
         register(
             "--owners-not-found-behavior",
             advanced=True,