Add ConvertToDoCatch refactoring to SwiftRefactor by Clemo97 · Pull Request #3233 · swiftlang/swift-syntax

Clemo97 · 2026-01-12T23:34:54Z

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

ahoppen

Very nice. I have a few comments on the test case but overall this looks good. Thanks for implementing it.

ahoppen · 2026-01-13T15:51:48Z

+        leftBrace: .leftBraceToken(trailingTrivia: .newline),
+        statements: CodeBlockItemListSyntax([
+          CodeBlockItemSyntax(
+            leadingTrivia: .spaces(2),


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

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.

ahoppen · 2026-01-13T15:53:21Z

+    let tryExpression = syntax.with(\.questionOrExclamationMark, nil)
+
+    // Create the do-catch statement with proper spacing
+    let doStatement = DoStmtSyntax(


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

let doStatement = DoStmtSyntax(""" do { … } """)

This is still outstanding.

ahoppen · 2026-01-13T15:53:47Z

+    try assertRefactorConvert(baseline, expected: expected)
+  }
+
+  func testForceTryWithVariableBinding() throws {


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

ahoppen · 2026-01-13T15:55:00Z

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


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

ahoppen · 2026-01-13T15:56:55Z

+    try assertRefactorConvert(baseline, expected: expected)
+  }
+
+  func testForceTryWithMultilineExpression() throws {


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

ahoppen · 2026-01-13T15:58:37Z

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


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.

ahoppen · 2026-01-13T15:59:59Z

+
+    try assertRefactorConvert(baseline, expected: expected)
+  }
+}


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

ahoppen · 2026-01-13T16:19:44Z

+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors


😉

Suggested change

// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors

// Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors

ahoppen

Very nice. Just a few more comments.

ahoppen · 2026-01-29T17:20:45Z

+    // Format the do-catch statement with the proper indentation
+    let format = BasicFormat(
+      indentationWidth: indentWidth,
+      initialIndentation: baseIndentation
+    )


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?

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?

Indenter in SwiftSyntaxBuilder has been deprecated in favor of the new indented(by:) in SwiftBasicFormat, so I used the newer .indented(by:) method.

ahoppen · 2026-01-29T17:21:52Z

+    try assertRefactorConvert(baseline, expected: expected)
+  }
+
+  func testBasicForceTryExpression() throws {


This is exactly the same as testBasicForceTryConversion, right?

Yes, you are right, I've removed 'testBasicForceTryExpression'.

ahoppen · 2026-02-09T09:48:09Z

@Clemo97 Did you mean to push changes that addressed my last set of comments?

ahoppen · 2026-02-09T11:04:47Z

My last review comments are still not addressed in your latest version of the PR. Just thinking that you might have addressed them locally because you reacted to them.

Clemo97 · 2026-02-09T12:14:20Z

My last review comments are still not addressed in your latest version of the PR. Just thinking that you might have addressed them locally because you reacted to them.

Hello, sorry for the confusion I had forgotten to push my changes remotely, let me work on it.

ahoppen

Just one more small thing.

ahoppen · 2026-02-15T14:26:48Z

+    }
+
+    // Remove leading trivia from the try expression since we'll manage indentation
+    let trimmedTryExpression = tryExpression.with(\.leadingTrivia, [])


This would also remove any comments in the leading trivia. Maybe I’m missing something but I think you don’t need to remove the leading trivia here if you only the CodeBlockItemListSyntax by indentationWidth in line 83.

A test case for this would be the following, which should keep the comment.

// Hope this doesn’t explode try! hopefullyNoBoom()

Could you add this test case to your PR?

ahoppen · 2026-02-21T15:23:51Z

Looks like you merged main into your branch but didn’t address my changes. Just want to make sure that you didn’t forget to push anything. Also, could you rebase and squash your changes instead of merging main into your branch, as described in https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#authoring-commits?

Clemo97 · 2026-02-24T09:33:41Z

Looks like you merged main into your branch but didn’t address my changes. Just want to make sure that you didn’t forget to push anything. Also, could you rebase and squash your changes instead of merging main into your branch, as described in https://github.com/swiftlang/sourcekit-lsp/blob/main/CONTRIBUTING.md#authoring-commits?

I accidentally closed the PR working on fixing it.

ahoppen

I re-opened the PR. Two more small things.

ahoppen · 2026-02-25T08:38:00Z

+        statements: CodeBlockItemListSyntax([
+          CodeBlockItemSyntax(
+            item: .expr(ExprSyntax(tryExprWithoutBaseIndent))
+          )
+        ]).indented(by: baseIndentation + indentationWidth, indentFirstLine: true),


Maybe I’m missing something but instead of stripping the base indentation and then adding it again, can’t this just be the following? At which point you don’t need stripBaseIndentation anymore.

Suggested change

statements: CodeBlockItemListSyntax([

CodeBlockItemSyntax(

item: .expr(ExprSyntax(tryExprWithoutBaseIndent))

)

]).indented(by: baseIndentation + indentationWidth, indentFirstLine: true),

statements: CodeBlockItemListSyntax([

CodeBlockItemSyntax(

item: .expr(ExprSyntax(tryExpression))

)

]).indented(by: indentationWidth, indentFirstLine: true),

ahoppen · 2026-02-25T08:39:05Z

+    }
+
+    // Remove leading trivia from the try expression since we'll manage indentation
+    let trimmedTryExpression = tryExpression.with(\.leadingTrivia, [])


