Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow exceptions to NoAssignmentInExpressions. #535

Merged
merged 1 commit into from
May 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Sources/SwiftFormatConfiguration/Configuration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public struct Configuration: Codable, Equatable {
case indentSwitchCaseLabels
case rules
case spacesAroundRangeFormationOperators
case noAssignmentInExpressions
}

/// The version of this configuration.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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"
]
}
30 changes: 29 additions & 1 deletion Sources/SwiftFormatRules/NoAssignmentInExpressions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//

import SwiftFormatConfiguration
import SwiftFormatCore
import SwiftSyntax

Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}