Refactor RemoveRedundantParentheses for improved correctness

Signed-off-by: Karan <[email protected]>

Author: PhantomInTheWire

Sources/SwiftRefactor/RemoveRedundantParentheses.swift

@@ -27,13 +27,8 @@ public struct RemoveRedundantParentheses: SyntaxRefactoringProvider {
     syntax: TupleExprSyntax,
     in context: Void
   ) -> ExprSyntax {
-    // If the tuple has any unexpected nodes, it's not a simple parenthetical expression
-    // we should be refactoring (e.g., parsing recovery might have put some parts in unexpected nodes).
-    guard syntax.unexpectedBeforeLeftParen == nil,
-      syntax.unexpectedBetweenLeftParenAndElements == nil,
-      syntax.unexpectedBetweenElementsAndRightParen == nil,
-      syntax.unexpectedAfterRightParen == nil
-    else {
+    // If the syntax tree has errors, we should not attempt to refactor it.
+    guard !syntax.hasError else {
       return ExprSyntax(syntax)
     }
 
@@ -49,7 +44,7 @@ public struct RemoveRedundantParentheses: SyntaxRefactoringProvider {
     }
 
     // Case 2: Parentheses around simple expressions
-    if canRemoveParentheses(around: innerExpr, in: syntax.parent) {
+    if canRemoveParentheses(tuple: syntax, around: innerExpr, in: syntax.parent) {
       return preserveTrivia(from: syntax, to: innerExpr)
     }
 
@@ -70,7 +65,7 @@ public struct RemoveRedundantParentheses: SyntaxRefactoringProvider {
       .with(\.trailingTrivia, trailingTrivia)
   }
 
-  private static func canRemoveParentheses(around expr: ExprSyntax, in parent: Syntax?) -> Bool {
+  private static func canRemoveParentheses(tuple: TupleExprSyntax, around expr: ExprSyntax, in parent: Syntax?) -> Bool {
     // Safety Check: Ambiguous Closures
     // Closures and trailing closures inside conditions need parentheses to avoid ambiguity.
     // e.g. `if ({ true }) == ({ true }) {}` or `if (call { true }) == false {}`
@@ -96,28 +91,33 @@ public struct RemoveRedundantParentheses: SyntaxRefactoringProvider {
       return false
     }
 
-    // Allowlist: Parents where parentheses are always redundant (unless blocked above)
-    if let parent = parent,
-      parent.is(InitializerClauseSyntax.self)
-        || parent.is(ConditionElementSyntax.self)
-        || parent.is(ReturnStmtSyntax.self)
-        || parent.is(ThrowStmtSyntax.self)
-        || parent.is(SwitchExprSyntax.self)
-        || parent.is(RepeatStmtSyntax.self)
-    {
-      return true
+    // Allowlist: Check keyPathInParent to explicitly know that this expression
+    // occurs in a place where the parentheses are redundant.
+    if let keyPath = tuple.keyPathInParent {
+      switch keyPath {
+      case \InitializerClauseSyntax.value,
+        \ConditionElementSyntax.condition,
+        \ReturnStmtSyntax.expression,
+        \ThrowStmtSyntax.expression,
+        \SwitchExprSyntax.subject,
+        \RepeatStmtSyntax.condition:
+        return true
+      default:
+        break
+      }
     }
 
     // Fallback: Allow if the expression itself is "simple"
     guard isSimpleExpression(expr) else {
       return false
     }
 
-    // Safety Check: Postfix Precedence
-    // Expressions like `try`, `await`, `consume`, and `copy` bind looser than postfix expressions.
+    // Safety Check: Postfix and Binary Precedence
+    // Expressions like `try`, `await`, `consume`, and `copy` bind looser than postfix and infix expressions.
     // e.g., `(try? f()).description` is different from `try? f().description`.
     // The former accesses `.description` on the Optional result, the latter on the unwrapped value.
-    if isPostfixParent(parent) {
+    // Similarly, `(try? f()) + 1` is different from `try? f() + 1` (Int? + Int vs Int + Int).
+    if parentHasTighterBindingThanEffect(parent) {
       switch expr.as(ExprSyntaxEnum.self) {
       case .tryExpr, .awaitExpr, .unsafeExpr, .consumeExpr, .copyExpr:
         return false
@@ -129,17 +129,32 @@ public struct RemoveRedundantParentheses: SyntaxRefactoringProvider {
     return true
   }
 
-  /// Returns true if parent is a postfix expression where the tuple is the base.
-  private static func isPostfixParent(_ parent: Syntax?) -> Bool {
+  /// Returns true if parent is an expression with higher precedence than effects (try/await/etc).
+  /// This includes postfix expressions (member access, subscript, call, force unwrap, optional chaining),
+  /// infix operators, type casting (as/is), and ternary expressions.
+  private static func parentHasTighterBindingThanEffect(_ parent: Syntax?) -> Bool {
     switch parent?.as(SyntaxEnum.self) {
-    case .memberAccessExpr(let memberAccess):
-      return memberAccess.base != nil
-    case .subscriptCallExpr(let subscriptCall):
-      return subscriptCall.calledExpression.is(TupleExprSyntax.self)
-    case .functionCallExpr(let functionCall):
-      return functionCall.calledExpression.is(TupleExprSyntax.self)
-    case .postfixOperatorExpr:
+    // Postfix expressions: member access, subscript, function call, force unwrap, and postfix operators
+    // These all bind tighter than effect expressions (try/await/etc).
+    // For member access, since we're a TupleExprSyntax, we are always the base.
+    case .memberAccessExpr, .subscriptCallExpr, .functionCallExpr, .forceUnwrapExpr, .postfixOperatorExpr:
       return true
+
+    case .optionalChainingExpr(let optionalChaining):
+      // Optional chaining (?.) binds tighter than effects
+      return optionalChaining.expression != nil
+
+    // Infix operators and sequence expressions bind tighter than effects.
+    // For sequence expressions (before SwiftOperators folding), the parent chain
+    // is: TupleExpr -> ExprList -> SequenceExpr, e.g., `(try? f()) + 1`.
+    case .infixOperatorExpr, .sequenceExpr, .exprList:
+      return true
+
+    // Type casting operators (as, is) bind tighter than effects.
+    // Ternary operator also binds tighter than effects.
+    case .asExpr, .isExpr, .ternaryExpr:
+      return true
+
     default:
       return false
     }

Tests/SwiftRefactorTest/RemoveRedundantParenthesesTests.swift

@@ -101,7 +101,6 @@ final class RemoveRedundantParenthesesTest: XCTestCase {
     try assertParenRemoval("if (call { true }) {}", expected: "if (call { true }) {}")
     try assertParenRemoval("while ({ true }) {}", expected: "while ({ true }) {}")
     try assertParenRemoval("guard (call { true }) else {}", expected: "guard (call { true }) else {}")
-    try assertParenRemoval("repeat {} while (call { true })", expected: "repeat {} while (call { true })")
     // Nested in sequence expressions
     try assertParenRemoval("if ({ true }) == ({ true }) {}", expected: "if ({ true }) == ({ true }) {}")
     // Macro expansions with trailing closures
@@ -129,7 +128,6 @@ final class RemoveRedundantParenthesesTest: XCTestCase {
     try assertParenRemoval("(any Equatable).Protocol", expected: "(any Equatable).Protocol")
     try assertParenRemoval("(A & B).Protocol", expected: "(A & B).Protocol")
     try assertParenRemoval("(@escaping () -> Int).self", expected: "(@escaping () -> Int).self")
-    try assertParenRemoval("(some P).self", expected: "(some P).self")
     try assertParenRemoval("(T...).self", expected: "(T...).self")
 
     // Simple types allow removing parentheses
@@ -144,12 +142,41 @@ final class RemoveRedundantParenthesesTest: XCTestCase {
     try assertParenRemoval("(try? f()).description", expected: "(try? f()).description")
     try assertParenRemoval("(try! f()).description", expected: "(try! f()).description")
 
+    // `try?` also binds looser than optional chaining.
+    // `(try? f())?.bar` is different from `try? f()?.bar`.
+    try assertParenRemoval("(try? f())?.bar", expected: "(try? f())?.bar")
+    try assertParenRemoval("(try! f())?.bar", expected: "(try! f())?.bar")
+
     // `await` also binds looser than member access.
     try assertParenRemoval("(await f()).description", expected: "(await f()).description")
 
     // `consume` and `copy` also bind looser than member access.
     try assertParenRemoval("(consume x).property", expected: "(consume x).property")
     try assertParenRemoval("(copy x).property", expected: "(copy x).property")
+
+    // Infix operators bind tighter than effects
+    // `(try? f()) + 1` is `Optional<Int> + Int` while `try? f() + 1` is `Int + Int`.
+    try assertParenRemoval("(try? f()) + 1", expected: "(try? f()) + 1")
+    try assertParenRemoval("(try! f()) + 1", expected: "(try! f()) + 1")
+    try assertParenRemoval("(await f()) + 1", expected: "(await f()) + 1")
+
+    // Type casting binds tighter than effects
+    // `(try? f()) as Int` is different from `try? f() as Int`.
+    try assertParenRemoval("(try? f()) as Int", expected: "(try? f()) as Int")
+    try assertParenRemoval("(try! f()) as Int", expected: "(try! f()) as Int")
+    try assertParenRemoval("(await f()) as Int", expected: "(await f()) as Int")
+    // `is` check
+    try assertParenRemoval("(try? f()) is Int", expected: "(try? f()) is Int")
+
+    // Ternary operator binds tighter than effects
+    // `(try? f()) ? x : y` is different from `try? f() ? x : y`.
+    try assertParenRemoval("(try? f()) ? x : y", expected: "(try? f()) ? x : y")
+    try assertParenRemoval("(await f()) ? x : y", expected: "(await f()) ? x : y")
+
+    // Force unwrap binds tighter than effects
+    // `(try? f())!` is different from `try? f()!`.
+    try assertParenRemoval("(try? f())!", expected: "(try? f())!")
+    try assertParenRemoval("(await f())!", expected: "(await f())!")
   }
 
   func testRedundantParenthesesInControlFlow() throws {
@@ -172,6 +199,18 @@ final class RemoveRedundantParenthesesTest: XCTestCase {
     // Multiple conditions
     try assertParenRemoval("if (x), (y) {}", expected: "if x, y {}")
   }
+
+  func testSequenceExpressions() throws {
+    // Sequence expressions (before SwiftOperators folding) bind tighter than effects.
+    // In `(try? f()) + 1`, the parentheses must be preserved because the sequence
+    // expression structure makes `try? f()` the left operand of `+`.
+    try assertParenRemoval("(try? f()) - 1", expected: "(try? f()) - 1")
+    try assertParenRemoval("(try? f()) * 1", expected: "(try? f()) * 1")
+
+    // Complex sequence expressions
+    try assertParenRemoval("(try? f()) + g() + h()", expected: "(try? f()) + g() + h()")
+    try assertParenRemoval("(await f()) + g()", expected: "(await f()) + g()")
+  }
 }
 
 // MARK: - Test Helper