feat(refactor): Implement ConvertToSwitchStmt (SourceKit-LSP #2424)#3264
feat(refactor): Implement ConvertToSwitchStmt (SourceKit-LSP #2424)#3264rickhohler wants to merge 4 commits intoswiftlang:mainfrom
Conversation
…g#2424) - Implements ConvertToSwitchStmt refactoring action. - Supports converting if/else if/else chains with == conditions on a common subject. - Adds comprehensive unit tests including failure scenarios. - Ensures generated switch statements are exhaustive (implicit default: break).
rickhohler
requested review from
ahoppen,
hamishknight,
hborla and
rintaro
as code owners
|
|
||
| // 1. Identify Subject and Pattern from the first `if`. | ||
| guard let (subject, firstPattern) = extractSubjectAndPattern(from: ifExpr) else { | ||
| throw RefactoringNotApplicableError("First condition invalid") |
There was a problem hiding this comment.
| throw RefactoringNotApplicableError("First condition invalid") | |
| throw RefactoringNotApplicableError("First condition does not of the format x == y") |
| // Else If block | ||
| // Must match subject and operator | ||
| guard let (nextSubject, nextPattern) = extractSubjectAndPattern(from: nextIf) else { | ||
| throw RefactoringNotApplicableError("Subsequent condition invalid") |
There was a problem hiding this comment.
Similar comment as for the first condition.
| // Ensure subject is structurally identical (ignoring trivia) | ||
| // We use trimmedDescription for comparison. | ||
| guard nextSubject.trimmedDescription == subject.trimmedDescription else { | ||
| throw RefactoringNotApplicableError("Subject mismatch") |
There was a problem hiding this comment.
I would be a little more descriptive here about what mismatches.
| // Add default: break | ||
| let breakStmt = CodeBlockItemSyntax( | ||
| leadingTrivia: .newlines(1) + .spaces(2), | ||
| item: .stmt(StmtSyntax(BreakStmtSyntax())) | ||
| ) | ||
| cases.append(createDefaultCase(body: CodeBlockSyntax(statements: [breakStmt]))) |
There was a problem hiding this comment.
Can you run swift-format as described in CONTRIBUTING.md
| // Therefore, if there is no `else`, we MUST likely abort OR add `default: break`. | ||
| // Adding `default: break` is safe. | ||
|
|
||
| if ifExpr.elseBody == nil || cases.last?.label.is(SwitchDefaultLabelSyntax.self) == false { |
There was a problem hiding this comment.
I personally wouldn’t add a default case. If the switch isn’t exhaustive, the compiler will add a diagnostic for it and then the user can add a default case if necessary.
| return SwitchExprSyntax( | ||
| switchKeyword: .keyword(.switch, trailingTrivia: .space), | ||
| subject: subject.trimmed, | ||
| leftBrace: .leftBraceToken(leadingTrivia: .space), | ||
| cases: caseList, | ||
| rightBrace: .rightBraceToken(leadingTrivia: .newlines(1)) | ||
| ) |
There was a problem hiding this comment.
You should be able to just break here, so we can have a single return constructing a SwitchExprSyntax for the function.
| ) | ||
| } | ||
|
|
||
| private static func createDefaultCase(body: CodeBlockSyntax) -> SwitchCaseSyntax { |
There was a problem hiding this comment.
If this ends up being called from only a single place, we can inline it and don’t need a function for it anymore.
| } | ||
| } | ||
|
|
||
| // Private helper to avoid redeclaration conflicts |
There was a problem hiding this comment.
Similar comment as in your other PR, ExprSyntax already conforms to ExpressibleByStringLiteral.
|
Looks like you pushed a new commit without addressing all of my comments. Please let me know when this is ready for another review. |
|
@rickhohler Are you planning to continue working on this? |
2 participants
This PR implements the
ConvertToSwitchStmtrefactoring action as requested in SourceKit-LSP #2424.It supports converting
if/else if/elsechains intoswitchstatements when simpler==comparisons are used against a common subject.Includes comprehensive unit tests to verify correctness and safety (e.g. exhaustiveness checks via implicit
default: breakinsertion).