Represent generated subtarget addresses as file names

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

@@ -86,21 +86,17 @@ async def map_first_party_modules_to_addresses() -> FirstPartyModuleToAddressMap
         unstripped_sources_per_explicit_target,
         stripped_sources_per_explicit_target,
     ):
-        only_one_source_file = len(unstripped_sources.snapshot.files) == 1
         for unstripped_f, stripped_f in zip(
             unstripped_sources.snapshot.files, stripped_sources.snapshot.files
         ):
             module = PythonModule.create_from_stripped_path(PurePath(stripped_f)).module
             if module in modules_to_addresses:
                 modules_with_multiple_owners.add(module)
             else:
-                modules_to_addresses[module] = (
-                    explicit_tgt.address
-                    if only_one_source_file
-                    else generate_subtarget_address(
-                        explicit_tgt.address, full_file_name=unstripped_f
-                    )
+                modules_to_addresses[module] = generate_subtarget_address(
+                    explicit_tgt.address, full_file_name=unstripped_f
                 )
+
     # Remove modules with ambiguous owners.
     for module in modules_with_multiple_owners:
         modules_to_addresses.pop(module)

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

@@ -120,7 +120,9 @@ def test_map_first_party_modules_to_addresses(self) -> None:
                     generated_base_target_name="util",
                 ),
                 "project_test.demo_test": Address(
-                    "tests/python/project_test/demo_test", target_name="demo_test"
+                    "tests/python/project_test/demo_test",
+                    target_name="__init__.py",
+                    generated_base_target_name="demo_test",
                 ),
             }
         )
@@ -198,7 +200,9 @@ def get_owner(module: str) -> Optional[Address]:
         self.create_file("source_root2/project/subdir/__init__.py")
         self.add_to_build_file("source_root2/project/subdir", "python_library()")
         assert get_owner("project.subdir") == Address(
-            "source_root2/project/subdir", target_name="subdir"
+            "source_root2/project/subdir",
+            target_name="__init__.py",
+            generated_base_target_name="subdir",
         )
 
         # Test a module with no owner (stdlib). This also sanity checks that we can handle when
@@ -209,4 +213,6 @@ def get_owner(module: str) -> Optional[Address]:
         # can handle when the module includes a symbol (like a class name) at the end.
         self.create_file("script.py")
         self.add_to_build_file("", "python_library(name='script')")
-        assert get_owner("script.Demo") == Address("", target_name="script")
+        assert get_owner("script.Demo") == Address(
+            "", target_name="script.py", generated_base_target_name="script"
+        )

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

@@ -97,6 +97,6 @@ def test_infer_python_dependencies(self) -> None:
             [
                 Address.parse("3rdparty/python:Django"),
                 Address("src/python", target_name="app.py", generated_base_target_name="python"),
-                Address("src/python/util", target_name="util"),
+                Address("src/python/util", target_name="dep.py", generated_base_target_name="util"),
             ]
         )

src/python/pants/build_graph/BUILD

@@ -2,3 +2,13 @@
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
 python_library()
+
+python_tests(
+  name="tests",
+  sources=["*_test.py", "!*_integration_test.py"],
+)
+
+python_integration_tests(
+  name='integration',
+  uses_pants_run=True,
+)

src/python/pants/build_graph/address.py

@@ -3,6 +3,7 @@
 
 import os
 import re
+from pathlib import PurePath
 from typing import Optional, Sequence, Tuple
 
 from pants.util.dirutil import fast_relpath, longest_dir_prefix
