Handle errors while removing inactive regions

When removing inactive regions via a visitor, accumulate any diagnostics
produced during this process so they can be reported. Introduce a test
harness so we can test these diagnostics more easily.

Author: DougGregor

Package.swift

@@ -152,7 +152,12 @@ let package = Package(
 
     .testTarget(
       name: "SwiftIfConfigTest",
-      dependencies: ["_SwiftSyntaxTestSupport", "SwiftIfConfig", "SwiftParser"]
+      dependencies: [
+        "_SwiftSyntaxTestSupport",
+        "SwiftIfConfig",
+        "SwiftParser",
+        "SwiftSyntaxMacrosGenericTestSupport",
+      ]
     ),
 
     // MARK: SwiftLexicalLookup

Sources/SwiftIfConfig/IfConfigEvaluation.swift

@@ -1,3 +1,4 @@
+import SwiftOperators
 //===----------------------------------------------------------------------===//
 //
 // This source file is part of the Swift.org open source project
@@ -10,7 +11,6 @@
 //
 //===----------------------------------------------------------------------===//
 import SwiftSyntax
-import SwiftOperators
 
 enum IfConfigError: Error, CustomStringConvertible {
   case unknownExpression(ExprSyntax)
@@ -196,15 +196,15 @@ private func evaluateIfConfig(
 
   // Logical '!'.
   if let prefixOp = condition.as(PrefixOperatorExprSyntax.self),
-     prefixOp.operator.text == "!"
+    prefixOp.operator.text == "!"
   {
     return try !evaluateIfConfig(condition: prefixOp.expression, configuration: configuration)
   }
 
   // Logical '&&' and '||'.
   if let binOp = condition.as(InfixOperatorExprSyntax.self),
-     let op = binOp.operator.as(BinaryOperatorExprSyntax.self),
-     (op.operator.text == "&&" || op.operator.text == "||")
+    let op = binOp.operator.as(BinaryOperatorExprSyntax.self),
+    (op.operator.text == "&&" || op.operator.text == "||")
   {
     // Evaluate the left-hand side.
     let lhsResult = try evaluateIfConfig(condition: binOp.leftOperand, configuration: configuration)
@@ -290,7 +290,10 @@ private func evaluateIfConfig(
       return try doSingleIdentifierArgumentCheck(configuration.isActiveTargetRuntime, role: "runtime")
 
     case ._ptrauth:
-      return try doSingleIdentifierArgumentCheck(configuration.isActiveTargetPointerAuthentication, role: "pointer authentication scheme")
+      return try doSingleIdentifierArgumentCheck(
+        configuration.isActiveTargetPointerAuthentication,
+        role: "pointer authentication scheme"
+      )
 
     case ._endian:
       // Ensure that we have a single argument that is a simple identifier,
@@ -313,7 +316,10 @@ private func evaluateIfConfig(
         argFirst == "_",
         let expectedPointerBitWidth = Int(arg.dropFirst())
       else {
-        throw IfConfigError.requiresUnlabeledArgument(name: fnName, role: "pointer bit with ('_' followed by an integer)")
+        throw IfConfigError.requiresUnlabeledArgument(
+          name: fnName,
+          role: "pointer bit with ('_' followed by an integer)"
+        )
       }
 
       return configuration.targetPointerBitWidth == expectedPointerBitWidth

Sources/SwiftIfConfig/IfConfigRewriter.swift

@@ -19,6 +19,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+import SwiftDiagnostics
 import SwiftSyntax
 
 /// Syntax rewriter that only visits syntax nodes that are active according
@@ -53,11 +54,17 @@ 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?
@@ -69,19 +76,30 @@ 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
+        }
+
         // 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])
         }
 
-        // FIXME: Swallowing errors
-        guard let activeClause = try? ifConfigDecl.activeClause(in: configuration) else {
-          continue
-        }
-
-        guard let elements = activeClause.elements else {
+        // Extract the elements from the active clause, if there are any.
+        guard let elements = activeClause?.elements else {
           continue
         }
 
@@ -231,9 +249,15 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
     postfixIfConfig: PostfixIfConfigExprSyntax
   ) -> ExprSyntax {
     // Determine the active clause within this syntax node.
-    // TODO: Swallows errors
-    guard let activeClause = try? postfixIfConfig.config.activeClause(in: configuration),
-      case .postfixExpression(let postfixExpr) = activeClause.elements
+    let activeClause: IfConfigClauseSyntax?
+    do {
+      activeClause = try postfixIfConfig.config.activeClause(in: configuration)
+    } catch {
+      reportEvaluationError(at: postfixIfConfig, error: error)
+      return ExprSyntax(postfixIfConfig)
+    }
+
+    guard case .postfixExpression(let postfixExpr) = activeClause?.elements
     else {
       // If there is no active clause, return the base.
 
@@ -263,11 +287,15 @@ class ActiveSyntaxRewriter<Configuration: BuildConfiguration>: SyntaxRewriter {
     return applyBaseToPostfixExpression(base: base, postfix: postfixExpr)
   }
 
-  // TODO: PostfixIfConfigExprSyntax has a different form that doesn't work
+  // FIXME: PostfixIfConfigExprSyntax has a different form that doesn't work
   // well with the way dropInactive is written. We essentially need to
   // thread a the "base" into the active clause.
   override func visit(_ node: PostfixIfConfigExprSyntax) -> ExprSyntax {
     let rewrittenNode = dropInactive(outerBase: nil, postfixIfConfig: node)
+    if rewrittenNode == ExprSyntax(node) {
+      return rewrittenNode
+    }
+
     return visit(rewrittenNode)
   }
 }
@@ -276,8 +304,11 @@ extension SyntaxProtocol {
   /// Produce a copy of this syntax node that removes all syntax regions that
   /// are inactive according to the given build configuration, leaving only
   /// the code that is active within that build configuration.
-  public func removingInactive(in configuration: some BuildConfiguration) -> Syntax {
+  ///
+  /// Returns the syntax node with all inactive regions removed, along with an
+  /// array containing any diagnostics produced along the way.
+  public func removingInactive(in configuration: some BuildConfiguration) -> (Syntax, [Diagnostic]) {
     let visitor = ActiveSyntaxRewriter(configuration: configuration)
-    return visitor.rewrite(Syntax(self))
+    return (visitor.rewrite(Syntax(self)), visitor.diagnostics)
   }
 }

Sources/SwiftIfConfig/IfConfigVisitor.swift

@@ -70,7 +70,8 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
     do {
       // If there is an active clause, visit it's children.
       if let activeClause = try node.activeClause(in: configuration),
-         let elements = activeClause.elements {
+        let elements = activeClause.elements
+      {
         walk(Syntax(elements))
       }
 
@@ -144,7 +145,8 @@ open class ActiveSyntaxAnyVisitor<Configuration: BuildConfiguration>: SyntaxAnyV
     do {
       // If there is an active clause, visit it's children.
       if let activeClause = try node.activeClause(in: configuration),
-         let elements = activeClause.elements {
+        let elements = activeClause.elements
+      {
         walk(Syntax(elements))
       }
 

Sources/SwiftIfConfig/SyntaxLiteralUtils.swift

@@ -36,7 +36,7 @@ extension ExprSyntax {
   /// Whether this is a simple identifier expression and, if so, what the identifier string is.
   var simpleIdentifierExpr: String? {
     guard let identExpr = self.as(DeclReferenceExprSyntax.self),
-          identExpr.argumentNames == nil
+      identExpr.argumentNames == nil
     else {
       return nil
     }

Sources/SwiftSyntaxMacrosGenericTestSupport/Assertions.swift

@@ -146,7 +146,7 @@ public struct NoteSpec {
 
 func assertNote(
   _ note: Note,
-  in expansionContext: BasicMacroExpansionContext,
+  in expansionContext: DiagnosticAssertionContext,
   expected spec: NoteSpec,
   failureHandler: (TestFailureSpec) -> Void
 ) {
@@ -338,9 +338,36 @@ extension DiagnosticSpec {
   }
 }
 
-func assertDiagnostic(
+/// Describes the context in which we are asserting diagnostic correctness.
+///
+/// This is used to map source locations.
+public enum DiagnosticAssertionContext {
+  case macroExpansion(BasicMacroExpansionContext)
+  case tree(any SyntaxProtocol)
+
+  func location(
+    for position: AbsolutePosition,
+    anchoredAt node: Syntax,
+    fileName: String
+  ) -> SourceLocation {
+    switch self {
+    case .macroExpansion(let expansionContext):
+      return expansionContext.location(
+        for: position,
+        anchoredAt: node,
+        fileName: fileName
+      )
+
+    case .tree(let syntax):
+      return SourceLocationConverter(fileName: fileName, tree: syntax)
+        .location(for: position)
+    }
+  }
+}
+
+public func assertDiagnostic(
   _ diag: Diagnostic,
-  in expansionContext: BasicMacroExpansionContext,
+  in expansionContext: DiagnosticAssertionContext,
   expected spec: DiagnosticSpec,
   failureHandler: (TestFailureSpec) -> Void
 ) {
@@ -533,7 +560,12 @@ public func assertMacroExpansion(
     )
   } else {
     for (actualDiag, expectedDiag) in zip(context.diagnostics, diagnostics) {
-      assertDiagnostic(actualDiag, in: context, expected: expectedDiag, failureHandler: failureHandler)
+      assertDiagnostic(
+        actualDiag,
+        in: .macroExpansion(context),
+        expected: expectedDiag,
+        failureHandler: failureHandler
+      )
     }
   }
 

Tests/SwiftIfConfigTest/Assertions.swift

@@ -0,0 +1,66 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+import SwiftDiagnostics
+import SwiftIfConfig
+import SwiftParser
+import SwiftSyntax
+@_spi(XCTestFailureLocation) import SwiftSyntaxMacrosGenericTestSupport
+import XCTest
+import _SwiftSyntaxGenericTestSupport
+import _SwiftSyntaxTestSupport
+
+/// Assert that applying the given build configuration to the source code
+/// returns the expected source and diagnostics.
+func assertRemoveInactive(
+  _ source: String,
+  configuration: any BuildConfiguration,
+  diagnostics expectedDiagnostics: [DiagnosticSpec] = [],
+  expectedSource: String,
+  file: StaticString = #filePath,
+  line: UInt = #line
+) {
+  var parser = Parser(source)
+  let tree = SourceFileSyntax.parse(from: &parser)
+
+  let (treeWithoutInactive, actualDiagnostics) = tree.removingInactive(in: configuration)
+
+  // Check the resulting tree.
+  assertStringsEqualWithDiff(
+    treeWithoutInactive.description,
+    expectedSource,
+    file: file,
+    line: line
+  )
+
+  // Check the diagnostics.
+  if actualDiagnostics.count != expectedDiagnostics.count {
+    XCTFail(
+      """
+      Expected \(expectedDiagnostics.count) diagnostics, but got \(actualDiagnostics.count):
+      \(actualDiagnostics.map(\.debugDescription).joined(separator: "\n"))
+      """,
+      file: file,
+      line: line
+    )
+  } else {
+    for (actualDiag, expectedDiag) in zip(actualDiagnostics, expectedDiagnostics) {
+      assertDiagnostic(
+        actualDiag,
+        in: .tree(tree),
+        expected: expectedDiag,
+        failureHandler: {
+          XCTFail($0.message, file: $0.location.staticFilePath, line: $0.location.unsignedLine)
+        }
+      )
+    }
+  }
+}

Tests/SwiftIfConfigTest/EvaluateTests.swift

@@ -1,3 +1,6 @@
+import SwiftIfConfig
+import SwiftParser
+import SwiftSyntax
 //===----------------------------------------------------------------------===//
 //
 // This source file is part of the Swift.org open source project
@@ -10,9 +13,6 @@
 //
 //===----------------------------------------------------------------------===//
 import XCTest
-import SwiftSyntax
-import SwiftParser
-import SwiftIfConfig
 import _SwiftSyntaxTestSupport
 
 public class EvaluateTests: XCTestCase {

Tests/SwiftIfConfigTest/TestingBuildConfiguration.swift

@@ -18,7 +18,7 @@ enum BuildConfigurationError: Error, CustomStringConvertible {
   var description: String {
     switch self {
     case .badAttribute(let attribute):
-      return "unacceptable attribute \(attribute)"
+      return "unacceptable attribute '\(attribute)'"
     }
   }
 }