Teach #if condition evaluation to reason about "versioned" checks

When a check is "versioned" and fails, we allow the failed #if block to
have syntactic errors in it. Start to reflect this distinction when
determining the `IfConfigState` for a particular condition, so that we
finally use the "unparsed" case.

Author: DougGregor

Sources/SwiftIfConfig/IfConfigEvaluation.swift

@@ -249,12 +249,15 @@ extension VersionTuple {
 ///     build configuration itself.
 /// - Throws: Throws if an error occurs occur during evaluation. The error will
 ///   also be provided to the diagnostic handler before doing so.
-/// - Returns: Whether the condition holds with the given build configuration.
+/// - Returns: A pair of Boolean values. The first describes whether the
+///   condition holds with the given build configuration. The second whether
+///   the build condition is a "versioned" check that implies that we shouldn't
+///   diagnose syntax errors in blocks where the check fails.
 private func evaluateIfConfig(
   condition: ExprSyntax,
   configuration: some BuildConfiguration,
   diagnosticHandler: ((Diagnostic) -> Void)?
-) throws -> Bool {
+) throws -> (active: Bool, versioned: Bool) {
   /// Record the error before returning it. Use this for every 'throw' site
   /// in this evaluation.
   func recordedError(_ error: any Error, at node: some SyntaxProtocol) -> any Error {
@@ -275,8 +278,8 @@ private func evaluateIfConfig(
   /// appropriate diagnostic for the handler before rethrowing it.
   func checkConfiguration(
     at node: some SyntaxProtocol,
-    body: () throws -> Bool
-  ) throws -> Bool {
+    body: () throws -> (Bool, Bool)
+  ) throws -> (active: Bool, versioned: Bool) {
     do {
       return try body()
     } catch let error {
@@ -286,7 +289,7 @@ private func evaluateIfConfig(
 
   // Boolean literals evaluate as-is
   if let boolLiteral = condition.as(BooleanLiteralExprSyntax.self) {
-    return boolLiteral.literalValue
+    return (active: boolLiteral.literalValue, versioned: false)
   }
 
   // Integer literals aren't allowed, but we recognize them.
@@ -302,7 +305,7 @@ private func evaluateIfConfig(
       ).asDiagnostic
     )
 
-    return result
+    return (active: result, versioned: false)
   }
 
   // Declaration references are for custom compilation flags.
@@ -312,19 +315,21 @@ private func evaluateIfConfig(
 
     // Evaluate the custom condition. If the build configuration cannot answer this query, fail.
     return try checkConfiguration(at: identExpr) {
-      try configuration.isCustomConditionSet(name: ident)
+      (active: try configuration.isCustomConditionSet(name: ident), versioned: false)
     }
   }
 
   // Logical '!'.
   if let prefixOp = condition.as(PrefixOperatorExprSyntax.self),
     prefixOp.operator.text == "!"
   {
-    return try !evaluateIfConfig(
+    let (innerActive, innerVersioned) = try evaluateIfConfig(
       condition: prefixOp.expression,
       configuration: configuration,
       diagnosticHandler: diagnosticHandler
     )
+
+    return (active: !innerActive, versioned: innerVersioned)
   }
 
   // Logical '&&' and '||'.
@@ -333,25 +338,45 @@ private func evaluateIfConfig(
     (op.operator.text == "&&" || op.operator.text == "||")
   {
     // Evaluate the left-hand side.
-    let lhsResult = try evaluateIfConfig(
+    let (lhsActive, lhsVersioned) = try evaluateIfConfig(
       condition: binOp.leftOperand,
       configuration: configuration,
       diagnosticHandler: diagnosticHandler
     )
 
-    // Short-circuit evaluation if we know the answer.
-    switch (lhsResult, op.operator.text) {
-    case (true, "||"): return true
-    case (false, "&&"): return false
-    default: break
+    // Short-circuit evaluation if we know the answer and the left-hand side
+    // was versioned.
+    if lhsVersioned {
+      switch (lhsActive, op.operator.text) {
+      case (true, "||"): return (active: true, versioned: lhsVersioned)
+      case (false, "&&"): return (active: false, versioned: lhsVersioned)
+      default: break
+      }
     }
 
-    // Evaluate the right-hand side and use that result.
-    return try evaluateIfConfig(
+    // Evaluate the right-hand side.
+    let (rhsActive, rhsVersioned) = try evaluateIfConfig(
       condition: binOp.rightOperand,
       configuration: configuration,
       diagnosticHandler: diagnosticHandler
     )
+
+    switch op.operator.text {
+    case "||":
+      return (
+        active: lhsActive || rhsActive,
+        versioned: lhsVersioned && rhsVersioned
+      )
+
+    case "&&":
+      return (
+        active: lhsActive && rhsActive,
+        versioned: lhsVersioned || rhsVersioned
+      )
+
+    default:
+      fatalError("prevented by condition for getting here")
+    }
   }
 
   // Look through parentheses.
@@ -371,7 +396,10 @@ private func evaluateIfConfig(
     let fn = IfConfigFunctions(rawValue: fnName)
   {
     /// Perform a check for an operation that takes a single identifier argument.
-    func doSingleIdentifierArgumentCheck(_ body: (String) throws -> Bool, role: String) throws -> Bool {
+    func doSingleIdentifierArgumentCheck(
+      _ body: (String) throws -> Bool,
+      role: String
+    ) throws -> (active: Bool, versioned: Bool) {
       // Ensure that we have a single argument that is a simple identifier.
       guard let argExpr = call.arguments.singleUnlabeledExpression,
         let arg = argExpr.simpleIdentifierExpr
@@ -382,12 +410,14 @@ private func evaluateIfConfig(
       }
 
       return try checkConfiguration(at: argExpr) {
-        try body(arg)
+        (active: try body(arg), versioned: fn.isVersioned)
       }
     }
 
     /// Perform a check for a version constraint as used in the "swift" or "compiler" version checks.
-    func doVersionComparisonCheck(_ actualVersion: VersionTuple) throws -> Bool {
+    func doVersionComparisonCheck(
+      _ actualVersion: VersionTuple
+    ) throws -> (active: Bool, versioned: Bool) {
       // Ensure that we have a single unlabeled argument that is either >= or < as a prefix
       // operator applied to a version.
       guard let argExpr = call.arguments.singleUnlabeledExpression,
@@ -410,9 +440,9 @@ private func evaluateIfConfig(
 
       switch opToken.text {
       case ">=":
-        return actualVersion >= version
+        return (active: actualVersion >= version, versioned: fn.isVersioned)
       case "<":
-        return actualVersion < version
+        return (active: actualVersion < version, versioned: fn.isVersioned)
       default:
         throw recordedError(.unsupportedVersionOperator(name: fnName, operator: opToken))
       }
@@ -459,7 +489,10 @@ private func evaluateIfConfig(
         )
       }
 
-      return configuration.endianness == expectedEndianness
+      return (
+        active: configuration.endianness == expectedEndianness,
+        versioned: fn.isVersioned
+      )
 
     case ._pointerBitWidth:
       // Ensure that we have a single argument that is a simple identifier, which
@@ -479,7 +512,10 @@ private func evaluateIfConfig(
         )
       }
 
-      return configuration.targetPointerBitWidth == expectedPointerBitWidth
+      return (
+        active: configuration.targetPointerBitWidth == expectedPointerBitWidth,
+        versioned: fn.isVersioned
+      )
 
     case .swift:
       return try doVersionComparisonCheck(configuration.languageVersion)
@@ -508,7 +544,10 @@ private func evaluateIfConfig(
       let versionString = stringSegment.content.text
       let expectedVersion = try VersionTuple(parsingCompilerBuildVersion: versionString, argExpr)
 
-      return configuration.compilerVersion >= expectedVersion
+      return (
+        active: configuration.compilerVersion >= expectedVersion,
+        versioned: fn.isVersioned
+      )
 
     case .canImport:
       // Retrieve the first argument, which must not have a label. This is
@@ -577,9 +616,12 @@ private func evaluateIfConfig(
       }
 
       return try checkConfiguration(at: call) {
-        try configuration.canImport(
-          importPath: importPath.map { String($0) },
-          version: version
+        (
+          active: try configuration.canImport(
+            importPath: importPath.map { String($0) },
+            version: version
+          ),
+          versioned: fn.isVersioned
         )
       }
     }
@@ -602,12 +644,17 @@ extension IfConfigState {
       throw error
     }.cast(ExprSyntax.self)
 
-    let result = try evaluateIfConfig(
+    let (active, versioned) = try evaluateIfConfig(
       condition: foldedCondition,
       configuration: configuration,
       diagnosticHandler: diagnosticHandler
     )
-    self = result ? .active : .inactive
+
+    switch (active, versioned) {
+    case (true, _): self = .active
+    case (false, false): self = .inactive
+    case (false, true): self = .unparsed
+    }
   }
 }
 
@@ -644,7 +691,7 @@ extension IfConfigDeclSyntax {
         condition: condition,
         configuration: configuration,
         diagnosticHandler: diagnosticHandler
-      ) {
+      ).active {
         return clause
       }
     }

Sources/SwiftIfConfig/IfConfigFunctions.swift

@@ -52,4 +52,17 @@ enum IfConfigFunctions: String {
   /// A check for the target's pointer authentication scheme (e.g., _arm64e)
   /// via `_ptrauth(<name>)`.
   case _ptrauth
+
+  /// Whether uses of this function consistute a "versioned" check. Such checks
+  /// suppress parser diagnostics if the block failed.
+  var isVersioned: Bool {
+    switch self {
+    case .swift, .compiler, ._compiler_version:
+      return true
+
+    case .hasAttribute, .hasFeature, .canImport, .os, .arch, .targetEnvironment,
+      ._endian, ._pointerBitWidth, ._runtime, ._ptrauth:
+      return false
+    }
+  }
 }

Sources/SwiftIfConfig/IfConfigState.swift

@@ -12,10 +12,8 @@
 
 /// Describes the state of a particular region guarded by `#if` or similar.
 public enum IfConfigState {
-  /// The region is not parsed, and may contain syntax that is invalid.
-  ///
-  /// TODO: For now, the IfConfig library does not distinguish between
-  /// inactive and unparsed regions, so this case is never used.
+  /// The region is not part of the compiled program and is not even parsed,
+  /// and therefore many contain syntax that is invalid.
   case unparsed
   /// The region is parsed but is not part of the compiled program.
   case inactive

Tests/SwiftIfConfigTest/EvaluateTests.swift

@@ -77,13 +77,46 @@ public class EvaluateTests: XCTestCase {
     assertIfConfig("DEBUG && ASSERTS", .active, configuration: buildConfig)
     assertIfConfig("DEBUG && nope", .inactive, configuration: buildConfig)
     assertIfConfig("nope && DEBUG", .inactive, configuration: buildConfig)
-    assertIfConfig("nope && 3.14159", .inactive, configuration: buildConfig)
+    assertIfConfig(
+      "nope && 3.14159",
+      nil,
+      configuration: buildConfig,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "invalid conditional compilation expression",
+          line: 1,
+          column: 9
+        )
+      ]
+    )
     assertIfConfig("DEBUG || ASSERTS", .active, configuration: buildConfig)
     assertIfConfig("DEBUG || nope", .active, configuration: buildConfig)
     assertIfConfig("nope || DEBUG", .active, configuration: buildConfig)
     assertIfConfig("nope || !DEBUG", .inactive, configuration: buildConfig)
-    assertIfConfig("DEBUG || 3.14159", .active, configuration: buildConfig)
-    assertIfConfig("(DEBUG) || 3.14159", .active, configuration: buildConfig)
+    assertIfConfig(
+      "DEBUG || 3.14159",
+      nil,
+      configuration: buildConfig,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "invalid conditional compilation expression",
+          line: 1,
+          column: 10
+        )
+      ]
+    )
+    assertIfConfig(
+      "(DEBUG) || 3.14159",
+      nil,
+      configuration: buildConfig,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "invalid conditional compilation expression",
+          line: 1,
+          column: 12
+        )
+      ]
+    )
   }
 
   func testBadExpressions() throws {
@@ -135,15 +168,39 @@ public class EvaluateTests: XCTestCase {
   }
 
   func testVersions() throws {
-    assertIfConfig("swift(>=5.5", .active)
-    assertIfConfig("swift(<6", .active)
-    assertIfConfig("swift(>=6", .inactive)
-    assertIfConfig("compiler(>=5.8", .active)
-    assertIfConfig("compiler(>=5.9", .active)
-    assertIfConfig("compiler(>=5.10", .inactive)
+    assertIfConfig("swift(>=5.5)", .active)
+    assertIfConfig("swift(<6)", .active)
+    assertIfConfig("swift(>=6)", .unparsed)
+    assertIfConfig("compiler(>=5.8)", .active)
+    assertIfConfig("compiler(>=5.9)", .active)
+    assertIfConfig("compiler(>=5.10)", .unparsed)
     assertIfConfig(#"_compiler_version("5009.*.1")"#, .active)
-    assertIfConfig(#"_compiler_version("5009.*.3.2.3")"#, .inactive)
-    assertIfConfig(#"_compiler_version("5010.*.0")"#, .inactive)
+    assertIfConfig(#"_compiler_version("5009.*.3.2.3")"#, .unparsed)
+    assertIfConfig(#"_compiler_version("5010.*.0")"#, .unparsed)
+    assertIfConfig("compiler(>=5.10) && 3.14159", .unparsed)
+    assertIfConfig(
+      "compiler(>=5.10) || 3.14159",
+      nil,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "invalid conditional compilation expression",
+          line: 1,
+          column: 21
+        )
+      ]
+    )
+    assertIfConfig("compiler(>=5.9) || 3.14159", .active)
+    assertIfConfig(
+      "compiler(>=5.9) && 3.14159",
+      nil,
+      diagnostics: [
+        DiagnosticSpec(
+          message: "invalid conditional compilation expression",
+          line: 1,
+          column: 20
+        )
+      ]
+    )
   }
 
   func testCanImport() throws {