Rework ActiveSyntaxRewriter to operate on top of ConfiguredRegions

The operation that removes all inactive (and unparsed) regions from a
syntax tree is a two-pass algorithm. For clients that have already created
a ConfiguredRegions instance, let that serve as the first pass.
ConfiguredRegions now tracks the active clauses of any evaluated `#if`s,
and the ActiveSyntaxRewriter uses that information to guide its traversal
and rewrite rather than re-evaluating all of the clauses.

This required reworking the way in which we handle recursion in
ActiveSyntaxRewriter. Before, we were rewriting nodes "outside-in", but
doing so broke node identity and, therefore, lookup of the active #if
clauses. Instead, make sure we rewrite from the leaves up to the root.

Clients still have the option of removing inactive/unparsed code from a
syntax node given just a build configuration, in which case we'll first
create a ConfiguredRegions.

Author: DougGregor

Sources/SwiftIfConfig/ActiveSyntaxRewriter.swift

@@ -52,27 +52,64 @@ extension SyntaxProtocol {
     in configuration: some BuildConfiguration,
     retainFeatureCheckIfConfigs: Bool
   ) -> (result: Syntax, diagnostics: [Diagnostic]) {
-    // First pass: Find all of the active clauses for the #ifs we need to
-    // visit, along with any diagnostics produced along the way. This process
-    // does not change the tree in any way.
-    let visitor = ActiveSyntaxVisitor(viewMode: .sourceAccurate, configuration: configuration)
-    visitor.walk(self)
+    let configuredRegions = configuredRegions(in: configuration)
+    return (
+      result: configuredRegions.removingInactive(
+        from: self,
+        retainFeatureCheckIfConfigs: retainFeatureCheckIfConfigs
+      ),
+      diagnostics: configuredRegions.diagnostics
+    )
+  }
+}
 
