[tools] gopls/internal/golang: MoveType - extract type to new file

[Madeline Kalil (Gerrit)]

Madeline Kalil has uploaded the change for review

Commit message

gopls/internal/golang: MoveType - extract type to new file

Implements computing the edits to a new file after
adding the type declaration, imports, and build constraint comments.
Also deletes the type declaration from its current location.

Adds several checks for currently illegal type moves, which may be
relaxed later in the implementation.

Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Change diff

diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go
index d09fc47..a38a4e0 100644
--- a/gopls/internal/golang/codeaction.go
+++ b/gopls/internal/golang/codeaction.go
@@ -255,7 +255,7 @@
 	{kind: settings.RefactorExtractVariableAll, fn: refactorExtractVariableAll, needPkg: true},
 	{kind: settings.RefactorInlineCall, fn: refactorInlineCall, needPkg: true},
 	{kind: settings.RefactorInlineVariable, fn: refactorInlineVariable, needPkg: true},
-	// {kind: settings.RefactorMoveType, fn: refactorMoveType, needPkg: true},
+	{kind: settings.RefactorMoveType, fn: refactorMoveType, needPkg: true},
 	{kind: settings.RefactorRewriteChangeQuote, fn: refactorRewriteChangeQuote},
 	{kind: settings.RefactorRewriteFillStruct, fn: refactorRewriteFillStruct, needPkg: true},
 	{kind: settings.RefactorRewriteFillSwitch, fn: refactorRewriteFillSwitch, needPkg: true},
@@ -1136,7 +1136,7 @@
 
 func refactorMoveType(ctx context.Context, req *codeActionsRequest) error {
 	curSel, _ := req.pgf.Cursor.FindByPos(req.start, req.end)
-	if _, _, _, typeName, ok := selectionContainsType(curSel); ok {
+	if _, _, _, typeName, ok := selectionContainsTypeDecl(curSel); ok {
 		cmd := command.NewMoveTypeCommand(fmt.Sprintf("Move type %s", typeName), command.MoveTypeArgs{Location: req.loc})
 		req.addCommandAction(cmd, false)
 	}
diff --git a/gopls/internal/golang/extracttofile.go b/gopls/internal/golang/extracttofile.go
index 6aef663..a07c8f5 100644
--- a/gopls/internal/golang/extracttofile.go
+++ b/gopls/internal/golang/extracttofile.go
@@ -135,54 +135,11 @@
 		return nil, err
 	}
 
-	var importDeletes []protocol.TextEdit
-	// For unparenthesised declarations like `import "fmt"` we remove
-	// the whole declaration because simply removing importSpec leaves
-	// `import \n`, which does not compile.
-	// For parenthesised declarations like `import ("fmt"\n "log")`
-	// we only remove the ImportSpec, because removing the whole declaration
-	// might remove other ImportsSpecs we don't want to touch.
-	unparenthesizedImports := unparenthesizedImports(pgf)
-	for _, importSpec := range deletes {
-		if decl := unparenthesizedImports[importSpec]; decl != nil {
-			importDeletes = append(importDeletes, removeNode(pgf, decl))
-		} else {
-			importDeletes = append(importDeletes, removeNode(pgf, importSpec))
-		}
-	}
+	importDeletes := importDeletes(pgf, deletes)
 
-	var buf bytes.Buffer
-	if c := CopyrightComment(pgf.File); c != nil {
-		text, err := pgf.NodeText(c)
-		if err != nil {
-			return nil, err
-		}
-		buf.Write(text)
-		// One empty line between copyright header and following.
-		buf.WriteString("\n\n")
-	}
-
-	if c := buildConstraintComment(pgf.File); c != nil {
-		text, err := pgf.NodeText(c)
-		if err != nil {
-			return nil, err
-		}
-		buf.Write(text)
-		// One empty line between build constraint and following.
-		buf.WriteString("\n\n")
-	}
-
-	fmt.Fprintf(&buf, "package %s\n", pgf.File.Name.Name)
-	if len(adds) > 0 {
-		buf.WriteString("import (")
-		for _, importSpec := range adds {
-			if importSpec.Name != nil {
-				fmt.Fprintf(&buf, "%s %s\n", importSpec.Name.Name, importSpec.Path.Value)
-			} else {
-				fmt.Fprintf(&buf, "%s\n", importSpec.Path.Value)
-			}
-		}
-		buf.WriteString(")\n")
+	buf, err := newFileContentFromImports(pgf, adds, pgf.File.Name.Name)
+	if err != nil {
+		return nil, err
 	}
 
 	newFile, err := chooseNewFile(ctx, snapshot, pgf.URI.DirPath(), firstSymbol)
@@ -208,6 +165,67 @@
 		})}, nil
 }
 
