Handle invalid #if conditions as "unparsed" regions consistently

When determining active regions, treat clauses with invalid conditions
as "unparsed" regions, but don't abort the computation by throwing. This
provides behavior that is more consistent with the compiler, and is also
generally easy for most clients. Those clients that want to report
diagnostics can certainly do so, but are not forced to work with
throwing APIs for invalid code.

While here, improve the active syntax rewriting operation by making it
a two-pass operation. The first pass emits diagnostics and determines
whether there is any rewriting to do, and the second pass performs the
rewriting. This fixes an existing bug where the diagnostic locations
were wrong because we were emitting them against partially-rewritten
trees.

Author: DougGregor

Sources/SwiftIfConfig/ConfiguredRegions.swift

@@ -59,8 +59,9 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
   }
 
   override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
-    // If're not within an
-    let activeClause = inActiveRegion ? (try? node.activeClause(in: configuration)) : nil
+    // If we're in an active region, find the active clause. Otherwise,
+    // there isn't one.
+    let activeClause = inActiveRegion ? node.activeClause(in: configuration) : nil
     for clause in node.clauses {
       // If this is the active clause, record it and then recurse into the
       // elements.
@@ -81,7 +82,7 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
         (try? clause.isVersioned(
           configuration: configuration,
           diagnosticHandler: nil
-        )) ?? false
+        )) ?? true
 
       // If this is within an active region, or this is an unparsed region,
       // record it.

Sources/SwiftIfConfig/IfConfigEvaluation.swift

@@ -451,23 +451,27 @@ extension IfConfigDeclSyntax {
   /// 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 occurred while processing any of the `#if` clauses, this function will throw that error.
+  /// If an error occurrs 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
-  ) throws -> IfConfigClauseSyntax? {
+  ) -> IfConfigClauseSyntax? {
     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
       }
 
       // If this condition evaluates true, return this clause.
-      if try evaluateIfConfig(
-        condition: condition,
-        configuration: configuration,
-        diagnosticHandler: diagnosticHandler
-      ).active {
+      let isActive =
+        (try? evaluateIfConfig(
+          condition: condition,
+          configuration: configuration,
+          diagnosticHandler: diagnosticHandler
+        ))?.active ?? false
+      if isActive {
         return clause
       }
     }
