Allow ignoring dependencies with !

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

@@ -1,6 +1,7 @@
 # Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
+import functools
 import itertools
 import logging
 import os.path
@@ -575,41 +576,56 @@ class InvalidFileDependency(Exception):
 class ParsedDependencies(NamedTuple):
     addresses: List[Address]
     files: List[str]
+    ignored_addresses: List[Address]
+    ignored_files: List[str]
 
 
 def parse_dependencies_field(
     raw_value: Iterable[str], *, spec_path: str, subproject_roots: Sequence[str]
 ) -> ParsedDependencies:
-    address_strings = []
-    files = []
-    for v in raw_value:
-        if ":" in v:
-            address_strings.append(v)
-            continue
-        elif v.startswith("./"):
-            files.append(PurePath(spec_path, v).as_posix())
-            continue
-
-        # Address specs for top-level targets take the form `//:foo`, meaning that we will have
-        # already recognized the `:`. Any other use of `//` is unnecessary and only used for
-        # clarity in BUILD files, so can be removed.
-        if v.startswith("//"):
-            v = v[2:]
+    parse_as_address = functools.partial(
+        Address.parse, relative_to=spec_path, subproject_roots=subproject_roots
+    )
 
-        if PurePath(v).suffix:
-            files.append(v)
+    def parse(value: str) -> Union[Address, str]:
+        # We allow `//` to specify the value is relative to the build root. This is only actually
+        # necessary for top-level addresses, though, like `//:tgt`. Otherwise, we can strip `//`.
+        if value.startswith("//") and not value.startswith("//:"):
+            value = value[2:]
+        if ":" in value:
+            return parse_as_address(value)
+        if value.startswith("./"):
+            return PurePath(spec_path, value).as_posix()
+        if PurePath(value).suffix:
+            return value
+        return parse_as_address(value)
+
+    addresses: List[Address] = []
+    files: List[str] = []
+    ignored_addresses: List[Address] = []
+    ignored_files: List[str] = []
+    for v in raw_value:
+        is_ignore = v.startswith("!")
+        if is_ignore:
+            v = v[1:]
+        result = parse(v)
+        if is_ignore:
+            collection = ignored_addresses if isinstance(result, Address) else ignored_files
         else:
-            address_strings.append(v)
-    addresses = [
-        Address.parse(v, relative_to=spec_path, subproject_roots=subproject_roots)
-        for v in address_strings
-    ]
-    return ParsedDependencies(addresses, files)
+            collection = addresses if isinstance(result, Address) else files
+        collection.append(result)  # type: ignore[attr-defined]
+    return ParsedDependencies(addresses, files, ignored_addresses, ignored_files)
 
 
-def validate_explicit_file_dep(address: Address, full_file: str, owners: Sequence[Address]) -> None:
+def validate_explicit_file_dep(
+    address: Address, full_file: str, owners: Sequence[Address], *, is_an_ignore: bool = False
+) -> None:
+    if is_an_ignore:
+        full_file = f"!{full_file}"
     if len(owners) > 1:
         original_addresses = sorted(owner.maybe_convert_to_base_target().spec for owner in owners)