@@ -121,6 +122,10 @@ class Address:
     def parse(cls, spec: str, relative_to: str = "", subproject_roots=None) -> "Address":
         """Parses an address from its serialized form.
 
+        Note that this does not work properly with generated subtargets, e.g. the address
+        `helloworld/app.py`. We would not be able to calculate the `generated_base_target_name`, so
+        we treat this like a normal target address.
+
         :param spec: An address in string form <path>:<name>.
         :param relative_to: For sibling specs, ie: ':another_in_same_build_family', interprets
                             the missing spec_path part as `relative_to`.
@@ -133,43 +138,34 @@ def parse(cls, spec: str, relative_to: str = "", subproject_roots=None) -> "Addr
         return cls(spec_path, target_name)
 
     @classmethod
-    def sanitize_path(cls, path: str) -> str:
+    def validate_path(cls, path: str) -> None:
         # A root or relative spec is OK
         if path == "":
-            return path
-
+            return
         components = path.split(os.sep)
         if any(component in (".", "..", "") for component in components):
-            raise InvalidSpecPath(
-                "Address spec has un-normalized path part '{path}'".format(path=path)
-            )
+            raise InvalidSpecPath(f"Address spec has un-normalized path part '{path}'")
         if components[-1].startswith("BUILD"):
             raise InvalidSpecPath(
-                "Address spec path {path} has {trailing} as the last path part and BUILD is "
-                "a reserved file".format(path=path, trailing=components[-1])
+                f"Address spec path {path} has {components[-1]} as the last path part and BUILD is "
+                "a reserved file."
             )
         if os.path.isabs(path):
             raise InvalidSpecPath(
-                "Address spec has absolute path {path}; expected a path relative "
-                "to the build root.".format(path=path)
+                f"Address spec has absolute path {path}; expected a path relative to the build "
+                "root."
             )
-        return path
 
     @classmethod
     def check_target_name(cls, spec_path: str, name: str) -> None:
         if not name:
-            raise InvalidTargetName(
-                "Address spec {spec}:{name} has no name part".format(spec=spec_path, name=name)
-            )
+            raise InvalidTargetName(f"Address spec {spec_path}:{name} has no name part")
 
         banned_chars = BANNED_CHARS_IN_TARGET_NAME & set(name)
-
         if banned_chars:
             raise InvalidTargetName(
-                "banned chars found in target name",
-                "{banned_chars} not allowed in target name: {name}".format(
-                    banned_chars=banned_chars, name=name
-                ),
+                f"Banned chars found in target name. {banned_chars} not allowed in target "
+                f"name: {name}"
             )
 
     def __init__(
@@ -181,8 +177,9 @@ def __init__(
         :param generated_base_target_name: If this Address refers to a generated subtarget, this
                                            stores the target_name of the original base target.
         """
-        self._spec_path = self.sanitize_path(spec_path)
+        self.validate_path(spec_path)
         self.check_target_name(spec_path, target_name)
+        self._spec_path = spec_path
         self._target_name = target_name
         self.generated_base_target_name = generated_base_target_name
         self._hash = hash((self._spec_path, self._target_name, self.generated_base_target_name))
