Refactor MoveMembersToExtension

[myaumura]

This PR implements the MoveMembersToExtension refactoring action as requested in issue.

[myaumura]

@ahoppen
Could you please take a look at the PR and let me know if I’m going in the right direction, or if there’s a better approach?
Also, what do you think is the best way to implement the tests — using markers or something else?
I’d really appreciate any feedback.

[ahoppen]

Thanks for adding this, @myaumura.

Also: There’s no need to ping me when opening the PR. I receive notifications for newly opened PRs and will review them when I’ve got time.

[ahoppen]

We should specify a range of declarations to move, not their identifiers. The base identifier names are not sufficient to identify a declaration if it’s overloaded.

[myaumura]

Fixed

[ahoppen]

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() {}
  }
}

[myaumura]

Added a couple of tests, the rest are in progress.

[ahoppen]

Should we insert the declaration after the updated item instead of at the end of the file?

[myaumura]

Fixed

[ahoppen]

I think the refactoring action almost works as I’d expect now, just a few more comments to give the implementation its final polish and make it easier to read / maintain in the future.

[ahoppen]

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 deinitializers cannot be moved to an extension if the deinitializer is the only declaration that was selected.

[myaumura]

Could you give some advice on how to handle error logging for this?

[ahoppen]

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
  }
}

[ahoppen]

Suggested change

	let updatedItem = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup)))
	let updatedStatement = statement.with(\.item, .decl(DeclSyntax(updatedDeclGroup)))

[ahoppen]

I think this is still outstanding.

[myaumura]

oops, missed this

[ahoppen]

Instead of hard-coding UTF-8 offsets, which are hard to correlate back to the input text, could you use position markers (see extractMarkers). I find that usually that makes the tests significantly easier to read. Also, I think it would be good to add an assertion handler function so that the test reads as follows

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.

[ahoppen]

Noticed a few more things while looking through this again 😉

[ahoppen]

Could you use a for … in ... loop instead of forEach. I think forEach will also be diagnosed as an issue by swift format lint.

[ahoppen]

Nitpick: The preferred style in this repo is

Suggested change

	var selectedMembers = [MemberBlockItemSyntax]()
	var selectedIdentifiers = [SyntaxIdentifier]()
	var selectedMembers: [MemberBlockItemSyntax] = []
	var selectedIdentifiers: [SyntaxIdentifier] = []

[ahoppen]

I think we can avoid selectedIdentifiers if you change this to

Suggested change

	updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !selectedIdentifiers.contains($0.id) }
	updatedDeclGroup.memberBlock.members = declGroup.memberBlock.members.filter { !selectedMembers.contains($0) }

[myaumura]

I tried this and found that it does not filter out unselected members.

[ahoppen]

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.

[myaumura]

 .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.

[ahoppen]

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.

[ahoppen]

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
  }
}

[ahoppen]

I think this is still outstanding.

[ahoppen]

callDecl seems like an odd name here because these aren’t calls and calls aren’t decls either. Wouldn’t source be a better name here?

[ahoppen]

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 bar here.

[ahoppen]

I think this test case encompasses all cases from testMoveFunctionFromClass and testMoveFunctionFromClass2, which should make those two tests superfluous.

[ahoppen]

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.

[ahoppen]

I think it would be good to also have test cases for:

• No members are selected (ie. empty selection range)
• Members in a nested type are selected
• Only members that cannot be moved are selected (eg. only a deinitializer is selected)

[ahoppen]

We recently decided that code actions should live in the SourceKit-LSP repository unless there is a good reason for it not to (https://github.com/swiftlang/sourcekit-lsp/blob/main/Contributor%20Documentation/Code%20Action.md). This means that we should implement this in the SourceKit-LSP repository. Could you open a PR to add it over there? That way you can also already include it in Code Actions.md and add it to the list of code actions reported by SourceKit-LSP.

[ahoppen]

We shouldn’t print the set with [ and ] in the error message.

Suggested change

	"Cannot move \(Set(selectedMembers.compactMap(\.validationResult)).map(\.description).sorted()) to extension"
	"Cannot move \(Set(selectedMembers.compactMap(\.validationResult)).map(\.description).sorted().joined(separator: ", ")) to extension"

[ahoppen]

Should we remove the empty line here?

[ahoppen]

What happens if you only select moveThis? Could you add a test case for it?