Add new features to ConvertStoredPropertyToComputed by myaumura · Pull Request #3229 · swiftlang/swift-syntax

myaumura · 2026-01-07T20:01:32Z

As part of swiftlang/sourcekit-lsp#2424.
Add new functionality to the existing code action analogous to ConvertToComputedProperty.cpp and tryComputedPropertyFixIts.

Support removing lazy attribute if present.
Support removing the = sign if variable like var foo: Int = { return 0 }().

I'm happy to make changes if needed

ahoppen

Thanks for your contribution, @myaumura.

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

Tests/SwiftRefactorTest/ConvertStoredPropertyToComputedTest.swift

myaumura · 2026-01-07T20:53:25Z

@ahoppen, Thanks for your review!

ahoppen

Looking at this again, I noticed that the = { … }() should already be handled.

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

Tests/SwiftRefactorTest/ConvertStoredPropertyToComputedTest.swift

ahoppen

Sorry, looks like the trivia handling is going to be a little more complex than I initially thought. But nothing you won’t be able to do 😉

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

Tests/SwiftRefactorTest/ConvertStoredPropertyToComputedTest.swift

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

myaumura · 2026-01-12T10:05:04Z

Sources/SwiftSyntax/NodeRemover.swift

I’m not sure it should be in this module

ahoppen · 2026-01-13T21:51:25Z

Sources/SwiftSyntax/NodeRemover.swift

