Add ConvertToDoCatch refactoring to SwiftRefactor#3233
Add ConvertToDoCatch refactoring to SwiftRefactor#3233Clemo97 wants to merge 4 commits intoswiftlang:mainfrom
Conversation
| leftBrace: .leftBraceToken(trailingTrivia: .newline), | ||
| statements: CodeBlockItemListSyntax([ | ||
| CodeBlockItemSyntax( | ||
| leadingTrivia: .spaces(2), |
There was a problem hiding this comment.
Could we pass in the indentation of the source file through the context, similar to what we do in ExpandEditorPlaceholder?
There was a problem hiding this comment.
Actually, sorry for my change of mind, but let’s infer the indentation using BasicFormat.inferIndentation. I just realized that that’s what we’re doing in the majority of other refactoring actions.
| let tryExpression = syntax.with(\.questionOrExclamationMark, nil) | ||
|
|
||
| // Create the do-catch statement with proper spacing | ||
| let doStatement = DoStmtSyntax( |
There was a problem hiding this comment.
Would it be easier to construct this statement using string interpolation? Ie.
let doStatement = DoStmtSyntax("""
do {
…
}
""")| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testForceTryWithVariableBinding() throws { |
There was a problem hiding this comment.
This test doesn’t contain a variable binding, right?
| func testForceTryWithArguments() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! processFile(at: path, with: options) | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try processFile(at: path, with: options) | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testForceTryWithChaining() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! getData().process().save() | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try getData().process().save() | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testForceTryWithPropertyAccess() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! object.riskyProperty.value | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try object.riskyProperty.value | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| // MARK: - Complex Expressions | ||
|
|
||
| func testForceTryWithSubscript() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! array[index] | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try array[index] | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } |
There was a problem hiding this comment.
Since we just copy the expression verbatim, I don’t think that these tests provide much value. It’s the exact same code path that’s taken in the refactor action, so I’d remove them
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testForceTryWithMultilineExpression() throws { |
There was a problem hiding this comment.
This should already be covered by testForceTryWithClosure, which also contains a multi-line expression (which is what I think is relevant here)
| func testJSONDecodingExample() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! JSONDecoder().decode(User.self, from: data) | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try JSONDecoder().decode(User.self, from: data) | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testFileReadingExample() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! String(contentsOf: url, encoding: .utf8) | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try String(contentsOf: url, encoding: .utf8) | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testDataWritingExample() throws { | ||
| let baseline: ExprSyntax = """ | ||
| try! data.write(to: fileURL) | ||
| """ | ||
|
|
||
| let expected: CodeBlockItemListSyntax = """ | ||
| do { | ||
| try data.write(to: fileURL) | ||
| } catch { | ||
| <#code#> | ||
| } | ||
| """ | ||
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } |
There was a problem hiding this comment.
I think all of these are effectively covered by the tests you already have above, so I’d remove them. JSONDecoder is really no different than any other types to swift-syntax.
|
|
||
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you add one test case where the try expression has a base indentation that needs to be applied to the do block? I think that would be valuable to test, ie. something like
func test() {
try! print(
"Hello"
)
}| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors |
There was a problem hiding this comment.
😉
| // Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors | |
| // Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors |
ahoppen
left a comment
There was a problem hiding this comment.
Very nice. Just a few more comments.
| public static func refactor(syntax: TryExprSyntax, in context: Context = Context()) throws -> CodeBlockItemListSyntax | ||
| { |
There was a problem hiding this comment.
Personal formatting preference
| public static func refactor(syntax: TryExprSyntax, in context: Context = Context()) throws -> CodeBlockItemListSyntax | |
| { | |
| public static func refactor( | |
| syntax: TryExprSyntax, | |
| in context: Context = Context() | |
| ) throws -> CodeBlockItemListSyntax { |
There was a problem hiding this comment.
Thanks, will remeber to run the formatter before pushing.
| // Format the do-catch statement with the proper indentation | ||
| let format = BasicFormat( | ||
| indentationWidth: indentWidth, | ||
| initialIndentation: baseIndentation | ||
| ) |
There was a problem hiding this comment.
This may change the user-provided formatting and I’d prefer to leave it as-is. Could you use Indenter instead to apply indentation to all code blocks that need indenting?
| try assertRefactorConvert(baseline, expected: expected) | ||
| } | ||
|
|
||
| func testBasicForceTryExpression() throws { |
There was a problem hiding this comment.
This is exactly the same as testBasicForceTryConversion, right?
Summary
This PR implements the ConvertToDoCatch refactoring in swift-syntax, which converts try! (force-try) expressions into proper do-catch blocks with error handling.
Related to swiftlang/sourcekit-lsp#2424
And replaces the sourcekitd implementation https://github.com/swiftlang/swift/blob/9b452820367ccc7b5d9effbc1565bcd945c81768/lib/Refactoring/ConvertToDoCatch.cpp