@@ -507,7 +511,7 @@ extension SyntaxProtocol {
       if let ifConfigClause = currentNode.as(IfConfigClauseSyntax.self),
         let ifConfigDecl = ifConfigClause.parent?.parent?.as(IfConfigDeclSyntax.self)
       {
-        let activeClause = try ifConfigDecl.activeClause(
+        let activeClause = ifConfigDecl.activeClause(
           in: configuration,
           diagnosticHandler: diagnosticHandler
         )
@@ -516,10 +520,12 @@ extension SyntaxProtocol {
           // This was not the active clause, so we know that we're in an
           // inactive block. However, if the condition is versioned, this is an
           // unparsed region.
-          if try ifConfigClause.isVersioned(
-            configuration: configuration,
-            diagnosticHandler: diagnosticHandler
-          ) {
+          let isVersioned =
+            (try? ifConfigClause.isVersioned(
+              configuration: configuration,
+              diagnosticHandler: diagnosticHandler
+            )) ?? true
+          if isVersioned {
             return .unparsed
           }
 

Sources/SwiftIfConfig/IfConfigRewriter.swift

@@ -23,7 +23,7 @@ import SwiftDiagnostics
 import SwiftSyntax
 
 /// Syntax rewriter that only visits syntax nodes that are active according
-/// to a particular build configuration build configuration.
+/// to a particular build configuration.
 ///
 /// Given an example such as
 ///
@@ -54,17 +54,11 @@ import SwiftSyntax
 /// than trivia).
 class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
   let configuration: Configuration
-  var diagnostics: [Diagnostic] = []
 
   init(configuration: Configuration) {
     self.configuration = configuration
   }
 
-  private func reportEvaluationError(at node: some SyntaxProtocol, error: Error) {
-    let newDiagnostics = error.asDiagnostics(at: node)
-    diagnostics.append(contentsOf: newDiagnostics)
-  }
-
   private func dropInactive<List: Collection & SyntaxCollection>(
     _ node: List,
     elementAsIfConfig: (List.Element) -> IfConfigDeclSyntax?
@@ -76,20 +70,8 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
 
       // Find #ifs within the list.
       if let ifConfigDecl = elementAsIfConfig(element) {
-        // Evaluate the `#if` condition.
-        let activeClause: IfConfigClauseSyntax?
-        do {
-          activeClause = try ifConfigDecl.activeClause(in: configuration)
-        } catch {
-          // When an error occurs in the evaluation of the condition,
-          // keep the entire `#if`.
-          if anyChanged {
-            newElements.append(element)
-          }
-
-          reportEvaluationError(at: element, error: error)
-          continue
-        }
+        // Retrieve the active `#if` clause
+        let activeClause = ifConfigDecl.activeClause(in: configuration)
 
         // If this is the first element that changed, note that we have
         // changes and add all prior elements to the list of new elements.
@@ -248,14 +230,8 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
     outerBase: ExprSyntax?,
     postfixIfConfig: PostfixIfConfigExprSyntax
   ) -> ExprSyntax {
-    // Determine the active clause within this syntax node.
-    let activeClause: IfConfigClauseSyntax?
-    do {
-      activeClause = try postfixIfConfig.config.activeClause(in: configuration)
-    } catch {
-      reportEvaluationError(at: postfixIfConfig, error: error)
-      return ExprSyntax(postfixIfConfig)
-    }
+    // Retrieve the active `if` clause.
+    let activeClause = postfixIfConfig.config.activeClause(in: configuration)
 
     guard case .postfixExpression(let postfixExpr) = activeClause?.elements
     else {
@@ -304,8 +280,29 @@ extension SyntaxProtocol {
   ///
   /// Returns the syntax node with all inactive regions removed, along with an
   /// array containing any diagnostics produced along the way.
+  ///
+  /// 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.
   public func removingInactive(in configuration: some BuildConfiguration) -> (Syntax, [Diagnostic]) {
-    let visitor = ActiveSyntaxRewriter(configuration: configuration)
-    return (visitor.rewrite(Syntax(self)), visitor.diagnostics)
+    // 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 {
+      return (Syntax(self), visitor.diagnostics)
+    }
+
+    // Second pass: Rewrite the syntax tree by removing the inactive clauses
+    // from each #if (along with the #ifs themselves).
+    let rewriter = ActiveSyntaxRewriter(configuration: configuration)
+    return (
+      rewriter.rewrite(Syntax(self)),
+      visitor.diagnostics
+    )
   }
 }

Sources/SwiftIfConfig/IfConfigVisitor.swift

@@ -37,49 +37,37 @@ import SwiftSyntax
 ///
 /// 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 reported via a call to
-/// `reportEvaluationError`, which can report the errors (the default is to
-/// turn them into diagnostics that go into the `diagnostics` array) and then
-/// choose whether to visit all of the `#if` clauses (the default) or skip them.
+/// throw. 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 number of "#if" clauses that were visited.
+  var numIfClausesVisited: Int = 0
+
   public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
     self.configuration = configuration
     super.init(viewMode: viewMode)
   }
 
-  /// Called when the evaluation of an `#if` condition produces an error.
-  ///
-  /// By default, this records diagnostics from the error into the `diagnostics`
-  /// array.
-  ///
-  /// - Returns: Whether to visit the children of the `#if` or not after the
-  /// error. By default, this function returns `.visitChildren`.
-  open func reportEvaluationError(at node: IfConfigDeclSyntax, error: Error) -> SyntaxVisitorContinueKind {
-    let newDiagnostics = error.asDiagnostics(at: node)
-    diagnostics.append(contentsOf: newDiagnostics)
-    return .visitChildren
-  }
-
   open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
-    do {
-      // If there is an active clause, visit it's children.
-      if let activeClause = try node.activeClause(in: configuration),
-        let elements = activeClause.elements
-      {
-        walk(Syntax(elements))
-      }
+    let activeClause = node.activeClause(in: configuration) { diag in
+      self.diagnostics.append(diag)
+    }
+
+    numIfClausesVisited += 1
 
-      // Skip everything else in the #if.
-      return .skipChildren
-    } catch {
-      return reportEvaluationError(at: node, error: error)
+    // If there is an active clause, visit it's children.
+    if let activeClause, let elements = activeClause.elements {
+      walk(Syntax(elements))
     }
+
+    // Skip everything else in the #if.
+    return .skipChildren
   }
 }
 
@@ -112,10 +100,8 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
 ///
 /// 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 reported via a call to
-/// `reportEvaluationError`, which can report the errors (the default is to
-/// turn them into diagnostics that go into the `diagnostics` array) and then
-/// choose whether to visit all of the `#if` clauses (the default) or skip them.
+/// throw. When errors occur, they will be recorded in the set of
+/// diagnostivs.
 open class ActiveSyntaxAnyVisitor<Configuration: BuildConfiguration>: SyntaxAnyVisitor {
   /// The build configuration, which will be queried for each relevant `#if`.
   public let configuration: Configuration
@@ -128,32 +114,16 @@ open class ActiveSyntaxAnyVisitor<Configuration: BuildConfiguration>: SyntaxAnyV
     super.init(viewMode: viewMode)
   }
 
-  /// Called when the evaluation of an `#if` condition produces an error.
-  ///
-  /// By default, this records diagnostics from the error into the `diagnostics`
-  /// array.
-  ///
-  /// - Returns: Whether to visit the children of the `#if` or not after the
-  /// error. By default, this function returns `.visitChildren`.
-  open func reportEvaluationError(at node: IfConfigDeclSyntax, error: Error) -> SyntaxVisitorContinueKind {
-    let newDiagnostics = error.asDiagnostics(at: node)
-    diagnostics.append(contentsOf: newDiagnostics)
-    return .visitChildren
-  }
-
   open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
-    do {
-      // If there is an active clause, visit it's children.
-      if let activeClause = try node.activeClause(in: configuration),
-        let elements = activeClause.elements
-      {
-        walk(Syntax(elements))
-      }
-
-      // Skip everything else in the
-      return .skipChildren
-    } catch {
-      return reportEvaluationError(at: node, error: error)
+    // If there is an active clause, visit it's children.
+    let activeClause = node.activeClause(in: configuration) { diag in
+      self.diagnostics.append(diag)
     }
+    if let activeClause, let elements = activeClause.elements {
+      walk(Syntax(elements))
+    }
+
+    // Skip everything else in the #if.
+    return .skipChildren
   }
 }

Tests/SwiftIfConfigTest/ActiveRegionTests.swift

@@ -78,6 +78,23 @@ public class ActiveRegionTests: XCTestCase {
         "5️⃣": .active,
       ]
     )
+  }
 
+  func testActiveRegionsWithErrors() throws {
+    try assertActiveCode(
+      """
+      #if FOO > 10
+      0️⃣class Foo {
+      }
+      #else
+      1️⃣class Fallback {
+      }
+      #endif
+      """,
+      states: [
+        "0️⃣": .unparsed,
+        "1️⃣": .active,
+      ]
+    )
   }
 }

Tests/SwiftIfConfigTest/VisitorTests.swift

@@ -166,7 +166,7 @@ public class VisitorTests: XCTestCase {
     configuration.badAttributes.insert("available")
     let visitor = NameCheckingVisitor(
       configuration: configuration,
-      expectedNames: ["f", "h", "i", "S", "generationCount", "value", "withAvail", "notAvail"]
+      expectedNames: ["f", "h", "i", "S", "generationCount", "value", "notAvail"]
     )
     visitor.walk(inputSource)
     XCTAssertEqual(visitor.diagnostics.count, 3)
@@ -213,27 +213,23 @@ public class VisitorTests: XCTestCase {
       diagnostics: [
         DiagnosticSpec(
           message: "unacceptable attribute 'available'",
-          line: 51,
-          column: 1
+          line: 3,
+          column: 18
         ),
         DiagnosticSpec(
           message: "unacceptable attribute 'available'",
-          line: 1,
-          column: 2
+          line: 42,
+          column: 20
         ),
         DiagnosticSpec(
           message: "unacceptable attribute 'available'",
-          line: 27,
-          column: 17
+          line: 51,
+          column: 18
         ),
       ],
       expectedSource: """
 
-        #if hasAttribute(available)
-        @available(*, deprecated, message: "use something else")
-        #else
         @MainActor
-        #endif
         func f() {
         }
 
@@ -250,19 +246,9 @@ public class VisitorTests: XCTestCase {
 
         func i() {
           a.b
-          #if DEBUG
             .c
-          #endif
-          #if hasAttribute(available)
-            .d()
-          #endif
         }
-
-        #if hasAttribute(available)
-        func withAvail() { }
-        #else
         func notAvail() { }
-        #endif
         """
     )
   }