Defer the recursive propagation of suite traits until after filtering (#820)

This PR includes some optimizations to the testing library's planning
logic (in `Runner.Plan`) to avoid unnecessary work in common cases, and
remove unnecessary nodes from the test graph once they're no longer
needed.

### Motivation

The testing library's planning phase includes taking all
runtime-discovered `Test` instances and organizing them into a tree-like
data structure using the internal `Graph` type. It traverses this graph
several times to do things like only run certain tests the user
indicated (by applying a filter), propagate eligible traits from suites
to their children (by recursively applying traits), and implement other
features.

Currently, the step of recursively propagating eligible suite traits to
their children must occur _before_ applying a test filter. This is
because a test filter may require knowledge of trait information --
specifically, the `.tags` and `.hidden` traits. Even though test
filtering only needs these specific traits, the planning phase currently
propagates _all_ recursive suite traits down to _all_ tests, even though
filtering may end up removing the majority of tests from the graph
entirely. This can lead to wasted effort for any tests which were
filtered out, and I anticipate this may become more pronounced in the
future if/when we enhance the trait system to support things like
consolidating same-typed traits with children.

Trait propagation is done by inserting all inherited traits from the
parent at the beginning of the child's trait list (to ensure
deterministic ordering), so this requires shifting the elements in an
array. And this work is only needed if the test filter being applied
includes a query of tests' `.tags` trait. (The `.hidden` trait is
semantically simpler and easier to handle: it's recursive, but it's a
simple Boolean flag which cannot be overridden by children, so it
doesn't need to be "merged" with children.)

There is an opportunity to optimize this flow and completely skip trait
propagation in the common case where a test filter doesn't care about
tags, and allow propagation to happen after the filtering stage.
Additionally, to reduce memory usage and simplify the graph for easier
debugging, the filtering logic could be modified to "prune" the graph by
removing branches which have zero children.

### Modifications

- Add a property to `Configuration.TestFilter` indicating whether the
filter requires knowledge of trait information.
- Consult this property when applying a filter to a test graph. When
necessary, temporarily convert the passed-in graph to a modified graph
and recursively propagate tags to it.
- Modify `Configuration.TestFilter` to prune unnecessary children from
the graph before finishing.
- Modify planning logic in `Runner.Plan` to move the recursive trait
propagation step to after filering.

### Checklist:

- [x] Code and documentation should follow the style of the [Style
Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md).
- [x] If public symbols are renamed or modified, DocC references should
be updated.

Author: stmontgomery

Sources/Testing/Running/Configuration.TestFilter.swift

