[tools] gopls/internal/golang: MoveType - extract type to new file
Madeline Kalil (Gerrit)
Madeline Kalil has uploaded the change for review![]( "Open in Gerrit")
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 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
- 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
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 1 comment![]( "Open in Gerrit")
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.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
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.
Nevermind - added a hacky "testing.Testing()" to disable the code action when not inside a test.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 16 comments![]( "Open in Gerrit")
Solid start. Many comments but mostly superficial, and the big stuff can be in a follow-up CL.
func importDeletes(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {How about 'importDeleteEdits'
// imports in the given pgf to a new file, plus copyright and build constraintIs this all the imports in the given file? Looks like it's only a fixed slice. Perhaps this comment is stale?
func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {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)
```
func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {How about 'newFileWithImports'?
// One empty line between copyright header and following."after the copyright header."
buf.WriteString(")\n")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).
_, decl, typS, _, _ := selectionContainsTypeDecl(curSel) // we also check this condition before providing the move type codeactionShouldn't we still check 'ok' here? If !ok, none of the other results are meaningful.
// Get the range to delete the type from the current file. If the type specThe 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.
// TODO(mkalil): comments inside struct types are lost, i.e:
// type A struct { // this comment gets lost.
// B int
// }Looks like a case for CommentMap and CommentedNode!
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) {If you pass in a CommentMap, you can create a CommentedNode for formatting, and preserve inline comments.
typSpec, ok := declNode.Specs[0].(*ast.TypeSpec)
if !ok {
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false
}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!
func canMoveType(info *types.Info, typ *ast.TypeSpec) error {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.
// 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+1, nice call out
Add a test, and then check the provided info: should be straightforward.
(For another CL).
return fmt.Errorf("cannot move type: would result in import cycle")s/would/could 😊
We don't know without actually doing the analysis!
// 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
}Prefer doing this with an internal gopls setting. WDYT?
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 14 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
14 comments
func importDeletes(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {Madeline KalilHow about 'importDeleteEdits'
Done
// imports in the given pgf to a new file, plus copyright and build constraintIs this all the imports in the given file? Looks like it's only a fixed slice. Perhaps this comment is stale?
Updated comment
func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {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)
```
Done
func newFileContentFromImports(pgf *parsego.File, adds []*ast.ImportSpec, pkgName string) (bytes.Buffer, error) {Madeline KalilHow about 'newFileWithImports'?
Done
// One empty line between copyright header and following.Madeline Kalil"after the copyright header."
Done
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).
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.
_, decl, typS, _, _ := selectionContainsTypeDecl(curSel) // we also check this condition before providing the move type codeactionShouldn't we still check 'ok' here? If !ok, none of the other results are meaningful.
We should be able to assume ok is true here, because we check selectionContainsTypeDecl before providing the codeaction that calls this command.
// Get the range to delete the type from the current file. If the type specThe 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.
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)
// TODO(mkalil): comments inside struct types are lost, i.e:
// type A struct { // this comment gets lost.
// B int
// }Looks like a case for CommentMap and CommentedNode!
Done
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) {If you pass in a CommentMap, you can create a CommentedNode for formatting, and preserve inline comments.
Fixed this particular comment preservation issue, but now comments above type specs are formatted strangely. I left a TODO
typSpec, ok := declNode.Specs[0].(*ast.TypeSpec)
if !ok {
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", false
}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!
Fixed and added a test case
func canMoveType(info *types.Info, typ *ast.TypeSpec) error {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.
Done
return fmt.Errorf("cannot move type: would result in import cycle")s/would/could 😊
We don't know without actually doing the analysis!
True, thanks!
// 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
}Prefer doing this with an internal gopls setting. WDYT?
- Alan Donovan
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 28 comments![]( "Open in Gerrit")
gopls/internal/golang: MoveType - extract type to new fileDoes 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.
importDeletes := importDeletesEdits(pgf, deletes)
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.)
newFile, err := chooseNewFile(ctx, snapshot, pgf.URI.DirPath(), firstSymbol)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
Move this down so the buf-related stuff stays together.
func importDeletesEdits(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {See above; I think this is no longer needed.
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File) (bytes.Buffer, error) {*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.
fmt.Fprintf(&buf, "package %s\n", pkgName) fmt.Fprintf(&buf, "package %s\n", pkgName)\n\n
for _, importSpec := range imports {spec (for brevity)
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)
}Factor the Path parts.
curSel, ok := pgf.Cursor.FindByPos(start, end)astutil.Select may be more appropriate here; it handles whitespace more permissively.
_, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionIs decl just the parent of spec? If so, return a cursor and let the caller call Parent.
_, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionspec
changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))This is redundant w.r.t pkg.
changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))Ditto w.r.t. newDir.
}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.
typeDeclRange, err := pgf.PosRange(typStart, typEnd)rng
return nil, fmt.Errorf("invalid range: %v", err)err is fine.
// 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)
}
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.
specCur, ok := moreiters.First(cursor.Enclosing((*ast.TypeSpec)(nil)))```
spec, curSpec := cursorutil.FirstEnclosing[*ast.TypeSpec](cur)
if spec == nil { ... }
```
then delete L170,
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", falsenil, nil
(throughout)
declNode := declCur.Node().(*ast.GenDecl)decl
typSpec, _ := specCur.Node().(*ast.TypeSpec) // can't faildelete (since it can't fail)
return declCur, declNode, typSpec, typSpec.Name.Name, trueThis seems redundant w.r.t. type typSpec result.
// TODO(mkalil): For now, we only allow moving a type if the type itself and all itsWe 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).
if typExp, ok := typ.Type.(*ast.StructType); ok {
for _, field := range typExp.Fields.List {
for _, name := range field.Names {This kind of check, which relates to symbol references, is best done using the type checker's data structures.
// 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?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).
// 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
}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?
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 27 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
27 comments
gopls/internal/golang: MoveType - extract type to new fileDoes 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.
The logic is inconsistent right now because I'm not sure of the way we'll take input from users once we have dialogue support (file chooser vs input package name, etc.). For now, the input is a URI and then it will add the type to a new file in the same package as the URI. The feature only makes sense moving to a new package but we should still allow people to move types within a package so there's no check for if the URI specified is part of a different package than where the type is currently declared.
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.)
From discussion: switching to using driverutil.FormatSourceRemoveImports would be a behavior change because previously, extracttofile would not format the file source, it would just delete the imports and the extracted block.
We should consider why we are computing unused imports to delete for a lot of gopls tools -- we get this formatting behavior on save so it might not be necessary to always compute the changes.
newFile, err := chooseNewFile(ctx, snapshot, pgf.URI.DirPath(), firstSymbol)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
Move this down so the buf-related stuff stays together.
Done
func importDeletesEdits(pgf *parsego.File, deletes []*ast.ImportSpec) []protocol.TextEdit {See above; I think this is no longer needed.
Acknowledged
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File) (bytes.Buffer, error) {*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.
Switched to caller providing buffer
(then the pkgName parameter is unneeded)
The call to newFileWithImports uses a different package name so I can't do this
fmt.Fprintf(&buf, "package %s\n", pkgName)Madeline Kalil\n\n
Done
for _, importSpec := range imports {Madeline Kalilspec (for brevity)
Done
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)
}Madeline KalilFactor the Path parts.
Done
astutil.Select may be more appropriate here; it handles whitespace more permissively.
I think Select won't work for my use case until cl/730320 is in; added a TODO
_, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionMadeline Kalilspec
Done
_, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionIs decl just the parent of spec? If so, return a cursor and let the caller call Parent.
I feel like there was some reason why I didn't assume that GenDecl is always the direct parent of the TypeSpec but now I can't remember why. Is this always true?
changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))Madeline KalilDitto w.r.t. newDir.
Sorry, I should've caught this
changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, pkg.TypesInfo(), typS, decl, newDir, filepath.Base(newDir))This is redundant w.r.t pkg.
Done
typeDeclRange, err := pgf.PosRange(typStart, typEnd)Madeline Kalilrng
Done
return nil, fmt.Errorf("invalid range: %v", err)Madeline Kalilerr is fine.
Done
// 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)
}
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.
I'm finding that NodeText doesn't include comment:
type A string // this comment gets lost
specCur, ok := moreiters.First(cursor.Enclosing((*ast.TypeSpec)(nil)))```
spec, curSpec := cursorutil.FirstEnclosing[*ast.TypeSpec](cur)
if spec == nil { ... }
```then delete L170,
Done
return inspector.Cursor{}, &ast.GenDecl{}, &ast.TypeSpec{}, "", falseMadeline Kalilnil, nil
(throughout)
Done
declNode := declCur.Node().(*ast.GenDecl)Madeline Kalildecl
Done
typSpec, _ := specCur.Node().(*ast.TypeSpec) // can't faildelete (since it can't fail)
Done
return declCur, declNode, typSpec, typSpec.Name.Name, trueThis seems redundant w.r.t. type typSpec result.
Done
// TODO(mkalil): For now, we only allow moving a type if the type itself and all itsWe 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).
Thanks, added a TODO
if typExp, ok := typ.Type.(*ast.StructType); ok {
for _, field := range typExp.Fields.List {
for _, name := range field.Names {This kind of check, which relates to symbol references, is best done using the type checker's data structures.
Done
// 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?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).
Makes sense to me, updated the TODO
// 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
}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?
Even with all the CLs in the current chain it won't be finished (and probably won't get finished over the holiday period) so I'd rather have the setting so I can merge in the partial implementation if that's okay. Will remove it when it's done.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
}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.
Madeline Kalil
Left a TODO to find the appropriate places to replace with PostComment
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 3 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
3 comments
gopls/internal/golang: MoveType - extract type to new fileMadeline KalilDoes 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.
The logic is inconsistent right now because I'm not sure of the way we'll take input from users once we have dialogue support (file chooser vs input package name, etc.). For now, the input is a URI and then it will add the type to a new file in the same package as the URI. The feature only makes sense moving to a new package but we should still allow people to move types within a package so there's no check for if the URI specified is part of a different package than where the type is currently declared.
I'd like to break the implementation of type move refactoring into 3 or so CLs if that's okay. This one produces the initial set of necessary edits to 1) move the type to a new file 2) remove the type declaration from its current location.
The exact behavior of determining which file in which package the type will be moved to will definitely change.
_, decl, typS, _, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionIs decl just the parent of spec? If so, return a cursor and let the caller call Parent.
I feel like there was some reason why I didn't assume that GenDecl is always the direct parent of the TypeSpec but now I can't remember why. Is this always true?
Nevermind, fixed. selectionContainsTypeSpec now just returns the cursor corresponding to the type spec.
// 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)
}
Madeline KalilIt 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.
I'm finding that NodeText doesn't include comment:
type A string // this comment gets lost
Leaving the handling of comments as-is for now.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 9 comments![]( "Open in Gerrit")
Thanks, this is converging. Sorry my review took so long.
importDeletes := importDeletesEdits(pgf, deletes)Inline this call? The golang package is very large so I'd like to avoid adding too many additional helper functions with tempting names to the package namespace when they are really bespoke parts of algorithms such as ExtractToNewFile.
protocol.DocumentChangeEdit(fh, append(importDeletes, protocol.TextEdit{Range: replaceRange, NewText: ""})),delete
specCur, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionThis is true, but it is good practice for a server not to assume that its clients won't fling any old junk at it, so we should check the validity of the result again.
if err := canMoveType(pkg.TypesInfo(), spec); err != nil {specCur
(see below)
{Range: rng, NewText: ""},delete
return fmt.Errorf("cannot move type: type is not exported") if structType, ok := typ.(*types.Struct); ok {This really wants to be recursive check over the entire TypeSpec. You should be able to use something like this:
```
for n := range curTypeSpec.Preorder() {
switch n := n.(type) {
case *ast.Ident:
// if it's a Use of an Object (e.g. type name or constant),
// check the object is exported (or also moving)
case *ast.StructType:
// check each field name is exported, like you do here,
// (or all its uses are also moving).
}
}
```
The "also moving" checks are fine to leave as a TODO.
// 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
}I don't think this option is necessary; let's aim to get it to readiness in this CL. If we need to fix some bugs before the 22 release, that's fine.
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 8 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
8 comments
Inline this call? The golang package is very large so I'd like to avoid adding too many additional helper functions with tempting names to the package namespace when they are really bespoke parts of algorithms such as ExtractToNewFile.
I think I originally extracted this to a separate function so that move type could also use it, but that is no longer happening. Done.
protocol.DocumentChangeEdit(fh, append(importDeletes, protocol.TextEdit{Range: replaceRange, NewText: ""})),Madeline Kalildelete
Done
specCur, _ := selectionContainsTypeSpec(curSel) // we also check this condition before providing the move type codeactionThis is true, but it is good practice for a server not to assume that its clients won't fling any old junk at it, so we should check the validity of the result again.
Done
if err := canMoveType(pkg.TypesInfo(), spec); err != nil {Madeline KalilspecCur
(see below)
Done
return fmt.Errorf("cannot move type: type is not exported") if structType, ok := typ.(*types.Struct); ok {This really wants to be recursive check over the entire TypeSpec. You should be able to use something like this:
```
for n := range curTypeSpec.Preorder() {
switch n := n.(type) {
case *ast.Ident:
// if it's a Use of an Object (e.g. type name or constant),
// check the object is exported (or also moving)
case *ast.StructType:
// check each field name is exported, like you do here,
// (or all its uses are also moving).
}
}
```
The "also moving" checks are fine to leave as a TODO.
Sorry, can you give an example of what you mean by "also moving"?
// 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
}I don't think this option is necessary; let's aim to get it to readiness in this CL. If we need to fix some bugs before the 22 release, that's fine.
Do you think the whole move type implementation should be done in this CL? I was going to merge and then chain the rest of the implementation
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 2 comments![]( "Open in Gerrit")
if structType, ok := typ.(*types.Struct); ok {Madeline KalilThis really wants to be recursive check over the entire TypeSpec. You should be able to use something like this:
```
for n := range curTypeSpec.Preorder() {
switch n := n.(type) {
case *ast.Ident:
// if it's a Use of an Object (e.g. type name or constant),
// check the object is exported (or also moving)
case *ast.StructType:
// check each field name is exported, like you do here,
// (or all its uses are also moving).
}
}
```
The "also moving" checks are fine to leave as a TODO.
Sorry, can you give an example of what you mean by "also moving"?
Sorry, I was vaguely thinking that we were moving a set of declarations at once, as in https://go.dev/gopls/features/web#splitpkg, in which case it's ok not to export b in `type A struct {b}; type b int` if we're moving both A and b, but now I remember that we're only moving a single type and its methods. So my "also moving" comments don't apply (though it will be something to think about when we get around to implementing the transformation part of the split-package feature).
// 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
}Madeline KalilI don't think this option is necessary; let's aim to get it to readiness in this CL. If we need to fix some bugs before the 22 release, that's fine.
Do you think the whole move type implementation should be done in this CL? I was going to merge and then chain the rest of the implementation
If you want to split it across several CLs that you plan to do in sequence, that's fine. We have plenty of time before the release.
Related details
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 2 comments![]( "Open in Gerrit")
if structType, ok := typ.(*types.Struct); ok {Madeline KalilThis really wants to be recursive check over the entire TypeSpec. You should be able to use something like this:
```
for n := range curTypeSpec.Preorder() {
switch n := n.(type) {
case *ast.Ident:
// if it's a Use of an Object (e.g. type name or constant),
// check the object is exported (or also moving)
case *ast.StructType:
// check each field name is exported, like you do here,
// (or all its uses are also moving).
}
}
```
The "also moving" checks are fine to leave as a TODO.
Alan DonovanSorry, can you give an example of what you mean by "also moving"?
Sorry, I was vaguely thinking that we were moving a set of declarations at once, as in https://go.dev/gopls/features/web#splitpkg, in which case it's ok not to export b in `type A struct {b}; type b int` if we're moving both A and b, but now I remember that we're only moving a single type and its methods. So my "also moving" comments don't apply (though it will be something to think about when we get around to implementing the transformation part of the split-package feature).
Ah got it. Maybe this feature could eventually be extended to also move multiple types
// 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
}Madeline KalilI don't think this option is necessary; let's aim to get it to readiness in this CL. If we need to fix some bugs before the 22 release, that's fine.
Alan DonovanDo you think the whole move type implementation should be done in this CL? I was going to merge and then chain the rest of the implementation
If you want to split it across several CLs that you plan to do in sequence, that's fine. We have plenty of time before the release.
Okay, that would be my preference, so I'll leave this setting. Thanks!
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 6 comments![]( "Open in Gerrit")
// selectionContainsTypeSpec returns the cursor of the type declaration that```
func foo() {
type fooa struct {
Buf bytes.Buffer
Ctx context.Context
}
}
```
Is this type `fooa` movable? The implementation for this function did not make sure the GenDecl is the file level decl, is this intended?
IMO, type that defined within a function is designed and only used within this function or the scope this type is declared.
If this is something we want to support, could you add a comment?
If not, let's mention only the file level type can be moved.
if !ok {
return fmt.Errorf("input is not a type spec")
}this is not possible right? but I don't really have a strong opinion here.
because the caller have the verification:
```
spec := specCur.Node().(*ast.TypeSpec)
decl := specCur.Parent().Node().(*ast.GenDecl)
if err := canMoveType(pkg.TypesInfo(), specCur); err != nil {
return nil, err
}
```
return fmt.Errorf("cannot move type %s: type is not exported", typSpec.Name.Name)we can ask the caller to do it, so we don't have to prepend `cannot move type %s` in all errors.
`fmt.Errorf("cannot move type %s: %w", spec.Name.Name, err)`
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg() {this import cycle is the cycle between the "from" package and the "to" package right? We can ask Alan about how to detect this kind of circle dependency. But it will be a fun problem to solve!
Problem statement: if the type we are moving depend on package ABC.., after the move, we may remove a sub set of "ABC.." from package "from", but will add "ABC..." to the package "to".
there is also possibility that is more complicated:
```
package "from" -> "foo" -> "to"
-- from/from.go --
package from
import "foo"
var _ foo.Foo
var _ From
type From struct {
foo foo.Foo
}-- foo/foo.go --
package foo
import "to"
type Foo int
var _ to.To
-- to/to.go --
package to
type To int
```
After the type `From` from package "from" to package "to" the dependency looks like:
```
from ----
| . |
v v
to <--> foo
```
```
-- from.go --
package from
import "to"
import "foo"
var _ foo.Foo
var _ to.From
-- foo.go --
package foo
import "to"
type Foo int
var _ to.To
-- to.go --
package to
import "foo"
type To int
type From struct {
foo foo.Foo
}
```Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 6 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
6 comments
// selectionContainsTypeSpec returns the cursor of the type declaration that```
func foo() {
type fooa struct {
Buf bytes.Buffer
Ctx context.Context
}
}
```Is this type `fooa` movable? The implementation for this function did not make sure the GenDecl is the file level decl, is this intended?
IMO, type that defined within a function is designed and only used within this function or the scope this type is declared.
If this is something we want to support, could you add a comment?
If not, let's mention only the file level type can be moved.
Thanks for bringing up this case, I will make a note to add a test case for this in cl/741700. I agree that a type defined within a function is normally only used within the function scope, but I see no reason why we shouldn't allow moving these types, and it's already supported with my current implementation. (This particular example, we don't support because fooa is not exported, but if the type is Fooa then it works)
if !ok {
return fmt.Errorf("input is not a type spec")
}this is not possible right? but I don't really have a strong opinion here.
because the caller have the verification:
```
spec := specCur.Node().(*ast.TypeSpec)
decl := specCur.Parent().Node().(*ast.GenDecl)
if err := canMoveType(pkg.TypesInfo(), specCur); err != nil {
return nil, err
}
```
Yeah good point, I'll just add a comment on this function about the invariant.
return fmt.Errorf("cannot move type %s: type is not exported", typSpec.Name.Name)we can ask the caller to do it, so we don't have to prepend `cannot move type %s` in all errors.
`fmt.Errorf("cannot move type %s: %w", spec.Name.Name, err)`
Done
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg() {Yes, this is the cycle I'm referring to.
Now that we have dialogue support, if we can calculate how to resolve the cycle by moving additional type declarations to the new package, we can ask the user how they would like us to proceed.
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 1 comment![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
1 comment
// selectionContainsTypeSpec returns the cursor of the type declaration thatMadeline Kalil```
func foo() {
type fooa struct {
Buf bytes.Buffer
Ctx context.Context
}
}
```Is this type `fooa` movable? The implementation for this function did not make sure the GenDecl is the file level decl, is this intended?
IMO, type that defined within a function is designed and only used within this function or the scope this type is declared.
If this is something we want to support, could you add a comment?
If not, let's mention only the file level type can be moved.
Thanks for bringing up this case, I will make a note to add a test case for this in cl/741700. I agree that a type defined within a function is normally only used within the function scope, but I see no reason why we shouldn't allow moving these types, and it's already supported with my current implementation. (This particular example, we don't support because fooa is not exported, but if the type is Fooa then it works)
Nevermind, this case isn't fully working in cl/741700, I need to make some updates.
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
I see. SG. Sorry I must misread your comment. Acked.
Just to make sure, the condition below
```
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg()
```
is preventing the case below, but not the case I mentioned above right? Because the case above is more complicated which involves 3 packages and this one is only two.
```
-- from.go --
package from
var _ Foo
type Bar string
type Foo struct {
bar Bar
}-- to.go --
package to
```
after move, package from depends on to and package to depends on from.
```
-- from.go --
package from
import "to"
var _ to.Foo
type Bar string
-- to.go --
package to
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 1 comment![]( "Open in Gerrit")
Oh no sorry my bad, I didn't fully process your example. I was only considering the case of the cycle between two packages; thank you for pointing out the more complicated case. Let me think about how we can handle this one
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Right now we always move the new type to a new file at {curDir}/newpkg/{typeName}.go, but we will need to handle this case when we implement moving a type to an existing package/file.
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
Oh, I see. If we are only moving new package, the example I mentioned above does not exist at all.
Let's add a TODO, this is not a easy problem to solve we probably can't solve this in this CL. We can have a meeting with Alan and Matloob together.
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 13 comments![]( "Open in Gerrit")
Sorry for the delay, this is not a very big change but require quite some thinking to understand this.
Alan is very busy so I will try to review them based on the knowledge I have.
// fields are exported.Update the doc comment:
-> Consider other types other than struct type. Field only exist in struct type.
The code below uses Preorder to figure out all the types.Object which is a very good solution. It covers not only struct type but also other kind of type and recursively as well.
like `type Foo func(string, net.Error)`
-> We detected not only the field name but also the field type.
// There are many ways that we could loosen this condition with further checks, such as:Allow moving unexpected type if the type is not referenced from the same package at all. (Maybe not very common though)
// - Verifying potential import cycles.Same here, `// -`
// TODO(mkalil): Additional checks:Try have a new line in between, if there is no new line, the content will be conncted together like `import cicles. TODO(mkalil)...`
```
//
// TODO(...
```
// - All references from the moved declarations are either internal to it, or toYou can have some indentation to make the hover content format it like markdown.
```
// - All ...
// imported ...
```
return fmt.Errorf("cannot move type %s: type is not exported", typSpec.Name.Name)delete: covered in the called
for n := range curTypeSpec.Preorder() {the iterator yield cursor, so maybe `c`
if _, isPkgName := obj.(*types.PkgName); !isPkgName && !obj.Exported() && obj.Pkg().Scope() != types.Universe {minor suggestion
```
if _, isPkgName := obj.(*types.PkgName); isPkgName {
continue
}
if !obj.Exported() && obj.Pkg().Scope() != types.Universe {
return fmt.Errorf("would result in reference to unexported object %s", obj.Name())
}...
```
if typesinternal.IsPackageLevel(obj) && obj.Pkg() == typNameObj.Pkg() {I'm not very sure what this means, the case below is not allowed because the limitation.
```
type Bar string
type Foo struct {
Bar Bar
}
```But the below is allowed?
```
func _() {
type Bar string
type Foo struct {
Bar Bar
}
}
``` return fmt.Errorf("would result in reference to unexported fields")%s" field name?
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg() {Acknowledged
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg() {I did not fully understand the circle dependency here. This check is saying, the struct type's fields should not have type defined in the same package.
I think use the ident detection is more appropriate, the code below will be caught by this check correct?
```
type Bar string
type Foo struct {
Bar Bar
}
```and but? the type of the field is func, but the code here did not dig deeper into the func...
```
type Bar string
type Foo struct {
Bar func(Bar) error
}
```Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 9 comments![]( "Open in Gerrit")
if err != nil {if err := ..; err != nil {
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File, buf *bytes.Buffer) error {if the function name is new file, then we should return a buffer instead of writing it to a buffer.
I assumed this function must be called first before the caller want to do anything, so let's return a buffer.
the doc comment also indicate it it returning a buffer.
changes, err := addTypeToPackage(ctx, snapshot, pgf, pkg, spec, decl, newDir)Instead of passing the decl to the function, maybe we can pass only the comment to it to make this more clear. I was a little bit confused when I saw the decl is also passed into it.
```
// move the decl's comment along with the type spec only if
// it is the only type declared.
var declComments []*ast.CommentGroup
if len(decl.Specs) == 1 {
....
}
```
so we can keep the comment handling logic in the same file, including the old file comment deletion and the new file comment addition.
// It only produces edits for the new file.the first sentence is clear enough.
if err != nil {if err := ...; err != nil {}
// Use CommentedNode to avoid losing comments inside struct typesMaybe it has been discussed, may I ask why not simply copy the entire content from the source file and move it here?
Or it is because using CommentedNode is more general solution for all kinds of cases? I think we used to prefer copy the content whenever possible.
// type A struct { // this comment gets lost.
// B int
// }
```
In sert a space, go doc will then intepret it as code.
// (See @multitypespecB/d/newpkg/b.go in movetype.txt)TIL. I thought this is not legal. 😂
}TODO: open the file that this type is moving to.
See AddTest command as example, test example is available
gopls/internal/test/integration/misc/addtest_test.go
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 11 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
11 comments
Update the doc comment:
-> Consider other types other than struct type. Field only exist in struct type.
The code below uses Preorder to figure out all the types.Object which is a very good solution. It covers not only struct type but also other kind of type and recursively as well.
like `type Foo func(string, net.Error)`
-> We detected not only the field name but also the field type.
Done
// There are many ways that we could loosen this condition with further checks, such as:Allow moving unexpected type if the type is not referenced from the same package at all. (Maybe not very common though)
Done (modified second bullet)
// - Verifying potential import cycles.Madeline KalilSame here, `// -`
Done
Try have a new line in between, if there is no new line, the content will be conncted together like `import cicles. TODO(mkalil)...`
```
//
// TODO(...
```
Looks much better now, thanks!
// - All references from the moved declarations are either internal to it, or toYou can have some indentation to make the hover content format it like markdown.
```
// - All ...
// imported ...
```
Done
return fmt.Errorf("cannot move type %s: type is not exported", typSpec.Name.Name)delete: covered in the called
Done
the iterator yield cursor, so maybe `c`
Done
if _, isPkgName := obj.(*types.PkgName); !isPkgName && !obj.Exported() && obj.Pkg().Scope() != types.Universe {minor suggestion
```
if _, isPkgName := obj.(*types.PkgName); isPkgName {
continue
}if !obj.Exported() && obj.Pkg().Scope() != types.Universe {
return fmt.Errorf("would result in reference to unexported object %s", obj.Name())
}...
```
Done
if typesinternal.IsPackageLevel(obj) && obj.Pkg() == typNameObj.Pkg() {I'm not very sure what this means, the case below is not allowed because the limitation.
```
type Bar stringtype Foo struct {
Bar Bar
}
```But the below is allowed?
```
func _() {
type Bar stringtype Foo struct {
Bar Bar
}
}
```
Thanks for pointing this out; the second case should not be allowed. It is not sufficient to check IsExported because this just checks whether the type name is capitalized. We should check if the type is package level.
return fmt.Errorf("would result in reference to unexported fields")Madeline Kalil%s" field name?
Done
if named, ok := field.Type().(*types.Named); ok && named.Obj().Pkg() == typNameObj.Pkg() {I did not fully understand the circle dependency here. This check is saying, the struct type's fields should not have type defined in the same package.
I think use the ident detection is more appropriate, the code below will be caught by this check correct?
```
type Bar stringtype Foo struct {
Bar Bar
}
```and but? the type of the field is func, but the code here did not dig deeper into the func...
```
type Bar stringtype Foo struct {
Bar func(Bar) error
}
```
Yes, moving the type Foo would result in an import cycle: Foo's destination package needs to import the package with "Bar" in it, and the package with "Bar" in it needs to import Foo's destination package.
We will inspect the Bar in func(Bar) when it inspects *ast.Idents in the Preorder traversal. Are you saying that you think we should remove the *ast.StructType case and just inspect Idents?
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted and added 7 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Commit-Queue | +1 |
7 comments
if err := ..; err != nil {
Done
func newFileWithImports(pkgName string, imports []*ast.ImportSpec, fromPGF *parsego.File, buf *bytes.Buffer) error {if the function name is new file, then we should return a buffer instead of writing it to a buffer.
I assumed this function must be called first before the caller want to do anything, so let's return a buffer.
the doc comment also indicate it it returning a buffer.
Thanks, I agree this was confusing. I've changed the function to writeNewFileWithImports because I think we should still write to the buffer in this case.
// It only produces edits for the new file.Madeline Kalilthe first sentence is clear enough.
Done
if err := ...; err != nil {}
Done
// Use CommentedNode to avoid losing comments inside struct typesMaybe it has been discussed, may I ask why not simply copy the entire content from the source file and move it here?
Or it is because using CommentedNode is more general solution for all kinds of cases? I think we used to prefer copy the content whenever possible.
Copying the content directly won't always work, for example if we have to move a type from within a type decl like:
```
type (
A int <- move type A
B string
)
```
But I think if we are in the "normal" case of moving a single type not within a type decl, we could copy the content directly. I will implement this
// type A struct { // this comment gets lost.```
// type A struct { // this comment gets lost.
// B int
// }
```In sert a space, go doc will then intepret it as code.
TODO: open the file that this type is moving to.
See AddTest command as example, test example is available
gopls/internal/test/integration/misc/addtest_test.go
Good idea, thanks
Related details
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |