[SwiftRefactor] EditRefactoringProvider: Make `textRefactor` and `ref… #3142

xedin · 2025-08-23T01:00:04Z

…actor` requirements throwing

The result of both has been made non-optional as well. The idea here is to be able to produce a description for every failure instead of nil result, this is something that package model refactoring actions already do.

The only error we currently have is "not applicable" but that's just a start.

ahoppen

Nice improvement. A couple nitpicks with respect to the new error messages.

ahoppen · 2025-08-23T06:25:44Z

Sources/SwiftRefactor/CallToTrailingClosures.swift

-  /// `nil` is returned.
-  // TODO: Rather than returning nil, we should consider throwing errors with
-  // appropriate messages instead.
+  /// error is thrown.


Suggested change

/// error is thrown.

/// an error is thrown.

ahoppen · 2025-08-23T06:27:58Z

Sources/SwiftRefactor/CallToTrailingClosures.swift

-    return converted?.formatted().as(C.self)
+  ) throws -> C {
+    guard let converted = call.convertToTrailingClosures(from: context.startAtArgument) else {
+      throw RefactoringNotApplicableError("")


Suggested change

throw RefactoringNotApplicableError("")

throw RefactoringNotApplicableError("failed to convert call to trailing closures")

Sorry, I forgot to commit a change to sync this down to convertToTrailingClosures 🤦

ahoppen · 2025-08-23T06:28:10Z

Sources/SwiftRefactor/CallToTrailingClosures.swift

+      return formatted
+    }
+
+    throw RefactoringNotApplicableError("")


Suggested change

throw RefactoringNotApplicableError("")

throw RefactoringNotApplicableError("failed to format converted expression")

ahoppen · 2025-08-23T06:29:46Z

Sources/SwiftRefactor/ConvertComputedPropertyToStored.swift

    }

-    return nil
+    throw RefactoringNotApplicableError("")


Suggested change

throw RefactoringNotApplicableError("")

throw RefactoringNotApplicableError("failed to extract initial value of stored variable")

ahoppen · 2025-08-23T06:30:47Z