+// importDeletes returns the text edits necessary to delete the given imports
+// from the given file.
+func importDeletes(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {
+	var importDeletes []protocol.TextEdit
+	// For unparenthesised declarations like `import "fmt"` we remove
+	// the whole declaration because simply removing importSpec leaves
+	// `import \n`, which does not compile.
+	// For parenthesised declarations like `import ("fmt"\n "log")`
+	// we only remove the ImportSpec, because removing the whole declaration
+	// might remove other ImportsSpecs we don't want to touch.
+	unparenthesizedImports := unparenthesizedImports(pgf)
+	for _, importSpec := range deletes {
+		if decl := unparenthesizedImports[importSpec]; decl != nil {
+			importDeletes = append(importDeletes, removeNode(pgf, decl))
+		} else {
+			importDeletes = append(importDeletes, removeNode(pgf, importSpec))
+		}
+	}
+	return importDeletes
+}
+
+// newFileContentFromImports returns the unformatted file content resulting from adding the
+// imports in the given pgf to a new file, plus copyright and build constraint
+// comments.
+func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {
+	var buf bytes.Buffer
+	if c := CopyrightComment(pgf.File); c != nil {
+		text, err := pgf.NodeText(c)
+		if err != nil {
+			return bytes.Buffer{}, err
+		}
+		buf.Write(text)
+		// One empty line between copyright header and following.
+		buf.WriteString("\n\n")
+	}
+
+	if c := buildConstraintComment(pgf.File); c != nil {
+		text, err := pgf.NodeText(c)
+		if err != nil {
+			return bytes.Buffer{}, err
+		}
+		buf.Write(text)
+		// One empty line between build constraint and following.
+		buf.WriteString("\n\n")
+	}
+
+	fmt.Fprintf(&buf, "package %s\n", pkgName)
+	if len(adds) > 0 {
+		buf.WriteString("import (")
+		for _, importSpec := range adds {
+			if importSpec.Name != nil {
+				fmt.Fprintf(&buf, "%s %s\n", importSpec.Name.Name, importSpec.Path.Value)
+			} else {
+				fmt.Fprintf(&buf, "%s\n", importSpec.Path.Value)
+			}
+		}
+		buf.WriteString(")\n")
+	}
+	return buf, nil
+}
+
 // chooseNewFile chooses a new filename in dir, based on the name of the
 // first extracted symbol, and if necessary to disambiguate, a numeric suffix.
 func chooseNewFile(ctx context.Context, snapshot *cache.Snapshot, dir string, firstSymbol string) (file.Handle, error) {
diff --git a/gopls/internal/golang/movetype.go b/gopls/internal/golang/movetype.go
index 3ea8ece..379a313 100644
--- a/gopls/internal/golang/movetype.go
+++ b/gopls/internal/golang/movetype.go
@@ -8,10 +8,14 @@
 	"context"
 	"fmt"
 	"go/ast"
+	"go/format"
 	"go/token"
+	"go/types"
+	"path/filepath"
 
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/gopls/internal/cache"
+	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/internal/moreiters"
@@ -19,12 +23,93 @@
 
 // MoveType moves the selected type declaration into a new package and updates all references.
 func MoveType(ctx context.Context, fh file.Handle, snapshot *cache.Snapshot, loc protocol.Location, newPkgDir string) ([]protocol.DocumentChange, error) {
-	return nil, fmt.Errorf("MoveType: not yet supported")
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, fh.URI())
+	if err != nil {
+		return nil, err
+	}
+
+	start, end, err := pgf.RangePos(loc.Range)
+	if err != nil {
+		return nil, err
+	}
+
+	curSel, ok := pgf.Cursor.FindByPos(start, end)
+	if !ok {
+		return nil, err
+	}
+	_, decl, typS, _, _ := selectionContainsTypeDecl(curSel) // we also check this condition before providing the move type codeaction
+
+	if err := canMoveType(pkg.TypesInfo(), typS); err != nil {
+		return nil, err
+	}
+
+	changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), decl, newPkgDir, filepath.Base(newPkgDir))
+	if err != nil {
+		return nil, err
+	}
+
+	// Get the range to delete the type decl from the current file including any
+	// comments associated with it.
+	declStart, declEnd := decl.Pos(), decl.End()
+	if decl.Doc != nil {
+		declStart = decl.Doc.Pos()
+	}
+	typeDeclRange, err := pgf.PosRange(declStart, declEnd)
+	if err != nil {
+		return nil, fmt.Errorf("invalid range: %v", err)
+	}
+
+	// Delete the old type declaration.
+	changes = append(changes, protocol.DocumentChangeEdit(fh, []protocol.TextEdit{
+		{Range: typeDeclRange, NewText: ""},
+	}))
+
+	return changes, nil
 }
 