+
+/// Removes attributes from a syntax tree while maintaining their surrounding trivia.
+@_spi(Testing)
+public class NodeRemover: SyntaxRewriter {


Let’s change this to the package access level. I think we didn’t use to have it when this was written.

ahoppen · 2026-01-13T21:52:09Z

Sources/SwiftSyntax/NodeRemover.swift

+//
+//===----------------------------------------------------------------------===//
+
+import Foundation


swift-syntax must not depend on Foundation because it’s lower in the stack than Foundation. Could you remove the Foundation dependency?

yeah, my miss

ahoppen · 2026-01-13T21:58:51Z

Sources/SwiftSyntax/NodeRemover.swift

+    return AttributeListSyntax(filteredAttributes)
+  }
+
+  public override func visit(_ node: DeclModifierListSyntax) -> DeclModifierListSyntax {


Hmm, I was hoping that we could extend NodeRemover to be able to remove any arbitrary node. This doesn’t seem to be the case right now and I don’t want to have a semi-general type that promises to remove arbitrary nodes but isn’t actually able to do so. Maybe the better solution in that case is to extract all the common functionality (like the trivia convenience accessors) to the SwiftSyntax module and write another dedicated rewriter to remove decl modifiers.

I’ll try to work on this idea

I’d like to clarify — is the current purpose to extract the base logic into a NodeRemover class and have AttributeRemover and DeclModifierRemover inherit from it, with only strictly typed visit methods remaining in them?

No, I’m not proposing a type hierarchy. Essentially just create two separate classes and whatever you can factor out, factor out. I was mostly talking about at least sharing Trivia.trimmingPrefix etc.

Hope I got it right this time, could you please check and sorry for the previous misunderstanding

ahoppen · 2026-01-13T21:59:08Z

Sources/SwiftSyntax/NodeRemover.swift

+    var filteredModifiers: [DeclModifierListSyntax.Element] = []
+
+    for modifier in node {
+      if self.predicate(Syntax(modifier)) {


Would this read easier as a guard self.predicate(Syntax(modifier)) else {? At least it saves an indentation level.

ahoppen · 2026-01-13T22:00:18Z

Sources/SwiftSyntax/NodeRemover.swift

+
+    for modifier in node {
+      if self.predicate(Syntax(modifier)) {
+        let trailingTrivia = modifier.trailingTrivia.trimmingPrefix(while: \.isSpaceOrTab)


Do we need to trim the spaces and tabs here. Shouldn’t that be handled already when merging triviaToAttachToNextToken into existing trivia? Or am I missing something?

When lazy is first before the comment, removing lazy leaves the space before the comment – it doesn't merge with trivia after comment.

case handled in test testRefactoringStoredPropertyWithModifiersAndComment

Oh, I see. That makes sense. Thanks for the explanation. Can you add that as a comment for future reference?

Added comment

ahoppen · 2026-01-13T22:01:05Z

Sources/SwiftSyntax/NodeRemover.swift

+        if triviaToAttachToNextToken.isEmpty,
+          let previousToken = modifier.previousToken(viewMode: .sourceAccurate),
+          previousToken.trailingTrivia.isEmpty
+        {
+          triviaToAttachToNextToken = modifier.trailingTrivia
+        }


Why is this check necessary? Could you maybe add a doc comment that explains it?

removed this code

ahoppen · 2026-01-13T22:02:18Z

Sources/SwiftSyntax/NodeRemover.swift

+    defer { triviaToAttachToNextToken = Trivia() }
+    return syntaxNode.with(\.leadingTrivia, triviaToAttachToNextToken + syntaxNode.leadingTrivia)


I think we should get a little better performance if we don’t modify the node if triviaToAttachToNextToken is empty (that way we don’t need to allocate a new node), ie. just do an early exist if triviaToAttachToNextToken is empty.

ahoppen · 2026-01-13T22:04:08Z

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

+        guard let keyword = $0.as(DeclModifierSyntax.self) else { return false }
+        return keyword.name.tokenKind == lazyKeyword.name.tokenKind


Shouldn’t we be able to use the identity on Syntax to find the token to remove, ie. do something like the following

Suggested change

guard let keyword = $0.as(DeclModifierSyntax.self) else { return false }

return keyword.name.tokenKind == lazyKeyword.name.tokenKind

$0.id == lazyKeyword.id

ahoppen

Nice. Just a few small comments, otherwise LGTM.

Sources/SwiftRefactor/DeclModifierRemover.swift

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift

Sources/SwiftRefactor/DeclModifierRemover.swift

ahoppen · 2026-01-15T07:21:30Z

Sources/SwiftRefactor/DeclModifierRemover.swift

+
+    return DeclModifierListSyntax(filteredModifiers)
+  }
+


Do we also need a visit function for Token that calls prependAndClearAccumulatedTrivia to transfer trivia to the token after the last modifier, eg. in lazy /* comment */ var test: Int?

Could you add a test case for this so we don’t regress it in the future?

Sources/SwiftRefactor/DeclModifierRemover.swift

ahoppen · 2026-01-17T09:23:05Z

Just #3229 (comment) left, after that this is good to go 🚀

ahoppen

Nice. I just realized that we didn’t report this refactoring action in SourceKit-LSP so far, fixed that in swiftlang/sourcekit-lsp#2458 because I was touching related code anyway.

ahoppen · 2026-01-18T10:38:29Z

@swift-ci Please test

ahoppen · 2026-01-18T20:03:37Z

@swift-ci Please test

ahoppen · 2026-01-18T20:06:11Z

@swift-ci Please test Windows

ahoppen · 2026-01-18T20:07:12Z

@myaumura We can ignore the Bazel build failure, it is because of #3232 (comment).

myaumura · 2026-01-18T20:16:45Z

@ahoppen, Thank you for the thorough review and detailed explanations—they really helped clarify the issues!

Should I squash commits?

ahoppen · 2026-01-18T21:00:48Z

Yes, squashing would be appreciated, it makes for a cleaner git history.

And it appears that the CMake build doesn’t support the package access level yet either (not just the Bazel build). Could you use @_spi(RawSyntax) public in the meantime instead?

myaumura · 2026-01-18T21:04:54Z

Sure, doing it now!

ahoppen · 2026-01-18T21:23:24Z

@swift-ci Please test

…ests

ahoppen · 2026-01-18T21:24:35Z

Also FWIW, adding support for the package access modifier in #3239

ahoppen · 2026-01-18T21:24:50Z

@swift-ci Please test

ahoppen · 2026-01-18T21:26:56Z

@swift-ci Please test Windows

ahoppen · 2026-01-19T08:01:38Z

Congrats to your first contribution to Swift, @myaumura 🚀

myaumura requested review from ahoppen, hamishknight, hborla and rintaro as code owners January 7, 2026 20:01

ahoppen reviewed Jan 7, 2026

View reviewed changes

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift Outdated Show resolved Hide resolved

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift Outdated Show resolved Hide resolved

Tests/SwiftRefactorTest/ConvertStoredPropertyToComputedTest.swift Show resolved Hide resolved

ahoppen reviewed Jan 7, 2026

View reviewed changes

ahoppen reviewed Jan 8, 2026

View reviewed changes

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift Outdated Show resolved Hide resolved

Tests/SwiftRefactorTest/ConvertStoredPropertyToComputedTest.swift Show resolved Hide resolved

Sources/SwiftRefactor/ConvertStoredPropertyToComputed.swift Show resolved Hide resolved

myaumura commented Jan 12, 2026

View reviewed changes

Sources/SwiftSyntax/NodeRemover.swift Outdated

Copy link

Contributor Author

myaumura Jan 12, 2026

Choose a reason for hiding this comment

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

I’m not sure it should be in this module

ahoppen reviewed Jan 13, 2026

View reviewed changes

ahoppen reviewed Jan 15, 2026

View reviewed changes

ahoppen mentioned this pull request Jan 17, 2026

Rewrite syntactic refactoring actions in sourcekitd using swift-syntax swiftlang/sourcekit-lsp#2424

Open

10 tasks

ahoppen approved these changes Jan 18, 2026

View reviewed changes

Add DeclModifierRemover for ConvertStoredPropertyToComputed and new t…

4b2774d

…ests

myaumura force-pushed the update-computed-properties-codeaction branch from ce91e33 to 4b2774d Compare January 18, 2026 21:23

ahoppen merged commit 65b02a9 into swiftlang:main Jan 19, 2026
37 checks passed

myaumura deleted the update-computed-properties-codeaction branch January 19, 2026 09:22

		defer { triviaToAttachToNextToken = Trivia() }
		return syntaxNode.with(\.leadingTrivia, triviaToAttachToNextToken + syntaxNode.leadingTrivia)

		guard let keyword = $0.as(DeclModifierSyntax.self) else { return false }
		return keyword.name.tokenKind == lazyKeyword.name.tokenKind

	guard let keyword = $0.as(DeclModifierSyntax.self) else { return false }
	return keyword.name.tokenKind == lazyKeyword.name.tokenKind
	$0.id == lazyKeyword.id

Conversation

myaumura commented Jan 7, 2026

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

myaumura commented Jan 7, 2026

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

ahoppen commented Jan 17, 2026

Uh oh!

ahoppen left a comment