Fix secondary ownership warning semantics (Cherry-pick of #19191) (#1…

…9224)

Fixes #19174 by boiling the criteria to warn down to:

1. The Address/Target/FieldSet is matched due __only__ to
`SecondaryOwnerMixin`
2. It was matched due to a file spec (literal or glob)
3. It wasn't matched due ton address spec
- This is just to handle the corner case where it was matched due to
file and address

In order to facilitate 1., `Owner` and `find_owner` have been
(temporarily) modified. Additionally, the rule that goes from
`RawSpecsWithOnlyFileOwners` -> `Addresses` has been split to return
`Owners`, with an additional rule added to complete the graph (`Owners`
-> `Addresses`).

Then 2 and 3 are facilitated by just requesting the addresses based on
the include specs.

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

@@ -11,7 +11,7 @@
 import os.path
 from dataclasses import dataclass
 from pathlib import PurePath
-from typing import Any, Iterable, Iterator, NamedTuple, Sequence, Type, cast
+from typing import Any, Iterable, Iterator, NamedTuple, NewType, Sequence, Type, cast
 
 from pants.base.deprecated import warn_or_error
 from pants.base.specs import AncestorGlobSpec, RawSpecsWithoutFileOwners, RecursiveGlobSpec
@@ -909,7 +909,15 @@ class OwnersRequest:
     match_if_owning_build_file_included_in_sources: bool = False
 
 
-class Owners(Collection[Address]):
+# NB: This was changed from:
+# class Owners(Collection[Address]):
+#     pass
+# In https://github.com/pantsbuild/pants/pull/19191 to facilitate surgical warning of deprecation
+# of SecondaryOwnerMixin. After the Deprecation ends, it can be changed back.
+IsPrimary = NewType("IsPrimary", bool)
+
+
+class Owners(FrozenDict[Address, IsPrimary]):
     pass
 
 
@@ -964,7 +972,7 @@ def create_live_and_deleted_gets(
     )
     live_candidate_tgts, deleted_candidate_tgts = await MultiGet(live_get, deleted_get)
 
-    matching_addresses: OrderedSet[Address] = OrderedSet()
+    result = {}
     unmatched_sources = set(owners_request.sources)
     for live in (True, False):
         candidate_tgts: Sequence[Target]