-// selectionContainsType returns the Cursor, GenDecl and TypeSpec of the type
+// addTypeToPackage returns the document changes necessary to add the type to a new file in the given package.
+// It only produces edits for the new file.
+func addTypeToPackage(ctx context.Context, snapshot *cache.Snapshot, pgf *parsego.File, pkg *cache.Package, info *types.Info, decl *ast.GenDecl, pkgDir, pkgName string) ([]protocol.DocumentChange, error) {
+	adds, _, err := findImportEdits(pgf.File, info, decl.Pos(), decl.End())
+	if err != nil {
+		return nil, fmt.Errorf("error calculating import edits: %v", err)
+	}
+
+	buf, err := newFileContentFromImports(pgf, adds, pkgName)
+	if err != nil {
+		return nil, fmt.Errorf("error getting new file content: %v", err)
+	}
+
+	// Add the type decl to the new file content.
+	err = format.Node(&buf, pkg.FileSet(), decl)
+	if err != nil {
+		return nil, fmt.Errorf("error adding type decl to new file: %v", err)
+	}
+
+	// Format the file content.
+	newFileContent, err := format.Source(buf.Bytes())
+	if err != nil {
+		return nil, fmt.Errorf("error formatting new file content: %v", err)
+	}
+
+	newFile, err := chooseNewFile(ctx, snapshot, pkgDir, pkgName)
+	if err != nil {
+		return nil, err
+	}
+
+	return []protocol.DocumentChange{
+		// Create the new file.
+		protocol.DocumentChangeCreate(newFile.URI()),
+		// Edit the created file.
+		protocol.DocumentChangeEdit(newFile, []protocol.TextEdit{
+			{Range: protocol.Range{}, NewText: string(newFileContent)},
+		}),
+	}, nil
+}
+
+// selectionContainsTypeDecl returns the Cursor, GenDecl and TypeSpec of the type
 // declaration that encloses cursor if one exists. Otherwise it returns false.
