-
Notifications
You must be signed in to change notification settings - Fork 525
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
Changes from 4 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,98 @@ | ||||||
| //===----------------------------------------------------------------------===// | ||||||
| // | ||||||
| // 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) | ||||||
| public import SwiftSyntax | ||||||
| #else | ||||||
| import SwiftSyntax | ||||||
| #endif | ||||||
|
|
||||||
| 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 index = syntax.statements.index(of: statement) | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
| else { | ||||||
| throw RefactoringNotApplicableError("Type declaration not found") | ||||||
| } | ||||||
|
|
||||||
| let selectedMembers = declGroup.memberBlock.members.filter { context.range.contains($0.range) } | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| guard !selectedMembers.isEmpty else { | ||||||
| throw RefactoringNotApplicableError("No members to move") | ||||||
| } | ||||||
|
|
||||||
| for member in selectedMembers { | ||||||
| try validateMember(member) | ||||||
| } | ||||||
|
|
||||||
| let remainingMembers = declGroup.memberBlock.members.filter { !context.range.contains($0.range) } | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| let updatedMemberBlock = declGroup.memberBlock.with(\.members, remainingMembers) | ||||||
| let updatedDeclGroup = declGroup.with(\.memberBlock, updatedMemberBlock) | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
| let updatedItem = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup))) | ||||||
|
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.
Suggested change
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. I think this is still outstanding.
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. oops, missed this |
||||||
|
|
||||||
| let extensionMemberBlockSyntax = declGroup.memberBlock.with(\.members, selectedMembers) | ||||||
|
|
||||||
| var declName = decl.name | ||||||
|
|
||||||
| // e.g, after `Outer<T>` trailing trivia is empty | ||||||
| if declName.trailingTrivia.isEmpty { | ||||||
| declName = declName.with(\.trailingTrivia, .space) | ||||||
| } | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| let extensionDecl = ExtensionDeclSyntax( | ||||||
| leadingTrivia: .newlines(2), | ||||||
| extendedType: IdentifierTypeSyntax( | ||||||
| leadingTrivia: .space, | ||||||
| name: declName | ||||||
| ), | ||||||
| memberBlock: extensionMemberBlockSyntax | ||||||
| ) | ||||||
|
|
||||||
| var updatedStatements = syntax.statements | ||||||
| updatedStatements.remove(at: index) | ||||||
| updatedStatements.insert(updatedItem, at: index) | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
| updatedStatements.insert( | ||||||
| CodeBlockItemSyntax(item: .decl(DeclSyntax(extensionDecl))), | ||||||
| at: syntax.statements.index(after: index) | ||||||
| ) | ||||||
| return syntax.with(\.statements, updatedStatements) | ||||||
|
ahoppen marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
|
|
||||||
| private static func validateMember(_ member: MemberBlockItemSyntax) throws { | ||||||
|
|
||||||
| if member.decl.is(AccessorDeclSyntax.self) || member.decl.is(DeinitializerDeclSyntax.self) | ||||||
| || member.decl.is(EnumCaseDeclSyntax.self) | ||||||
| { | ||||||
| throw RefactoringNotApplicableError("Cannot move this type of declaration") | ||||||
|
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 be a little more specific in the error message about what we can’t move. I would imagine that this error message is annoying to get if you selected 200 lines of code and you don’t know what this is. Actually, just thinking about it, when you select 5 functions and one deinitializer, maybe we should just move the 5 functions and leave the deinitializer where it is. And only emit an error like
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. Could you give some advice on how to handle error logging for this?
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. Just played around a little bit how I would model this. What do you think of the following? 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:
break
}
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 = declGroup.memberBlock.members.filter { context.range.overlaps($0.trimmedRange) }
.map { (member: $0, validationResult: ValidationResult($0)) }
guard !selectedMembers.isEmpty else {
throw RefactoringNotApplicableError("No members to move")
}
let 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
updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !membersToMove.contains($0) }
let updatedItem = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup)))
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
syntax.statements[statementIndex] = updatedItem
syntax.statements.insert(
CodeBlockItemSyntax(item: .decl(DeclSyntax(extensionDecl))),
at: syntax.statements.index(after: statementIndex)
)
return syntax
}
} |
||||||
| } | ||||||
|
|
||||||
| if let varDecl = member.decl.as(VariableDeclSyntax.self), | ||||||
| varDecl.bindings.contains(where: { $0.accessorBlock == nil || $0.initializer != nil }) | ||||||
| { | ||||||
| throw RefactoringNotApplicableError("Cannot move stored properties to extension") | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,207 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // 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 { | ||
| let baseline: SourceFileSyntax = """ | ||
| class Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| let expected: SourceFileSyntax = """ | ||
| class Foo { | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| let context = MoveMembersToExtension.Context( | ||
| range: AbsolutePosition(utf8Offset: 11)..<AbsolutePosition(utf8Offset: 56) | ||
| ) | ||
|
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. Instead of hard-coding UTF-8 offsets, which are hard to correlate back to the input text, could you use position markers (see assertMoveMembersToExtension(
"""
class Foo 1️⃣{
func foo() {
print("Hello world!")
}2️⃣
func bar() {
print("Hello world!")
}
}
""",
expected: """
class Foo {
func bar() {
print("Hello world!")
}
}
extension Foo {
func foo() {
print("Hello world!")
}
}
"""
)And I probably mis-placed the position markers here, please double check. |
||
| try assertRefactorConvert(baseline, expected: expected, context: context) | ||
| } | ||
|
|
||
| func testMoveFunctionFromClass2() throws { | ||
| let baseline: SourceFileSyntax = """ | ||
| class Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
|
|
||
| let expected: SourceFileSyntax = """ | ||
| class Foo { | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
|
|
||
| let context = MoveMembersToExtension.Context( | ||
| range: AbsolutePosition(utf8Offset: 11)..<AbsolutePosition(utf8Offset: 56) | ||
| ) | ||
| try assertRefactorConvert(baseline, expected: expected, context: context) | ||
| } | ||
|
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 testMoveNestedFromStruct() throws { | ||
| let baseline: SourceFileSyntax = """ | ||
| struct 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. What happens if you only select |
||
| } | ||
| } | ||
| """ | ||
|
|
||
| let expected: SourceFileSyntax = """ | ||
| struct Outer { | ||
| } | ||
|
|
||
| extension Outer { | ||
| struct Inner { | ||
| func moveThis() {} | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| let context = MoveMembersToExtension.Context( | ||
| range: AbsolutePosition(utf8Offset: 14)..<AbsolutePosition(utf8Offset: 58) | ||
| ) | ||
| try assertRefactorConvert(baseline, expected: expected, context: context) | ||
| } | ||
|
|
||
| func testMoveNestedFromStruct2() throws { | ||
| let baseline: SourceFileSyntax = """ | ||
| struct Outer<T> { | ||
| struct Inner { | ||
| func moveThis() {} | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| let expected: SourceFileSyntax = """ | ||
| struct Outer<T> { | ||
| } | ||
|
|
||
| extension Outer { | ||
| struct Inner { | ||
| func moveThis() {} | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| let context = MoveMembersToExtension.Context( | ||
| range: AbsolutePosition(utf8Offset: 17)..<AbsolutePosition(utf8Offset: 61) | ||
| ) | ||
| try assertRefactorConvert(baseline, expected: expected, context: context) | ||
| } | ||
|
|
||
| func testMoveMembersFromEnum() throws { | ||
|
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. Enums are treated exactly the same as any other type here, so I’m not sure if this test adds a lot of value. Correct me if I’m wrong. |
||
| let baseline: SourceFileSyntax = """ | ||
| enum Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
|
|
||
| let expected: SourceFileSyntax = """ | ||
| enum Foo { | ||
|
|
||
| func bar() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| extension Foo { | ||
| func foo() { | ||
| print("Hello world!") | ||
| } | ||
| } | ||
|
|
||
| struct Bar { | ||
| func foo() {} | ||
| } | ||
| """ | ||
|
|
||
| let context = MoveMembersToExtension.Context( | ||
| range: AbsolutePosition(utf8Offset: 10)..<AbsolutePosition(utf8Offset: 55) | ||
| ) | ||
| try assertRefactorConvert(baseline, expected: expected, context: context) | ||
| } | ||
|
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. I think it would be good to also have test cases for:
|
||
| } | ||
|
|
||
| private func assertRefactorConvert( | ||
| _ callDecl: SourceFileSyntax, | ||
| expected: SourceFileSyntax?, | ||
| context: MoveMembersToExtension.Context, | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) throws { | ||
| try assertRefactor( | ||
| callDecl, | ||
| context: context, | ||
| provider: MoveMembersToExtension.self, | ||
| expected: expected, | ||
| file: file, | ||
| line: line | ||
| ) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.