Merge pull request swiftlang#535 from allevato/xctnothrow-assignment-…

…fixes

Allow exceptions to `NoAssignmentInExpressions`.

Sources/SwiftFormatConfiguration/Configuration.swift

@@ -36,6 +36,7 @@ public struct Configuration: Codable, Equatable {
     case indentSwitchCaseLabels
     case rules
     case spacesAroundRangeFormationOperators
+    case noAssignmentInExpressions
   }
 
   /// The version of this configuration.
@@ -147,6 +148,9 @@ public struct Configuration: Codable, Equatable {
   /// `...` and `..<`.
   public var spacesAroundRangeFormationOperators = false
 
+  /// Contains exceptions for the `NoAssignmentInExpressions` rule.
+  public var noAssignmentInExpressions = NoAssignmentInExpressionsConfiguration()
+
   /// Constructs a Configuration with all default values.
   public init() {
     self.version = highestSupportedConfigurationVersion
@@ -208,6 +212,10 @@ public struct Configuration: Codable, Equatable {
       ?? FileScopedDeclarationPrivacyConfiguration()
     self.indentSwitchCaseLabels
       = try container.decodeIfPresent(Bool.self, forKey: .indentSwitchCaseLabels) ?? false
+    self.noAssignmentInExpressions =
+      try container.decodeIfPresent(
+        NoAssignmentInExpressionsConfiguration.self, forKey: .noAssignmentInExpressions)
+      ?? NoAssignmentInExpressionsConfiguration()
 
     // If the `rules` key is not present at all, default it to the built-in set
     // so that the behavior is the same as if the configuration had been
@@ -238,6 +246,7 @@ public struct Configuration: Codable, Equatable {
       spacesAroundRangeFormationOperators, forKey: .spacesAroundRangeFormationOperators)
     try container.encode(fileScopedDeclarationPrivacy, forKey: .fileScopedDeclarationPrivacy)
     try container.encode(indentSwitchCaseLabels, forKey: .indentSwitchCaseLabels)
+    try container.encode(noAssignmentInExpressions, forKey: .noAssignmentInExpressions)
     try container.encode(rules, forKey: .rules)
   }
 
@@ -287,3 +296,15 @@ public struct FileScopedDeclarationPrivacyConfiguration: Codable, Equatable {
   /// private access.
   public var accessLevel: AccessLevel = .private
 }
+
+/// Configuration for the `NoAssignmentInExpressions` rule.
+public struct NoAssignmentInExpressionsConfiguration: Codable, Equatable {
+  /// A list of function names where assignments are allowed to be embedded in expressions that are
+  /// passed as parameters to that function.
+  public var allowedFunctions: [String] = [
+    // Allow `XCTAssertNoThrow` because `XCTAssertNoThrow(x = try ...)` is clearer about intent than
+    // `x = try XCTUnwrap(try? ...)` or force-unwrapped if you need to use the value `x` later on
+    // in the test.
+    "XCTAssertNoThrow"
+  ]
+}

Sources/SwiftFormatRules/NoAssignmentInExpressions.swift

@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+import SwiftFormatConfiguration
 import SwiftFormatCore
 import SwiftSyntax
 
@@ -27,7 +28,10 @@ public final class NoAssignmentInExpressions: SyntaxFormatRule {
   public override func visit(_ node: InfixOperatorExprSyntax) -> ExprSyntax {
     // Diagnose any assignment that isn't directly a child of a `CodeBlockItem` (which would be the
     // case if it was its own statement).
-    if isAssignmentExpression(node) && !isStandaloneAssignmentStatement(node) {
+    if isAssignmentExpression(node)
+      && !isStandaloneAssignmentStatement(node)
+      && !isInAllowedFunction(node)
+    {
       diagnose(.moveAssignmentToOwnStatement, on: node)
     }
     return ExprSyntax(node)
@@ -131,6 +135,30 @@ public final class NoAssignmentInExpressions: SyntaxFormatRule {
     }
     return parent.is(CodeBlockItemSyntax.self)
   }
+
+  /// Returns true if the infix operator expression is in the (non-closure) parameters of an allowed
+  /// function call.
+  private func isInAllowedFunction(_ node: InfixOperatorExprSyntax) -> Bool {
+    let allowedFunctions = context.configuration.noAssignmentInExpressions.allowedFunctions
+    // Walk up the tree until we find a FunctionCallExprSyntax, and if the name matches, return
+    // true. However, stop early if we hit a CodeBlockItemSyntax first; this would represent a
+    // closure context where we *don't* want the exception to apply (for example, in
+    // `someAllowedFunction(a, b) { return c = d }`, the `c = d` is a descendent of a function call
+    // but we want it to be evaluated in its own context.
+    var node = Syntax(node)
+    while let parent = node.parent {
+      node = parent
+      if node.is(CodeBlockItemSyntax.self) {
+        break
+      }
+      if let functionCallExpr = node.as(FunctionCallExprSyntax.self),
+        allowedFunctions.contains(functionCallExpr.calledExpression.trimmedDescription)
+      {
+        return true
+      }
+    }
+    return false
+  }
 }
 
 extension Finding.Message {

Tests/SwiftFormatRulesTests/NoAssignmentInExpressionsTests.swift

@@ -180,4 +180,35 @@ final class NoAssignmentInExpressionsTests: LintOrFormatRuleTestCase {
     )
     XCTAssertNotDiagnosed(.moveAssignmentToOwnStatement)
   }
+
+  func testAssignmentExpressionsInAllowedFunctions() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        // These should not diagnose.
+        XCTAssertNoThrow(a = try b())
+        XCTAssertNoThrow { a = try b() }
+        XCTAssertNoThrow({ a = try b() })
+        someRegularFunction({ a = b })
+        someRegularFunction { a = b }
+
+        // This should be diagnosed.
+        someRegularFunction(a = b)
+        """,
+      expected: """
+        // These should not diagnose.
+        XCTAssertNoThrow(a = try b())
+        XCTAssertNoThrow { a = try b() }
+        XCTAssertNoThrow({ a = try b() })
+        someRegularFunction({ a = b })
+        someRegularFunction { a = b }
+
+        // This should be diagnosed.
+        someRegularFunction(a = b)
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 9, column: 21)
+    // Make sure no other expressions were diagnosed.
+    XCTAssertNotDiagnosed(.moveAssignmentToOwnStatement)
+  }
 }