Merge pull request #647 from allevato/nil-comparison-precedence

Add parentheses when needed to convert `let _ = expr` to `expr != nil`.

Author: allevato

Sources/SwiftFormat/Rules/UseExplicitNilCheckInConditions.swift

@@ -47,7 +47,7 @@ public final class UseExplicitNilCheckInConditions: SyntaxFormatRule {
       operatorExpr.trailingTrivia = [.spaces(1)]
 
       var inequalExpr = InfixOperatorExprSyntax(
-        leftOperand: value,
+        leftOperand: addingParenthesesIfNecessary(to: value),
         operator: operatorExpr,
         rightOperand: NilLiteralExprSyntax())
       inequalExpr.leadingTrivia = node.leadingTrivia
@@ -69,6 +69,54 @@ public final class UseExplicitNilCheckInConditions: SyntaxFormatRule {
     }
     return exprPattern.expression.is(DiscardAssignmentExprSyntax.self)
   }
+
+  /// Adds parentheses around the given expression if necessary to ensure that it will be parsed
+  /// correctly when followed by `!= nil`.
+  ///
+  /// Specifically, if `expr` is a `try` expression, ternary expression, or an infix operator with
+  /// the same or lower precedence, we wrap it.
+  private func addingParenthesesIfNecessary(to expr: ExprSyntax) -> ExprSyntax {
+    func addingParentheses(to expr: ExprSyntax) -> ExprSyntax {
+      var expr = expr
+      let leadingTrivia = expr.leadingTrivia
+      let trailingTrivia = expr.trailingTrivia
+      expr.leadingTrivia = []
+      expr.trailingTrivia = []
+
+      var tupleExpr = TupleExprSyntax(elements: [LabeledExprSyntax(expression: expr)])
+      tupleExpr.leadingTrivia = leadingTrivia
+      tupleExpr.trailingTrivia = trailingTrivia
+      return ExprSyntax(tupleExpr)
+    }
+
+    switch Syntax(expr).as(SyntaxEnum.self) {
+    case .tryExpr, .ternaryExpr:
+      return addingParentheses(to: expr)
+
+    case .infixOperatorExpr:
+      // There's no public API in SwiftSyntax to get the relationship between two precedence groups.
+      // Until that exists, here's a workaround I'm only mildly ashamed of: we reparse
+      // "\(expr) != nil" and then fold it. If the top-level node is anything but an
+      // `InfixOperatorExpr` whose operator is `!=` and whose RHS is `nil`, then it parsed
+      // incorrectly and we need to add parentheses around `expr`.
+      //
+      // Note that we could also cover the `tryExpr` and `ternaryExpr` cases above with this, but
+      // this reparsing trick is going to be slower so we should avoid it whenever we can.
+      let reparsedExpr = "\(expr) != nil" as ExprSyntax
+      if
+        let infixExpr = reparsedExpr.as(InfixOperatorExprSyntax.self),
+        let binOp = infixExpr.operator.as(BinaryOperatorExprSyntax.self),
+        binOp.operator.text == "!=",
+        infixExpr.rightOperand.is(NilLiteralExprSyntax.self)
+      {
+        return expr
+      }
+      return addingParentheses(to: expr)
+
+    default:
+      return expr
+    }
+  }
 }
 
 extension Finding.Message {

Tests/SwiftFormatTests/Rules/UseExplicitNilCheckInConditionsTests.swift

@@ -95,4 +95,58 @@ final class UseExplicitNilCheckInConditionsTests: LintOrFormatRuleTestCase {
       ]
     )
   }
+
+  func testAddNecessaryParenthesesAroundTryExpr() {
+    assertFormatting(
+      UseExplicitNilCheckInConditions.self,
+      input: """
+        if 1️⃣let _ = try? x {}
+        if 2️⃣let _ = try x {}
+        """,
+      expected: """
+        if (try? x) != nil {}
+        if (try x) != 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 testAddNecessaryParenthesesAroundTernaryExpr() {
+    assertFormatting(
+      UseExplicitNilCheckInConditions.self,
+      input: """
+        if 1️⃣let _ = x ? y : z {}
+        """,
+      expected: """
+        if (x ? y : z) != nil {}
+        """,
+      findings: [
+        FindingSpec("1️⃣", message: "compare this value using `!= nil` instead of binding and discarding it"),
+      ]
+    )
+  }
+
+  func testAddNecessaryParenthesesAroundSameOrLowerPrecedenceOperator() {
+    // The use of `&&` and `==` are semantically meaningless here because they don't return
+    // optionals. We just need them to stand in for any potential custom operator with lower or same
+    // precedence, respectively.
+    assertFormatting(
+      UseExplicitNilCheckInConditions.self,
+      input: """
+        if 1️⃣let _ = x && y {}
+        if 2️⃣let _ = x == y {}
+        """,
+      expected: """
+        if (x && y) != nil {}
+        if (x == y) != 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"),
+      ]
+    )
+  }
 }