Merge pull request swiftlang#442 from allevato/no-assignment-expr-rule

Add a rule that prevents assignment from appearing in expression contexts.

Author: allevato

Sources/SwiftFormat/Pipelines+Generated.swift

@@ -63,6 +63,7 @@ class LintPipeline: SyntaxVisitor {
 
   override func visit(_ node: CodeBlockItemListSyntax) -> SyntaxVisitorContinueKind {
     visitIfEnabled(DoNotUseSemicolons.visit, for: node)
+    visitIfEnabled(NoAssignmentInExpressions.visit, for: node)
     visitIfEnabled(OneVariableDeclarationPerLine.visit, for: node)
     visitIfEnabled(UseEarlyExits.visit, for: node)
     return .visitChildren
@@ -165,6 +166,11 @@ class LintPipeline: SyntaxVisitor {
     return .visitChildren
   }
 
+  override func visit(_ node: InfixOperatorExprSyntax) -> SyntaxVisitorContinueKind {
+    visitIfEnabled(NoAssignmentInExpressions.visit, for: node)
+    return .visitChildren
+  }
+
   override func visit(_ node: InitializerDeclSyntax) -> SyntaxVisitorContinueKind {
     visitIfEnabled(AllPublicDeclarationsHaveDocumentation.visit, for: node)
     visitIfEnabled(BeginDocumentationCommentWithOneLineSummary.visit, for: node)
@@ -312,6 +318,7 @@ extension FormatPipeline {
     node = FullyIndirectEnum(context: context).visit(node)
     node = GroupNumericLiterals(context: context).visit(node)
     node = NoAccessLevelOnExtensionDeclaration(context: context).visit(node)
+    node = NoAssignmentInExpressions(context: context).visit(node)
     node = NoCasesWithOnlyFallthrough(context: context).visit(node)
     node = NoEmptyTrailingClosureParentheses(context: context).visit(node)
     node = NoLabelsInCasePatterns(context: context).visit(node)

Sources/SwiftFormatConfiguration/RuleRegistry+Generated.swift

@@ -28,6 +28,7 @@ enum RuleRegistry {
     "NeverUseForceTry": false,
     "NeverUseImplicitlyUnwrappedOptionals": false,
     "NoAccessLevelOnExtensionDeclaration": true,
+    "NoAssignmentInExpressions": true,
     "NoBlockComments": true,
     "NoCasesWithOnlyFallthrough": true,
     "NoEmptyTrailingClosureParentheses": true,

Sources/SwiftFormatCore/Trivia+Convenience.swift

@@ -44,6 +44,16 @@ extension Trivia {
       })
   }
 
+  /// Returns this set of trivia, without any leading spaces.
+  public func withoutLeadingSpaces() -> Trivia {
+    return Trivia(
+      pieces: Array(drop {
+        if case .spaces = $0 { return false }
+        if case .tabs = $0 { return false }
+        return true
+      }))
+  }
+
   /// Returns this set of trivia, without any newlines.
   public func withoutNewlines() -> Trivia {
     return Trivia(

Sources/SwiftFormatRules/NoAssignmentInExpressions.swift

@@ -0,0 +1,116 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2019 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 SwiftFormatCore
+import SwiftSyntax
+
+/// Assignment expressions must be their own statements.
+///
+/// Assignment should not be used in an expression context that expects a `Void` value. For example,
+/// assigning a variable within a `return` statement existing a `Void` function is prohibited.
+///
+/// Lint: If an assignment expression is found in a position other than a standalone statement, a
+///       lint finding is emitted.
+///
+/// Format: A `return` statement containing an assignment expression is expanded into two separate
+///         statements.
+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) && node.parent?.is(CodeBlockItemSyntax.self) == false {
+      diagnose(.moveAssignmentToOwnStatement, on: node)
+    }
+    return ExprSyntax(node)
+  }
+
+  public override func visit(_ node: CodeBlockItemListSyntax) -> CodeBlockItemListSyntax {
+    var newItems = [CodeBlockItemSyntax]()
+    newItems.reserveCapacity(node.count)
+
+    for item in node {
+      // Make sure to visit recursively so that any nested decls get processed first.
+      let newItem = visit(item)
+
+      // Rewrite any `return <assignment>` expressions as `<assignment><newline>return`.
+      switch newItem.item {
+      case .stmt(let stmt):
+        guard
+          let returnStmt = stmt.as(ReturnStmtSyntax.self),
+          let assignmentExpr = assignmentExpression(from: returnStmt)
+        else {
+          // Head to the default case where we just keep the original item.
+          fallthrough
+        }
+
+        // Move the leading trivia from the `return` statement to the new assignment statement,
+        // since that's a more sensible place than between the two.
+        newItems.append(
+          CodeBlockItemSyntax(
+            item: .expr(ExprSyntax(assignmentExpr)),
+            semicolon: nil,
+            errorTokens: nil
+          )
+          .withLeadingTrivia(
+            (returnStmt.leadingTrivia ?? []) + (assignmentExpr.leadingTrivia ?? []))
+          .withTrailingTrivia([]))
+        newItems.append(
+          CodeBlockItemSyntax(
+            item: .stmt(StmtSyntax(returnStmt.withExpression(nil))),
+            semicolon: nil,
+            errorTokens: nil
+          )
+          .withLeadingTrivia([.newlines(1)])
+          .withTrailingTrivia(returnStmt.trailingTrivia?.withoutLeadingSpaces() ?? []))
+
+      default:
+        newItems.append(newItem)
+      }
+    }
+
+    return CodeBlockItemListSyntax(newItems)
+  }
+
+  /// Extracts and returns the assignment expression in the given `return` statement, if there was
+  /// one.
+  ///
+  /// If the `return` statement did not have an expression or if its expression was not an
+  /// assignment expression, nil is returned.
+  private func assignmentExpression(from returnStmt: ReturnStmtSyntax) -> InfixOperatorExprSyntax? {
+    guard
+      let returnExpr = returnStmt.expression,
+      let infixOperatorExpr = returnExpr.as(InfixOperatorExprSyntax.self)
+    else {
+      return nil
+    }
+    return isAssignmentExpression(infixOperatorExpr) ? infixOperatorExpr : nil
+  }
+
+  /// Returns a value indicating whether the given infix operator expression is an assignment
+  /// expression (either simple assignment with `=` or compound assignment with an operator like
+  /// `+=`).
+  private func isAssignmentExpression(_ expr: InfixOperatorExprSyntax) -> Bool {
+    if expr.operatorOperand.is(AssignmentExprSyntax.self) {
+      return true
+    }
+    guard let binaryOp = expr.operatorOperand.as(BinaryOperatorExprSyntax.self) else {
+      return false
+    }
+    return context.operatorTable.infixOperator(named: binaryOp.operatorToken.text)?.precedenceGroup
+      == "AssignmentPrecedence"
+  }
+}
+
+extension Finding.Message {
+  public static let moveAssignmentToOwnStatement: Finding.Message =
+    "move assignment expression into its own statement"
+}

Sources/SwiftFormatRules/RuleNameCache+Generated.swift

@@ -28,6 +28,7 @@ public let ruleNameCache: [ObjectIdentifier: String] = [
   ObjectIdentifier(NeverUseForceTry.self): "NeverUseForceTry",
   ObjectIdentifier(NeverUseImplicitlyUnwrappedOptionals.self): "NeverUseImplicitlyUnwrappedOptionals",
   ObjectIdentifier(NoAccessLevelOnExtensionDeclaration.self): "NoAccessLevelOnExtensionDeclaration",
+  ObjectIdentifier(NoAssignmentInExpressions.self): "NoAssignmentInExpressions",
   ObjectIdentifier(NoBlockComments.self): "NoBlockComments",
   ObjectIdentifier(NoCasesWithOnlyFallthrough.self): "NoCasesWithOnlyFallthrough",
   ObjectIdentifier(NoEmptyTrailingClosureParentheses.self): "NoEmptyTrailingClosureParentheses",

Tests/SwiftFormatRulesTests/NoAssignmentInExpressionsTests.swift

@@ -0,0 +1,164 @@
+import SwiftFormatRules
+
+final class NoAssignmentInExpressionsTests: LintOrFormatRuleTestCase {
+  func testAssignmentInExpressionContextIsDiagnosed() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        foo(bar, baz = quux, a + b)
+        """,
+      expected: """
+        foo(bar, baz = quux, a + b)
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 1, column: 10)
+    // Make sure no other expressions were diagnosed.
+    XCTAssertNotDiagnosed(.moveAssignmentToOwnStatement)
+  }
+
+  func testReturnStatementWithoutExpressionIsUnchanged() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return
+        }
+        """,
+      expected: """
+        func foo() {
+          return
+        }
+        """
+    )
+    XCTAssertNotDiagnosed(.moveAssignmentToOwnStatement)
+  }
+
+  func testReturnStatementWithNonAssignmentExpressionIsUnchanged() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return a + b
+        }
+        """,
+      expected: """
+        func foo() {
+          return a + b
+        }
+        """
+    )
+    XCTAssertNotDiagnosed(.moveAssignmentToOwnStatement)
+  }
+
+  func testReturnStatementWithSimpleAssignmentExpressionIsExpanded() {
+    // For this and similar tests below, we don't try to match the leading indentation in the new
+    // `return` statement; the pretty-printer will fix it up.
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return a = b
+        }
+        """,
+      expected: """
+        func foo() {
+          a = b
+        return
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 2, column: 10)
+  }
+
+  func testReturnStatementWithCompoundAssignmentExpressionIsExpanded() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return a += b
+        }
+        """,
+      expected: """
+        func foo() {
+          a += b
+        return
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 2, column: 10)
+  }
+
+  func testReturnStatementWithAssignmentDealsWithLeadingLineCommentSensibly() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          // some comment
+          return a = b
+        }
+        """,
+      expected: """
+        func foo() {
+          // some comment
+          a = b
+        return
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 3, column: 10)
+  }
+
+  func testReturnStatementWithAssignmentDealsWithTrailingLineCommentSensibly() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return a = b  // some comment
+        }
+        """,
+      expected: """
+        func foo() {
+          a = b
+        return  // some comment
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 2, column: 10)
+  }
+
+  func testReturnStatementWithAssignmentDealsWithTrailingBlockCommentSensibly() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return a = b  /* some comment */
+        }
+        """,
+      expected: """
+        func foo() {
+          a = b
+        return  /* some comment */
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 2, column: 10)
+  }
+
+  func testReturnStatementWithAssignmentDealsWithNestedBlockCommentSensibly() {
+    XCTAssertFormatting(
+      NoAssignmentInExpressions.self,
+      input: """
+        func foo() {
+          return /* some comment */ a = b
+        }
+        """,
+      expected: """
+        func foo() {
+          /* some comment */ a = b
+        return
+        }
+        """
+    )
+    XCTAssertDiagnosed(.moveAssignmentToOwnStatement, line: 2, column: 29)
+  }
+}