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]

Superfluous newline, same in the functions below.

[ahoppen]

Should we check for intersects here so that you also move those declarations that you partially selected, eg. where you missed the function body?
Also, should we use $0.trimmedRange so that you don’t also move declarations whose leading whitespace or comments you may have selected?

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

[ahoppen]

Instead of doing a range-based check here, should we check if selectedMembers contains that member? Something like the following.

Suggested change

	let remainingMembers = declGroup.memberBlock.members.filter { !context.range.contains($0.range) }
	let remainingMembers = declGroup.memberBlock.members.filter { !selectedMembers..contains($0) }

[ahoppen]

Suggested change

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

[ahoppen]

You can ensure that we have a space here by running

Suggested change

	var declName = decl.name
	// e.g, after `Outer<T>` trailing trivia is empty
	if declName.trailingTrivia.isEmpty {
	declName = declName.with(\.trailingTrivia, .space)
	}
	var declName = decl.name
	declName.trailingTrivia = declName.trailingTrivia.merging(.space)

[ahoppen]

Isn’t this the same as

Suggested change

	updatedStatements.remove(at: index)
	updatedStatements.insert(updatedItem, at: index)
	updatedStatements[index] = updatedItem

[ahoppen]

Let’s be a little more specific which index this is.

Suggested change

	let index = syntax.statements.index(of: statement)
	let statementIndex = syntax.statements.index(of: statement)

[ahoppen]

You can also modify syntax directly and don’t have to go through updatedStatements.

Suggested change

	var updatedStatements = syntax.statements
	updatedStatements.remove(at: index)
	updatedStatements.insert(updatedItem, at: index)
	updatedStatements.insert(
	CodeBlockItemSyntax(item: .decl(DeclSyntax(extensionDecl))),
	at: syntax.statements.index(after: index)
	)
	return syntax.with(\.statements, updatedStatements)
	var syntax = syntax
	syntax.statements[index] = updatedItem
	syntax.statements.insert(
	CodeBlockItemSyntax(item: .decl(DeclSyntax(extensionDecl))),
	at: syntax.statements.index(after: index)
	)
	return syntax

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