Merge pull request #627 from allevato/no-discard-conditional-assignment

Add the `UseExplicitNilCheckInConditions` rule.

Author: allevato

Documentation/RuleDocumentation.md

@@ -12,6 +12,7 @@ automatically.
 Here's the list of available rules:
 
 - [AllPublicDeclarationsHaveDocumentation](#AllPublicDeclarationsHaveDocumentation)
+- [AlwaysUseLiteralForEmptyCollectionInit](#AlwaysUseLiteralForEmptyCollectionInit)
 - [AlwaysUseLowerCamelCase](#AlwaysUseLowerCamelCase)
 - [AmbiguousTrailingClosureOverload](#AmbiguousTrailingClosureOverload)
 - [BeginDocumentationCommentWithOneLineSummary](#BeginDocumentationCommentWithOneLineSummary)
@@ -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)
@@ -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).
+
+`AlwaysUseLiteralForEmptyCollectionInit` rule can format your code automatically.
+
 ### AlwaysUseLowerCamelCase
 
 All values should be written in lower camel-case (`lowerCamelCase`).
@@ -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`.

Package.swift

@@ -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"),

Sources/SwiftFormat/Core/Pipelines+Generated.swift

@@ -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
   }
 
@@ -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)

Sources/SwiftFormat/Core/RuleNameCache+Generated.swift

@@ -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",

Sources/SwiftFormat/Core/RuleRegistry+Generated.swift

@@ -47,6 +47,7 @@
     "ReturnVoidInsteadOfEmptyTuple": true,
     "TypeNamesShouldBeCapitalized": true,
     "UseEarlyExits": false,
+    "UseExplicitNilCheckInConditions": true,
     "UseLetInEveryBoundCaseVariable": true,
     "UseShorthandTypeNames": true,
     "UseSingleLinePropertyGetter": true,

Sources/SwiftFormat/Rules/UseExplicitNilCheckInConditions.swift

@@ -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"
+}

Tests/SwiftFormatTests/Rules/UseExplicitNilCheckInConditionsTests.swift

@@ -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"),
+      ]
+    )
+  }
+}