Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions Sources/SwiftRefactor/ConvertToDoCatch.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😉

Suggested change
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
// 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
public import SwiftSyntaxBuilder
#else
import SwiftSyntax
import SwiftSyntaxBuilder
#endif

/// Converts a `try!` expression to a `do-catch` block.
///
/// This refactoring transforms force-try expressions into proper error handling
/// using do-catch blocks with a placeholder for the catch body.
///
/// Example:
/// ```swift
/// // Before:
/// let result = try! riskyFunction()
///
/// // After:
/// let result: ResultType
/// do {
/// result = try riskyFunction()
/// } catch {
/// <#code#>
/// }
/// ```
public struct ConvertToDoCatch: SyntaxRefactoringProvider {
public static func refactor(syntax: TryExprSyntax, in context: ()) throws -> CodeBlockItemListSyntax {
// Validate that this is a force-try (try!) expression
guard syntax.questionOrExclamationMark?.tokenKind == .exclamationMark else {
throw RefactoringNotApplicableError("not a force-try expression")
}

// Create the try expression without the exclamation mark
let tryExpression = syntax.with(\.questionOrExclamationMark, nil)

// Create the do-catch statement with proper spacing
let doStatement = DoStmtSyntax(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be easier to construct this statement using string interpolation? Ie.

let doStatement = DoStmtSyntax("""
do {

}
""")

doKeyword: .keyword(.do, trailingTrivia: .space),
body: CodeBlockSyntax(
leftBrace: .leftBraceToken(trailingTrivia: .newline),
statements: CodeBlockItemListSyntax([
CodeBlockItemSyntax(
leadingTrivia: .spaces(2),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we pass in the indentation of the source file through the context, similar to what we do in ExpandEditorPlaceholder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

item: .expr(ExprSyntax(tryExpression))
)
]),
rightBrace: .rightBraceToken(leadingTrivia: .newline)
),
catchClauses: CatchClauseListSyntax([
CatchClauseSyntax(
catchKeyword: .keyword(.catch, leadingTrivia: .space, trailingTrivia: .space),
body: CodeBlockSyntax(
leftBrace: .leftBraceToken(trailingTrivia: .newline),
statements: CodeBlockItemListSyntax([
CodeBlockItemSyntax(
leadingTrivia: .spaces(2),
item: .expr(ExprSyntax("<#code#>" as ExprSyntax))
)
]),
rightBrace: .rightBraceToken(leadingTrivia: .newline)
)
)
])
)

return CodeBlockItemListSyntax([
CodeBlockItemSyntax(item: .stmt(StmtSyntax(doStatement)))
])
}
}
300 changes: 300 additions & 0 deletions Tests/SwiftRefactorTest/ConvertToDoCatchTest.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,300 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2024 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 SwiftRefactor
import SwiftSyntax
import SwiftSyntaxBuilder
import XCTest
import _SwiftSyntaxTestSupport

final class ConvertToDoCatchTest: XCTestCase {

// MARK: - Basic Conversions

func testBasicForceTryConversion() throws {
let baseline: ExprSyntax = """
try! riskyFunction()
"""

let expected: CodeBlockItemListSyntax = """
do {
try riskyFunction()
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

func testForceTryWithVariableBinding() throws {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn’t contain a variable binding, right?

let baseline: ExprSyntax = """
try! fetchData()
"""

let expected: CodeBlockItemListSyntax = """
do {
try fetchData()
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


func testForceTryWithClosure() throws {
let baseline: ExprSyntax = """
try! performAsync { result in
print(result)
}
"""

let expected: CodeBlockItemListSyntax = """
do {
try performAsync { result in
print(result)
}
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

func testForceTryWithAwait() throws {
let baseline: ExprSyntax = """
try! await fetchRemoteData()
"""

let expected: CodeBlockItemListSyntax = """
do {
try await fetchRemoteData()
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

// MARK: - Edge Cases

func testForceTryWithComments() throws {
let baseline: ExprSyntax = """
try! /* important */ riskyFunction()
"""

let expected: CodeBlockItemListSyntax = """
do {
try /* important */ riskyFunction()
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

func testForceTryWithMultilineExpression() throws {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should already be covered by testForceTryWithClosure, which also contains a multi-line expression (which is what I think is relevant here)

let baseline: ExprSyntax = """
try! complexFunction(
parameter1: value1,
parameter2: value2
)
"""

let expected: CodeBlockItemListSyntax = """
do {
try complexFunction(
parameter1: value1,
parameter2: value2
)
} catch {
<#code#>
}
"""

try assertRefactorConvert(baseline, expected: expected)
}

// MARK: - Negative Tests (Should Not Apply)

func testOptionalTryShouldNotApply() throws {
let baseline: ExprSyntax = """
try? riskyFunction()
"""

// Should throw RefactoringNotApplicableError
try assertRefactorConvert(baseline, expected: nil)
}

func testRegularTryShouldNotApply() throws {
let baseline: ExprSyntax = """
try riskyFunction()
"""

// Should throw RefactoringNotApplicableError
try assertRefactorConvert(baseline, expected: nil)
}

func testNonTryExpressionShouldNotApply() throws {
let baseline: ExprSyntax = """
regularFunction()
"""

// Should throw RefactoringNotApplicableError
try assertRefactorConvert(baseline, expected: nil)
}

// MARK: - Real-World Examples

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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
  )
}


// MARK: - Helper Functions

private func assertRefactorConvert(
_ input: ExprSyntax,
expected: CodeBlockItemListSyntax?,
file: StaticString = #filePath,
line: UInt = #line
) throws {
guard let tryExpr = input.as(TryExprSyntax.self) else {
if expected != nil {
XCTFail("Input is not a TryExprSyntax", file: file, line: line)
}
return
}

try assertRefactor(
tryExpr,
context: (),
provider: ConvertToDoCatch.self,
expected: expected,
file: file,
line: line
)
}