Merge pull request #2759 from DougGregor/swift-if-config-cleanup

Address most of the code review comments on the SwiftIfConfig library

Author: DougGregor

Package.swift

@@ -143,7 +143,7 @@ let package = Package(
 
     .target(
       name: "SwiftIfConfig",
-      dependencies: ["SwiftSyntax", "SwiftOperators"],
+      dependencies: ["SwiftSyntax", "SwiftDiagnostics", "SwiftOperators"],
       exclude: ["CMakeLists.txt"]
     ),
 

Release Notes/601.md

@@ -15,6 +15,10 @@
   - Description: This method translates an error into one or more diagnostics, recognizing `DiagnosticsError` and `DiagnosticMessage` instances or providing its own `Diagnostic` as needed.
   - Pull Request: https://github.com/swiftlang/swift-syntax/pull/1816
 
+- Added a new library `SwiftIfConfig`.
+  - Description: This new library provides facilities for evaluating `#if` conditions and determining which regions of a syntax tree are active according to a given build configuration.
+  - Pull Request: https://github.com/swiftlang/swift-syntax/pull/1816
+
 ## API Behavior Changes
 
 - `SyntaxProtocol.trimmed` detaches the node

Sources/SwiftIfConfig/ActiveSyntaxRewriter.swift

@@ -10,15 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-//
-// This file defines the SyntaxRewriter, a class that performs a standard walk
-// and tree-rebuilding pattern.
-//
-// Subclassers of this class can override the walking behavior for any syntax
-// node and transform nodes however they like.
-//
-//===----------------------------------------------------------------------===//
-
 import SwiftDiagnostics
 import SwiftSyntax
 
@@ -34,15 +25,17 @@ extension SyntaxProtocol {
   /// clauses, e.g., `#if FOO > 10`, then the condition will be
   /// considered to have failed and the clauses's elements will be
   /// removed.
-  public func removingInactive(in configuration: some BuildConfiguration) -> (Syntax, [Diagnostic]) {
+  public func removingInactive(
+    in configuration: some BuildConfiguration
+  ) -> (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)
 
     // If there were no active clauses to visit, we're done!
-    if visitor.numIfClausesVisited == 0 {
+    if !visitor.visitedAnyIfClauses {
       return (Syntax(self), visitor.diagnostics)
     }
 
@@ -88,12 +81,13 @@ extension SyntaxProtocol {
 /// than trivia).
 class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
   let configuration: Configuration
+  var diagnostics: [Diagnostic] = []
 
   init(configuration: Configuration) {
     self.configuration = configuration
   }
 
-  private func dropInactive<List: Collection & SyntaxCollection>(
+  private func dropInactive<List: SyntaxCollection>(
     _ node: List,
     elementAsIfConfig: (List.Element) -> IfConfigDeclSyntax?
   ) -> List {
@@ -105,7 +99,10 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
       // Find #ifs within the list.
       if let ifConfigDecl = elementAsIfConfig(element) {
         // Retrieve the active `#if` clause
-        let activeClause = ifConfigDecl.activeClause(in: configuration)
+        let (activeClause, localDiagnostics) = ifConfigDecl.activeClause(in: configuration)
+
+        // Add these diagnostics.
+        diagnostics.append(contentsOf: localDiagnostics)
 
         // If this is the first element that changed, note that we have
         // changes and add all prior elements to the list of new elements.
@@ -255,7 +252,8 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
       return dropInactive(outerBase: base, postfixIfConfig: postfixIfConfig)
     }
 
-    preconditionFailure("Unhandled postfix expression in #if elimination")
+    assertionFailure("Unhandled postfix expression in #if elimination")
+    return postfix
   }
 
   /// Drop inactive regions from a postfix `#if` configuration, applying the
@@ -265,7 +263,10 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
     postfixIfConfig: PostfixIfConfigExprSyntax
   ) -> ExprSyntax {
     // Retrieve the active `if` clause.
-    let activeClause = postfixIfConfig.config.activeClause(in: configuration)
+    let (activeClause, localDiagnostics) = postfixIfConfig.config.activeClause(in: configuration)
+
+    // Record these diagnostics.
+    diagnostics.append(contentsOf: localDiagnostics)
 
     guard case .postfixExpression(let postfixExpr) = activeClause?.elements
     else {

Sources/SwiftIfConfig/ActiveSyntaxVisitor.swift

@@ -9,6 +9,7 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import SwiftDiagnostics
 import SwiftSyntax
 
@@ -36,34 +37,35 @@ import SwiftSyntax
 /// it would not visit either `f` or `g`.
 ///
 /// All notes visited by this visitor will have the "active" state, i.e.,
-/// `node.isActive(in: configuration)` will evaluate to `.active` or will
-/// throw. When errors occur, they will be recorded in the set of
+/// `node.isActive(in: configuration)` will have evaluated to `.active`
+/// When errors occur, they will be recorded in the set of
 /// diagnostics.
 open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor {
   /// The build configuration, which will be queried for each relevant `#if`.
   public let configuration: Configuration
 
-  /// The set of diagnostics accumulated during this walk of active syntax.
-  public var diagnostics: [Diagnostic] = []
+  /// The diagnostics accumulated during this walk of active syntax.
+  public private(set) var diagnostics: [Diagnostic] = []
 
-  /// The number of "#if" clauses that were visited.
-  var numIfClausesVisited: Int = 0
+  /// Whether we visited any "#if" clauses.
+  var visitedAnyIfClauses: Bool = false
 
   public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
     self.configuration = configuration
     super.init(viewMode: viewMode)
   }
 
   open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
-    let activeClause = node.activeClause(in: configuration) { diag in
-      self.diagnostics.append(diag)
-    }
+    // Note: there is a clone of this code in ActiveSyntaxAnyVisitor. If you
+    // change one, please also change the other.
+    let (activeClause, localDiagnostics) = node.activeClause(in: configuration)
+    diagnostics.append(contentsOf: localDiagnostics)
 
-    numIfClausesVisited += 1
+    visitedAnyIfClauses = true
 
     // If there is an active clause, visit it's children.
     if let activeClause, let elements = activeClause.elements {
-      walk(Syntax(elements))
+      walk(elements)
     }
 
     // Skip everything else in the #if.
@@ -95,32 +97,30 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
 /// it would not visit either `f` or `g`.
 ///
 /// All notes visited by this visitor will have the "active" state, i.e.,
-/// `node.isActive(in: configuration)` will evaluate to `.active` or will
-/// throw.
-///
-/// All notes visited by this visitor will have the "active" state, i.e.,
-/// `node.isActive(in: configuration)` will evaluate to `.active` or will
-/// throw. When errors occur, they will be recorded in the set of
-/// diagnostivs.
+/// `node.isActive(in: configuration)` will have evaluated to `.active`.
+/// When errors occur, they will be recorded in the array of diagnostics.
 open class ActiveSyntaxAnyVisitor<Configuration: BuildConfiguration>: SyntaxAnyVisitor {
   /// The build configuration, which will be queried for each relevant `#if`.
   public let configuration: Configuration
 
-  /// The set of diagnostics accumulated during this walk of active syntax.
-  public var diagnostics: [Diagnostic] = []
+  /// The diagnostics accumulated during this walk of active syntax.
+  public private(set) var diagnostics: [Diagnostic] = []
 
   public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
     self.configuration = configuration
     super.init(viewMode: viewMode)
   }
 
   open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
+    // Note: there is a clone of this code in ActiveSyntaxVisitor. If you
+    // change one, please also change the other.
+
     // If there is an active clause, visit it's children.
-    let activeClause = node.activeClause(in: configuration) { diag in
-      self.diagnostics.append(diag)
-    }
+    let (activeClause, localDiagnostics) = node.activeClause(in: configuration)
+    diagnostics.append(contentsOf: localDiagnostics)
+
     if let activeClause, let elements = activeClause.elements {
-      walk(Syntax(elements))
+      walk(elements)
     }
 
     // Skip everything else in the #if.

Sources/SwiftIfConfig/BuildConfiguration.swift

@@ -253,7 +253,7 @@ public protocol BuildConfiguration {
   ///
   /// The language version can be queried with the `swift` directive that checks
   /// how the supported language version compares, as described by
-  /// [SE-0212](https://github.com/apple/swift-evolution/blob/main/proposals/0212-compiler-version-directive.md). For example:
+  /// [SE-0212](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0212-compiler-version-directive.md). For example:
   ///
   /// ```swift
   /// #if swift(>=5.5)

Sources/SwiftIfConfig/CMakeLists.txt

@@ -11,7 +11,7 @@ add_swift_syntax_library(SwiftIfConfig
   ActiveSyntaxRewriter.swift
   BuildConfiguration.swift
   ConfiguredRegions.swift
-  ConfiguredRegionState.swift
+  IfConfigRegionState.swift
   IfConfigDecl+IfConfig.swift
   IfConfigError.swift
   IfConfigEvaluation.swift
@@ -25,5 +25,4 @@ add_swift_syntax_library(SwiftIfConfig
 target_link_swift_syntax_libraries(SwiftIfConfig PUBLIC
   SwiftSyntax
   SwiftDiagnostics
-  SwiftOperators
-  SwiftParser)
+  SwiftOperators)

Sources/SwiftIfConfig/ConfiguredRegions.swift

@@ -9,15 +9,14 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import SwiftDiagnostics
 import SwiftSyntax
 
 extension SyntaxProtocol {
   /// Find all of the #if/#elseif/#else clauses within the given syntax node,
-  /// indicating their active state. This operation will recurse into active
-  /// clauses to represent the flattened nested structure, while nonactive
-  /// clauses need no recursion (because there is no relevant structure in
-  /// them).
+  /// indicating their active state. This operation will recurse into all
+  /// clauses to indicate regions of active / inactive / unparsed code.
   ///
   /// For example, given code like the following:
   /// #if DEBUG
@@ -37,7 +36,7 @@ extension SyntaxProtocol {
   ///   - Inactive region for the final `#else`.
   public func configuredRegions(
     in configuration: some BuildConfiguration
-  ) -> [(IfConfigClauseSyntax, ConfiguredRegionState)] {
+  ) -> [(IfConfigClauseSyntax, IfConfigRegionState)] {
     let visitor = ConfiguredRegionVisitor(configuration: configuration)
     visitor.walk(self)
     return visitor.regions
@@ -49,7 +48,7 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
   let configuration: Configuration
 
   /// The regions we've found so far.
-  var regions: [(IfConfigClauseSyntax, ConfiguredRegionState)] = []
+  var regions: [(IfConfigClauseSyntax, IfConfigRegionState)] = []
 
   /// Whether we are currently within an active region.
   var inActiveRegion = true
@@ -62,7 +61,7 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
   override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
     // If we're in an active region, find the active clause. Otherwise,
     // there isn't one.
-    let activeClause = inActiveRegion ? node.activeClause(in: configuration) : nil
+    let activeClause = inActiveRegion ? node.activeClause(in: configuration).clause : nil
     for clause in node.clauses {
       // If this is the active clause, record it and then recurse into the
       // elements.
@@ -79,11 +78,9 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
       }
 
       // For inactive clauses, distinguish between inactive and unparsed.
-      let isVersioned =
-        (try? clause.isVersioned(
-          configuration: configuration,
-          diagnosticHandler: nil
-        )) ?? true
+      let isVersioned = clause.isVersioned(
+        configuration: configuration
+      ).versioned
 
       // If this is within an active region, or this is an unparsed region,
       // record it.

Sources/SwiftIfConfig/IfConfigDecl+IfConfig.swift

@@ -9,6 +9,7 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import SwiftDiagnostics
 import SwiftSyntax
 
@@ -25,35 +26,35 @@ extension IfConfigDeclSyntax {
   /// ```
   ///
   /// If the `A` configuration option was passed on the command line (e.g. via `-DA`), the first clause
-  /// (containing `func f()`) would be returned. If not, and if the `B`configuration was passed on the
+  /// (containing `func f()`) would be returned. If not, and if the `B` configuration was passed on the
   /// command line, the second clause (containing `func g()`) would be returned. If neither was
   /// passed, this function will return `nil` to indicate that none of the regions are active.
   ///
-  /// If an error occurrs while processing any of the `#if` clauses,
+  /// If an error occurs while processing any of the `#if` clauses,
   /// that clause will be considered inactive and this operation will
   /// continue to evaluate later clauses.
   public func activeClause(
-    in configuration: some BuildConfiguration,
-    diagnosticHandler: ((Diagnostic) -> Void)? = nil
-  ) -> IfConfigClauseSyntax? {
+    in configuration: some BuildConfiguration
+  ) -> (clause: IfConfigClauseSyntax?, diagnostics: [Diagnostic]) {
+    var diagnostics: [Diagnostic] = []
     for clause in clauses {
       // If there is no condition, we have reached an unconditional clause. Return it.
       guard let condition = clause.condition else {
-        return clause
+        return (clause, diagnostics: diagnostics)
       }
 
       // If this condition evaluates true, return this clause.
-      let isActive =
-        (try? evaluateIfConfig(
-          condition: condition,
-          configuration: configuration,
-          diagnosticHandler: diagnosticHandler
-        ))?.active ?? false
+      let (isActive, _, localDiagnostics) = evaluateIfConfig(
+        condition: condition,
+        configuration: configuration
+      )
+      diagnostics.append(contentsOf: localDiagnostics)
+
       if isActive {
-        return clause
+        return (clause, diagnostics: diagnostics)
       }
     }
 
-    return nil
+    return (nil, diagnostics: diagnostics)
   }
 }

Sources/SwiftIfConfig/IfConfigError.swift

@@ -9,6 +9,7 @@
 // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 //
 //===----------------------------------------------------------------------===//
+
 import SwiftDiagnostics
 import SwiftSyntax