Sources/SwiftRefactor/ConvertComputedPropertyToStored.swift


    guard body.count == 1, let item = body.first?.item else {
-      return nil
+      throw RefactoringNotApplicableError("getter body is not a single-expression")


Suggested change

throw RefactoringNotApplicableError("getter body is not a single-expression")

throw RefactoringNotApplicableError("getter body is not a single expression")

ahoppen · 2025-08-23T06:31:31Z

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

    {
-      guard functionExpression.arguments.isEmpty else { return nil }
+      guard functionExpression.arguments.isEmpty else {
+        throw RefactoringNotApplicableError("not a nullary call")


Suggested change

throw RefactoringNotApplicableError("not a nullary call")

throw RefactoringNotApplicableError("cannot convert stored property whose initializer is a closure that takes arguments")

ahoppen · 2025-08-23T06:35:12Z

Sources/SwiftRefactor/RefactoringProvider.swift

 import SwiftSyntax
 #endif

+public struct RefactoringNotApplicableError: Error, CustomStringConvertible {


Suggested change

public struct RefactoringNotApplicableError: Error, CustomStringConvertible {

/// Error that refactoring actions can throw when the refactoring fails.

///

/// The reason should start with a single character and should be formatted similarly to compiler error messages.

public struct RefactoringNotApplicableError: Error, CustomStringConvertible {

Thanks! I was thinking whether I should add a comment there or it's self-evident what this is since none other errors declared in swift-syntax do...

What does "The reason should start with a single character" mean?

xedin · 2025-08-25T07:32:07Z

swiftlang/sourcekit-lsp#2251
@swift-ci please test

bnbarham · 2025-08-25T18:27:13Z

Sources/SwiftRefactor/CallToTrailingClosures.swift

+    let converted = try call.convertToTrailingClosures(from: context.startAtArgument)
+
+    guard let formatted = converted.formatted().as(C.self) else {
+      throw RefactoringNotApplicableError("cannot cast formatted call to \(C.self)")


I'd prefer keeping them more user-focused. This one is a little weird since it suggests more of an internal failure, maybe just "format error"?

bnbarham · 2025-08-25T18:28:08Z

Sources/SwiftRefactor/ConvertComputedPropertyToStored.swift

    }

-    return nil
+    throw RefactoringNotApplicableError("failed to extract initial value of stored variable")


The rest are all failures too, how about "could not extract initial value of stored variable"?

Sounds good, I think it should say "property" instead of a "variable" to be consistent with compiler diagnostics.

bnbarham · 2025-08-25T18:31:48Z

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

-      guard functionExpression.arguments.isEmpty else { return nil }
+      guard functionExpression.arguments.isEmpty else {
+        throw RefactoringNotApplicableError(
+          "cannot convert stored property whose initializer is a closure that takes arguments"


Just "initializer is a closure that takes arguments" seems fine? The "cannot convert stored property" is essentially the error itself

bnbarham · 2025-08-25T18:34:46Z

Sources/SwiftRefactor/RefactoringProvider.swift

 import SwiftSyntax
 #endif

+public struct RefactoringNotApplicableError: Error, CustomStringConvertible {


What does "The reason should start with a single character" mean?

bnbarham · 2025-08-25T18:37:05Z

Sources/SwiftRefactor/RefactoringProvider.swift

+  /// - Throws: Throws an error if the refactoring action although applicable
+  ///           fails for some other reason.


Is this true? Aren't we throwing if it isn't applicable, or did I miss a catch?

Thanks for catching this! There were a few places left from the time when Output was still optional in refactor.

bnbarham · 2025-08-25T18:38:05Z

Sources/SwiftRefactor/RemoveSeparatorsFromIntegerLiteral.swift

-      return lit
-    }
+  public static func refactor(syntax lit: IntegerLiteralExprSyntax, in context: Void) -> IntegerLiteralExprSyntax {
+    guard lit.literal.text.contains("_") else { return lit }


Shouldn't this throw with "literal does not contain '_'"?

I originally changed it but then reverted, I'd rather fix it in a separate PR.

…actor` requirements throwing The result of both has been made non-optional as well. The idea here is to be able to produce a description for every failure instead of `nil` result, this is something that package model refactoring actions already do. The only error we currently have is "not applicable" but that's just a start.

xedin · 2025-08-25T19:16:09Z

swiftlang/sourcekit-lsp#2251
@swift-ci please test

xedin · 2025-08-25T20:12:34Z

swiftlang/sourcekit-lsp#2251
@swift-ci please test Windows platform

xedin requested review from ahoppen, bnbarham, hamishknight and rintaro as code owners August 23, 2025 01:00

xedin mentioned this pull request Aug 23, 2025

[SwiftLanguageService] Adopt to throwing refactoring actions swiftlang/sourcekit-lsp#2251

Merged

ahoppen reviewed Aug 23, 2025

View reviewed changes

xedin force-pushed the text-refactor-should-be-able-to-throw branch from a82a2df to ecd3f9d Compare August 23, 2025 06:39

ahoppen approved these changes Aug 23, 2025

View reviewed changes

xedin force-pushed the text-refactor-should-be-able-to-throw branch from ecd3f9d to b6bb15e Compare August 23, 2025 06:45

bnbarham reviewed Aug 25, 2025

View reviewed changes

xedin force-pushed the text-refactor-should-be-able-to-throw branch from b6bb15e to a379116 Compare August 25, 2025 19:15

bnbarham approved these changes Aug 25, 2025

View reviewed changes

xedin merged commit 9c2371a into swiftlang:main Aug 26, 2025
30 checks passed

	throw RefactoringNotApplicableError("")
	throw RefactoringNotApplicableError("failed to convert call to trailing closures")

	throw RefactoringNotApplicableError("getter body is not a single-expression")
	throw RefactoringNotApplicableError("getter body is not a single expression")

	throw RefactoringNotApplicableError("not a nullary call")
	throw RefactoringNotApplicableError("cannot convert stored property whose initializer is a closure that takes arguments")

		/// - Throws: Throws an error if the refactoring action although applicable
		/// fails for some other reason.

[SwiftRefactor] EditRefactoringProvider: Make textRefactor and `ref… #3142

[SwiftRefactor] EditRefactoringProvider: Make textRefactor and `ref… #3142

Uh oh!

Conversation

xedin commented Aug 23, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Aug 25, 2025

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!

xedin commented Aug 25, 2025

Uh oh!

xedin commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

[SwiftRefactor] EditRefactoringProvider: Make `textRefactor` and `ref… #3142

[SwiftRefactor] EditRefactoringProvider: Make `textRefactor` and `ref… #3142