Implement warning to recognize conditions that should be simulator checks

Implement a warning to recognize `#if` conditions like this:

```swift
((os(iOS) || os(tvOS)) && (arch(i386) || arch(x86_64)))
```

and suggest `targetEnvironment(simultator)` instead.

Author: DougGregor

Package.swift

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

Sources/SwiftIfConfig/CMakeLists.txt

@@ -24,5 +24,6 @@ add_swift_syntax_library(SwiftIfConfig
 
 target_link_swift_syntax_libraries(SwiftIfConfig PUBLIC
   SwiftSyntax
+  SwiftSyntaxBuilder
   SwiftDiagnostics
   SwiftOperators)

Sources/SwiftIfConfig/IfConfigError.swift

@@ -12,6 +12,7 @@
 
 import SwiftDiagnostics
 import SwiftSyntax
+import SwiftSyntaxBuilder
 
 /// Describes the kinds of errors that can occur when processing #if conditions.
 enum IfConfigError: Error, CustomStringConvertible {
@@ -29,6 +30,7 @@ enum IfConfigError: Error, CustomStringConvertible {
   case canImportTwoParameters(syntax: ExprSyntax)
   case ignoredTrailingComponents(version: VersionTuple, syntax: ExprSyntax)
   case integerLiteralCondition(syntax: ExprSyntax, replacement: Bool)
+  case likelySimulatorPlatform(syntax: ExprSyntax)
 
   var description: String {
     switch self {
@@ -75,6 +77,10 @@ enum IfConfigError: Error, CustomStringConvertible {
 
     case .integerLiteralCondition(syntax: let syntax, replacement: let replacement):
       return "'\(syntax.trimmedDescription)' is not a valid conditional compilation expression, use '\(replacement)'"
+
+    case .likelySimulatorPlatform:
+      return
+        "platform condition appears to be testing for simulator environment; use 'targetEnvironment(simulator)' instead"
     }
   }
 
@@ -93,7 +99,8 @@ enum IfConfigError: Error, CustomStringConvertible {
       .canImportLabel(syntax: let syntax),
       .canImportTwoParameters(syntax: let syntax),
       .ignoredTrailingComponents(version: _, syntax: let syntax),
-      .integerLiteralCondition(syntax: let syntax, replacement: _):
+      .integerLiteralCondition(syntax: let syntax, replacement: _),
+      .likelySimulatorPlatform(syntax: let syntax):
       return Syntax(syntax)
 
     case .unsupportedVersionOperator(name: _, operator: let op):
@@ -111,7 +118,7 @@ extension IfConfigError: DiagnosticMessage {
 
   var severity: SwiftDiagnostics.DiagnosticSeverity {
     switch self {
-    case .ignoredTrailingComponents: return .warning
+    case .ignoredTrailingComponents, .likelySimulatorPlatform: return .warning
     default: return .error
     }
   }
@@ -142,6 +149,21 @@ extension IfConfigError: DiagnosticMessage {
       )
     }
 
+    // For the likely targetEnvironment(simulator) condition we have a Fix-It.
+    if case .likelySimulatorPlatform(let syntax) = self {
+      return Diagnostic(
+        node: syntax,
+        message: self,
+        fixIt: .replace(
+          message: SimpleFixItMessage(
+            message: "replace with 'targetEnvironment(simulator)'"
+          ),
+          oldNode: syntax,
+          newNode: "targetEnvironment(simulator)" as ExprSyntax
+        )
+      )
+    }
+
     return Diagnostic(node: syntax, message: self)
   }
 }

Sources/SwiftIfConfig/IfConfigEvaluation.swift

@@ -123,6 +123,11 @@ func evaluateIfConfig(
     let op = binOp.operator.as(BinaryOperatorExprSyntax.self),
     (op.operator.text == "&&" || op.operator.text == "||")
   {
+    // Check whether this was likely to be a check for targetEnvironment(simulator).
+    if let targetEnvironmentDiag = diagnoseLikelySimulatorEnvironmentTest(binOp) {
+      extraDiagnostics.append(targetEnvironmentDiag)
+    }
+
     // Evaluate the left-hand side.
     let (lhsActive, lhssyntaxErrorsAllowed, lhsDiagnostics) = evaluateIfConfig(
       condition: binOp.leftOperand,
@@ -134,9 +139,17 @@ func evaluateIfConfig(
     if lhssyntaxErrorsAllowed {
       switch (lhsActive, op.operator.text) {
       case (true, "||"):
-        return (active: true, syntaxErrorsAllowed: lhssyntaxErrorsAllowed, diagnostics: lhsDiagnostics)
+        return (
+          active: true,
+          syntaxErrorsAllowed: lhssyntaxErrorsAllowed,
+          diagnostics: extraDiagnostics + lhsDiagnostics
+        )
       case (false, "&&"):
-        return (active: false, syntaxErrorsAllowed: lhssyntaxErrorsAllowed, diagnostics: lhsDiagnostics)
+        return (
+          active: false,
+          syntaxErrorsAllowed: lhssyntaxErrorsAllowed,
+          diagnostics: extraDiagnostics + lhsDiagnostics
+        )
       default:
         break
       }
@@ -153,14 +166,14 @@ func evaluateIfConfig(
       return (
         active: lhsActive || rhsActive,
         syntaxErrorsAllowed: lhssyntaxErrorsAllowed && rhssyntaxErrorsAllowed,
-        diagnostics: lhsDiagnostics + rhsDiagnostics
+        diagnostics: extraDiagnostics + lhsDiagnostics + rhsDiagnostics
       )
 
     case "&&":
       return (
         active: lhsActive && rhsActive,
         syntaxErrorsAllowed: lhssyntaxErrorsAllowed || rhssyntaxErrorsAllowed,
-        diagnostics: lhsDiagnostics + rhsDiagnostics
+        diagnostics: extraDiagnostics + lhsDiagnostics + rhsDiagnostics
       )
 
     default:
@@ -433,6 +446,88 @@ func evaluateIfConfig(
   return recordError(.unknownExpression(condition))
 }
 
+/// Determine whether the given condition only involves disjunctions that
+/// check the given config function against one of the provided values.
+///
+/// For example, this will match a condition like `os(iOS) ||  os(tvOS)`
+/// when passed `IfConfigFunctions.os` and `["iOS", "tvOS"]`.
+private func isConditionDisjunction(
+  _ condition: some ExprSyntaxProtocol,
+  function: IfConfigFunctions,
+  anyOf values: [String]
+) -> Bool {
+  // Recurse into disjunctions. Both sides need to match.
+  if let binOp = condition.as(InfixOperatorExprSyntax.self),
+    let op = binOp.operator.as(BinaryOperatorExprSyntax.self),
+    op.operator.text == "||"
+  {
+    return isConditionDisjunction(binOp.leftOperand, function: function, anyOf: values)
+      && isConditionDisjunction(binOp.rightOperand, function: function, anyOf: values)
+  }
+
+  // Look through parentheses.
+  if let tuple = condition.as(TupleExprSyntax.self), tuple.isParentheses,
+    let element = tuple.elements.first
+  {
+    return isConditionDisjunction(element.expression, function: function, anyOf: values)
+  }
+
+  // If we have a call to this function, check whether the argument is one of
+  // the acceptable values.
+  if let call = condition.as(FunctionCallExprSyntax.self),
+    let fnName = call.calledExpression.simpleIdentifierExpr,
+    let callFn = IfConfigFunctions(rawValue: fnName),
+    callFn == function,
+    let argExpr = call.arguments.singleUnlabeledExpression,
+    let arg = argExpr.simpleIdentifierExpr
+  {
+    return values.contains(arg)
+  }
+
+  return false
+}
+
+/// If this binary operator looks like it could be replaced by a
+/// targetEnvironment(simulator) check, produce a diagnostic that does so.
+///
+/// For example, this checks for conditions like:
+///
+/// ```
+/// #if (os(iOS) ||  os(tvOS)) && (arch(i386) || arch(x86_64))
+/// ```
+///
+/// which should be replaced with
+///
+/// ```
+/// #if targetEnvironment(simulator)
+/// ```
+private func diagnoseLikelySimulatorEnvironmentTest(
+  _ binOp: InfixOperatorExprSyntax
+) -> Diagnostic? {
+  guard let op = binOp.operator.as(BinaryOperatorExprSyntax.self),
+    op.operator.text == "&&"
+  else {
+    return nil
+  }
+
+  func isSimulatorPlatformOSTest(_ condition: ExprSyntax) -> Bool {
+    return isConditionDisjunction(condition, function: .os, anyOf: ["iOS", "tvOS", "watchOS"])
+  }
+
+  func isSimulatorPlatformArchTest(_ condition: ExprSyntax) -> Bool {
+    return isConditionDisjunction(condition, function: .arch, anyOf: ["i386", "x86_64"])
+  }
+
+  guard
+    (isSimulatorPlatformOSTest(binOp.leftOperand) && isSimulatorPlatformArchTest(binOp.rightOperand))
+      || (isSimulatorPlatformOSTest(binOp.rightOperand) && isSimulatorPlatformArchTest(binOp.leftOperand))
+  else {
+    return nil
+  }
+
+  return IfConfigError.likelySimulatorPlatform(syntax: ExprSyntax(binOp)).asDiagnostic
+}
+
 extension IfConfigClauseSyntax {
   /// Fold the operators within an #if condition, turning sequence expressions
   /// involving the various allowed operators (&&, ||, !) into well-structured

Tests/SwiftIfConfigTest/EvaluateTests.swift

@@ -278,6 +278,30 @@ public class EvaluateTests: XCTestCase {
       ]
     )
   }
+
+  func testLikelySimulatorEnvironment() throws {
+    assertIfConfig(
+      "((os(iOS) || os(tvOS)) && (arch(i386) || arch(x86_64))) && DEBUG",
+      .inactive,
+      diagnostics: [
+        DiagnosticSpec(
+          message:
+            "platform condition appears to be testing for simulator environment; use 'targetEnvironment(simulator)' instead",
+          line: 1,
+          column: 2,
+          severity: .warning,
+          fixIts: [
+            FixItSpec(message: "replace with 'targetEnvironment(simulator)'")
+          ]
+        )
+      ]
+    )
+
+    assertIfConfig(
+      "((os(iOS) || os(tvOS)) && (arch(arm64) || arch(x86_64))) && DEBUG",
+      .inactive
+    )
+  }
 }
 
 /// Assert the results of evaluating the condition within an `#if` against the