-
Notifications
You must be signed in to change notification settings - Fork 501
Feat(SwiftRefactor): Implement InvertIfCondition refactoring #3263
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
624d0bf
ccc7b4d
08c1c8b
58c1796
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,104 @@ | ||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||
| // | ||||||||||
| // This source file is part of the Swift.org open source project | ||||||||||
| // | ||||||||||
| // Copyright (c) 2014 - 2026 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 | ||||||||||
| // | ||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||
|
|
||||||||||
| #if compiler(>=6) | ||||||||||
| public import SwiftSyntax | ||||||||||
| #else | ||||||||||
| import SwiftSyntax | ||||||||||
| #endif | ||||||||||
|
|
||||||||||
| /// Inverts a negated `if` condition and swaps the branches. | ||||||||||
| /// | ||||||||||
| /// ## Before | ||||||||||
| /// | ||||||||||
| /// ```swift | ||||||||||
| /// if !x { | ||||||||||
| /// foo() | ||||||||||
| /// } else { | ||||||||||
| /// bar() | ||||||||||
| /// } | ||||||||||
| /// ``` | ||||||||||
| /// | ||||||||||
| /// ## After | ||||||||||
| /// | ||||||||||
| /// ```swift | ||||||||||
| /// if x { | ||||||||||
| /// bar() | ||||||||||
| /// } else { | ||||||||||
| /// foo() | ||||||||||
| /// } | ||||||||||
| /// ``` | ||||||||||
| public struct InvertIfCondition: SyntaxRefactoringProvider { | ||||||||||
| public static func refactor(syntax ifExpr: IfExprSyntax, in context: Void) -> IfExprSyntax { | ||||||||||
| // 1. Must have an `else` block (and it must be a CodeBlock, not another `if`). | ||||||||||
| guard let elseBody = ifExpr.elseBody, | ||||||||||
| case .codeBlock(let elseBlock) = elseBody | ||||||||||
| else { | ||||||||||
| return ifExpr | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is nothing to refactor we should throw a |
||||||||||
| } | ||||||||||
|
|
||||||||||
| // 2. Must have exactly one condition. | ||||||||||
| guard ifExpr.conditions.count == 1, | ||||||||||
| let condition = ifExpr.conditions.first | ||||||||||
| else { | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fits on one line, same above.
Suggested change
|
||||||||||
| return ifExpr | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 3. Condition must be an expression (not a binding like `let x`). | ||||||||||
| guard case .expression(let expr) = condition.condition else { | ||||||||||
| return ifExpr | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 4. Expression must be a PrefixOperatorExpr with operator "!". | ||||||||||
| guard let prefixOpExpr = expr.as(PrefixOperatorExprSyntax.self), | ||||||||||
| prefixOpExpr.operator.text == "!" | ||||||||||
| else { | ||||||||||
| return ifExpr | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // 5. Extract inner expression (remove the `!`). | ||||||||||
| // Preserve trivia: The `!` might have leading trivia (e.g. comments/spaces). | ||||||||||
| // Usually standard formatting is `if !cond`. | ||||||||||
| // We should probably apply the `PrefixOperatorExpr`'s leading trivia to the inner expression | ||||||||||
| // to preserve any comments attached to the negation. | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are things left to address, please address them. Otherwise please don’t talk about things that we should probably do. Either do them or don’t and explain why.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to use |
||||||||||
| let innerExpr = prefixOpExpr.expression | ||||||||||
| .with(\.leadingTrivia, prefixOpExpr.leadingTrivia) | ||||||||||
|
|
||||||||||
| // 6. Create new condition list. | ||||||||||
| let newCondition = condition.with(\.condition, .expression(innerExpr)) | ||||||||||
| let newConditions = ifExpr.conditions.with(\.[ifExpr.conditions.startIndex], newCondition) | ||||||||||
|
Comment on lines
+60
to
+61
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
|
||||||||||
| // 7. Swap bodies with Trivia preservation. | ||||||||||
| // We strictly swap the trivia: | ||||||||||
| // New body (was else) takes old body's trivia (space before `{`, space after `}`). | ||||||||||
| // New else (was body) takes old else's trivia (space before `{`?, newline after `}`). | ||||||||||
|
|
||||||||||
| let oldBody = ifExpr.body | ||||||||||
| let oldElseBlock = elseBlock | ||||||||||
|
|
||||||||||
| let newBody = | ||||||||||
| oldElseBlock | ||||||||||
| .with(\.leadingTrivia, oldBody.leadingTrivia) | ||||||||||
| .with(\.trailingTrivia, oldBody.trailingTrivia) | ||||||||||
|
|
||||||||||
| let newElseBody = | ||||||||||
| oldBody | ||||||||||
| .with(\.leadingTrivia, oldElseBlock.leadingTrivia) | ||||||||||
| .with(\.trailingTrivia, oldElseBlock.trailingTrivia) | ||||||||||
|
|
||||||||||
| return | ||||||||||
| ifExpr | ||||||||||
| .with(\.conditions, newConditions) | ||||||||||
| .with(\.body, newBody) | ||||||||||
| .with(\.elseBody, .codeBlock(newElseBody)) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unrelated change. Please make sure your PR only addresses one issue. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2026 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 | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if compiler(>=6) | ||
| public import SwiftSyntax | ||
| #else | ||
| import SwiftSyntax | ||
| #endif | ||
|
|
||
| /// Removes redundant parentheses from a single-element tuple. | ||
| /// | ||
| /// ## Before | ||
| /// | ||
| /// ```swift | ||
| /// let x = (10) | ||
| /// ``` | ||
| /// | ||
| /// ## After | ||
| /// | ||
| /// ```swift | ||
| /// let x = 10 | ||
| /// ``` | ||
| public struct RemoveRedundantParens: SyntaxRefactoringProvider { | ||
| public static func refactor(syntax tupleExpr: TupleExprSyntax, in context: Void) -> ExprSyntax { | ||
| // 1. Must be exactly one element. | ||
| guard tupleExpr.elements.count == 1, | ||
| let element = tupleExpr.elements.first | ||
| else { | ||
| return ExprSyntax(tupleExpr) | ||
| } | ||
|
|
||
| // 2. Must not have a label. | ||
| guard element.label == nil else { | ||
| return ExprSyntax(tupleExpr) | ||
| } | ||
|
|
||
| // 3. Extract the inner expression. | ||
| let innerExpr = element.expression | ||
|
|
||
| // 4. Safety Check: Precedence Ambiguity | ||
| // If we are unwrapping a SequenceExpr (binary op) AND we are inside another SequenceExpr, | ||
| // we assume it is UNSAFE because we don't know operator precedence. | ||
| // Example: x * (y + z) -> unwrapping makes it x * y + z (different logic). | ||
|
|
||
| // Check if inner is SequenceExpr | ||
| // Check if inner is SequenceExpr | ||
| if innerExpr.is(SequenceExprSyntax.self) { | ||
| // Case: Parent is ExprList (standard SequenceExpr elements) -> Grandparent is SequenceExpr | ||
| if let parent = tupleExpr.parent, | ||
| parent.is(ExprListSyntax.self), | ||
| let grandParent = parent.parent, | ||
| grandParent.is(SequenceExprSyntax.self) | ||
| { | ||
| return ExprSyntax(tupleExpr) | ||
| } | ||
|
|
||
| // Case: Direct parent is SequenceExpr (fallback) | ||
| if let parent = tupleExpr.parent, parent.is(SequenceExprSyntax.self) { | ||
| return ExprSyntax(tupleExpr) | ||
| } | ||
| } | ||
|
|
||
| // 5. Preserve Trivia | ||
| // We want to keep comments attached to the parens, but maybe not the newlines inside? | ||
| // A safe default is to take the tuple's leading/trailing trivia and apply it to the inner expression. | ||
| // This replaces the inner expression's existing leading/trailing trivia if we use .with, | ||
| // so we might want to append? | ||
| // Actually, usually `( /* comment */ x )` -> `/* comment */ x`. | ||
| // Let's replace. | ||
|
|
||
| return | ||
| innerExpr | ||
| .with(\.leadingTrivia, tupleExpr.leadingTrivia) | ||
| .with(\.trailingTrivia, tupleExpr.trailingTrivia) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2026 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 SwiftParser | ||
| import SwiftRefactor | ||
| import SwiftSyntax | ||
| import SwiftSyntaxBuilder | ||
| import XCTest | ||
| import _SwiftSyntaxTestSupport | ||
|
|
||
| final class InvertIfConditionTest: XCTestCase { | ||
| func testInvertIfCondition() throws { | ||
| let tests = [ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about writing an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment still applies |
||
| ( | ||
| """ | ||
| if !x { | ||
| foo() | ||
| } else { | ||
| bar() | ||
| } | ||
| """, | ||
| """ | ||
| if x { | ||
| bar() | ||
| } else { | ||
| foo() | ||
| } | ||
| """ | ||
| ), | ||
| ( | ||
| """ | ||
| if !(x == y) { | ||
| return | ||
| } else { | ||
| continue | ||
| } | ||
| """, | ||
| """ | ||
| if (x == y) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we strip the parentheses here since they are no longer necessary? |
||
| continue | ||
| } else { | ||
| return | ||
| } | ||
| """ | ||
| ), | ||
| // Trivia preservation | ||
| ( | ||
| """ | ||
| if /* comment */ !x { | ||
| a | ||
| } else { | ||
| b | ||
| } | ||
| """, | ||
| """ | ||
| if /* comment */ x { | ||
| b | ||
| } else { | ||
| a | ||
| } | ||
| """ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add a test that has comments in the bodies? |
||
| ), | ||
| ] | ||
|
|
||
| for (input, expected) in tests { | ||
| let inputSyntax = try XCTUnwrap( | ||
| ExprSyntax.parse(from: input).as(IfExprSyntax.self), | ||
| "Failed validity check: \(input)" | ||
| ) | ||
| let expectedSyntax = try XCTUnwrap(ExprSyntax.parse(from: expected), "Failed validity check: \(expected)") | ||
|
|
||
| try assertRefactor(inputSyntax, context: (), provider: InvertIfCondition.self, expected: expectedSyntax) | ||
| } | ||
| } | ||
|
|
||
| func testInvertIfConditionFails() throws { | ||
| let tests = [ | ||
| // Not negated | ||
| """ | ||
| if x { | ||
| a | ||
| } else { | ||
| b | ||
| } | ||
| """, | ||
| // No else | ||
| """ | ||
| if !x { | ||
| a | ||
| } | ||
| """, | ||
| // Else if (not a CodeBlock) | ||
| """ | ||
| if !x { | ||
| a | ||
| } else if y { | ||
| b | ||
| } | ||
| """, | ||
| // Multiple conditions | ||
| """ | ||
| if !x, !y { | ||
| a | ||
| } else { | ||
| b | ||
| } | ||
| """, | ||
| // Binding | ||
| """ | ||
| if let x = y { | ||
| a | ||
| } else { | ||
| b | ||
| } | ||
| """, | ||
| ] | ||
|
|
||
| for input in tests { | ||
| let inputSyntax = try XCTUnwrap( | ||
| ExprSyntax.parse(from: input).as(IfExprSyntax.self), | ||
| "Failed validity check: \(input)" | ||
| ) | ||
| try assertRefactor(inputSyntax, context: (), provider: InvertIfCondition.self, expected: inputSyntax) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Private helper to avoid redeclaration conflicts | ||
| private extension ExprSyntax { | ||
| static func parse(from source: String) -> ExprSyntax { | ||
| var parser = Parser(source) | ||
| return ExprSyntax.parse(from: &parser) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my comment in your other PR, I wouldn’t have these numbered comments that explain exactly what the code blow does.