@@ -989,6 +997,8 @@ def create_live_and_deleted_gets(
             matching_files = set(
                 candidate_tgt.get(SourcesField).filespec_matcher.matches(list(sources_set))
             )
+            is_primary = bool(matching_files)
+
             # Also consider secondary ownership, meaning it's not a `SourcesField` field with
             # primary ownership, but the target still should match the file. We can't use
             # `tgt.get()` because this is a mixin, and there technically may be >1 field.
@@ -999,16 +1009,17 @@ def create_live_and_deleted_gets(
             )
             for secondary_owner_field in secondary_owner_fields:
                 matching_files.update(
-                    *secondary_owner_field.filespec_matcher.matches(list(sources_set))
+                    secondary_owner_field.filespec_matcher.matches(list(sources_set))
                 )
+
             if not matching_files and not (
                 owners_request.match_if_owning_build_file_included_in_sources
                 and bfa.rel_path in sources_set
             ):
                 continue
 
             unmatched_sources -= matching_files
-            matching_addresses.add(candidate_tgt.address)
+            result[candidate_tgt.address] = IsPrimary(is_primary)
 
     if (
         unmatched_sources
@@ -1018,7 +1029,7 @@ def create_live_and_deleted_gets(
             [PurePath(path) for path in unmatched_sources], owners_request.owners_not_found_behavior
         )
 
-    return Owners(matching_addresses)
+    return Owners(result)
 
 
 # -----------------------------------------------------------------------------------------------

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

@@ -3,6 +3,7 @@
 
 from __future__ import annotations
 
+import collections.abc
 import dataclasses
 import itertools
 import logging
@@ -46,7 +47,6 @@
     FilteredTargets,
     NoApplicableTargetsBehavior,
     RegisteredTargetTypes,
-    SecondaryOwnerMixin,
     SourcesField,
     SourcesPaths,
     SourcesPathsRequest,
@@ -227,7 +227,7 @@ def valid_tgt(
 @rule(_masked_types=[EnvironmentName])
 async def addresses_from_raw_specs_with_only_file_owners(
     specs: RawSpecsWithOnlyFileOwners,
-) -> Addresses:
+) -> Owners:
     """Find the owner(s) for each spec."""
     paths_per_include = await MultiGet(
         Get(Paths, PathGlobs, specs.path_globs_for_spec(spec)) for spec in specs.all_specs()
@@ -242,6 +242,11 @@ async def addresses_from_raw_specs_with_only_file_owners(
             match_if_owning_build_file_included_in_sources=False,
         ),
     )
+    return owners
+
+
+@rule(_masked_types=[EnvironmentName])
+async def addresses_from_owners(owners: Owners) -> Addresses:
     return Addresses(sorted(owners))
 
 
@@ -482,6 +487,48 @@ def __init__(
         )
 
 
+# NB: Remove when SecondaryOwnerMixin is removed
+def _maybe_warn_deprecated_secondary_owner_semantics(
+    addresses_from_nonfile_specs: Addresses,
+    owners_from_filespecs: Owners,
+    matched_addresses: collections.abc.Set[Address],
+):
+    """Warn about deprecated semantics of implicitly referring to a target through "Secondary
+    Ownership".
+
+    E.g. If there's a `pex_binary` whose entry point is `foo.py`, using `foo.py` as a spec to mean
+    "package the pex binary" is deprecated, and the caller should specify the binary directly.
+
+    This shouldn't warn if both the primary and secondary owner are in the specs (which is common
+    with specs like `::` or `dir:`).
+    """
+    problematic_target_specs = {
+        address.spec
+        for address in matched_addresses
+        if address in owners_from_filespecs
+        and not owners_from_filespecs[address]
+        and address not in addresses_from_nonfile_specs
+    }
+
+    if problematic_target_specs:
+        warn_or_error(
+            removal_version="2.18.0.dev1",
+            entity=softwrap(
+                """
+                indirectly referring to a target by using a corresponding file argument, when the
+                target owning the file isn't applicable
+                """
+            ),
+            hint=softwrap(
+                f"""
+                Refer to the following targets by their addresses:
+
+                {bullet_list(sorted(problematic_target_specs))}
+                """
+            ),
+        )
+
+
 @rule
 async def find_valid_field_sets_for_target_roots(
     request: TargetRootsToFieldSetsRequest,
@@ -530,33 +577,25 @@ async def find_valid_field_sets_for_target_roots(
         ):
             logger.warning(str(no_applicable_exception))
 
-    secondary_owner_targets = set()
-    specified_literal_addresses = {
-        address_literal.to_address() for address_literal in specs.includes.address_literals
-    }
-    for tgt, field_sets in targets_to_applicable_field_sets.items():
-        is_secondary = any(
-            isinstance(field, SecondaryOwnerMixin) for field in tgt.field_values.values()
-        )
-        is_explicitly_specified = tgt.address in specified_literal_addresses
-        if is_secondary and not is_explicitly_specified:
-            secondary_owner_targets.add(tgt)
-    if secondary_owner_targets:
-        warn_or_error(
-            removal_version="2.18.0.dev0",
-            entity=softwrap(
-                """
-                indirectly referring to a target by using a corresponding file argument, when the
-                target owning the file isn't applicable
-                """
-            ),
-            hint=softwrap(
-                f"""
-                Refer to the following targets by their addresses:
-
-                {bullet_list(sorted(tgt.address.spec for tgt in secondary_owner_targets))}
-                """
+    # NB: Remove when SecondaryOwnerMixin is removed
+    if targets_to_applicable_field_sets:
+        _maybe_warn_deprecated_secondary_owner_semantics(
+            # NB: All of these should be memoized, so it's not inappropriate to request simply for warning sake.
+            *(
+                await MultiGet(
+                    Get(
+                        Addresses,
+                        RawSpecsWithoutFileOwners,
+                        RawSpecsWithoutFileOwners.from_raw_specs(specs.includes),
+                    ),
+                    Get(
+                        Owners,
+                        RawSpecsWithOnlyFileOwners,
+                        RawSpecsWithOnlyFileOwners.from_raw_specs(specs.includes),
+                    ),
+                )
             ),
+            {tgt.address for tgt in targets_to_applicable_field_sets},
         )
 
     if request.num_shards > 0:

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

@@ -1132,3 +1132,7 @@ def run_rule(specs: Iterable[Spec]):
     assert len(caplog.records) == 1
     assert "secondary1" not in caplog.text
     assert "secondary2" in caplog.text
+
+    result = run_rule([RecursiveGlobSpec("")])
+    assert len(caplog.records) == 0
+    assert result.mapping