-
Notifications
You must be signed in to change notification settings - Fork 498
Refactor MoveMembersToExtension
#3265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5522021
e6cc240
7e0e90d
7d91d8a
0486a2d
866bab9
218c688
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // 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) | ||
| @_spi(RawSyntax) public import SwiftSyntax | ||
| #else | ||
| @_spi(RawSyntax) import SwiftSyntax | ||
| #endif | ||
|
|
||
| private enum ValidationResult: CustomStringConvertible { | ||
| case accessor | ||
| case deinitializer | ||
| case enumCase | ||
| case storedProperty | ||
|
|
||
| var description: String { | ||
| switch self { | ||
| case .accessor: return "accessor" | ||
| case .deinitializer: return "deinitializer" | ||
| case .enumCase: return "enum case" | ||
| case .storedProperty: return "stored property" | ||
| } | ||
| } | ||
|
|
||
| /// Validates that `member` can be moved to an extension. If it can, return `nil`, otherwise return the reason why | ||
| /// `member` cannot be moved to an extension. | ||
| init?(_ member: MemberBlockItemSyntax) { | ||
| switch member.decl.kind { | ||
| case .accessorDecl: | ||
| self = .accessor | ||
| case .deinitializerDecl: | ||
| self = .deinitializer | ||
| case .enumCaseDecl: | ||
| self = .enumCase | ||
| default: | ||
| if let varDecl = member.decl.as(VariableDeclSyntax.self), | ||
| varDecl.bindings.contains(where: { $0.accessorBlock == nil || $0.initializer != nil }) | ||
| { | ||
| self = .storedProperty | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public struct MoveMembersToExtension: SyntaxRefactoringProvider { | ||
| public struct Context { | ||
| public let range: Range<AbsolutePosition> | ||
|
|
||
| public init(range: Range<AbsolutePosition>) { | ||
| self.range = range | ||
| } | ||
| } | ||
|
|
||
| public static func refactor(syntax: SourceFileSyntax, in context: Context) throws -> SourceFileSyntax { | ||
| guard | ||
| let statement = syntax.statements.first(where: { $0.item.range.contains(context.range) }), | ||
| let decl = statement.item.asProtocol(NamedDeclSyntax.self), | ||
| let declGroup = statement.item.asProtocol(DeclGroupSyntax.self), | ||
| let statementIndex = syntax.statements.index(of: statement) | ||
| else { | ||
| throw RefactoringNotApplicableError("Type declaration not found") | ||
| } | ||
|
|
||
| let selectedMembers = Array(declGroup.memberBlock.members).filter { context.range.overlaps($0.trimmedRange) } | ||
| .map { (member: $0, validationResult: ValidationResult($0)) } | ||
|
|
||
| var membersToMove = selectedMembers.filter({ $0.validationResult == nil }).map(\.member) | ||
|
|
||
| guard !membersToMove.isEmpty else { | ||
| throw RefactoringNotApplicableError( | ||
| "Cannot move \(Set(selectedMembers.compactMap(\.validationResult)).map(\.description).sorted()) to extension" | ||
| ) | ||
| } | ||
|
|
||
| var updatedDeclGroup = declGroup | ||
| let remainingMembers = Array(declGroup.memberBlock.members).filter { !membersToMove.contains($0) } | ||
| membersToMove[0].decl.leadingTrivia = membersToMove[0].decl.leadingTrivia.trimmingPrefix(while: \.isSpaceOrTab) | ||
|
|
||
| updatedDeclGroup.memberBlock.members = MemberBlockItemListSyntax(remainingMembers) | ||
| let extensionMemberBlockSyntax = declGroup.memberBlock.with(\.members, MemberBlockItemListSyntax(membersToMove)) | ||
|
|
||
| var declName = decl.name | ||
| declName.trailingTrivia = declName.trailingTrivia.merging(.space) | ||
|
|
||
| let extensionDecl = ExtensionDeclSyntax( | ||
| leadingTrivia: .newlines(2), | ||
| extendedType: IdentifierTypeSyntax( | ||
| leadingTrivia: .space, | ||
| name: declName | ||
| ), | ||
| memberBlock: extensionMemberBlockSyntax | ||
| ) | ||
|
|
||
| var syntax = syntax | ||
| let updatedStatement = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup))) | ||
| syntax.statements[statementIndex] = updatedStatement | ||
| syntax.statements.insert( | ||
| CodeBlockItemSyntax(item: .decl(DeclSyntax(extensionDecl))), | ||
| at: syntax.statements.index(after: statementIndex) | ||
| ) | ||
| return syntax | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,198 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // This source file is part of the Swift.org open source project | ||
| // | ||
| // 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 | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import SwiftParser | ||
| import SwiftRefactor | ||
| import SwiftSyntax | ||
| import SwiftSyntaxBuilder | ||
| import XCTest | ||
| import _SwiftSyntaxTestSupport | ||
|
|
||
| final class MoveMembersToExtensionTests: XCTestCase { | ||
| func testMoveFunctionFromClass() throws { | ||
| try assertMoveMembersToExtension( | ||
| """ | ||
| class Foo {1️⃣ | ||
| func foo() { | ||
| print("Hello world!") | ||
| }2️⃣ | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
| """, | ||
| expected: """ | ||
| class Foo { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we could do something to remove this superfluous newline? Eg. by trimming leading newlines of the new first item if we removed the previous first item? Similarly, we shouldn’t end up with empty newlines if you move |
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testMoveParticiallySelectedFunctionFromClass() throws { | ||
| try assertMoveMembersToExtension( | ||
| """ | ||
| class Foo { | ||
| func foo() { | ||
| 1️⃣print("Hello world!") | ||
| }2️⃣ | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """, | ||
| expected: """ | ||
| class Foo { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the empty line here? |
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testMoveSelectedFromClass() throws { | ||
| try assertMoveMembersToExtension( | ||
| """ | ||
| class Foo {1️⃣ | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
|
|
||
| deinit() {} | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| }2️⃣ | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """, | ||
| expected: """ | ||
| class Foo { | ||
|
|
||
| deinit() {} | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
|
||
| func testMoveNestedFromStruct() throws { | ||
| try assertMoveMembersToExtension( | ||
| """ | ||
| struct Outer {1️⃣ | ||
| struct Inner { | ||
| func moveThis() {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if you only select |
||
| }2️⃣ | ||
| } | ||
| """, | ||
| expected: """ | ||
| struct Outer { | ||
| } | ||
|
|
||
| extension Outer { | ||
| struct Inner { | ||
| func moveThis() {} | ||
| } | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make sure that we either disallow or correctly handle extracting members from nested types. Suggested test cases: struct Outer {
struct Inner {
func moveThis() {}
}
}struct Outer<T> {
struct Inner {
func moveThis() {}
}
}func outer() {
struct Inner {
func moveThis() {}
}
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a couple of tests, the rest are in progress. |
||
|
|
||
| func testMoveNestedFromStruct2() throws { | ||
| try assertMoveMembersToExtension( | ||
| """ | ||
| struct Outer<T> {1️⃣ | ||
| struct Inner { | ||
| func moveThis() {} | ||
| }2️⃣ | ||
| } | ||
| """, | ||
| expected: """ | ||
| struct Outer<T> { | ||
| } | ||
|
|
||
| extension Outer { | ||
| struct Inner { | ||
| func moveThis() {} | ||
| } | ||
| } | ||
| """ | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private func assertMoveMembersToExtension( | ||
| _ source: String, | ||
| expected: SourceFileSyntax, | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) throws { | ||
| let (markers, source) = extractMarkers(source.description) | ||
| let positions = markers.mapValues { AbsolutePosition(utf8Offset: $0) } | ||
| var parser = Parser(source) | ||
| let tree = SourceFileSyntax.parse(from: &parser) | ||
|
|
||
| let range = try XCTUnwrap(positions["1️⃣"])..<XCTUnwrap(positions["2️⃣"]) | ||
| let context = MoveMembersToExtension.Context(range: range) | ||
|
|
||
| try assertRefactor( | ||
| tree, | ||
| context: context, | ||
| provider: MoveMembersToExtension.self, | ||
| expected: expected, | ||
| file: file, | ||
| line: line | ||
| ) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn’t print the set with
[and]in the error message.