Could you add this test case to your PR?

Clemo97 · 2026-02-25T12:32:22Z

I re-opened the PR. Two more small things.

Thanks

ahoppen · 2026-03-08T10:34:15Z

@Clemo97 I don’t see any recent changes made by you in this PR. Just want to make sure that you don’t have them implemented locally and just forgot to push them.

…dentation management

ahoppen

We recently decided that code actions should live in the SourceKit-LSP repository unless there is a good reason for it not to (https://github.com/swiftlang/sourcekit-lsp/blob/main/Contributor%20Documentation/Code%20Action.md). This means that we should implement this in the SourceKit-LSP repository. Could you open a PR to add it over there? That way you can also already include it in Code Actions.md and add it to the list of code actions reported by SourceKit-LSP.

Clemo97 requested review from ahoppen, hamishknight, hborla and rintaro as code owners January 12, 2026 23:34

ahoppen reviewed Jan 13, 2026

View reviewed changes

Comment thread Sources/SwiftRefactor/ConvertToDoCatch.swift

Clemo97 requested a review from ahoppen January 21, 2026 16:44

ahoppen reviewed Jan 29, 2026

View reviewed changes

ahoppen reviewed Feb 15, 2026

View reviewed changes

Clemo97 closed this Feb 24, 2026

Clemo97 force-pushed the main branch from 3be3dc6 to 491b82f Compare February 24, 2026 09:11

ahoppen reopened this Feb 25, 2026

ahoppen reviewed Feb 25, 2026

View reviewed changes

Clemo97 added 5 commits March 9, 2026 16:09

Add ConvertToDoCatch refactoring provider and corresponding tests

19265ab

Add ConvertToDoCatch refactoring provider and update tests

5120027

Refactor ConvertToDoCatch to simplify context handling and improve in…

8ca01c0

…dentation management

fix identation

5540019

Address review: use tryExpression directly and add leading trivia test

c76058e

Clemo97 force-pushed the main branch from dbcebfc to c76058e Compare March 9, 2026 13:09

ahoppen reviewed Mar 15, 2026

View reviewed changes

	// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
	// Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors

Conversation

Clemo97 commented Jan 12, 2026

Uh oh!

ahoppen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Feb 9, 2026

Uh oh!

ahoppen commented Feb 9, 2026

Uh oh!

Clemo97 commented Feb 9, 2026

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Feb 21, 2026

Uh oh!

Clemo97 commented Feb 24, 2026

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Clemo97 commented Feb 25, 2026

Uh oh!

ahoppen commented Mar 8, 2026

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ahoppen left a comment •

edited

Loading