+        if is_an_ignore:
+            original_addresses = [f"!{addr}" for addr in original_addresses]
         raise InvalidFileDependency(
             f"The target {address} included {repr(full_file)} in its `dependencies` "
             "field, but there are multiple owners of that file so it is ambiguous which one "
@@ -623,8 +639,9 @@ def validate_explicit_file_dep(address: Address, full_file: str, owners: Sequenc
     if not owners or file_does_not_exist:
         raise InvalidFileDependency(
             f"The target {address} included {repr(full_file)} in its `dependencies` "
-            "field, but there are no owners of that file. Please check that the file exists "
-            f"and that you spelled the file correctly."
+            "field, but there are no owners of that file. Please check that the file exists, "
+            "that you spelled the file correctly, and that there is a target that includes the "
+            "file in its `sources` field."
         )
 
 
@@ -644,6 +661,12 @@ async def resolve_dependencies(
     for f, owners in zip(provided.files, explicit_file_deps_owners):
         validate_explicit_file_dep(request.field.address, f, owners)
 
+    explicit_file_deps_ignore_owners = await MultiGet(
+        Get(Owners, OwnersRequest((f,))) for f in provided.ignored_files
+    )
+    for f, owners in zip(provided.ignored_files, explicit_file_deps_ignore_owners):
+        validate_explicit_file_dep(request.field.address, f, owners, is_an_ignore=True)
+
     # 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
     # of FortranDependencies will use that rule.
@@ -678,19 +701,33 @@ async def resolve_dependencies(
                 inference_request_type(sources_field),
             )
 
-    original_addresses = {*provided.addresses, *itertools.chain.from_iterable(injected)}
-    generated_addresses = {*inferred, *itertools.chain.from_iterable(explicit_file_deps_owners)}
+    original_addresses = {
+        addr
+        for addr in (*provided.addresses, *itertools.chain.from_iterable(injected))
+        if addr not in provided.ignored_addresses
+    }
+    flattened_ignore_file_deps_owners = set(
+        itertools.chain.from_iterable(explicit_file_deps_ignore_owners)
+    )
+    generated_addresses = {
+        addr
+        for addr in (*inferred, *itertools.chain.from_iterable(explicit_file_deps_owners))
+        if addr not in flattened_ignore_file_deps_owners
+    }
     # If a generated subtarget's original base target is already included, then we leave off the
     # generated subtarget. We also leave it off if the generated subtarget's original base target
-    # is the target we're resolving dependencies for.
-    remaining_generated_addresses = {
-        addr
-        for addr in generated_addresses
+    # is the target we're resolving dependencies for. Finally, we leave it off if the user said to
+    # ignore the original owning target.
+    remaining_generated_addresses = set()
+    for generated_addr in generated_addresses:
+        base_addr = generated_addr.maybe_convert_to_base_target()
         if (
-            addr.maybe_convert_to_base_target() not in original_addresses
-            and addr.maybe_convert_to_base_target() != request.field.address
-        )
-    }
+            base_addr in original_addresses
+            or base_addr in provided.ignored_addresses
+            or base_addr == request.field.address
+        ):
+            continue
+        remaining_generated_addresses.add(generated_addr)
     return Addresses(sorted({*original_addresses, *remaining_generated_addresses}))
 
 

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

@@ -848,69 +848,94 @@ class IrrelevantSources(Sources):
 
 
 def test_parse_dependencies_field() -> None:
+    given_values = [
+        ":relative",
+        "//:top_level",
+        "demo:tgt",
+        "demo",
+        "./relative.txt",
+        "./child/f.txt",
+        "demo/f.txt",
+        "//top_level.txt",
+        "top_level2.txt",
+        # For files without an extension, you must use `./` There is no way (yet) to reference
+        # a file above you without a file extension.
+        "demo/no_extension",
+        "//demo/no_extension",
+        "./no_extension",
+    ]
     result = parse_dependencies_field(
-        [
-            ":relative",
-            "//:top_level",
-            "demo:tgt",
-            "demo",
-            "./relative.txt",
-            "./child/f.txt",
-            "demo/f.txt",
-            "//top_level.txt",
-            "top_level2.txt",
-            # For files without an extension, you must use `./` There is no way (yet) to reference
-            # a file above you without a file extension.
-            "demo/no_extension",
-            "//demo/no_extension",
-            "./no_extension",
-        ],
+        [*given_values, *(f"!{v}" for v in given_values)],
         spec_path="demo/subdir",
         subproject_roots=[],
     )
-    assert set(result.addresses) == {
+    expected_addresses = {
         Address("demo/subdir", "relative"),
         Address("", "top_level"),
         Address("demo", "tgt"),
         Address("demo", "demo"),
         Address("demo/no_extension", "no_extension"),
     }
-    assert set(result.files) == {
+    assert set(result.addresses) == expected_addresses
+    assert set(result.ignored_addresses) == expected_addresses
+    expected_files = {
         "demo/subdir/relative.txt",
         "demo/subdir/child/f.txt",
         "demo/f.txt",
         "top_level.txt",
         "top_level2.txt",
         "demo/subdir/no_extension",
     }
+    assert set(result.files) == expected_files
+    assert set(result.ignored_files) == expected_files
 
 
 def test_validate_explicit_file_dep() -> None:
     addr = Address("demo", "tgt")
 
-    def assert_raises(owners: Sequence[Address], *, expected_snippets: Iterable[str]) -> None:
+    def assert_raises(
+        owners: Sequence[Address], *, expected_snippets: Iterable[str], is_an_ignore: bool = False
+    ) -> None:
         with pytest.raises(InvalidFileDependency) as exc:
-            validate_explicit_file_dep(addr, full_file="f.txt", owners=owners)
+            validate_explicit_file_dep(
+                addr, full_file="f.txt", owners=owners, is_an_ignore=is_an_ignore
+            )
         assert addr.spec in str(exc.value)
-        assert "f.txt" in str(exc.value)
+        if is_an_ignore:
+            assert "!f.txt" in str(exc.value)
+        else:
+            assert "f.txt" in str(exc.value)
         for snippet in expected_snippets:
             assert snippet in str(exc.value)
 
     assert_raises(owners=[], expected_snippets=["no owners"])
-    # Even if there is one owner, if it was not generated, then we fail. We can assume that the
-    # file in question does not ectually exist.
+    assert_raises(owners=[], is_an_ignore=True, expected_snippets=["no owners"])
+    # Even if there is one owner, if it was not generated, then we fail because we can assume that
+    # the file in question does not actually exist.
     assert_raises(owners=[Address.parse(":t")], expected_snippets=["no owners"])
+    assert_raises(owners=[Address.parse(":t")], is_an_ignore=True, expected_snippets=["no owners"])
     assert_raises(
         owners=[Address.parse(":t1"), Address.parse(":t2")],
         expected_snippets=["multiple owners", "//:t1", "//:t2"],
     )
+    assert_raises(
+        owners=[Address.parse(":t1"), Address.parse(":t2")],
+        is_an_ignore=True,
+        expected_snippets=["multiple owners", "!//:t1", "!//:t2"],
+    )
 
     # Do not raise if there is one single generated owner.
     validate_explicit_file_dep(
         addr,
         full_file="f.txt",
         owners=[Address("", target_name="f.txt", generated_base_target_name="demo")],
     )
+    validate_explicit_file_dep(
+        addr,
+        full_file="f.txt",
+        owners=[Address("", target_name="f.txt", generated_base_target_name="demo")],
+        is_an_ignore=True,
+    )
 
 
 class SmalltalkDependencies(Dependencies):
@@ -931,7 +956,7 @@ class InjectCustomSmalltalkDependencies(InjectDependenciesRequest):
 
 @rule
 def inject_smalltalk_deps(_: InjectSmalltalkDependencies) -> InjectedDependencies:
-    return InjectedDependencies([Address.parse("//:injected")])
+    return InjectedDependencies([Address.parse("//:injected1"), Address.parse("//:injected2")])
 
 
 @rule
@@ -1029,11 +1054,27 @@ def test_normal_resolution(self) -> None:
         self.add_to_build_file("no_deps", "smalltalk()")
         self.assert_dependencies_resolved(requested_address=Address.parse("no_deps"), expected=[])
 
+        # An ignore should override an include.
+        self.add_to_build_file("ignore", "smalltalk(dependencies=['//:dep', '!//:dep'])")
+        self.assert_dependencies_resolved(requested_address=Address.parse("ignore"), expected=[])
+
     def test_explicit_file_dependencies(self) -> None:
-        self.create_files("src/smalltalk/util", ["f1.st", "f2.st"])
+        self.create_files("src/smalltalk/util", ["f1.st", "f2.st", "f3.st"])
         self.add_to_build_file("src/smalltalk/util", "smalltalk(sources=['*.st'])")
         self.add_to_build_file(
-            "src/smalltalk", "smalltalk(dependencies=['./util/f1.st', 'src/smalltalk/util/f2.st'])"
+            "src/smalltalk",
+            dedent(
+                """\
+                smalltalk(
+                  dependencies=[
+                    './util/f1.st',
+                    'src/smalltalk/util/f2.st',
+                    './util/f3.st',
+                    '!./util/f3.st'
+                  ]
+                )
+                """
+            ),
         )
         self.assert_dependencies_resolved(
             requested_address=Address.parse("src/smalltalk"),
@@ -1051,7 +1092,9 @@ def test_dependency_injection(self) -> None:
         self.add_to_build_file("", "smalltalk(name='target')")
 
         def assert_injected(deps_cls: Type[Dependencies], *, injected: List[str]) -> None:
-            deps_field = deps_cls(["//:provided"], address=Address.parse("//:target"))
+            deps_field = deps_cls(
+                ["//:provided", "!//:injected2"], address=Address.parse("//:target")
+            )
             result = self.request_single_product(
                 Addresses, Params(DependenciesRequest(deps_field), create_options_bootstrapper())
             )
@@ -1060,8 +1103,10 @@ def assert_injected(deps_cls: Type[Dependencies], *, injected: List[str]) -> Non
             )
 
         assert_injected(Dependencies, injected=[])
-        assert_injected(SmalltalkDependencies, injected=["//:injected"])
-        assert_injected(CustomSmalltalkDependencies, injected=["//:custom_injected", "//:injected"])
+        assert_injected(SmalltalkDependencies, injected=["//:injected1"])
+        assert_injected(
+            CustomSmalltalkDependencies, injected=["//:custom_injected", "//:injected1"]
+        )
 
     def test_dependency_inference(self) -> None:
         """We test that dependency inference works generally and that we merge it correctly with
@@ -1070,12 +1115,15 @@ def test_dependency_inference(self) -> None:
         If dep inference returns a generated subtarget, but the original owning target was
         explicitly provided, then we should not use the generated subtarget.
         """
+        self.create_files("", ["inferred_but_ignored1.st", "inferred_but_ignored2.st"])
         self.add_to_build_file(
             "",
             dedent(
                 """\
                 smalltalk(name='inferred1')
                 smalltalk(name='inferred2')
+                smalltalk(name='inferred_but_ignored1', sources=['inferred_but_ignored1.st'])
+                smalltalk(name='inferred_but_ignored2', sources=['inferred_but_ignored2.st'])
                 smalltalk(name='inferred_and_provided1')
                 smalltalk(name='inferred_and_provided2')
                 """
@@ -1096,6 +1144,8 @@ def test_dependency_inference(self) -> None:
                 """\
                 //:inferred_and_provided1
                 inferred_and_provided2.st from //:inferred_and_provided2
+                inferred_but_ignored1.st from //:inferred_but_ignored1
+                inferred_but_ignored2.st from //:inferred_but_ignored2
                 """
             ),
         )
@@ -1105,7 +1155,12 @@ def test_dependency_inference(self) -> None:
                 """\
                 smalltalk(
                   sources=['*.st'],
-                  dependencies=['//:inferred_and_provided1', '//:inferred_and_provided2'],
+                  dependencies=[
+                    '//:inferred_and_provided1',
+                    '//:inferred_and_provided2',
+                    '!inferred_but_ignored1.st',
+                    '!//:inferred_but_ignored2',
+                  ],
                 )
                 """
             ),

src/python/pants/engine/target.py

@@ -1356,6 +1356,10 @@ class Dependencies(AsyncField):
     create a new target from that which only includes the file in its `sources` field. For files
     relative to the current BUILD file, prefix with `./`; otherwise, put the full path, e.g.
     ['./sibling.txt', 'resources/demo.json'].
+
+    You may exclude dependencies by prefixing with `!`, e.g. `['!helloworld/subdir:lib',
+    '!./sibling.txt']`. Ignores are intended for false positives with dependency inference;
+    otherwise, simply leave off the dependency from the BUILD file.
     """
 
     alias = "dependencies"