-    // If there were no active clauses to visit, we're done!
-    if !visitor.visitedAnyIfClauses {
-      return (Syntax(self), visitor.diagnostics)
+extension ConfiguredRegions {
+  /// Produce a copy of some syntax node in the configured region that removes
+  /// all syntax regions that are inactive according to the build configuration,
+  /// leaving only the code that is active within that build configuration.
+  ///
+  /// If there are errors in the conditions of any configuration
+  /// clauses, e.g., `#if FOO > 10`, then the condition will be
+  /// considered to have failed and the clauses's elements will be
+  /// removed.
+  /// - Parameters:
+  ///   - node: the stnrax node from which inactive regions will be removed.
+  /// - Returns: the syntax node with all inactive regions removed.
+  public func removingInactive(from node: some SyntaxProtocol) -> Syntax {
+    return removingInactive(from: node, retainFeatureCheckIfConfigs: false)
+  }
+
+  /// Produce a copy of some syntax node in the configured region that removes
+  /// all syntax regions that are inactive according to the build configuration,
+  /// leaving only the code that is active within that build configuration.
+  ///
+  /// If there are errors in the conditions of any configuration
+  /// clauses, e.g., `#if FOO > 10`, then the condition will be
+  /// considered to have failed and the clauses's elements will be
+  /// removed.
+  /// - Parameters:
+  ///   - node: the stnrax node from which inactive regions will be removed.
+  ///   - retainFeatureCheckIfConfigs: whether to retain `#if` blocks involving
+  ///     compiler version checks (e.g., `compiler(>=6.0)`) and `$`-based
+  ///     feature checks.
+  /// - Returns: the syntax node with all inactive regions removed.
+  @_spi(Compiler)
+  public func removingInactive(
+    from node: some SyntaxProtocol,
+    retainFeatureCheckIfConfigs: Bool
+  ) -> Syntax {
+    // If there are no inactive regions, there's nothing to do.
+    if regions.isEmpty {
+      return Syntax(node)
     }
 
-    // Second pass: Rewrite the syntax tree by removing the inactive clauses
+    // Rewrite the syntax tree by removing the inactive clauses
     // from each #if (along with the #ifs themselves).
     let rewriter = ActiveSyntaxRewriter(
-      configuration: configuration,
+      configuredRegions: self,
       retainFeatureCheckIfConfigs: retainFeatureCheckIfConfigs
     )
-    return (
-      rewriter.rewrite(Syntax(self)),
-      visitor.diagnostics
-    )
+    return rewriter.rewrite(Syntax(node))
   }
 }
 
@@ -106,15 +143,16 @@ extension SyntaxProtocol {
 ///
 /// For any other target platforms, the resulting tree will be empty (other
 /// than trivia).
-class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
-  let configuration: Configuration
-  var diagnostics: [Diagnostic] = []
+class ActiveSyntaxRewriter: SyntaxRewriter {
+  let configuredRegions: ConfiguredRegions
+  var diagnostics: [Diagnostic]
 
   /// Whether to retain `#if` blocks containing compiler and feature checks.
   var retainFeatureCheckIfConfigs: Bool
 
-  init(configuration: Configuration, retainFeatureCheckIfConfigs: Bool) {
-    self.configuration = configuration
+  init(configuredRegions: ConfiguredRegions, retainFeatureCheckIfConfigs: Bool) {
+    self.configuredRegions = configuredRegions
+    self.diagnostics = configuredRegions.diagnostics
     self.retainFeatureCheckIfConfigs = retainFeatureCheckIfConfigs
   }
 
@@ -124,6 +162,19 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
   ) -> List {
     var newElements: [List.Element] = []
     var anyChanged = false
+
+    // Note that an element changed at the given index.
+    func noteElementChanged(at elementIndex: List.Index) {
+      if anyChanged {
+        return
+      }
+
+      // This is the first element that changed, so note that we have
+      // changes and add all prior elements to the list of new elements.
+      anyChanged = true
+      newElements.append(contentsOf: node[..<elementIndex])
+    }
+
     for elementIndex in node.indices {
       let element = node[elementIndex]
 
@@ -132,17 +183,9 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
         (!retainFeatureCheckIfConfigs || !ifConfigDecl.containsFeatureCheck)
       {
         // Retrieve the active `#if` clause
-        let (activeClause, localDiagnostics) = ifConfigDecl.activeClause(in: configuration)
-
-        // Add these diagnostics.
-        diagnostics.append(contentsOf: localDiagnostics)
+        let activeClause = configuredRegions.activeClause(for: ifConfigDecl)
 
-        // If this is the first element that changed, note that we have
-        // changes and add all prior elements to the list of new elements.
-        if !anyChanged {
-          anyChanged = true
-          newElements.append(contentsOf: node[..<elementIndex])
-        }
+        noteElementChanged(at: elementIndex)
 
         // Extract the elements from the active clause, if there are any.
         guard let elements = activeClause?.elements else {
@@ -161,6 +204,15 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
         continue
       }
 
+      // Transform the element directly. If it changed, note the changes.
+      if let transformedElement = rewrite(Syntax(element)).as(List.Element.self),
+        transformedElement.id != element.id
+      {
+        noteElementChanged(at: elementIndex)
+        newElements.append(transformedElement)
+        continue
+      }
+
       if anyChanged {
         newElements.append(element)
       }
@@ -174,47 +226,39 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
   }
 
   override func visit(_ node: CodeBlockItemListSyntax) -> CodeBlockItemListSyntax {
-    let rewrittenNode = dropInactive(node) { element in
+    return dropInactive(node) { element in
       guard case .decl(let declElement) = element.item else {
         return nil
       }
 
       return declElement.as(IfConfigDeclSyntax.self)
     }
-
-    return super.visit(rewrittenNode)
   }
 
   override func visit(_ node: MemberBlockItemListSyntax) -> MemberBlockItemListSyntax {
-    let rewrittenNode = dropInactive(node) { element in
+    return dropInactive(node) { element in
       return element.decl.as(IfConfigDeclSyntax.self)
     }
-
-    return super.visit(rewrittenNode)
   }
 
   override func visit(_ node: SwitchCaseListSyntax) -> SwitchCaseListSyntax {
-    let rewrittenNode = dropInactive(node) { element in
+    return dropInactive(node) { element in
       if case .ifConfigDecl(let ifConfigDecl) = element {
         return ifConfigDecl
       }
 
       return nil
     }
-
-    return super.visit(rewrittenNode)
   }
 
   override func visit(_ node: AttributeListSyntax) -> AttributeListSyntax {
-    let rewrittenNode = dropInactive(node) { element in
+    return dropInactive(node) { element in
       if case .ifConfigDecl(let ifConfigDecl) = element {
         return ifConfigDecl
       }
 
       return nil
     }
-
-    return super.visit(rewrittenNode)
   }
 
   /// Apply the given base to the postfix expression.
@@ -234,7 +278,7 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
         return nil
       }
 
-      let newExpr = applyBaseToPostfixExpression(base: base, postfix: node[keyPath: keyPath])
+      let newExpr = applyBaseToPostfixExpression(base: base, postfix: visit(node[keyPath: keyPath]))
       return ExprSyntax(node.with(keyPath, newExpr))
     }
 
@@ -243,7 +287,7 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
       guard let memberBase = memberAccess.base else {
         // If this member access has no base, this is the base we are
         // replacing, terminating the recursion. Do so now.
-        return ExprSyntax(memberAccess.with(\.base, base))
+        return ExprSyntax(memberAccess.with(\.base, visit(base)))
       }
 
       let newBase = applyBaseToPostfixExpression(base: base, postfix: memberBase)
@@ -302,10 +346,7 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
     }
 
     // Retrieve the active `if` clause.
-    let (activeClause, localDiagnostics) = postfixIfConfig.config.activeClause(in: configuration)
-
-    // Record these diagnostics.
-    diagnostics.append(contentsOf: localDiagnostics)
+    let activeClause = configuredRegions.activeClause(for: postfixIfConfig.config)
 
     guard case .postfixExpression(let postfixExpr) = activeClause?.elements
     else {
@@ -315,7 +356,7 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
       // only have both in an ill-formed syntax tree that was manually
       // created.
       if let base = postfixIfConfig.base ?? outerBase {
-        return base
+        return visit(base)
       }
 
       // If there was no base, we're in an erroneous syntax tree that would
@@ -330,20 +371,15 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
 
     // If there is no base, return the postfix expression.
     guard let base = postfixIfConfig.base ?? outerBase else {
-      return postfixExpr
+      return visit(postfixExpr)
     }
 
     // Apply the base to the postfix expression.
     return applyBaseToPostfixExpression(base: base, postfix: postfixExpr)
   }
 
   override func visit(_ node: PostfixIfConfigExprSyntax) -> ExprSyntax {
-    let rewrittenNode = dropInactive(outerBase: nil, postfixIfConfig: node)
-    if rewrittenNode == ExprSyntax(node) {
-      return rewrittenNode
-    }
-
-    return visit(rewrittenNode)
+    return dropInactive(outerBase: nil, postfixIfConfig: node)
   }
 }
 

Sources/SwiftIfConfig/ActiveSyntaxVisitor.swift

@@ -46,9 +46,6 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
   /// The diagnostics accumulated during this walk of active syntax.
   public private(set) var diagnostics: [Diagnostic] = []
 
-  /// Whether we visited any "#if" clauses.
-  var visitedAnyIfClauses: Bool = false
-
   public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
     self.configuration = configuration
     super.init(viewMode: viewMode)
@@ -60,8 +57,6 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
     let (activeClause, localDiagnostics) = node.activeClause(in: configuration)
     diagnostics.append(contentsOf: localDiagnostics)
 
-    visitedAnyIfClauses = true
-
     // If there is an active clause, visit it's children.
     if let activeClause, let elements = activeClause.elements {
       walk(elements)

Sources/SwiftIfConfig/ConfiguredRegions.swift

@@ -39,6 +39,12 @@ import SwiftSyntax
 public struct ConfiguredRegions {
   let regions: [(ifClause: IfConfigClauseSyntax, state: IfConfigRegionState)]
 
+  // A mapping from each of the #if declarations that have been evaluated to
+  // the active clause. Absence from this map means that there is no active
+  // clause, either because every clause failed or because the entire #if
+  // itself is inactive.
+  var activeClauses: [IfConfigDeclSyntax: IfConfigClauseSyntax]
+
   /// The set of diagnostics produced when evaluating the configured regions.
   public let diagnostics: [Diagnostic]
 
@@ -80,6 +86,17 @@ public struct ConfiguredRegions {
       (region.ifClause.regionStart...region.ifClause.endPosition).contains(node.position)
     }?.state ?? .active
   }
+
+  /// Determine which clause of an `#if` declaration was active within this
+  /// set of configured regions.
+  ///
+  /// A particular `#if` declaration might have no active clause (e.g., this
+  /// operation will return a `nil`) if either none of the clauses had
+  /// conditions that succeeded, or the `#if` declaration itself is within an
+  /// inactive (or unparsed) region and therefore cannot have an active clause.
+  public func activeClause(for node: IfConfigDeclSyntax) -> IfConfigClauseSyntax? {
+    return activeClauses[node]
+  }
 }
 
 extension ConfiguredRegions: RandomAccessCollection {
@@ -150,6 +167,7 @@ extension SyntaxProtocol {
     visitor.walk(self)
     return ConfiguredRegions(
       regions: visitor.regions,
+      activeClauses: visitor.activeClauses,
       diagnostics: visitor.diagnostics
     )
   }
@@ -171,6 +189,12 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
   // All diagnostics encountered along the way.
   var diagnostics: [Diagnostic] = []
 
+  // A mapping from each of the #if declarations that have been evaluated to
+  // the active clause. Absence from this map means that there is no active
+  // clause, either because every clause failed or because the entire #if
+  // itself is inactive.
+  var activeClauses: [IfConfigDeclSyntax: IfConfigClauseSyntax] = [:]
+
   init(configuration: Configuration) {
     self.configuration = configuration
     super.init(viewMode: .sourceAccurate)
@@ -242,6 +266,11 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
         isActive = !foundActive && inActiveRegion
       }
 
+      // If this is the active clause, record it as such.
+      if isActive {
+        activeClauses[node] = clause
+      }
+
       // Determine and record the current state.
       let currentState: IfConfigRegionState
       switch (isActive, syntaxErrorsAllowed) {