diff --git a/Sources/SwiftFormat/Pipelines+Generated.swift b/Sources/SwiftFormat/Pipelines+Generated.swift index f2e9f8f72..6701a0a96 100644 --- a/Sources/SwiftFormat/Pipelines+Generated.swift +++ b/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) diff --git a/Sources/SwiftFormatConfiguration/RuleRegistry+Generated.swift b/Sources/SwiftFormatConfiguration/RuleRegistry+Generated.swift index 776990d1e..6264ade27 100644 --- a/Sources/SwiftFormatConfiguration/RuleRegistry+Generated.swift +++ b/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, diff --git a/Sources/SwiftFormatCore/Trivia+Convenience.swift b/Sources/SwiftFormatCore/Trivia+Convenience.swift index ad380a3e6..f69091b88 100644 --- a/Sources/SwiftFormatCore/Trivia+Convenience.swift +++ b/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( diff --git a/Sources/SwiftFormatRules/NoAssignmentInExpressions.swift b/Sources/SwiftFormatRules/NoAssignmentInExpressions.swift new file mode 100644 index 000000000..16ddb416f --- /dev/null +++ b/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 ` expressions as `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" +} diff --git a/Sources/SwiftFormatRules/RuleNameCache+Generated.swift b/Sources/SwiftFormatRules/RuleNameCache+Generated.swift index 56f5c3a12..ec75e73b1 100644 --- a/Sources/SwiftFormatRules/RuleNameCache+Generated.swift +++ b/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", diff --git a/Tests/SwiftFormatRulesTests/NoAssignmentInExpressionsTests.swift b/Tests/SwiftFormatRulesTests/NoAssignmentInExpressionsTests.swift new file mode 100644 index 000000000..238c2f92f --- /dev/null +++ b/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) + } +}