Skip to content

Add the UseExplicitNilCheckInConditions rule. #627

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

Merged
merged 1 commit into from
Sep 18, 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
27 changes: 27 additions & 0 deletions Documentation/RuleDocumentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ automatically.
Here's the list of available rules:

- [AllPublicDeclarationsHaveDocumentation](#AllPublicDeclarationsHaveDocumentation)
- [AlwaysUseLiteralForEmptyCollectionInit](#AlwaysUseLiteralForEmptyCollectionInit)
- [AlwaysUseLowerCamelCase](#AlwaysUseLowerCamelCase)
- [AmbiguousTrailingClosureOverload](#AmbiguousTrailingClosureOverload)
- [BeginDocumentationCommentWithOneLineSummary](#BeginDocumentationCommentWithOneLineSummary)
Expand Down Expand Up @@ -43,6 +44,7 @@ Here's the list of available rules:
- [ReturnVoidInsteadOfEmptyTuple](#ReturnVoidInsteadOfEmptyTuple)
- [TypeNamesShouldBeCapitalized](#TypeNamesShouldBeCapitalized)
- [UseEarlyExits](#UseEarlyExits)
- [UseExplicitNilCheckInConditions](#UseExplicitNilCheckInConditions)
- [UseLetInEveryBoundCaseVariable](#UseLetInEveryBoundCaseVariable)
- [UseShorthandTypeNames](#UseShorthandTypeNames)
- [UseSingleLinePropertyGetter](#UseSingleLinePropertyGetter)
Expand All @@ -59,6 +61,17 @@ Lint: If a public declaration is missing a documentation comment, a lint error i

`AllPublicDeclarationsHaveDocumentation` is a linter-only rule.

### AlwaysUseLiteralForEmptyCollectionInit

Never use `[<Type>]()` syntax. In call sites that should be replaced with `[]`,
for initializations use explicit type combined with empty array literal `let _: [<Type>] = []`
Static properties of a type that return that type should not include a reference to their type.

Lint: Non-literal empty array initialization will yield a lint error.
Format: All invalid use sites would be related with empty literal (with or without explicit type annotation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"would be replaced with empty literal..."?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a typo from earlier, there was a race between the generate-the-docs PR and the one adding this rule so it didn't get generated the first time. I'll update that in a separate follow-up because I need to do a pass over all the rule docs anyway to make sure they're formatted nicely in the new Markdown file.


`AlwaysUseLiteralForEmptyCollectionInit` rule can format your code automatically.

### AlwaysUseLowerCamelCase

All values should be written in lower camel-case (`lowerCamelCase`).
Expand Down Expand Up @@ -446,6 +459,20 @@ Format: `if ... else { return/throw/break/continue }` constructs will be replace

`UseEarlyExits` rule can format your code automatically.

### UseExplicitNilCheckInConditions

When checking an optional value for `nil`-ness, prefer writing an explicit `nil` check rather
than binding and immediately discarding the value.

For example, `if let _ = someValue { ... }` is forbidden. Use `if someValue != nil { ... }`
instead.

Lint: `let _ = expr` inside a condition list will yield a lint error.

Format: `let _ = expr` inside a condition list will be replaced by `expr != nil`.

`UseExplicitNilCheckInConditions` rule can format your code automatically.

### UseLetInEveryBoundCaseVariable

Every variable bound in a `case` pattern must have its own `let/var`.
Expand Down
1 change: 1 addition & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ let package = Package(
dependencies: [
.product(name: "Markdown", package: "swift-markdown"),
.product(name: "SwiftSyntax", package: "swift-syntax"),
.product(name: "SwiftSyntaxBuilder", package: "swift-syntax"),
.product(name: "SwiftOperators", package: "swift-syntax"),
.product(name: "SwiftParser", package: "swift-syntax"),
.product(name: "SwiftParserDiagnostics", package: "swift-syntax"),
Expand Down
2 changes: 2 additions & 0 deletions Sources/SwiftFormat/Core/Pipelines+Generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class LintPipeline: SyntaxVisitor {

override func visit(_ node: ConditionElementSyntax) -> SyntaxVisitorContinueKind {
visitIfEnabled(NoParensAroundConditions.visit, for: node)
visitIfEnabled(UseExplicitNilCheckInConditions.visit, for: node)
return .visitChildren
}

Expand Down Expand Up @@ -376,6 +377,7 @@ extension FormatPipeline {
node = OrderedImports(context: context).rewrite(node)
node = ReturnVoidInsteadOfEmptyTuple(context: context).rewrite(node)
node = UseEarlyExits(context: context).rewrite(node)
node = UseExplicitNilCheckInConditions(context: context).rewrite(node)
node = UseShorthandTypeNames(context: context).rewrite(node)
node = UseSingleLinePropertyGetter(context: context).rewrite(node)
node = UseTripleSlashForDocumentationComments(context: context).rewrite(node)
Expand Down
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Core/RuleNameCache+Generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public let ruleNameCache: [ObjectIdentifier: String] = [
ObjectIdentifier(ReturnVoidInsteadOfEmptyTuple.self): "ReturnVoidInsteadOfEmptyTuple",
ObjectIdentifier(TypeNamesShouldBeCapitalized.self): "TypeNamesShouldBeCapitalized",
ObjectIdentifier(UseEarlyExits.self): "UseEarlyExits",
ObjectIdentifier(UseExplicitNilCheckInConditions.self): "UseExplicitNilCheckInConditions",
ObjectIdentifier(UseLetInEveryBoundCaseVariable.self): "UseLetInEveryBoundCaseVariable",
ObjectIdentifier(UseShorthandTypeNames.self): "UseShorthandTypeNames",
ObjectIdentifier(UseSingleLinePropertyGetter.self): "UseSingleLinePropertyGetter",
Expand Down
1 change: 1 addition & 0 deletions Sources/SwiftFormat/Core/RuleRegistry+Generated.swift
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"ReturnVoidInsteadOfEmptyTuple": true,
"TypeNamesShouldBeCapitalized": true,
"UseEarlyExits": false,
"UseExplicitNilCheckInConditions": true,
"UseLetInEveryBoundCaseVariable": true,
"UseShorthandTypeNames": true,
"UseSingleLinePropertyGetter": true,
Expand Down
67 changes: 67 additions & 0 deletions Sources/SwiftFormat/Rules/UseExplicitNilCheckInConditions.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2023 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 SwiftSyntax
import SwiftSyntaxBuilder

/// When checking an optional value for `nil`-ness, prefer writing an explicit `nil` check rather
/// than binding and immediately discarding the value.
///
/// For example, `if let _ = someValue { ... }` is forbidden. Use `if someValue != nil { ... }`
/// instead.
///
/// Lint: `let _ = expr` inside a condition list will yield a lint error.
///
/// Format: `let _ = expr` inside a condition list will be replaced by `expr != nil`.
@_spi(Rules)
public final class UseExplicitNilCheckInConditions: SyntaxFormatRule {
public override func visit(_ node: ConditionElementSyntax) -> ConditionElementSyntax {
switch node.condition {
case .optionalBinding(let optionalBindingCondition):
guard
let initializerClause = optionalBindingCondition.initializer,
isDiscardedAssignmentPattern(optionalBindingCondition.pattern)
else {
return node
}

diagnose(.useExplicitNilComparison, on: optionalBindingCondition)

// Since we're moving the initializer value from the RHS to the LHS of an expression/pattern,
// preserve the relative position of the trailing trivia. Similarly, preserve the leading
// trivia of the original node, since that token is being removed entirely.
var value = initializerClause.value
let trailingTrivia = value.trailingTrivia
value.trailingTrivia = []

return ConditionElementSyntax(
condition: .expression("\(node.leadingTrivia)\(value) != nil\(trailingTrivia)"))
default:
return node
}
}

/// Returns true if the given pattern is a discarding assignment expression (for example, the `_`
/// in `let _ = x`).
private func isDiscardedAssignmentPattern(_ pattern: PatternSyntax) -> Bool {
guard let exprPattern = pattern.as(ExpressionPatternSyntax.self) else {
return false
}
return exprPattern.expression.is(DiscardAssignmentExprSyntax.self)
}
}

extension Finding.Message {
@_spi(Rules)
public static let useExplicitNilComparison: Finding.Message =
"compare this value using `!= nil` instead of binding and discarding it"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import _SwiftFormatTestSupport

@_spi(Rules) import SwiftFormat

final class UseExplicitNilCheckInConditionsTests: LintOrFormatRuleTestCase {
func testIfExpressions() {
assertFormatting(
UseExplicitNilCheckInConditions.self,
input: """
if 1️⃣let _ = x {}
if let x = y, 2️⃣let _ = x.m {}
if let x = y {} else if 3️⃣let _ = z {}
""",
expected: """
if x != nil {}
if let x = y, x.m != nil {}
if let x = y {} else if z != nil {}
""",
findings: [
FindingSpec("1️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
FindingSpec("2️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
FindingSpec("3️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
]
)
}

func testGuardStatements() {
assertFormatting(
UseExplicitNilCheckInConditions.self,
input: """
guard 1️⃣let _ = x else {}
guard let x = y, 2️⃣let _ = x.m else {}
""",
expected: """
guard x != nil else {}
guard let x = y, x.m != nil else {}
""",
findings: [
FindingSpec("1️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
FindingSpec("2️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
]
)
}

func testWhileStatements() {
assertFormatting(
UseExplicitNilCheckInConditions.self,
input: """
while 1️⃣let _ = x {}
while let x = y, 2️⃣let _ = x.m {}
""",
expected: """
while x != nil {}
while let x = y, x.m != nil {}
""",
findings: [
FindingSpec("1️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
FindingSpec("2️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
]
)
}

func testTriviaPreservation() {
assertFormatting(
UseExplicitNilCheckInConditions.self,
input: """
if /*comment*/ 1️⃣let _ = x /*comment*/ {}
if 2️⃣let _ = x // comment
{}
""",
expected: """
if /*comment*/ x != nil /*comment*/ {}
if x != nil // comment
{}
""",
findings: [
FindingSpec("1️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
FindingSpec("2️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
]
)
}
}