-func selectionContainsType(cursor inspector.Cursor) (inspector.Cursor, *ast.GenDecl, *ast.TypeSpec, string, bool) {
+func selectionContainsTypeDecl(cursor inspector.Cursor) (inspector.Cursor, *ast.GenDecl, *ast.TypeSpec, string, bool) {
 	declCur, ok := moreiters.First(cursor.Enclosing((*ast.GenDecl)(nil)))
 	if !ok {
 		return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false
@@ -44,3 +129,36 @@
 
 	return declCur, declNode, declNode.Specs[0].(*ast.TypeSpec), typSpec.Name.Name, true
 }
+
+// canMoveType reports if it is valid to move the input type spec to a new package, returning
+// an error if invalid.
+func canMoveType(info *types.Info, typ *ast.TypeSpec) error {
+	if !typ.Name.IsExported() {
+		// TODO(mkalil): have an option to turn unexported type into exported type
+		return fmt.Errorf("cannot move type: type is not exported")
+	}
+	typObj := info.Defs[typ.Name]
+	if typExp, ok := typ.Type.(*ast.StructType); ok {
+		for _, field := range typExp.Fields.List {
+
+			for _, name := range field.Names {
+				// TODO(mkalil): check if this field is accessed. What if they are moving the
+				// type before using it? We can have an option to turn unexported type fields into
+				// exported fields.
+				if !name.IsExported() {
+					return fmt.Errorf("cannot move type: would result in reference to unexported fields")
+				}
+			}
+
+			// TODO(mkalil): How should we handle moving types that have fields which are
+			// types defined at the package level? Should we also move those types to the
+			// new package, recursively? (If we don't, it could cause an import cycle).
+			// Or throw an error?
+			fieldType := info.TypeOf(field.Type)
+			if fieldObj, ok := fieldType.(*types.Named); ok && fieldObj.Obj().Pkg() == typObj.Pkg() {
+				return fmt.Errorf("cannot move type: would result in import cycle")
+			}
+		}
+	}
+	return nil
+}
diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go
index 7e62ab5..576a035 100644
--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -1897,7 +1897,7 @@
 	err := c.run(ctx, commandConfig{
 		forURI: args.Location.URI,
 	}, func(ctx context.Context, deps commandDeps) error {
-		changes, err := golang.MoveType(ctx, deps.fh, deps.snapshot, args.Location, "newpkg/new.go")
+		changes, err := golang.MoveType(ctx, deps.fh, deps.snapshot, args.Location, "newpkg/new")
 		if err != nil {
 			return err
 		}
diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go
index f197ad2..612d95d 100644
--- a/gopls/internal/settings/default.go
+++ b/gopls/internal/settings/default.go
@@ -71,7 +71,7 @@
 						RefactorExtractVariable:          true,
 						RefactorExtractVariableAll:       true,
 						RefactorExtractToNewFile:         true,
-						RefactorMoveType:                 true, // off while implementation unfinished
+						RefactorMoveType:                 true,
 						// Not GoTest: it must be explicit in CodeActionParams.Context.Only
 					},
 					file.Mod: {
diff --git a/gopls/internal/test/marker/testdata/codeaction/movetype.txt b/gopls/internal/test/marker/testdata/codeaction/movetype.txt
new file mode 100644
index 0000000..54be1fa
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/codeaction/movetype.txt
@@ -0,0 +1,35 @@
+This test checks the behavior of the 'move type' code action.
+
+-- flags --
+-ignore_extra_diags
+
+-- go.mod --
+module example.com
+
+-- a/a.go --
+package a
+
+import (
+    "bytes"
+    "context"
+)
+
+type a struct { //@codeaction("a", "refactor.move.moveType", err=re"type is not exported")
+    Buf bytes.Buffer
+    Ctx context.Context
+}
+
+type A struct { //@codeaction("A", "refactor.move.moveType", err=re"unexported fields")
+    buf bytes.Buffer
+    ctx context.Context
+}
+
+type B struct { //@codeaction("B", "refactor.move.moveType", err=re"import cycle")
+    C
+}
+
+type BB struct { //@codeaction("B", "refactor.move.moveType", err=re"import cycle")
+    Field C
+}
+
+type C string

Change information

Files:

• M gopls/internal/golang/codeaction.go
• M gopls/internal/golang/extracttofile.go
• M gopls/internal/golang/movetype.go
• M gopls/internal/server/command.go
• M gopls/internal/settings/default.go
• A gopls/internal/test/marker/testdata/codeaction/movetype.txt

Change size: L

Delta: 6 files changed, 225 insertions(+), 54 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 1

Gerrit-Owner: Madeline Kalil <mka...@google.com>

[Madeline Kalil (Gerrit)]

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #3 to this change.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 3

Gerrit-Owner: Madeline Kalil <mka...@google.com>

[Madeline Kalil (Gerrit)]

Madeline Kalil voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 3

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Tue, 25 Nov 2025 22:04:11 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Madeline Kalil (Gerrit)]

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #4 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 4

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

[Madeline Kalil (Gerrit)]

Madeline Kalil added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Madeline Kalil . unresolved

I was hoping there would be a way to enable the code action in the marker tests but disable producing the code action in real requests until it's fully implemented but could not figure this out.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 4

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Tue, 02 Dec 2025 17:44:44 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Madeline Kalil (Gerrit)]

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #5 to this change.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 5

[Madeline Kalil (Gerrit)]

Madeline Kalil voted and added 1 comment

Votes added by Madeline Kalil

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4:

Madeline Kalil . resolved

I was hoping there would be a way to enable the code action in the marker tests but disable producing the code action in real requests until it's fully implemented but could not figure this out.

Madeline Kalil

Nevermind - added a hacky "testing.Testing()" to disable the code action when not inside a test.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 5

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Tue, 02 Dec 2025 20:38:25 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Madeline Kalil <mka...@google.com>

[Robert Findley (Gerrit)]

Attention needed from Madeline Kalil

Robert Findley added 16 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Robert Findley . resolved

Solid start. Many comments but mostly superficial, and the big stuff can be in a follow-up CL.

File gopls/internal/golang/extracttofile.go

Line 170, Patchset 5 (Latest):func importDeletes(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {

Robert Findley . unresolved

How about 'importDeleteEdits'

Line 190, Patchset 5 (Latest):// imports in the given pgf to a new file, plus copyright and build constraint

Robert Findley . unresolved

Is this all the imports in the given file? Looks like it's only a fixed slice. Perhaps this comment is stale?

Line 192, Patchset 5 (Latest):func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {

Robert Findley . unresolved

Document this.

Also, how about rearranging the arguments a bit:

```
// newFileWithImports creates a new file with the given package name and imports
// as well as copyright comment and build constraints matching fromPGF.
//
// It returns a buffer containing the file content, or an error.
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File) (bytes.Buffer, error)
```

Line 192, Patchset 5 (Latest):func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {

Robert Findley . unresolved

How about 'newFileWithImports'?

Line 200, Patchset 5 (Latest): // One empty line between copyright header and following.

Robert Findley . unresolved

"after the copyright header."

Line 224, Patchset 5 (Latest): buf.WriteString(")\n")

Robert Findley . unresolved

Nit: should we add a trailing newline? What should the state of the buffer be, exactly (let's also document this in the doc comment).

File gopls/internal/golang/movetype.go

Line 40, Patchset 5 (Latest): _, decl, typS, _, _ := selectionContainsTypeDecl(curSel) // we also check this condition before providing the move type codeaction

Robert Findley . unresolved

Shouldn't we still check 'ok' here? If !ok, none of the other results are meaningful.

Line 51, Patchset 5 (Latest): // Get the range to delete the type from the current file. If the type spec

Robert Findley . unresolved

The asymmetry of this bothers me: if we're able to handle moving an individual spec, not just an entire decl, then why don't we check selectionContainsTypeSpec, instead of looking at whole decls.

Or maybe I'm missing something.

Line 64, Patchset 5 (Latest): // TODO(mkalil): comments inside struct types are lost, i.e:
// type A struct { // this comment gets lost.
// B int
// }

Robert Findley . unresolved

Looks like a case for CommentMap and CommentedNode!

Line 92, Patchset 5 (Latest):func addTypeToPackage(ctx context.Context, snapshot *cache.Snapshot, pgf *parsego.File, pkg *cache.Package, info *types.Info, spec *ast.TypeSpec, pkgDir, pkgName string) ([]protocol.DocumentChange, error) {

Robert Findley . unresolved

If you pass in a CommentMap, you can create a CommentedNode for formatting, and preserve inline comments.

Line 148, Patchset 5 (Latest): typSpec, ok := declNode.Specs[0].(*ast.TypeSpec)


if !ok {
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false
}

Robert Findley . unresolved

This looks wrong--shouldn't we find the best matching spec?

(I know we discussed what to do here, but this has the following unintuitive behavior, IIUC)

```
type (
  A int
  B string // <- cursor here
)
```

With the cursor on B, the move will actually move A!

Line 158, Patchset 5 (Latest):func canMoveType(info *types.Info, typ *ast.TypeSpec) error {

Robert Findley . unresolved

What about methods on the type? We need to move them as well.

...and then we should allow moving the type if the unexported fields are only accessed from its own methods.

Perhaps add a big TODO explaining the plan.

Line 168, Patchset 5 (Latest): // TODO(mkalil): check if this field is accessed. What if they are moving the


// type before using it? We can have an option to turn unexported type fields into

Robert Findley . resolved

+1, nice call out

Add a test, and then check the provided info: should be straightforward.

(For another CL).

Line 182, Patchset 5 (Latest): return fmt.Errorf("cannot move type: would result in import cycle")

Robert Findley . unresolved

s/would/could 😊

We don't know without actually doing the analysis!

File gopls/internal/server/code_action.go

Line 87, Patchset 5 (Latest): // RefactorMoveType is enabled for testing but disabled otherwise.
// This is temporary while the implementation of move type refactoring is unfinished.
if !testing.Testing() && kind == settings.RefactorMoveType {
return false
}

Robert Findley . unresolved

Prefer doing this with an internal gopls setting. WDYT?

Open in Gerrit

Related details

Attention is currently required from:

• Madeline Kalil

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 5

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-Attention: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Mon, 08 Dec 2025 15:31:34 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Madeline Kalil (Gerrit)]

Attention needed from Alan Donovan

Madeline Kalil uploaded new patchset

Madeline Kalil uploaded patch set #6 to this change.

Following approvals got outdated and were removed:

• TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI

Open in Gerrit

Related details

Attention is currently required from:

• Alan Donovan

Submit Requirements:

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newpatchset

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 6

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-CC: Alan Donovan <adon...@google.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

[Madeline Kalil (Gerrit)]

Attention needed from Alan Donovan and Robert Findley

Madeline Kalil voted and added 14 comments

Votes added by Madeline Kalil

Commit-Queue

+1

14 comments

File gopls/internal/golang/extracttofile.go

Line 170, Patchset 5:func importDeletes(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {

Robert Findley . resolved

How about 'importDeleteEdits'

Madeline Kalil

Done

Line 190, Patchset 5:// imports in the given pgf to a new file, plus copyright and build constraint

Robert Findley . resolved

Is this all the imports in the given file? Looks like it's only a fixed slice. Perhaps this comment is stale?

Madeline Kalil

Updated comment

Line 192, Patchset 5:func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {

Robert Findley . resolved

Document this.

Also, how about rearranging the arguments a bit:

```
// newFileWithImports creates a new file with the given package name and imports
// as well as copyright comment and build constraints matching fromPGF.
//
// It returns a buffer containing the file content, or an error.
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File) (bytes.Buffer, error)
```

Madeline Kalil

Done

Line 192, Patchset 5:func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {

Robert Findley . resolved

How about 'newFileWithImports'?

Madeline Kalil

Done

Line 200, Patchset 5: // One empty line between copyright header and following.

Robert Findley . resolved

"after the copyright header."

Madeline Kalil

Done

Line 224, Patchset 5: buf.WriteString(")\n")

Robert Findley . resolved

Nit: should we add a trailing newline? What should the state of the buffer be, exactly (let's also document this in the doc comment).

Madeline Kalil

This was preexisting, so I'm not sure of the original reasoning, but when I comment this line out and run all the tests, a few fail. I'll update the doc comment to specify that we add a new line after the import block.

File gopls/internal/golang/movetype.go

Line 40, Patchset 5: _, decl, typS, _, _ := selectionContainsTypeDecl(curSel) // we also check this condition before providing the move type codeaction

Robert Findley . resolved

Shouldn't we still check 'ok' here? If !ok, none of the other results are meaningful.

Madeline Kalil

We should be able to assume ok is true here, because we check selectionContainsTypeDecl before providing the codeaction that calls this command.

Line 51, Patchset 5: // Get the range to delete the type from the current file. If the type spec

Robert Findley . resolved

The asymmetry of this bothers me: if we're able to handle moving an individual spec, not just an entire decl, then why don't we check selectionContainsTypeSpec, instead of looking at whole decls.

Or maybe I'm missing something.

Madeline Kalil

Yeah, realizing this logic is messed up. Fixing and renaming. (But we still need the GenDecl node since if the type spec in question is the only one in the type decl, we should remove the entire decl)

Line 64, Patchset 5: // TODO(mkalil): comments inside struct types are lost, i.e:


// type A struct { // this comment gets lost.
// B int
// }

Robert Findley . resolved

Looks like a case for CommentMap and CommentedNode!

Madeline Kalil

Done

Line 92, Patchset 5:func addTypeToPackage(ctx context.Context, snapshot *cache.Snapshot, pgf *parsego.File, pkg *cache.Package, info *types.Info, spec *ast.TypeSpec, pkgDir, pkgName string) ([]protocol.DocumentChange, error) {

Robert Findley . resolved

If you pass in a CommentMap, you can create a CommentedNode for formatting, and preserve inline comments.

Madeline Kalil

Fixed this particular comment preservation issue, but now comments above type specs are formatted strangely. I left a TODO

Line 148, Patchset 5: typSpec, ok := declNode.Specs[0].(*ast.TypeSpec)


if !ok {
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false
}

Robert Findley . resolved

This looks wrong--shouldn't we find the best matching spec?

(I know we discussed what to do here, but this has the following unintuitive behavior, IIUC)

```
type (
  A int
  B string // <- cursor here
)
```

With the cursor on B, the move will actually move A!

Madeline Kalil

Fixed and added a test case

Line 158, Patchset 5:func canMoveType(info *types.Info, typ *ast.TypeSpec) error {

Robert Findley . resolved

What about methods on the type? We need to move them as well.

...and then we should allow moving the type if the unexported fields are only accessed from its own methods.

Perhaps add a big TODO explaining the plan.

Madeline Kalil

Done

Line 182, Patchset 5: return fmt.Errorf("cannot move type: would result in import cycle")

Robert Findley . resolved

s/would/could 😊

We don't know without actually doing the analysis!

Madeline Kalil

True, thanks!

File gopls/internal/server/code_action.go

Line 87, Patchset 5: // RefactorMoveType is enabled for testing but disabled otherwise.


// This is temporary while the implementation of move type refactoring is unfinished.
if !testing.Testing() && kind == settings.RefactorMoveType {
return false
}

Robert Findley . resolved

Prefer doing this with an internal gopls setting. WDYT?

Madeline Kalil

Sure, testing.Testing does feel weird

Open in Gerrit

Related details

Attention is currently required from:

• Alan Donovan
• Robert Findley

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement
• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 6

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Robert Findley <rfin...@google.com>

Gerrit-CC: Alan Donovan <adon...@google.com>

Gerrit-Attention: Alan Donovan <adon...@google.com>

Gerrit-Attention: Robert Findley <rfin...@google.com>

Gerrit-Comment-Date: Mon, 08 Dec 2025 19:38:48 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Robert Findley <rfin...@google.com>

[Alan Donovan (Gerrit)]

Attention needed from Madeline Kalil

Alan Donovan added 28 comments

Commit Message

Line 7, Patchset 6 (Latest):gopls/internal/golang: MoveType - extract type to new file

Alan Donovan . unresolved

Does this always move the type to a new package? I was confused because the extract logic appears to move it to a new file in the same package (based on the package declaration), but the command handler seems to create the file name of a new directory.

File gopls/internal/golang/extracttofile.go

Line 137, Patchset 6 (Latest):
importDeletes := importDeletesEdits(pgf, deletes)


Alan Donovan . unresolved

The combination of importDeletesEdits and format.Source is now provided by driverutil.FormatSourceRemoveImports. This should make things easier in findImportEdits and eliminate this step.

(I should probably move it out of driverutil, but it's a legitimate dependency of the golang package.)

Line 145, Patchset 6 (Latest): newFile, err := chooseNewFile(ctx, snapshot, pgf.URI.DirPath(), firstSymbol)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}


Alan Donovan . unresolved

Move this down so the buf-related stuff stays together.

Line 170, Patchset 6 (Latest):func importDeletesEdits(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {

Alan Donovan . unresolved

See above; I think this is no longer needed.

Line 194, Patchset 6 (Latest):func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File) (bytes.Buffer, error) {

Alan Donovan . unresolved

*bytes.Buffer

(Values of this type, and indeed most opaque types T with *T methods, should not be copied.)

But why not have the caller provide the buffer? That's usually more efficient since there is often stuff to be written to it before and after.

Line 216, Patchset 6 (Latest): fmt.Fprintf(&buf, "package %s\n", pkgName)

Alan Donovan . unresolved

fromPGF.File.Name.Name

(then the pkgName parameter is unneeded)

Line 216, Patchset 6 (Latest): fmt.Fprintf(&buf, "package %s\n", pkgName)

Alan Donovan . unresolved

\n\n

Line 219, Patchset 6 (Latest): for _, importSpec := range imports {

Alan Donovan . unresolved

spec (for brevity)

Line 220, Patchset 6 (Latest): if importSpec.Name != nil {


fmt.Fprintf(&buf, "%s %s\n", importSpec.Name.Name, importSpec.Path.Value)


} else {


fmt.Fprintf(&buf, "%s\n", importSpec.Path.Value)
}

Alan Donovan . unresolved

Factor the Path parts.

Line 226, Patchset 6 (Latest): buf.WriteString(")\n")

Alan Donovan . unresolved

\n\n

File gopls/internal/golang/movetype.go

Line 37, Patchset 6 (Latest): curSel, ok := pgf.Cursor.FindByPos(start, end)

Alan Donovan . unresolved

astutil.Select may be more appropriate here; it handles whitespace more permissively.

Line 41, Patchset 6 (Latest): _, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeaction

Alan Donovan . unresolved

Is decl just the parent of spec? If so, return a cursor and let the caller call Parent.

Line 41, Patchset 6 (Latest): _, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeaction

Alan Donovan . unresolved

spec

Line 47, Patchset 6 (Latest): changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))

Alan Donovan . unresolved

This is redundant w.r.t pkg.

Line 47, Patchset 6 (Latest): changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))

Alan Donovan . unresolved

Ditto w.r.t. newDir.

Line 73, Patchset 6 (Latest): }

Alan Donovan . unresolved

This is a recurring pattern. The common parts could be factored as:
```
var n Node = spec
if len(decl.Specs) == 1 {
	n = decl // singleton: delete entire decl
}	
start, end = n.Pos(), n.End()
if doc := astutil.DocComment(n); doc != nil {
    start = doc.Pos() // include doc comment
}
if c := astutil.PostComment(n); c != nil {
    end = c.End() // include inline comment
}
```
However, only DocComment exists; you'll need to define PostComment. We could use it in dozens of places.

Line 74, Patchset 6 (Latest): typeDeclRange, err := pgf.PosRange(typStart, typEnd)

Alan Donovan . unresolved

rng

Line 76, Patchset 6 (Latest): return nil, fmt.Errorf("invalid range: %v", err)

Alan Donovan . unresolved

err is fine.

Line 100, Patchset 6 (Latest): // Use CommentedNode to avoid losing comments inside struct types
// that are neither doc comments nor line comments.


// type A struct { // this comment gets lost.
// B int
// }


// TODO(mkalil): Now these comments are preserved but
// comments above a type spec are formatted in an undesirable way.
// (See @multitypespecB/d/newpkg/b.go in movetype.txt)
var declComments []*ast.CommentGroup
if len(decl.Specs) == 1 {
for _, comment := range pgf.File.Comments {
if decl.Pos() <= comment.Pos() && comment.End() <= decl.End() {
declComments = append(declComments, comment)


}
}
}

// Add the type decl to the new file content.


newDecl := &ast.GenDecl{
Tok: token.TYPE,
Specs: []ast.Spec{spec},
}
commentedNode := &printer.CommentedNode{
Node: newDecl,
Comments: declComments,
}
err = format.Node(&buf, pkg.FileSet(), commentedNode)
if err != nil {


return nil, fmt.Errorf("error adding type decl to new file: %v", err)
}


Alan Donovan . unresolved

It might be simpler here to copy `pgf.NodeText(spec or decl)` directly into buf. Then you don't to think about comments or format.Source.

Line 155, Patchset 6 (Latest): specCur, ok := moreiters.First(cursor.Enclosing((*ast.TypeSpec)(nil)))

Alan Donovan . unresolved

```
spec, curSpec := cursorutil.FirstEnclosing[*ast.TypeSpec](cur)
if spec == nil { ... }
```

then delete L170,

Line 157, Patchset 6 (Latest): return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false

Alan Donovan . unresolved

nil, nil

(throughout)

Line 165, Patchset 6 (Latest): declNode := declCur.Node().(*ast.GenDecl)

Alan Donovan . unresolved

decl

Line 170, Patchset 6 (Latest): typSpec, _ := specCur.Node().(*ast.TypeSpec) // can't fail

Alan Donovan . unresolved

delete (since it can't fail)

Line 171, Patchset 6 (Latest): return declCur, declNode, typSpec, typSpec.Name.Name, true

Alan Donovan . unresolved

This seems redundant w.r.t. type typSpec result.

Line 176, Patchset 6 (Latest):// TODO(mkalil): For now, we only allow moving a type if the type itself and all its

Alan Donovan . unresolved

We also need to check that:

• all references from the moved declarations are either internal to it, or to imported packages, but not to other declarations in the old package.
• any internal packages that would be imported by the new package are visible to it (see packagepath.CanImport).

Line 187, Patchset 6 (Latest): if typExp, ok := typ.Type.(*ast.StructType); ok {


for _, field := range typExp.Fields.List {



for _, name := range field.Names {

Alan Donovan . unresolved

This kind of check, which relates to symbol references, is best done using the type checker's data structures.

Line 199, Patchset 6 (Latest): // TODO(mkalil): How should we handle moving types that have fields which are


// types defined at the package level? Should we also move those types to the


// new package, recursively? (If we don't, it could cause an import cycle).


// Or throw an error?

Alan Donovan . unresolved

We should probably attempt to move them recursively (applying the same validity checks). If the user didn't want that, they can always undo, but there's no good way to ask the user "move type T2 too?" (yet).

File gopls/internal/server/code_action.go

Line 86, Patchset 6 (Latest): // RefactorMoveType is enabled for testing but disabled otherwise.


// This is temporary while the implementation of move type refactoring is unfinished.


if kind == settings.RefactorMoveType && !snapshot.Options().MoveType {
return false
}

Alan Donovan . unresolved

I wonder why this option is needed. How many steps are there to finishing the feature? Is it too much for this CL, or a small stack that can be merged in the same day or two?

Open in Gerrit

Related details

Attention is currently required from:

• Madeline Kalil

Submit Requirements:

• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

• TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: tools

Gerrit-Branch: master

Gerrit-Change-Id: I063fcf6b15b03ae4c89297b73942e2c89b02c51b

Gerrit-Change-Number: 723982

Gerrit-PatchSet: 6

Gerrit-Owner: Madeline Kalil <mka...@google.com>

Gerrit-Reviewer: Alan Donovan <adon...@google.com>

Gerrit-Reviewer: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Madeline Kalil <mka...@google.com>

Gerrit-CC: Robert Findley <rfin...@golang.org>

Gerrit-Attention: Madeline Kalil <mka...@google.com>

Gerrit-Comment-Date: Tue, 16 Dec 2025 21:07:42 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No