Feat(SwiftRefactor): Implement CollapseNestedIfStmt refactoring#3260
Feat(SwiftRefactor): Implement CollapseNestedIfStmt refactoring#3260rickhohler wants to merge 2 commits intoswiftlang:mainfrom
Conversation
rickhohler
requested review from
ahoppen,
hamishknight,
hborla and
rintaro
as code owners
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Thanks for the PR, @rickhohler. I think the biggest remaining item to do is to adjust indentation after merging the if statements.
| guard outerIf.body.statements.count == 1, | ||
| let firstStatement = outerIf.body.statements.first | ||
| else { |
There was a problem hiding this comment.
| guard outerIf.body.statements.count == 1, | |
| let firstStatement = outerIf.body.statements.first | |
| else { | |
| guard let onlyStatement = outerIf.body.statements.only else { |
| // 3. Check if that statement is an `IfExprSyntax` (inner `if`). | ||
| // It could be an `expression` statement containing the `IfExpr` or just the expression itself depending on parsing context, | ||
| // but typically in a code block it's an expression statement. | ||
| guard let expressionStmt = firstStatement.item.as(ExpressionStmtSyntax.self), | ||
| let innerIf = expressionStmt.expression.as(IfExprSyntax.self) | ||
| else { | ||
| return outerIf | ||
| } |
There was a problem hiding this comment.
The code doesn’t account for the fact that firstStatement might be an IfExprSyntax directly, right? So it mismatches the comment.
| return outerIf | ||
| } | ||
|
|
||
| // 2. Ensure the outer `if` body has exactly one statement. |
There was a problem hiding this comment.
I personally wouldn’t add numbered lists here because it makes it harder to insert more checks in the future, if necessary. At which point this comment becomes trivial because it describes exactly what’s done below and can thus be removed.
| // If the last condition doesn't have a trailing comma, verify we need to add one. | ||
| // Actually, `ConditionElementListSyntax` elements usually handle commas. | ||
| // We just need to make sure the last element of the *outer* list has a comma now that it's not last. |
There was a problem hiding this comment.
You seem to contradict the first line of this comment in the second line. Could you merge these comments to make them consistent? Probably also the one right after 5. Merge conditions.
| // Remove existing trailing spaces/trivia before comma to be clean, logic can be refined. | ||
| // But crucially, we must add a comma. |
There was a problem hiding this comment.
If the logic can be refined, please do so.
| } | ||
|
|
||
| // Add inner conditions. | ||
| // We might need to adjust trivia on the first inner condition to look nice (e.g., remove leading newlines if any). |
There was a problem hiding this comment.
You don’t seem to be doing what you describe in the comment though.
|
|
||
| final class CollapseNestedIfStmtTest: XCTestCase { | ||
| func testCollapseNestedIfStmt() throws { | ||
| let tests = [ |
There was a problem hiding this comment.
If any of these tests fail, it’s hard to know which one it was. Could you either use the #line trick we use elsewhere or write an assertCollapseIfStmt function that you call for different inputs. I’d prefer the latter option.
| ( | ||
| """ | ||
| if a { | ||
| // comment |
There was a problem hiding this comment.
It would be great if we could retain the comments as much as possible.
| print("hello") | ||
| } |
There was a problem hiding this comment.
Could you adjust the indentation here so we get
if a, b {
print("hello")
}You can take a look at what we do with IndentationRemover in SourceKit-LSP.
| } | ||
|
|
||
| extension ExprSyntax { | ||
| static func parse(from source: String) -> ExprSyntax { |
There was a problem hiding this comment.
ExprSyntax conforms to ExpressibleByStringLiteral, so this isn’t necessary if you convert the string literals straight to an ExprSyntax.
2 participants
Description
This PR implements the
CollapseNestedIfStmtrefactoring action, which merges nestedifstatements into a singleifstatement with a combined condition list.Transformation:
Detailed Design
CollapseNestedIfStmtprovider toSwiftRefactor.ifhas exactly one statement (the innerif).ifhas anelseblock (to avoid ambiguity).IfExprSyntaxinside the block.Fixes
Addresses one part of swiftlang/sourcekit-lsp#2424 (Porting legacy refactoring actions).
Pre-PR Checklist
swift-format.CollapseNestedIfStmtTest.swift.