@@ -211,7 +208,11 @@ def spec(self) -> str:
 
         :API: public
         """
-        return f"{self._spec_path or '//'}:{self._target_name}"
+        prefix = "//" if not self._spec_path else ""
+        if self.generated_base_target_name:
+            path = os.path.join(self._spec_path, self._target_name)
+            return f"{prefix}{path}"
+        return f"{prefix}{self._spec_path}:{self._target_name}"
 
     @property
     def path_safe_spec(self) -> str:
@@ -225,14 +226,19 @@ def relative_spec(self) -> str:
         """
         :API: public
         """
-        return f":{self._target_name}"
+        # NB: It's possible for the target name of a generated subtarget to be in a subdirectory,
+        # e.g. 'subdir/f.txt'. When this happens, the relative spec is not safe.
+        if self.generated_base_target_name and PurePath(self.target_name).parent.as_posix() != ".":
+            return self.spec
+        prefix = ":" if not self.generated_base_target_name else ""
+        return f"{prefix}{self._target_name}"
 
     def reference(self, referencing_path: Optional[str] = None) -> str:
         """How to reference this address in a BUILD file.
 
         :API: public
         """
-        if referencing_path is not None and self._spec_path == referencing_path:
+        if referencing_path and self._spec_path == referencing_path:
             return self.relative_spec
         if os.path.basename(self._spec_path) != self._target_name:
             return self.spec
@@ -250,9 +256,6 @@ def __eq__(self, other):
     def __hash__(self):
         return self._hash
 
-    def __ne__(self, other):
-        return not self == other
-
     def __repr__(self) -> str:
         prefix = f"Address({self.spec_path}, {self.target_name}"
         return (
@@ -265,7 +268,11 @@ def __str__(self) -> str:
         return self.spec
 
     def __lt__(self, other):
-        return (self._spec_path, self._target_name) < (other._spec_path, other._target_name)
+        return (self._spec_path, self._target_name, self.generated_base_target_name) < (
+            other._spec_path,
+            other._target_name,
+            other.generated_base_target_name,
+        )
 
 
 class BuildFileAddress(Address):

tests/python/pants_test/build_graph/test_address.py → src/python/pants/build_graph/address_test.py

@@ -164,20 +164,63 @@ def test_address_equality() -> None:
     )
 
 
-class AddressTest(unittest.TestCase):
-    def assert_address(self, spec_path: str, target_name: str, address: Address) -> None:
-        self.assertEqual(spec_path, address.spec_path)
-        self.assertEqual(target_name, address.target_name)
-
-    def test_parse(self) -> None:
-        self.assert_address("a/b", "target", Address.parse("a/b:target"))
-        self.assert_address("a/b", "target", Address.parse("//a/b:target"))
-        self.assert_address("a/b", "b", Address.parse("a/b"))
-        self.assert_address("a/b", "b", Address.parse("//a/b"))
-        self.assert_address("a/b", "target", Address.parse(":target", relative_to="a/b"))
-        self.assert_address("", "target", Address.parse("//:target", relative_to="a/b"))
-        self.assert_address("", "target", Address.parse(":target"))
-        self.assert_address("a/b", "target", Address.parse(":target", relative_to="a/b"))
+def test_address_spec() -> None:
+    normal_addr = Address("a/b", "c")
+    assert normal_addr.spec == "a/b:c" == str(normal_addr) == normal_addr.reference()
+    assert normal_addr.relative_spec == ":c"
+    assert normal_addr.path_safe_spec == "a.b.c"
+
+    top_level_addr = Address("", "root")
+    assert top_level_addr.spec == "//:root" == str(top_level_addr) == top_level_addr.reference()
+    assert top_level_addr.relative_spec == ":root"
+    assert top_level_addr.path_safe_spec == ".root"
+
+    generated_addr = Address("a/b", "c.txt", generated_base_target_name="c")
+    assert generated_addr.spec == "a/b/c.txt" == str(generated_addr) == generated_addr.reference()
+    assert generated_addr.relative_spec == "c.txt"
+    assert generated_addr.path_safe_spec == "a.b.c.txt"
+
+    top_level_generated_addr = Address("", "root.txt", generated_base_target_name="root")
+    assert (
+        top_level_generated_addr.spec
+        == "//root.txt"
+        == str(top_level_generated_addr)
+        == top_level_generated_addr.reference()
+    )
+    assert top_level_generated_addr.relative_spec == "root.txt"
+    assert top_level_generated_addr.path_safe_spec == ".root.txt"
+
+    generated_subdirectory_addr = Address(
+        "a/b", "subdir/c.txt", generated_base_target_name="original"
+    )
+    assert (
+        generated_subdirectory_addr.spec
+        == "a/b/subdir/c.txt"
+        == str(generated_subdirectory_addr)
+        == generated_subdirectory_addr.reference()
+        # NB: A relative spec is not safe, so we use the full spec.
+        == generated_subdirectory_addr.relative_spec
+    )
+    assert generated_subdirectory_addr.path_safe_spec == "a.b.subdir.c.txt"
+
+
+def test_address_parse_method() -> None:
+    def assert_parsed(spec_path: str, target_name: str, address: Address) -> None:
+        assert spec_path == address.spec_path
+        assert target_name == address.target_name
+
+    assert_parsed("a/b", "target", Address.parse("a/b:target"))
+    assert_parsed("a/b", "target", Address.parse("//a/b:target"))
+    assert_parsed("a/b", "b", Address.parse("a/b"))
+    assert_parsed("a/b", "b", Address.parse("//a/b"))
+    assert_parsed("a/b", "target", Address.parse(":target", relative_to="a/b"))
+    assert_parsed("", "target", Address.parse("//:target", relative_to="a/b"))
+    assert_parsed("", "target", Address.parse(":target"))
+    assert_parsed("a/b", "target", Address.parse(":target", relative_to="a/b"))
+
+    # Do not attempt to parse generated subtargets, as we would have no way to find the
+    # generated_base_target_name.
+    assert_parsed("a/b/f.py", "f.py", Address.parse("a/b/f.py"))
 
 
 def test_build_file_address() -> None:
@@ -188,7 +231,8 @@ def test_build_file_address() -> None:
     assert bfa.to_address() == Address("dir", "example")
 
     generated_bfa = BuildFileAddress(
-        rel_path="dir/BUILD", target_name="example", generated_base_target_name="original"
+        rel_path="dir/BUILD", target_name="example.txt", generated_base_target_name="original"
     )
-    assert generated_bfa != BuildFileAddress(rel_path="dir/BUILD", target_name="example")
-    assert generated_bfa == Address("dir", "example", generated_base_target_name="original")
+    assert generated_bfa != BuildFileAddress(rel_path="dir/BUILD", target_name="example.txt")
+    assert generated_bfa == Address("dir", "example.txt", generated_base_target_name="original")
+    assert generated_bfa.spec == "dir/example.txt"