-
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 6 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,101 @@ | ||||||
| //===----------------------------------------------------------------------===// | ||||||
| // | ||||||
| // 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 statementIndex = syntax.statements.index(of: statement) | ||||||
| else { | ||||||
| throw RefactoringNotApplicableError("Type declaration not found") | ||||||
| } | ||||||
|
|
||||||
| var selectedMembers = [MemberBlockItemSyntax]() | ||||||
| var selectedIdentifiers = [SyntaxIdentifier]() | ||||||
|
|
||||||
| var notMovedMembers: [MemberBlockItemSyntax] = [] | ||||||
|
|
||||||
| declGroup.memberBlock.members.forEach { | ||||||
|
||||||
| if context.range.overlaps($0.trimmedRange) { | ||||||
| if validateMember($0) { | ||||||
| selectedMembers.append($0) | ||||||
| selectedIdentifiers.append($0.id) | ||||||
| } else { | ||||||
| notMovedMembers.append($0) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| guard !selectedMembers.isEmpty else { | ||||||
| throw RefactoringNotApplicableError("No members to move") | ||||||
| } | ||||||
|
|
||||||
| var updatedDeclGroup = declGroup | ||||||
| updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !selectedIdentifiers.contains($0.id) } | ||||||
|
||||||
| updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !selectedIdentifiers.contains($0.id) } | |
| updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !selectedMembers.contains($0) } |
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.
I tried this and found that it does not filter out unselected members.
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.
That surprises me a lot because equality of syntax nodes is implemented by comparing their IDs. If you are sure that we need a selectedIdentifiers stick with it but I’d like to really understand why it’s necessary.
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.
.filter { context.range.overlaps($0.trimmedRange) } returns a new collection with new identifiers, and when comparing them, it does not remove the selected ones. Correct me if I’m wrong.
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.
Oh, nice find. I totally forgot about that. If you convert the syntax node to an array first, we just hold on to element nodes and won’t perform a syntax tree transformation on filter, which should work around this. Ie. you should be able to do the following
let selectedMembers = Array(declGroup.memberBlock.members).filter { context.range.overlaps($0.trimmedRange) }If that doesn’t work, let’s stick with the selectedIdentifiers approach that you have.
Outdated
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.
| let updatedItem = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup))) | |
| let updatedStatement = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup))) |
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.
I think this is still outstanding.
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.
oops, missed this
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.
Nitpick: The preferred style in this repo is