Add DepsTraversalBehavior(Enum) and some docstrings (pantsbuild#19387)

This addresses feedback from
pantsbuild#19306 (review)

The `ShouldTraverseDepsPredicate` returned a bool before. But, the bool
was sometimes unclear, so this adds an enum to make the code more
readable.

NB: The values of the enum are not important.

src/python/pants/core/goals/package.py

@@ -21,6 +21,7 @@
     AllTargets,
     AsyncFieldMixin,
     Dependencies,
+    DepsTraversalBehavior,
     FieldSet,
     FieldSetsPerTarget,
     FieldSetsPerTargetRequest,
@@ -187,9 +188,19 @@ async def package_asset(workspace: Workspace, dist_dir: DistDir) -> Package:
 
 @dataclass(frozen=True)
 class TraverseIfNotPackageTarget(ShouldTraverseDepsPredicate):
+    """This predicate stops dep traversal after package targets.
+
+    When traversing deps, such as when collecting a list of transitive deps,
+    this predicate effectively turns any package targets into graph leaf nodes.
+    The package targets are included, but the deps of package targets are not.
+
+    Also, this excludes dependencies from any SpecialCasedDependencies fields,
+    which mirrors the behavior of the default predicate: TraverseIfDependenciesField.
+    """
+
     package_field_set_types: FrozenOrderedSet[PackageFieldSet]
     roots: FrozenOrderedSet[Address]
-    always_traverse_roots: bool = True  # traverse roots even if they are package targets
+    always_traverse_roots: bool  # traverse roots even if they are package targets
 
     def __init__(
         self,
@@ -203,16 +214,18 @@ def __init__(
         object.__setattr__(self, "always_traverse_roots", always_traverse_roots)
         super().__init__()
 
-    def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
+    def __call__(
+        self, target: Target, field: Dependencies | SpecialCasedDependencies
+    ) -> DepsTraversalBehavior:
         if isinstance(field, SpecialCasedDependencies):
-            return False
+            return DepsTraversalBehavior.EXCLUDE
         if self.always_traverse_roots and target.address in self.roots:
-            return True
-        for field_set_type in self.package_field_set_types:
-            if field_set_type.is_applicable(target):
-                # False means do not traverse dependencies of this target
-                return False
-        return True
+            return DepsTraversalBehavior.INCLUDE
+        if any(
+            field_set_type.is_applicable(target) for field_set_type in self.package_field_set_types
+        ):
+            return DepsTraversalBehavior.EXCLUDE
+        return DepsTraversalBehavior.INCLUDE
 
 
 def rules():

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

@@ -46,6 +46,7 @@
     CoarsenedTargetsRequest,
     Dependencies,
     DependenciesRequest,
+    DepsTraversalBehavior,
     ExplicitlyProvidedDependencies,
     ExplicitlyProvidedDependenciesRequest,
     Field,
@@ -1322,7 +1323,7 @@ async def resolve_dependencies(
     # This predicate allows the dep graph to ignore dependencies of selected targets
     # including any explicit deps and any inferred deps.
     # For example, to avoid traversing the deps of package targets.
-    if not request.should_traverse_deps_predicate(tgt, request.field):
+    if request.should_traverse_deps_predicate(tgt, request.field) == DepsTraversalBehavior.EXCLUDE:
         return Addresses([])
 
     try:
@@ -1412,7 +1413,7 @@ async def resolve_dependencies(
         field
         for field in tgt.field_values.values()
         if isinstance(field, SpecialCasedDependencies)
-        and request.should_traverse_deps_predicate(tgt, field)
+        and request.should_traverse_deps_predicate(tgt, field) == DepsTraversalBehavior.INCLUDE
     )
     # We can't use the normal `Get(Addresses, UnparsedAddressInputs)` due to a graph cycle.
     special_cased = await MultiGet(

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

@@ -40,6 +40,7 @@
     CoarsenedTargets,
     Dependencies,
     DependenciesRequest,
+    DepsTraversalBehavior,
     ExplicitlyProvidedDependencies,
     Field,
     FieldDefaultFactoryRequest,
@@ -420,8 +421,12 @@ def get_target(name: str) -> Target:
     assert direct_deps == Targets([d1, d2, d3, d4])
 
     class SkipDepsTagOrTraverse(ShouldTraverseDepsPredicate):
-        def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
-            return "skip_deps" not in (target[Tags].value or [])
+        def __call__(
+            self, target: Target, field: Dependencies | SpecialCasedDependencies
+        ) -> DepsTraversalBehavior:
+            if "skip_deps" in (target[Tags].value or []):
+                return DepsTraversalBehavior.EXCLUDE
+            return DepsTraversalBehavior.INCLUDE
 
     predicate = SkipDepsTagOrTraverse()
     # Assert the class is frozen even though it was not decorated with @dataclass(frozen=True)

src/python/pants/engine/target.py

@@ -714,6 +714,19 @@ def expect_single(self) -> Target:
         return self[0]
 
 
+class DepsTraversalBehavior(Enum):
+    """The return value for ShouldTraverseDepsPredicate.
+
+    NB: This only indicates whether to traverse the deps of a target;
+    It does not control the inclusion of the target itself (though that
+    might be added in the future). By the time the predicate is called,
+    the target itself was already included.
+    """
+
+    INCLUDE = "include"
+    EXCLUDE = "exclude"
+
+
 @dataclass(frozen=True)
 class ShouldTraverseDepsPredicate(metaclass=ABCMeta):
     """This callable determines whether to traverse through deps of a given Target + Field.
@@ -733,15 +746,17 @@ class ShouldTraverseDepsPredicate(metaclass=ABCMeta):
     # That is extremely important because two predicates with different implementations but the same data
     # (or no data) need to have different hashes and compare unequal.
     _callable: Callable[
-        [Any, Target, Dependencies | SpecialCasedDependencies], bool
+        [Any, Target, Dependencies | SpecialCasedDependencies], DepsTraversalBehavior
     ] = dataclasses.field(init=False, repr=False)
 
     def __post_init__(self):
         object.__setattr__(self, "_callable", type(self).__call__)
 
     @abstractmethod
-    def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
-        """If this predicate returns false, then the target's field's deps will be ignored."""
+    def __call__(
+        self, target: Target, field: Dependencies | SpecialCasedDependencies
+    ) -> DepsTraversalBehavior:
+        """This predicate decides when to INCLUDE or EXCLUDE the target's field's deps."""
 
 
 class TraverseIfDependenciesField(ShouldTraverseDepsPredicate):
@@ -751,8 +766,12 @@ class TraverseIfDependenciesField(ShouldTraverseDepsPredicate):
     subclasses of Dependencies.
     """
 
-    def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
-        return isinstance(field, Dependencies)
+    def __call__(
+        self, target: Target, field: Dependencies | SpecialCasedDependencies
+    ) -> DepsTraversalBehavior:
+        if isinstance(field, Dependencies):
+            return DepsTraversalBehavior.INCLUDE
+        return DepsTraversalBehavior.EXCLUDE
 
 
 class AlwaysTraverseDeps(ShouldTraverseDepsPredicate):
@@ -761,8 +780,10 @@ class AlwaysTraverseDeps(ShouldTraverseDepsPredicate):
     This includes deps from fields like SpecialCasedDependencies which are ignored in most cases.
     """
 
-    def __call__(self, target: Target, field: Dependencies | SpecialCasedDependencies) -> bool:
-        return True
+    def __call__(
+        self, target: Target, field: Dependencies | SpecialCasedDependencies
+    ) -> DepsTraversalBehavior:
+        return DepsTraversalBehavior.INCLUDE
 
 
 class CoarsenedTarget(EngineAwareParameter):