@@ -191,7 +191,7 @@ extension Configuration.TestFilter {
 extension Configuration.TestFilter {
   /// An enumeration which represents filtering logic to be applied to a test
   /// graph.
-  fileprivate enum Operation: Sendable {
+  fileprivate enum Operation<Item>: Sendable where Item: _FilterableItem {
     /// A filter operation which has no effect.
     ///
     /// All tests are allowed when this operation is applied.
@@ -211,7 +211,7 @@ extension Configuration.TestFilter {
     /// - Parameters:
     ///   - predicate: The function to predicate tests against.
     ///   - membership: How to interpret the result when predicating tests.
-    case function(_ predicate: @Sendable (borrowing Test) -> Bool, membership: Membership)
+    case function(_ predicate: @Sendable (borrowing Item) -> Bool, membership: Membership)
 
     /// A filter operation which is a combination of other operations.
     ///
@@ -235,34 +235,31 @@ extension Configuration.TestFilter.Kind {
   ///   test filter kind. One example is the creation of a `Regex` from a
   ///   `.pattern` kind: if the pattern is not a valid regular expression, an
   ///   error will be thrown.
-  var operation: Configuration.TestFilter.Operation {
-    get throws {
-      switch self {
-      case .unfiltered:
-        return .unfiltered
-      case let .testIDs(testIDs, membership):
-        return .precomputed(Test.ID.Selection(testIDs: testIDs), membership: membership)
-      case let .tags(tags, anyOf, membership):
-        return .function({ test in
-          if anyOf {
-            !test.tags.isDisjoint(with: tags) // .intersects()
-          } else {
-            test.tags.isSuperset(of: tags)
-          }
-        }, membership: membership)
-      case let .patterns(patterns, membership):
-        guard #available(_regexAPI, *) else {
-          throw SystemError(description: "Filtering by regular expression matching is unavailable")
-        }
-
-        nonisolated(unsafe) let regexes = try patterns.map(Regex.init)
-        return .function({ test in
-          let id = String(describing: test.id)
-          return regexes.contains { id.contains($0) }
-        }, membership: membership)
-      case let .combination(lhs, rhs, op):
-        return try .combination(lhs.operation, rhs.operation, op)
+  func operation<T>(itemType: T.Type = T.self) throws -> Configuration.TestFilter.Operation<T> where T: _FilterableItem {
+    switch self {
+    case .unfiltered:
+      return .unfiltered
+    case let .testIDs(testIDs, membership):
+      return .precomputed(Test.ID.Selection(testIDs: testIDs), membership: membership)
+    case let .tags(tags, anyOf, membership):
+      let predicate: @Sendable (borrowing T) -> Bool = if anyOf {
+        { !$0.tags.isDisjoint(with: tags) /* .intersects() */ }
+      } else {
+        { $0.tags.isSuperset(of: tags) }
+      }
+      return .function(predicate, membership: membership)
+    case let .patterns(patterns, membership):
+      guard #available(_regexAPI, *) else {
+        throw SystemError(description: "Filtering by regular expression matching is unavailable")
       }
+
+      nonisolated(unsafe) let regexes = try patterns.map(Regex.init)
+      return .function({ item in
+        let id = String(describing: item.test.id)
+        return regexes.contains { id.contains($0) }
+      }, membership: membership)
+    case let .combination(lhs, rhs, op):
+      return try .combination(lhs.operation(), rhs.operation(), op)
     }
   }
 }
@@ -278,20 +275,25 @@ extension Configuration.TestFilter.Operation {
   ///
   /// This function provides the bulk of the implementation of
   /// ``Configuration/TestFilter/apply(to:)``.
-  fileprivate func apply(to testGraph: Graph<String, Test?>) -> Graph<String, Test?> {
+  fileprivate func apply(to testGraph: Graph<String, Item?>) -> Graph<String, Item?> {
     switch self {
     case .unfiltered:
       return testGraph
     case let .precomputed(selection, membership):
-      return testGraph.mapValues { _, test in
-        guard let test else {
-          return nil
+      switch membership {
+      case .including:
+        return testGraph.mapValues { _, item in
+          guard let item else {
+            return nil
+          }
+          return selection.contains(item.test) ? item : nil
         }
-        return switch membership {
-        case .including:
-          selection.contains(test) ? test : nil
-        case .excluding:
-          !selection.contains(test, inferAncestors: false) ? test : nil
+      case .excluding:
+        return testGraph.mapValues { _, item in
+          guard let item else {
+            return nil
+          }
+          return !selection.contains(item.test, inferAncestors: false) ? item : nil
         }
       }
     case let .function(function, membership):
@@ -307,15 +309,15 @@ extension Configuration.TestFilter.Operation {
       let testIDs = testGraph
         .compactMap(\.value).lazy
         .filter(function)
-        .map(\.id)
+        .map(\.test.id)
       let selection = Test.ID.Selection(testIDs: testIDs)
       return Self.precomputed(selection, membership: membership).apply(to: testGraph)
     case let .combination(lhs, rhs, op):
       return zip(
         lhs.apply(to: testGraph),
         rhs.apply(to: testGraph)
       ).mapValues { _, value in
-        op.functionValue(value.0, value.1)
+        op(value.0, value.1)
       }
     }
   }
@@ -330,20 +332,81 @@ extension Configuration.TestFilter {
   ///
   /// - Returns: A copy of `testGraph` with filtered tests replaced with `nil`.
   func apply(to testGraph: Graph<String, Test?>) throws -> Graph<String, Test?> {
-    var result = try _kind.operation.apply(to: testGraph)
-
-    // After performing the test function, run through one more time and remove
-    // hidden tests. (Note that this property's value is not recursively set on
-    // combined test filters. It is only consulted on the outermost call to
-    // apply(to:), not in _apply(to:).)
-    if !includeHiddenTests {
-      result = result.mapValues { _, test in
-        (test?.isHidden == true) ? nil : test
-      }
+    var result: Graph<String, Test?>
+
+    if _kind.requiresTraitPropagation {
+      // Convert the specified test graph to a graph of filter items temporarily
+      // while performing filtering, and apply inheritance for the properties
+      // which are relevant when performing filtering (e.g. tags).
+      var filterItemGraph = testGraph.mapValues { $1.map(FilterItem.init(test:)) }
+      _recursivelyApplyFilterProperties(to: &filterItemGraph)
+
+      result = try _kind.operation().apply(to: filterItemGraph)
+        .mapValues { $1?.test }
+    } else {
+      result = try _kind.operation().apply(to: testGraph)
     }
 
+    // After filtering, run through one more time and prune the test graph to
+    // remove any unnecessary nodes, since that reduces work in later stages of
+    // planning.
+    //
+    // If `includeHiddenTests` is false, this will also remove any nodes
+    // representing hidden tests. (Note that the value of the
+    // `includeHiddenTests` property is not recursively set on combined test
+    // filters. It is only consulted on the outermost call to apply(to:), not in
+    // _apply(to:).)
+    _recursivelyPruneTestGraph(&result)
+
     return result
   }
+
+  /// Recursively apply filtering-related properties from test suites to their
+  /// children in a graph.
+  ///
+  /// - Parameters:
+  ///   - graph: The graph of filter items to modify.
+  ///   - tags: Tags from the parent of `graph` which `graph` should inherit.
+  private func _recursivelyApplyFilterProperties(to graph: inout Graph<String, FilterItem?>, tags: Set<Tag> = []) {
+    var tags = tags
+    if let item = graph.value {
+      tags.formUnion(item.tags)
+      graph.value?.tags = tags
+    }
+
+    for (key, var childGraph) in graph.children {
+      _recursivelyApplyFilterProperties(to: &childGraph, tags: tags)
+      graph.children[key] = childGraph
+    }
+  }
+
+  /// Recursively prune a test graph to remove unnecessary nodes.
+  ///
+  /// - Parameters:
+  ///   - testGraph: The graph of tests to modify.
+  private func _recursivelyPruneTestGraph(_ graph: inout Graph<String, Test?>) {
+    // The recursive function. This is structured as a distinct function to
+    // ensure that the root node itself is always preserved (despite its value
+    // being `nil`).
+    func pruneGraph(_ graph: Graph<String, Test?>) -> Graph<String, Test?>? {
+      if !includeHiddenTests, let test = graph.value, test.isHidden {
+        return nil
+      }
+
+      var graph = graph
+      for (key, childGraph) in graph.children {
+        graph.children[key] = pruneGraph(childGraph)
+      }
+      if graph.value == nil && graph.children.isEmpty {
+        return nil
+      }
+      return graph
+    }
+
+    for (key, childGraph) in graph.children {
+      graph.children[key] = pruneGraph(childGraph)
+    }
+  }
 }
 
 // MARK: - Combining
@@ -364,21 +427,23 @@ extension Configuration.TestFilter {
     /// This operator is equivalent to `||`.
     case or
 
-    /// The equivalent of this instance as a callable function.
-    fileprivate var functionValue: @Sendable (Test?, Test?) -> Test? {
+    /// Evaluate this combination operator with two optional operands.
+    ///
+    /// - Parameters:
+    ///   - lhs: The left-hand argument
+    ///   - rhs: The right-hand argument.
+    ///
+    /// - Returns: The combined result of evaluating this operator.
+    fileprivate func callAsFunction<T>(_ lhs: T?, _ rhs: T?) -> T? where T: _FilterableItem {
       switch self {
       case .and:
-        return { lhs, rhs in
-          if lhs != nil && rhs != nil {
-            lhs
-          } else {
-            nil
-          }
+        if lhs != nil && rhs != nil {
+          lhs
+        } else {
+          nil
         }
       case .or:
-        return { lhs, rhs in
-          lhs ?? rhs
-        }
+        lhs ?? rhs
       }
     }
   }
@@ -422,3 +487,60 @@ extension Configuration.TestFilter {
     self = combining(with: other, using: op)
   }
 }
+
+// MARK: - Filterable types
+
+extension Configuration.TestFilter.Kind {
+  /// Whether this kind of test filter requires knowledge of test traits.
+  ///
+  /// If the value of this property is `true`, the values of a test graph must
+  /// be converted to `FilterItem` and have trait information recursively
+  /// propagated before the filter can be applied, or else the results may be
+  /// inaccurate. This facilitates a performance optimization where trait
+  /// propagation can be skipped for filters which don't require such knowledge.
+  fileprivate var requiresTraitPropagation: Bool {
+    switch self {
+    case .unfiltered,
+         .testIDs,
+         .patterns:
+      false
+    case .tags:
+      true
+    case let .combination(lhs, rhs, _):
+      lhs.requiresTraitPropagation || rhs.requiresTraitPropagation
+    }
+  }
+}
+
+/// A protocol representing a value which can be filtered using
+/// ``Configuration/TestFilter-swift.struct``.
+private protocol _FilterableItem {
+  /// The test this item represents.
+  var test: Test { get }
+
+  /// The complete set of tags for ``test``, including those inherited from
+  /// containing suites.
+  var tags: Set<Tag> { get }
+}
+
+extension Test: _FilterableItem {
+  var test: Test {
+    self
+  }
+}
+
+/// An item representing a test and its filtering-related properties.
+///
+/// Instances of this type are needed when applying a test graph to a kind of
+/// filter for which the value of the `requiresFilterItemConversion` property
+/// is `true`.
+fileprivate struct FilterItem: _FilterableItem {
+  var test: Test
+
+  var tags: Set<Tag>
+
+  init(test: Test) {
+    self.test = test
+    self.tags = test.tags
+  }
+}

Sources/Testing/Running/Runner.Plan.swift

@@ -141,8 +141,8 @@ extension Runner.Plan {
   /// The traits in `testGraph.value?.traits` are added to each node in
   /// `testGraph`, and then this function is called recursively on each child
   /// node.
-  private static func _recursivelyApplyTraits(_ parentTraits: [any Trait] = [], to testGraph: inout Graph<String, Test?>) {
-    let traits: [any SuiteTrait] = (parentTraits + (testGraph.value?.traits ?? [])).lazy
+  private static func _recursivelyApplyTraits(_ parentTraits: [any SuiteTrait] = [], to testGraph: inout Graph<String, Test?>) {
+    let traits: [any SuiteTrait] = parentTraits + (testGraph.value?.traits ?? []).lazy
       .compactMap { $0 as? any SuiteTrait }
       .filter(\.isRecursive)
 
@@ -181,11 +181,6 @@ extension Runner.Plan {
       actionGraph.insertValue(runAction, at: idComponents, intermediateValue: runAction)
     }
 
-    // Ensure the trait lists are complete for all nested tests. (Make sure to
-    // do this before we start calling configuration.testFilter or prepare(for:)
-    // or we'll miss the recursively-added traits.)
-    _recursivelyApplyTraits(to: &testGraph)
-
     // Remove any tests that should be filtered out per the runner's
     // configuration. The action graph is not modified here: actions that lose
     // their corresponding tests are effectively filtered out by the call to
@@ -203,6 +198,16 @@ extension Runner.Plan {
       // and that is already guarded earlier in the SwiftPM entry point.
     }
 
+    // Recursively apply all recursive suite traits to children.
+    //
+    // This must be done _before_ calling `prepare(for:)` on the traits below.
+    // It is safe to do this _after_ filtering the test graph since filtering
+    // internally propagates information about traits which are needed to
+    // correctly evaluate the filter. It's also more efficient, since it avoids
+    // needlessly applying non-filtering related traits to tests which might be
+    // filtered out.
+    _recursivelyApplyTraits(to: &testGraph)
+
     // For each test value, determine the appropriate action for it.
     //
     // FIXME: Parallelize this work. Calling `prepare(...)` on all traits and

Tests/TestingTests/PlanTests.swift

@@ -421,6 +421,14 @@ struct PlanTests {
     #expect(plan.stepGraph.subgraph(at: typeInfo.fullyQualifiedNameComponents + CollectionOfOne("reserved1(reserved2:)")) != nil)
   }
 
+  @Test("Nodes for filtered-out tests are removed from the step graph")
+  func filteringPrunesGraph() async throws {
+    let plan = await Runner.Plan(selecting: SendableTests.self)
+    #expect(plan.stepGraph.children.count == 1)
+    let moduleGraph = try #require(plan.stepGraph.children.values.first)
+    #expect(moduleGraph.children.count == 1)
+  }
+
 #if !SWT_NO_SNAPSHOT_TYPES
   @Test("Test cases of a disabled test are not evaluated")
   func disabledTestCases() async throws {