Skip to content

[SwiftSyntaxCodeActions] Move SwiftRefactor code actions in from swift-syntax#2671

Open
25harsh wants to merge 1 commit into
swiftlang:mainfrom
25harsh:move-refactor-code-actions-in
Open

[SwiftSyntaxCodeActions] Move SwiftRefactor code actions in from swift-syntax#2671
25harsh wants to merge 1 commit into
swiftlang:mainfrom
25harsh:move-refactor-code-actions-in

Conversation

@25harsh

@25harsh 25harsh commented May 27, 2026

Copy link
Copy Markdown
Contributor

Moved SyntaxRefactoringProvider types mentioned in #2639 out of swift-syntax and intoSources/SwiftSyntaxCodeActions/.
Made access specifier changes and dependency changes.

Companion PR #3345

Notes:

  1. Removed compiler(>=6) checks

  2. Ported helpers from Sources/SwiftRefactor/SyntaxUtils.swift to Sources/SwiftSyntaxCodeActions/SyntaxRefactoringCodeActionProvider.swift

  3. Test cases are yet to be moved.
    RemoveSeparatorsFromIntegerLiteral, RemoveRedundantParentheses andConvertStoredPropertyToComputed need coverage. (follow-on PRs)

  4. Helpers moved DeclModifierRemover & IntegerLiteralUtilities

@ahoppen ahoppen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR, @25harsh. I have two requests:

  • Could you ensure that each file has a trailing newline and that they pass the swift-format check as described in CONTRIBUTING.md?
  • Could you also migrate the tests to SourceKit-LSP. If they need some adjustment, that’s fine. I just don’t want to add the refactoring actions here without test coverage.

@25harsh 25harsh closed this May 27, 2026
@25harsh 25harsh force-pushed the move-refactor-code-actions-in branch from 0ae609b to 8e8a6ce Compare May 27, 2026 15:25
@25harsh 25harsh reopened this May 27, 2026
@25harsh

25harsh commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@ahoppen all changes are done, overloaded assertRefactor for SyntaxRefactoringProvider that accepts some SyntaxProtocol for input and expected to avoid ambiguity

@25harsh 25harsh requested a review from ahoppen May 28, 2026 17:53

@ahoppen ahoppen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you. This looks good to me, just one request: Could you create a separate SwiftSyntaxCodeActionsTests target for these new tests? We might want to move more of the existing code action tests to that target in a follow-up PR but I think it would be good to at least not inflate the current SourceKitLSPTests target further.

Also: Could you make sure that all test tests file and with Tests.swift to match the existing naming convention (and yes, I realize that they weren’t suffixed with Tests in swift-syntax but would be a good clean-up step now)

@25harsh 25harsh force-pushed the move-refactor-code-actions-in branch 2 times, most recently from 818349c to e3a8ffd Compare June 2, 2026 13:38
@25harsh 25harsh requested a review from ahoppen June 2, 2026 13:38
@25harsh

25harsh commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@ahoppen all changes done, created new target SwiftSyntaxCodeActionsTests also i moved assertRefactor overload from Tests/SourceKitLSPTests/SyntaxRefactorTests.swift (added in last commit) to Tests/SwiftSyntaxCodeActionsTests/RefactorTestUtils.swift (similar name as swift-syntax)

@ahoppen ahoppen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perfect 👌🏽 Thank you.

@ahoppen

ahoppen commented Jun 2, 2026

Copy link
Copy Markdown
Member

@swift-ci Please test

@ahoppen

ahoppen commented Jun 2, 2026

Copy link
Copy Markdown
Member

@swift-ci Please test Windows

@25harsh

25harsh commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@ahoppen tests are failing due to ambiguity, this removal pr swift-syntax #3345 needs to be merged first

@rintaro

rintaro commented Jun 2, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345
@swift-ci Please test

@25harsh 25harsh force-pushed the move-refactor-code-actions-in branch from e3a8ffd to d19dc0b Compare June 2, 2026 20:51
@25harsh

25harsh commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

rebased with main (Sources/SwiftSyntaxCodeActions/CMakeLists.txt conflicted)

@ahoppen

ahoppen commented Jun 2, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345
@swift-ci Please test

@ahoppen

ahoppen commented Jun 3, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345
@swift-ci Please test macOS

@ahoppen

ahoppen commented Jun 3, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345
@swift-ci Please test Windows

@25harsh 25harsh force-pushed the move-refactor-code-actions-in branch from d19dc0b to 9a114c9 Compare June 3, 2026 21:49
@25harsh 25harsh force-pushed the move-refactor-code-actions-in branch from 870b034 to 5d8e483 Compare June 3, 2026 23:09
@25harsh

25harsh commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Hey @ahoppen @rintaro, I noticed Windows CI errors related to @unknown default cases just pushed a commit adding #if RESILIENT_LIBRARIES

Note: for idealGroupSize in IntegerLiteralUtilities.swift I'm returning 3 since radix also falls back to .decimal https://github.com/swiftlang/swift-syntax/blob/d42bdde1ac20c3890639939b9bc9f3e390c7bf32/Sources/SwiftSyntax/Convenience.swift#L105

@ahoppen

ahoppen commented Jun 5, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345
@swift-ci Please test

@ahoppen

ahoppen commented Jun 5, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345

@swift-ci Please test Windows

1 similar comment
@ahoppen

ahoppen commented Jun 5, 2026

Copy link
Copy Markdown
Member

swiftlang/swift-syntax#3345

@swift-ci Please test Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants