[tools] gopls/internal: add code action "move to a new file"
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal: add code action "move to a new file"
This code action moves selected code sections to a newly created file within the same package. The created filename is chosen as the first {function, type, const, var} name encountered. In addition, import declarations are added or removed as needed.
Fixes golang/go#65707
Change diff
diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
index d81fd20..1725cea 100644
--- a/gopls/internal/cache/snapshot.go
+++ b/gopls/internal/cache/snapshot.go
@@ -1658,6 +1658,22 @@
return found && strings.Contains(after, "/")
}
+// Sandbox clones the reciever, applying given file modfications as overlays.
+func (s *Snapshot) Sandbox(ctx, bgCtx context.Context, modifications []file.Modification) *Snapshot {
+ updatedFiles := make(map[protocol.DocumentURI]file.Handle)
+ for _, m := range modifications {
+ updatedFiles[m.URI] = &overlay{
+ uri: m.URI,
+ content: m.Text,
+ }
+ }
+ cloned, _ := s.clone(ctx, bgCtx, StateChange{
+ Modifications: modifications,
+ Files: updatedFiles,
+ }, func() {})
+ return cloned
+}
+
// clone copies state from the receiver into a new Snapshot, applying the given
// state changes.
//
diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go
index df4ca51..6fdd118 100644
--- a/gopls/internal/golang/codeaction.go
+++ b/gopls/internal/golang/codeaction.go
@@ -86,6 +86,11 @@
return nil, err
}
actions = append(actions, extractions...)
+ moves, err := getMoveToNewFileCodeAction(pgf, rng, snapshot.Options())
+ if err != nil {
+ return nil, err
+ }
+ actions = append(actions, moves)
}
}
diff --git a/gopls/internal/golang/format.go b/gopls/internal/golang/format.go
index 3e5668b..1db320f 100644
--- a/gopls/internal/golang/format.go
+++ b/gopls/internal/golang/format.go
@@ -92,6 +92,38 @@
return computeTextEdits(ctx, pgf, formatted)
}
+func formatImportsBytes(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle) ([]byte, error) {
+ _, done := event.Start(ctx, "golang.formatImportsBytes")
+ defer done()
+
+ errorPrefix := "formatImportsBytes"
+
+ text, err := fh.Content()
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ var out []byte
+ if err := snapshot.RunProcessEnvFunc(ctx, func(ctx context.Context, opts *imports.Options) error {
+ fixes, err := imports.FixImports(ctx, fh.URI().Path(), text, opts)
+ if err != nil {
+ return fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+ out, err = imports.ApplyFixes(fixes, fh.URI().Path(), text, opts, parsego.Full)
+ if err != nil {
+ return fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+ return nil
+ }); err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+ out, err = format.Source(out)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+ return out, nil
+}
+
func formatSource(ctx context.Context, fh file.Handle) ([]byte, error) {
_, done := event.Start(ctx, "golang.formatSource")
defer done()
diff --git a/gopls/internal/golang/move_to_new_file.go b/gopls/internal/golang/move_to_new_file.go
new file mode 100644
index 0000000..4f0d40e
--- /dev/null
+++ b/gopls/internal/golang/move_to_new_file.go
@@ -0,0 +1,326 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package golang
+
+// This file defines the code action "move to a new file".
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "go/ast"
+ "go/token"
+ "os"
+ "path/filepath"
+ "strings"
+
+ "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/gopls/internal/protocol/command"
+ "golang.org/x/tools/gopls/internal/settings"
+)
+
+func getMoveToNewFileCodeAction(pgf *parsego.File, rng protocol.Range, _ *settings.Options) (protocol.CodeAction, error) {
+ ok := canMoveToANewFile(pgf, rng)
+ if !ok {
+ return protocol.CodeAction{}, nil
+ }
+ cmd, err := command.NewMoveToANewFileCommand("m", command.MoveToANewFileArgs{URI: pgf.URI, Range: rng})
+ if err != nil {
+ return protocol.CodeAction{}, err
+ }
+ return protocol.CodeAction{
+ Title: "Move to a new file",
+ Kind: protocol.RefactorExtract,
+ Command: &cmd,
+ }, nil
+}
+
+// canMoveToANewFile reports whether the code in the given range can be move to a new file.
+func canMoveToANewFile(pgf *parsego.File, rng protocol.Range) bool {
+ _, err := moveToANewFileInternal(context.Background(), nil, nil, pgf, rng, true)
+ if err != nil {
+ return false
+ } else {
+ return true
+ }
+}
+
+// MoveToANewFile moves selected declarations into a new file.
+func MoveToANewFile(
+ ctx context.Context,
+ snapshot *cache.Snapshot,
+ fh file.Handle,
+ rng protocol.Range,
+) (*protocol.WorkspaceEdit, error) {
+ pgf, err := snapshot.ParseGo(ctx, fh, parsego.Full)
+ if err != nil {
+ return nil, fmt.Errorf("MoveToANewFile: %w", err)
+ }
+ return moveToANewFileInternal(ctx, snapshot, fh, pgf, rng, false)
+}
+
+// moveToANewFileInternal moves selected declarations into a new file.
+func moveToANewFileInternal(
+ ctx context.Context,
+ snapshot *cache.Snapshot,
+ fh file.Handle,
+ pgf *parsego.File,
+ rng protocol.Range,
+ dry bool,
+) (*protocol.WorkspaceEdit, error) {
+ // todo: check client allows create file
+ errorPrefix := "moveToANewFileInternal"
+
+ start, end, err := pgf.RangePos(rng)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ start, end, filename, err := findRangeAndFilename(pgf, start, end)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ if dry {
+ return nil, nil
+ }
+
+ start, end = adjustRangeForEmptyLines(pgf, start, end)
+
+ createFileURI, err := resolveCreateFileURI(pgf.URI.Dir().Path(), filename)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ replaceRange, err := pgf.PosRange(start, end)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ start -= pgf.File.FileStart
+ end -= pgf.File.FileStart
+
+ modifiedText := make([]byte, 0)
+ modifiedText = append(modifiedText, pgf.Src[:start]...)
+ modifiedText = append(modifiedText, pgf.Src[end:]...)
+
+ packageName := pgf.File.Name.Name
+ createFileTextWithoutImports := []byte("package " + packageName + "\n" + string(pgf.Src[start:end]))
+
+ modifications := []file.Modification{
+ {
+ URI: fh.URI(),
+ Action: file.Change,
+ Text: modifiedText,
+ },
+ {
+ URI: createFileURI,
+ Action: file.Change,
+ Text: createFileTextWithoutImports,
+ },
+ }
+
+ // apply edits into a cloned snapshot and calculate import edits
+ snapshot = snapshot.Sandbox(ctx, ctx, modifications)
+ newFh, err := snapshot.ReadFile(ctx, fh.URI())
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ newPgf, err := snapshot.ParseGo(ctx, newFh, parsego.Full)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ importEdits, _, err := allImportsFixes(ctx, snapshot, newPgf)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ createFh, err := snapshot.ReadFile(ctx, createFileURI)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ createFileText, err := formatImportsBytes(ctx, snapshot, createFh)
+ if err != nil {
+ return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+ }
+
+ return &protocol.WorkspaceEdit{
+ DocumentChanges: []protocol.DocumentChanges{
+ {
+ // original file edits
+ TextDocumentEdit: &protocol.TextDocumentEdit{
+ TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
+ TextDocumentIdentifier: protocol.TextDocumentIdentifier{
+ URI: fh.URI(),
+ },
+ Version: fh.Version(),
+ },
+ Edits: protocol.AsAnnotatedTextEdits(
+ append(
+ importEdits,
+ protocol.TextEdit{
+ Range: replaceRange,
+ NewText: "",
+ }),
+ ),
+ },
+ },
+ {
+ CreateFile: &protocol.CreateFile{
+ Kind: "create",
+ URI: createFileURI,
+ },
+ },
+ {
+ // created file edits
+ TextDocumentEdit: &protocol.TextDocumentEdit{
+ TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
+ TextDocumentIdentifier: protocol.TextDocumentIdentifier{
+ URI: createFileURI,
+ },
+ Version: 0,
+ },
+ Edits: protocol.AsAnnotatedTextEdits([]protocol.TextEdit{
+ {
+ Range: protocol.Range{},
+ NewText: string(createFileText),
+ },
+ })},
+ },
+ },
+ }, nil
+}
+
+// resolveCreateFileURI checks that basename.go does not exists in dir, otherwise
+// select basename.{1,2,3,4,5}.go as filename.
+func resolveCreateFileURI(dir string, basename string) (protocol.DocumentURI, error) {
+ basename = strings.ToLower(basename)
+ newPath := filepath.Join(dir, basename+".go")
+ for count := 1; ; count++ {
+ if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) {
+ break
+ }
+ if count >= 5 {
+ return "", fmt.Errorf("resolveNewFileURI: exceeded retry limit")
+ }
+ filename := fmt.Sprintf("%s.%d.go", basename, count)
+ newPath = filepath.Join(dir, filename)
+ }
+ return protocol.URIFromPath(newPath), nil
+}
+
+// findRangeAndFilename checks the selection is valid and extends range as needed and returns adjusted
+// range and selected filename.
+func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {
+ if intersect(start, end, pgf.File.Package, pgf.File.Name.End()) {
+ return 0, 0, "", errors.New("selection cannot intersect a package declaration")
+ }
+ firstName := ""
+ for _, node := range pgf.File.Decls {
+ if intersect(start, end, node.Pos(), node.End()) {
+ if v, ok := node.(*ast.GenDecl); ok && v.Tok == token.IMPORT {
+ return 0, 0, "", errors.New("selection cannot intersect an import declaration")
+ }
+ if _, ok := node.(*ast.BadDecl); ok {
+ return 0, 0, "", errors.New("selection cannot intersect a bad declaration")
+ }
+ // should work when only selecting keyword "func" or function name
+ if v, ok := node.(*ast.FuncDecl); ok && contain(v.Pos(), v.Name.End(), start, end) {
+ start, end = v.Pos(), v.End()
+ }
+ // should work when only selecting keyword "type", "var", "const"
+ if v, ok := node.(*ast.GenDecl); ok && (v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||
+ v.Tok == token.CONST && contain(v.Pos(), v.Pos()+5, start, end) ||
+ v.Tok == token.VAR && contain(v.Pos(), v.Pos()+3, start, end)) {
+ start, end = v.Pos(), v.End()
+ }
+ if !contain(start, end, node.Pos(), node.End()) {
+ return 0, 0, "", errors.New("selection cannot partially intersect a node")
+ } else {
+ if firstName == "" {
+ firstName = getNodeName(node)
+ }
+ // extends selection to docs comments
+ if c := getCommentGroup(node); c != nil {
+ if c.Pos() < start {
+ start = c.Pos()
+ }
+ }
+ }
+ }
+ }
+ for _, node := range pgf.File.Comments {
+ if intersect(start, end, node.Pos(), node.End()) {
+ if !contain(start, end, node.Pos(), node.End()) {
+ return 0, 0, "", errors.New("selection cannot partially intersect a comment")
+ }
+ }
+ }
+ if firstName == "" {
+ return 0, 0, "", errors.New("nothing selected")
+ }
+ return start, end, firstName, nil
+}
+
+func adjustRangeForEmptyLines(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos) {
+ i := int(end)
+ for ; i-int(pgf.File.FileStart) < len(pgf.Src); i++ {
+ c := pgf.Src[i-int(pgf.File.FileStart)]
+ if c == ' ' || c == '\t' || c == '\n' {
+ continue
+ } else {
+ break
+ }
+ }
+ return start, token.Pos(i)
+}
+
+func getCommentGroup(node ast.Node) *ast.CommentGroup {
+ switch n := node.(type) {
+ case *ast.GenDecl:
+ return n.Doc
+ case *ast.FuncDecl:
+ return n.Doc
+ }
+ return nil
+}
+
+// getNodeName returns the first func name or variable name
+func getNodeName(node ast.Node) string {
+ switch n := node.(type) {
+ case *ast.FuncDecl:
+ return n.Name.Name
+ case *ast.GenDecl:
+ if len(n.Specs) == 0 {
+ return ""
+ }
+ switch m := n.Specs[0].(type) {
+ case *ast.TypeSpec:
+ return m.Name.Name
+ case *ast.ValueSpec:
+ if len(m.Names) == 0 {
+ return ""
+ }
+ return m.Names[0].Name
+ }
+ }
+ return ""
+}
+
+// intersect checks if [a, b) and [c, d) intersect, assumming a <= b and c <= d
+func intersect(a, b, c, d token.Pos) bool {
+ return !(b <= c || d <= a)
+}
+
+// contain checks if [a, b) contains [c, d), assumming a <= b and c <= d
+func contain(a, b, c, d token.Pos) bool {
+ return a <= c && d <= b
+}
diff --git a/gopls/internal/protocol/command/command_gen.go b/gopls/internal/protocol/command/command_gen.go
index 2ee0a5d..2195abf 100644
--- a/gopls/internal/protocol/command/command_gen.go
+++ b/gopls/internal/protocol/command/command_gen.go
@@ -38,6 +38,7 @@
ListKnownPackages Command = "list_known_packages"
MaybePromptForTelemetry Command = "maybe_prompt_for_telemetry"
MemStats Command = "mem_stats"
+ MoveToANewFile Command = "move_to_a_new_file"
RegenerateCgo Command = "regenerate_cgo"
RemoveDependency Command = "remove_dependency"
ResetGoModDiagnostics Command = "reset_go_mod_diagnostics"
@@ -74,6 +75,7 @@
ListKnownPackages,
MaybePromptForTelemetry,
MemStats,
+ MoveToANewFile,
RegenerateCgo,
RemoveDependency,
ResetGoModDiagnostics,
@@ -183,6 +185,12 @@
return nil, s.MaybePromptForTelemetry(ctx)
case "gopls.mem_stats":
return s.MemStats(ctx)
+ case "gopls.move_to_a_new_file":
+ var a0 MoveToANewFileArgs
+ if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
+ return nil, err
+ }
+ return nil, s.MoveToANewFile(ctx, a0)
case "gopls.regenerate_cgo":
var a0 URIArg
if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
@@ -475,6 +483,18 @@
}, nil
}
+func NewMoveToANewFileCommand(title string, a0 MoveToANewFileArgs) (protocol.Command, error) {
+ args, err := MarshalArgs(a0)
+ if err != nil {
+ return protocol.Command{}, err
+ }
+ return protocol.Command{
+ Title: title,
+ Command: "gopls.move_to_a_new_file",
+ Arguments: args,
+ }, nil
+}
+
func NewRegenerateCgoCommand(title string, a0 URIArg) (protocol.Command, error) {
args, err := MarshalArgs(a0)
if err != nil {
diff --git a/gopls/internal/protocol/command/interface.go b/gopls/internal/protocol/command/interface.go
index b0dd060..aeda6aa 100644
--- a/gopls/internal/protocol/command/interface.go
+++ b/gopls/internal/protocol/command/interface.go
@@ -148,6 +148,11 @@
// themselves.
AddImport(context.Context, AddImportArgs) error
+ // MoveToANewFile: Move selected codes to a new file
+ //
+ // Used by the code action of the same name.
+ MoveToANewFile(context.Context, MoveToANewFileArgs) error
+
// StartDebugging: Start the gopls debug server
//
// Start the gopls debug server if it isn't running, and return the debug
@@ -330,6 +335,12 @@
URI protocol.DocumentURI
}
+type MoveToANewFileArgs struct {
+ // URI of the file
+ URI protocol.DocumentURI
+ Range protocol.Range
+}
+
type ListKnownPackagesResult struct {
// Packages is a list of packages relative
// to the URIArg passed by the command request.
diff --git a/gopls/internal/protocol/tsdocument_changes.go b/gopls/internal/protocol/tsdocument_changes.go
index 2c7a524..393c5c8 100644
--- a/gopls/internal/protocol/tsdocument_changes.go
+++ b/gopls/internal/protocol/tsdocument_changes.go
@@ -6,14 +6,16 @@
import (
"encoding/json"
+ "errors"
"fmt"
)
-// DocumentChanges is a union of a file edit and directory rename operations
+// DocumentChanges is a union of a file edit, file creation, and directory rename operations
// for package renaming feature. At most one field of this struct is non-nil.
type DocumentChanges struct {
TextDocumentEdit *TextDocumentEdit
RenameFile *RenameFile
+ CreateFile *CreateFile
}
func (d *DocumentChanges) UnmarshalJSON(data []byte) error {
@@ -26,10 +28,16 @@
if _, ok := m["textDocument"]; ok {
d.TextDocumentEdit = new(TextDocumentEdit)
return json.Unmarshal(data, d.TextDocumentEdit)
+ } else if kind, ok := m["kind"]; ok {
+ if kind == "create" {
+ d.CreateFile = new(CreateFile)
+ return json.Unmarshal(data, d.CreateFile)
+ } else if kind == "rename" {
+ d.RenameFile = new(RenameFile)
+ return json.Unmarshal(data, d.RenameFile)
+ }
}
-
- d.RenameFile = new(RenameFile)
- return json.Unmarshal(data, d.RenameFile)
+ return errors.New("don't know how to unmarshal")
}
func (d *DocumentChanges) MarshalJSON() ([]byte, error) {
@@ -37,6 +45,8 @@
return json.Marshal(d.TextDocumentEdit)
} else if d.RenameFile != nil {
return json.Marshal(d.RenameFile)
+ } else if d.CreateFile != nil {
+ return json.Marshal(d.CreateFile)
}
- return nil, fmt.Errorf("Empty DocumentChanges union value")
+ return nil, fmt.Errorf("empty DocumentChanges union value")
}
diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go
index 19ea884..039393b 100644
--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -870,6 +870,22 @@
})
}
+func (c *commandHandler) MoveToANewFile(ctx context.Context, args command.MoveToANewFileArgs) error {
+ return c.run(ctx, commandConfig{
+ progress: "Move to a new file",
+ forURI: args.URI,
+ }, func(ctx context.Context, deps commandDeps) error {
+ edit, err := golang.MoveToANewFile(ctx, deps.snapshot, deps.fh, args.Range)
+ if err != nil {
+ return err
+ }
+ if _, err := c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{Edit: *edit}); err != nil {
+ return fmt.Errorf("could not apply edits: %v", err)
+ }
+ return nil
+ })
+}
+
func (c *commandHandler) StartDebugging(ctx context.Context, args command.DebuggingArgs) (result command.DebuggingResult, _ error) {
addr := args.Addr
if addr == "" {
diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go
index e937764..8125f04 100644
--- a/gopls/internal/test/integration/fake/editor.go
+++ b/gopls/internal/test/integration/fake/editor.go
@@ -761,6 +761,18 @@
return e.setBufferContentLocked(ctx, path, true, patched, edits)
}
+// OffsetLocation returns the Location for integers offsets start and end
+func (e *Editor) OffsetLocation(bufName string, start, end int) (protocol.Location, error) {
+ e.mu.Lock()
+ buf, ok := e.buffers[bufName]
+ e.mu.Unlock()
+ if !ok {
+ return protocol.Location{}, ErrUnknownBuffer
+ }
+
+ return buf.mapper.OffsetLocation(start, end)
+}
+
// EditBuffer applies the given test edits to the buffer identified by path.
func (e *Editor) EditBuffer(ctx context.Context, path string, edits []protocol.TextEdit) error {
e.mu.Lock()
@@ -1402,7 +1414,11 @@
if change.TextDocumentEdit != nil {
return e.applyTextDocumentEdit(ctx, *change.TextDocumentEdit)
}
- panic("Internal error: one of RenameFile or TextDocumentEdit must be set")
+ if change.CreateFile != nil {
+ path := e.sandbox.Workdir.URIToPath(change.CreateFile.URI)
+ return e.sandbox.Workdir.WriteFile(ctx, path, "")
+ }
+ panic("Internal error: one of RenameFile, CreateFile, or TextDocumentEdit must be set")
}
func (e *Editor) applyTextDocumentEdit(ctx context.Context, change protocol.TextDocumentEdit) error {
diff --git a/gopls/internal/test/integration/misc/move_to_new_file_test.go b/gopls/internal/test/integration/misc/move_to_new_file_test.go
new file mode 100644
index 0000000..adf1ec8
--- /dev/null
+++ b/gopls/internal/test/integration/misc/move_to_new_file_test.go
@@ -0,0 +1,386 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+package misc
+
+import (
+ "fmt"
+ "regexp"
+ "strings"
+ "testing"
+
+ . "golang.org/x/tools/gopls/internal/test/integration"
+
+ "golang.org/x/tools/gopls/internal/protocol"
+)
+
+func dedent(s string) string {
+ s = strings.TrimPrefix(s, "\n")
+ indents := regexp.MustCompile("^\t*").FindString(s)
+ return regexp.MustCompile(fmt.Sprintf("(?m)^\t{0,%d}", len(indents))).ReplaceAllString(s, "")
+}
+
+func indent(s string) string {
+ return regexp.MustCompile("(?m)^").ReplaceAllString(s, "\t")
+}
+
+// compileTemplate replaces two â–ˆ characters in text and write to dest and returns
+// the location enclosed by the two â–ˆ
+func compileTemplate(env *Env, text string, dest string) protocol.Location {
+ i := strings.Index(text, "â–ˆ")
+ j := strings.LastIndex(text, "â–ˆ")
+ if strings.Count(text, "â–ˆ") != 2 {
+ panic("expecting exactly two â–ˆ characters in source")
+ }
+ out := text[:i] + text[i+len("â–ˆ"):j] + text[j+len("â–ˆ"):]
+ env.Sandbox.Workdir.WriteFile(env.Ctx, dest, out)
+ env.OpenFile(dest)
+ loc, err := env.Editor.OffsetLocation(dest, i, j-len("â–ˆ"))
+ if err != nil {
+ panic(err)
+ }
+ return loc
+}
+
+func TestMoveToANewFile(t *testing.T) {
+ const files = `
+-- go.mod --
+module mod.com
+
+go 1.18
+-- main.go --
+package main
+
+-- filealreadexists.go --
+package main
+
+-- filealreadexistsmore.go --
+package main
+
+-- filealreadexistsmore.1.go --
+package main
+
+`
+ for _, tc := range []struct {
+ name string
+ source string
+ fixed string
+ createdFilename string
+ created string
+ }{
+ {
+ name: "func declaration",
+ source: `
+ package main
+
+ func _() {}
+
+ // fn docs
+ â–ˆfunc fn() {}â–ˆ
+ `,
+ fixed: `
+ package main
+
+ func _() {}
+
+ `,
+ createdFilename: "fn.go",
+ created: `
+ package main
+
+ // fn docs
+ func fn() {}
+ `,
+ },
+ {
+ name: "only select function name",
+ source: `
+ package main
+ func â–ˆFâ–ˆ() {}
+ `,
+ fixed: `
+ package main
+ `,
+ createdFilename: "f.go",
+ created: `
+ package main
+
+ func F() {}
+ `,
+ },
+ {
+ name: "type declaration",
+ source: `
+ package main
+
+ // T docs
+ â–ˆtype T int
+ type S intâ–ˆ
+ `,
+ fixed: `
+ package main
+
+ `,
+ createdFilename: "t.go",
+ created: `
+ package main
+
+ // T docs
+ type T int
+ type S int
+ `,
+ },
+ {
+ name: "const and var declaration",
+ source: `
+ package main
+
+ // c docs
+ â–ˆconst c = 0
+ var v = 0â–ˆ
+ `,
+ fixed: `
+ package main
+
+ `,
+ createdFilename: "c.go",
+ created: `
+ package main
+
+ // c docs
+ const c = 0
+
+ var v = 0
+ `,
+ },
+ {
+ name: "select only const keyword",
+ source: `
+ package main
+
+ â–ˆconstâ–ˆ (
+ A = iota
+ B
+ C
+ )
+ `,
+ fixed: `
+ package main
+
+ `,
+ createdFilename: "a.go",
+ created: `
+ package main
+
+ const (
+ A = iota
+ B
+ C
+ )
+ `,
+ },
+ {
+ name: "select surrounding comments",
+ source: `
+ package main
+
+ â–ˆ// above
+
+ func fn() {}
+
+ // belowâ–ˆ
+ `,
+ fixed: `
+ package main
+
+ `,
+ createdFilename: "fn.go",
+ created: `
+ package main
+
+ // above
+
+ func fn() {}
+
+ // below
+ `,
+ },
+
+ {
+ name: "create file name conflict",
+ source: `
+ package main
+ â–ˆfunc filealreadexists() {}â–ˆ
+ `,
+ fixed: `
+ package main
+ `,
+ createdFilename: "filealreadexists.1.go",
+ created: `
+ package main
+
+ func filealreadexists() {}
+ `,
+ },
+ {
+ name: "create file name conflict once more",
+ source: `
+ package main
+ â–ˆfunc filealreadexistsmore() {}â–ˆ
+ `,
+ fixed: `
+ package main
+ `,
+ createdFilename: "filealreadexistsmore.2.go",
+ created: `
+ package main
+
+ func filealreadexistsmore() {}
+ `,
+ },
+ {
+ name: "imports",
+ source: `
+ package main
+ import "fmt"
+ â–ˆfunc F() {
+ fmt.Println()
+ }â–ˆ
+ `,
+ fixed: `
+ package main
+ `,
+ createdFilename: "f.go",
+ created: `
+ package main
+
+ import "fmt"
+
+ func F() {
+ fmt.Println()
+ }
+ `,
+ },
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ Run(t, files, func(t *testing.T, env *Env) {
+ tc.source, tc.fixed, tc.created = dedent(tc.source), dedent(tc.fixed), dedent(tc.created)
+ filename := "source.go"
+ loc := compileTemplate(env, tc.source, filename)
+ actions, err := env.Editor.CodeAction(env.Ctx, loc, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var codeAction *protocol.CodeAction
+ for _, action := range actions {
+ if action.Title == "Move to a new file" {
+ codeAction = &action
+ break
+ }
+ }
+ if codeAction == nil {
+ t.Fatal("cannot find move to a new file action")
+ }
+
+ env.ApplyCodeAction(*codeAction)
+ got := env.BufferText(filename)
+ if tc.fixed != got {
+ t.Errorf(`incorrect output of fixed file:
+source:
+%s
+got:
+%s
+want:
+%s
+`, indent(tc.source), indent(got), indent(tc.fixed))
+ }
+ gotMoved := env.BufferText(tc.createdFilename)
+ if tc.created != gotMoved {
+ t.Errorf(`incorrect output of created file:
+source:
+%s
+got created file:
+%s
+want created file:
+%s
+`, indent(tc.source), indent(gotMoved), indent(tc.created))
+ }
+
+ })
+ })
+ }
+}
+
+func TestMoveToANewFileInvalidSelection(t *testing.T) {
+ const files = `
+-- go.mod --
+module mod.com
+
+go 1.18
+-- main.go --
+package main
+
+`
+ for _, tc := range []struct {
+ name string
+ source string
+ }{
+ {
+ name: "select package declaration",
+ source: `
+ â–ˆpackage mainâ–ˆ
+ func fn() {}
+ `,
+ },
+ {
+ name: "select imports",
+ source: `
+ package main
+ â–ˆimport fmtâ–ˆ
+ `,
+ },
+ {
+ name: "select only comment",
+ source: `
+ package main
+ â–ˆ// commentâ–ˆ
+ `,
+ },
+ {
+ name: "selection does not contain whole top-level node",
+ source: `
+ package main
+ func fn() {
+ â–ˆprint(0)â–ˆ
+ }
+ `,
+ },
+ {
+ name: "selection cross a comment",
+ source: `
+ package main
+ // comâ–ˆment
+
+ func fn() {}â–ˆ
+ `,
+ },
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ Run(t, files, func(t *testing.T, env *Env) {
+ filename := "source.go"
+ loc := compileTemplate(env, dedent(tc.source), filename)
+ actions, err := env.Editor.CodeAction(env.Ctx, loc, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ for _, action := range actions {
+ if action.Title == "Move to a new file" {
+ t.Errorf("should not offer code action")
+ return
+ }
+ }
+ })
+ })
+ }
+}
Change information
- M gopls/internal/cache/snapshot.go
- M gopls/internal/golang/codeaction.go
- M gopls/internal/golang/format.go
- A gopls/internal/golang/move_to_new_file.go
- M gopls/internal/protocol/command/command_gen.go
- M gopls/internal/protocol/command/interface.go
- M gopls/internal/protocol/tsdocument_changes.go
- M gopls/internal/server/command.go
- M gopls/internal/test/integration/fake/editor.go
- A gopls/internal/test/integration/misc/move_to_new_file_test.go
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. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 250 character line.
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
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. |
Gerrit Bot (Gerrit)
Gerrit Bot 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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 250 character line.The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Seem like a false alarm
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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 1 comment![]( "Open in Gerrit")
There seems to be no big problems handling comment nodes.
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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 3 comments![]( "Open in Gerrit")
}, func() {})I am not sure whether I need to call defer cloned.decRef() here. The docs for Snapshot.clone say it must be called but doing so cause panics.
snapshot = snapshot.Sandbox(ctx, ctx, modifications)Not sure what to put in the second context argument.
// for package renaming feature. At most one field of this struct is non-nil.I think making this into an interface with TextDocumentEdit, RenameFile, and CreateFile implementing it would make it easier to use.
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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 1 comment![]( "Open in Gerrit")
Need to do this because the code action needs to update import declarations after moving codes, but allImportsFixes requires snapshot as an argument. So I first made code moving changes in a cloned snapshot, and then call allImportsFixes on the cloned snapshot.
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 13 comments![]( "Open in Gerrit")
Hi Chiawen, many thanks for contributing this feature! It looks like you've done a thorough job with testing.
I've taken a first pass over it and added a number of comments.
Need to do this because the code action needs to update import declarations after moving codes, but allImportsFixes requires snapshot as an argument. So I first made code moving changes in a cloned snapshot, and then call allImportsFixes on the cloned snapshot.
This seems unnecessarily complex. The required import updates can be computed from the the moved code: (1) copy all the import declarations from the original file, then, (2) for both files (old and new), delete any import declarations that aren't needed.
To compute the necessary imports, iterate over types.Info.Uses map looking for (id, obj) pairs where obj is a PkgName in the old file. Divide them into two buckets based on whether the refactoring moved id or not. Then see which PkgNames in each bucket have no ids and delete its ImportSpec from the corresponding file.
Alternatively, you could omit step 2 and just wait for the user to save both files, triggering goimports.
moves, err := getMoveToNewFileCodeAction(pgf, rng, snapshot.Options())MoveToNewFile is an "extract" code action, so I think you should move it into getExtractCodeActions.
cmd, err := command.NewMoveToANewFileCommand("m", command.MoveToANewFileArgs{URI: pgf.URI, Range: rng})"Extract declarations to new file"
(I don't think this name appear in the VS Code UI, but let's choose something meaningful. I does appear in the CLI I think.)
Title: "Move to a new file","Extract declarations to new file"
// apply edits into a cloned snapshot and calculate import editsI don't think you need any of this section (up to L153) if you compute the appropriate import edits as described in my other comment.
return &protocol.WorkspaceEdit{In an ideal world the user's cursor would be moved to the new file---otherwise they may wonder what file name was created. That's possible to achieve, but a little tricky: rather than returning a WorkspaceEdit, we would have to make a Client.ApplyEdit downcall, followed by a ShowDocument downcall featuring the new URI. (The Client.ApplyEdit need contain only the file-creation edit; we could return a WorkspaceEdit for the actual changes to the text.)
{
// original file edits
TextDocumentEdit: &protocol.TextDocumentEdit{
TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
TextDocumentIdentifier: protocol.TextDocumentIdentifier{
URI: fh.URI(),
},
Version: fh.Version(),
},
Edits: protocol.AsAnnotatedTextEdits(
append(
importEdits,
protocol.TextEdit{
Range: replaceRange,
NewText: "",
}),
),
},
},TextEditsToDocumentChanges(fh.URI(), fh.Version(), edits...)
TextDocumentEdit: &protocol.TextDocumentEdit{TextEditsToDocumentChanges(createFileURI, 0, edits...)
if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) {Code in package golang should not access the file system directly; instead ask the snapshot for the content of the file. This ensures (a) that the analysis operates on a consistent view even if the file system is changing underfoot, and (b) that the dependency analysis knows which files were relevant.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {Take a look at PathEnclosingInterval(file, start, end). I think you just need to walk up to the enclosing Decl immediately before the File.
func compileTemplate(env *Env, text string, dest string) protocol.Location {Neat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
-- filealreadexists.go --existing.go
existing2.go
etc
Related details
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chiawen Chen (Gerrit)
Chiawen Chen added 16 comments![]( "Open in Gerrit")
Thanks for the thorough code review! I have pushed a new commit and added some comments.
Alan DonovanNeed to do this because the code action needs to update import declarations after moving codes, but allImportsFixes requires snapshot as an argument. So I first made code moving changes in a cloned snapshot, and then call allImportsFixes on the cloned snapshot.
This seems unnecessarily complex. The required import updates can be computed from the the moved code: (1) copy all the import declarations from the original file, then, (2) for both files (old and new), delete any import declarations that aren't needed.
To compute the necessary imports, iterate over types.Info.Uses map looking for (id, obj) pairs where obj is a PkgName in the old file. Divide them into two buckets based on whether the refactoring moved id or not. Then see which PkgNames in each bucket have no ids and delete its ImportSpec from the corresponding file.
Alternatively, you could omit step 2 and just wait for the user to save both files, triggering goimports.
Sure! I implemented your approach in the new commits. Omitting the import fixes is okay but not ideal because it leaves the files in warnings after taking the code action.
I am not sure whether I need to call defer cloned.decRef() here. The docs for Snapshot.clone say it must be called but doing so cause panics.
Done
moves, err := getMoveToNewFileCodeAction(pgf, rng, snapshot.Options())MoveToNewFile is an "extract" code action, so I think you should move it into getExtractCodeActions.
Done
cmd, err := command.NewMoveToANewFileCommand("m", command.MoveToANewFileArgs{URI: pgf.URI, Range: rng})"Extract declarations to new file"
(I don't think this name appear in the VS Code UI, but let's choose something meaningful. I does appear in the CLI I think.)
Done
"Extract declarations to new file"
Done
// apply edits into a cloned snapshot and calculate import editsI don't think you need any of this section (up to L153) if you compute the appropriate import edits as described in my other comment.
Done
Not sure what to put in the second context argument.
Done
return &protocol.WorkspaceEdit{In an ideal world the user's cursor would be moved to the new file---otherwise they may wonder what file name was created. That's possible to achieve, but a little tricky: rather than returning a WorkspaceEdit, we would have to make a Client.ApplyEdit downcall, followed by a ShowDocument downcall featuring the new URI. (The Client.ApplyEdit need contain only the file-creation edit; we could return a WorkspaceEdit for the actual changes to the text.)
In vscode the CreateFile operation will show the created file in an opened tab (not focused) and shown as unsaved, This should be enough for users to take notice about it. For other editors I have no idea.
// original file edits
TextDocumentEdit: &protocol.TextDocumentEdit{
TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{
TextDocumentIdentifier: protocol.TextDocumentIdentifier{
URI: fh.URI(),
},
Version: fh.Version(),
},
Edits: protocol.AsAnnotatedTextEdits(
append(
importEdits,
protocol.TextEdit{
Range: replaceRange,
NewText: "",
}),
),
},
},Chiawen ChenTextEditsToDocumentChanges(fh.URI(), fh.Version(), edits...)
Done
TextDocumentEdit: &protocol.TextDocumentEdit{Chiawen ChenTextEditsToDocumentChanges(createFileURI, 0, edits...)
Done
if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) {Code in package golang should not access the file system directly; instead ask the snapshot for the content of the file. This ensures (a) that the analysis operates on a consistent view even if the file system is changing underfoot, and (b) that the dependency analysis knows which files were relevant.
I met problem on checking the existence of a file using a snapshot. The closest solution I find is snapshot.ReadFile, but it succeeds even if the file does not exists, and the returned file handle does not seem to contain enough informations.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {Take a look at PathEnclosingInterval(file, start, end). I think you just need to walk up to the enclosing Decl immediately before the File.
Sorry but I am not sure what to proceed with this comment.
// for package renaming feature. At most one field of this struct is non-nil.I think making this into an interface with TextDocumentEdit, RenameFile, and CreateFile implementing it would make it easier to use.
Done
func compileTemplate(env *Env, text string, dest string) protocol.Location {Neat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
I tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
-- filealreadexists.go --Chiawen Chenexisting.go
existing2.go
etc
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. |
Gerrit Bot (Gerrit)
Gerrit Bot 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. |
Alan Donovan (Gerrit)
Alan Donovan added 39 comments![]( "Open in Gerrit")
Just another partial pass--lots of comments again, but it's moving in the right direction.
var actions []protocol.CodeAction
extractToNewFileActions, err := getExtractToNewFileCodeActions(pgf, rng, options)
if err != nil {
return nil, err
}
actions = append(actions, extractToNewFileActions...)
We might want to move this step after the Start==End check, so that you have to select some text to get the action. That seems a little more intuitive to me; it also reduces the amount of work in the common case of typing, when Start==End.
start, end, err := pgf.RangePos(rng)Pass start and end to getExtractToNewFileCommand, so it needn't call RangePos.
var commands []protocol.CommandAppend the new refactoring to this list of commands, so that the existing loop at L237 does its thing; then getExtractToNewFileCodeActions needn't return an action, just the command.
// todo: rename file to extract_to_new_file.go after code reviewYou can do it during the review. I suggest extracttofile.go.
func getExtractToNewFileCodeActions(pgf *parsego.File, rng protocol.Range, _ *settings.Options) ([]protocol.CodeAction, error) {Command
and return (cmd, err). The CodeAction can be created by the common logic in codeaction.go.
cmd, err := command.NewExtractToNewFileCommand(return
func findImportEdits(pkg *cache.Package, pgf *parsego.File, start, end token.Pos) (adds []*ast.ImportSpec, deletes []*ast.ImportSpec) {file *ast.File, info *types.Info
and call with (pgf.File, pkg.GetTypesInfo())
var (Each step of a complex algorithm should be documented:
// Record which PkgNames are referenced from within the
// extracted selection, and from outside of it. Those in the
// extracted selection must be imported in the new file.
// Those only in the extracted selection must be deleted
// from the original file.
type NamePath struct {Use uncapitalized names (x3).
Path stringpkg *types.Package
and use
typesutil.ImportedPkgName(info, spec) at L99, L101
and
pkgName.Imported() (without .Path()) at L106, 108.
path := strings.Trim(v.Path.Value, `"`)strconv.Unquote(spec.Path.Value)
or metadata.UnquoteImportPath(spec), and change the type of Path string to ImportPath.
if importSpec == nil {How can this happen? If it was foundInSelection, there must have been an ImportSpec, and PkgName and a Package.
start, end, err := pgf.RangePos(rng)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
start, end, filename, err := findRangeAndFilename(pgf, start, end)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
if dry {
return nil, nilThis is a false abstraction. The only real work in the "dry" case is findRangeAndFilename. So I suggest you call RangePos and findRangeAndFilename directly from codeaction.go.
end = skipWhiteSpaces(pgf, end)Is this necessary? I think it would be fine to replace the selected declarations by "\n" in the source file, and let the formatter tidy up the whitespace on the next save.
return nil, fmt.Errorf("%s: %w", errorPrefix, err)return nil, bug.Reportf("findRangeAndFilename returned invalid range: %v", err)
parenthesisFreeImports := findParenthesisFreeImports(pgf)I wonder whether this is necessary. If we delete the sole ImportSpec from an import GenDecl, won't the formatter eliminate the declaration entirely? Or does it replace it with import ()? Or "import \n", which doesn't compile?
In any case, this section needs more commentary.
createFileURI, err := resolveCreateFileURI(pgf.URI.Dir().Path(), filename)newFileURI
creatFileText, err := format.Source([]byte(newContent
creatFileText, err := format.Source([]byte(TODO: attempt to duplicate the copyright header, if any.
if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) {Chiawen ChenCode in package golang should not access the file system directly; instead ask the snapshot for the content of the file. This ensures (a) that the analysis operates on a consistent view even if the file system is changing underfoot, and (b) that the dependency analysis knows which files were relevant.
I met problem on checking the existence of a file using a snapshot. The closest solution I find is snapshot.ReadFile, but it succeeds even if the file does not exists, and the returned file handle does not seem to contain enough informations.
That's right, ReadFile always returns a handle. Check the error from Handle.Content() to test whether the file exists.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {Chiawen ChenTake a look at PathEnclosingInterval(file, start, end). I think you just need to walk up to the enclosing Decl immediately before the File.
Sorry but I am not sure what to proceed with this comment.
Never mind, I see now that it wouldn't help in this situation.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {// selectedToplevelDecls returns the lexical extent of the top-level
// declarations enclosing (start, end), along with the name of the
// first declaration. It returns zeros on error.
func selectedToplevelDecls
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {You don't need to return an error, since it is ignored in the dryrun mode, and it "can't happen" in the real mode. Just return (0, 0, "") on error, and document this.
if intersect(start, end, pgf.File.Package, pgf.File.Name.End()) {
return 0, 0, "", errors.New("selection cannot intersect a package declaration")
}This isn't necessary. If only package decl is selected, the loop below will be a no-op and the function will return an error.
I think it's fine to be permissive: the only declarations that matter are the ones that start after the imports. If stuff before that is selected, pretend it isn't.
if v, ok := node.(*ast.GenDecl); ok && v.Tok == token.IMPORT {Use a type switch.
if v, ok := node.(*ast.FuncDecl); ok && contain(v.Pos(), v.Name.End(), start, end) {I don't think it's necessary that the name be selected.
if ok {...}
if v, ok := node.(*ast.GenDecl); ok && (v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||Again I don't think the contains checks are necessary.
if v, ok := node.(*ast.GenDecl); ok && (v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||Simpler: Tok != types.IMPORT
Combine with the case for IMPORT:
case *ast.GenDecl:
if Tok == types.IMPORT { return ... }
...
if !contain(start, end, node.Pos(), node.End()) {Why not? If I select most of two adjacent functions, it's pretty clear what to do.
if c := getCommentGroup(node); c != nil {
if c.Pos() < start {&&
for _, node := range pgf.File.Comments {comment
if !contain(start, end, node.Pos(), node.End()) {
return 0, 0, "", errors.New("selection cannot partially intersect a comment")
}No need for an error; reduce start to comment.Pos, or increase end to comment.End.
if c == ' ' || c == '\t' || c == '\n' {
continue
} else {if !... { break }
There's no need for a continue and a break.
func getCommentGroup(node ast.Node) *ast.CommentGroup {Inline this function to its sole call.
func getNodeName(node ast.Node) string {Combine this with the main loop over decls, which already switches between decl types:
for ... decls {
var id *ast.Ident
switch ... {
case *ast.FuncDecl:
id = decl.Name
...
}
if id != nil && firstName != "" { firstName = id.Name }
} // ExtractToNewFile: Move selected codes to a new filedeclarations
Related details
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chiawen Chen (Gerrit)
Chiawen Chen added 34 comments![]( "Open in Gerrit")
var actions []protocol.CodeAction
extractToNewFileActions, err := getExtractToNewFileCodeActions(pgf, rng, options)
if err != nil {
return nil, err
}
actions = append(actions, extractToNewFileActions...)
We might want to move this step after the Start==End check, so that you have to select some text to get the action. That seems a little more intuitive to me; it also reduces the amount of work in the common case of typing, when Start==End.
This is handy for when a user place cursor on the function name (single click) to get code action.
Another code action that allows zero-width selection in gopls is inline call.
Pass start and end to getExtractToNewFileCommand, so it needn't call RangePos.
Done
Append the new refactoring to this list of commands, so that the existing loop at L237 does its thing; then getExtractToNewFileCodeActions needn't return an action, just the command.
Done
func getExtractToNewFileCodeActions(pgf *parsego.File, rng protocol.Range, _ *settings.Options) ([]protocol.CodeAction, error) {Command
and return (cmd, err). The CodeAction can be created by the common logic in codeaction.go.
Done
cmd, err := command.NewExtractToNewFileCommand(Chiawen Chenreturn
Done
func findImportEdits(pkg *cache.Package, pgf *parsego.File, start, end token.Pos) (adds []*ast.ImportSpec, deletes []*ast.ImportSpec) {file *ast.File, info *types.Info
and call with (pgf.File, pkg.GetTypesInfo())
Done
Each step of a complex algorithm should be documented:
// Record which PkgNames are referenced from within the
// extracted selection, and from outside of it. Those in the
// extracted selection must be imported in the new file.
// Those only in the extracted selection must be deleted
// from the original file.
Done
type NamePath struct {Chiawen ChenUse uncapitalized names (x3).
Done
Path stringpkg *types.Package
and use
typesutil.ImportedPkgName(info, spec) at L99, L101
and
pkgName.Imported() (without .Path()) at L106, 108.
I made a big rewrite of the function using typesutil.ImportedPkgName. I believe the rewrite is easier to follow.
Also I found that my implementation does not handle dot imports correctly. If the extracted section contains identifiers defined by a dot import, the dot import should be added to the new file.
I left the bug unfixed in the new commit.
for _, v := range pgf.File.Imports {Chiawen Chenspec
Done
strconv.Unquote(spec.Path.Value)
or metadata.UnquoteImportPath(spec), and change the type of Path string to ImportPath.
Done
if importSpec == nil {How can this happen? If it was foundInSelection, there must have been an ImportSpec, and PkgName and a Package.
There is a perfect reason this happens but it is moot after I rewrite function with typesutil.ImportedPkgName.
start, end, err := pgf.RangePos(rng)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
start, end, filename, err := findRangeAndFilename(pgf, start, end)
if err != nil {
return nil, fmt.Errorf("%s: %w", errorPrefix, err)
}
if dry {
return nil, nilThis is a false abstraction. The only real work in the "dry" case is findRangeAndFilename. So I suggest you call RangePos and findRangeAndFilename directly from codeaction.go.
Done
end = skipWhiteSpaces(pgf, end)Is this necessary? I think it would be fine to replace the selected declarations by "\n" in the source file, and let the formatter tidy up the whitespace on the next save.
This is not necessary but nice to have. Otherwise the code action outputs 3 consecutive empty lines in the most common case.
return nil, bug.Reportf("findRangeAndFilename returned invalid range: %v", err)
Done
parenthesisFreeImports := findParenthesisFreeImports(pgf)I wonder whether this is necessary. If we delete the sole ImportSpec from an import GenDecl, won't the formatter eliminate the declaration entirely? Or does it replace it with import ()? Or "import \n", which doesn't compile?
In any case, this section needs more commentary.
I have added the following comment:
```
// 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.
```
createFileURI, err := resolveCreateFileURI(pgf.URI.Dir().Path(), filename)Chiawen ChennewFileURI
Done
TODO: attempt to duplicate the copyright header, if any.
Done
creatFileText, err := format.Source([]byte(Chiawen ChennewContent
Done
if _, err := os.Stat(newPath); errors.Is(err, os.ErrNotExist) {Chiawen ChenCode in package golang should not access the file system directly; instead ask the snapshot for the content of the file. This ensures (a) that the analysis operates on a consistent view even if the file system is changing underfoot, and (b) that the dependency analysis knows which files were relevant.
Alan DonovanI met problem on checking the existence of a file using a snapshot. The closest solution I find is snapshot.ReadFile, but it succeeds even if the file does not exists, and the returned file handle does not seem to contain enough informations.
That's right, ReadFile always returns a handle. Check the error from Handle.Content() to test whether the file exists.
Done
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {// selectedToplevelDecls returns the lexical extent of the top-level
// declarations enclosing (start, end), along with the name of the
// first declaration. It returns zeros on error.
func selectedToplevelDecls
This description does not agree with what I have in my mind:
1. partial selection of a declaration should not be offered code action.
2. as an exception selecting only function name should be offered code action.
I modified the comment to this:
// selectedToplevelDecls returns the lexical extent of the top-level
// declarations enclosed by [start, end), along with the name of the
// first declaration. The returned boolean reports whether the selection
// should be offered code action.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {You don't need to return an error, since it is ignored in the dryrun mode, and it "can't happen" in the real mode. Just return (0, 0, "") on error, and document this.
I changed the error return value to an ok boolean.
if intersect(start, end, pgf.File.Package, pgf.File.Name.End()) {
return 0, 0, "", errors.New("selection cannot intersect a package declaration")
}This isn't necessary. If only package decl is selected, the loop below will be a no-op and the function will return an error.
I think it's fine to be permissive: the only declarations that matter are the ones that start after the imports. If stuff before that is selected, pretend it isn't.
I think the main concern of this comment is about the unnecessary use of an error. I changed the error return value to an ok boolean. If you are concerned about that the code action should be offered when package decl is selected, let me know.
for _, node := range pgf.File.Decls {Chiawen Chendecl
Done
if v, ok := node.(*ast.GenDecl); ok && v.Tok == token.IMPORT {Chiawen ChenUse a type switch.
Done
if v, ok := node.(*ast.FuncDecl); ok && contain(v.Pos(), v.Name.End(), start, end) {I don't think it's necessary that the name be selected.
if ok {...}
I think you read it in reverse, this reads as "function name contains the selection", not the other way.
This is useful when a user want to refactor out a function and double click select the function name.
if !contain(start, end, node.Pos(), node.End()) {Why not? If I select most of two adjacent functions, it's pretty clear what to do.
There are two reasons not to offer code action for partial selection:
1. There is an ambiguity for when there is a type declaration inside of a function body, and only that declaration is selected. A user might want to extract only that type declaration but instead we extract the ambient function.
Another ambiguity is when a var declaration declared several variables, but only one of them is selected, a user need to guess wether only the selected value specifier is extracted, or the whole variable declaration is extracted.
2. The user story is that a user want to refactor codes and select the code section intended to move. If a user only selected part of a function, the user often don't want this code action, then offering it is a noise.
if c := getCommentGroup(node); c != nil {
if c.Pos() < start {Chiawen Chen&&
Done
for _, node := range pgf.File.Comments {Chiawen Chencomment
Done
if !contain(start, end, node.Pos(), node.End()) {
return 0, 0, "", errors.New("selection cannot partially intersect a comment")
}No need for an error; reduce start to comment.Pos, or increase end to comment.End.
When a comment is partially selected, does the user want the comment extracted or not? If we choose to extract it, some users will be surprised, if we chose not to extract it, some others user will be surprised. Not offering code action in this case is a fine way to eliminate this ambiguity.
if c == ' ' || c == '\t' || c == '\n' {
continue
} else {if !... { break }
There's no need for a continue and a break.
Done
Inline this function to its sole call.
Done
Combine this with the main loop over decls, which already switches between decl types:
for ... decls {
var id *ast.Ident
switch ... {
case *ast.FuncDecl:
id = decl.Name
...
}
if id != nil && firstName != "" { firstName = id.Name }
}
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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 2 comments![]( "Open in Gerrit")
// ExtractToNewFile: Move selected codes to a new fileChiawen Chendeclarations
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. |
Gerrit Bot (Gerrit)
Gerrit Bot 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. |
Alan Donovan (Gerrit)
Alan Donovan added 37 comments![]( "Open in Gerrit")
Thanks Chiawen. Sorry for the long delay, and for all the comments, though most should be easy to deal with. The production code is close to ready; and the tests should be expressible in the marker framework very soon (see below).
Feel free to resolve the comments you think are resolved.
Alan DonovanNeed to do this because the code action needs to update import declarations after moving codes, but allImportsFixes requires snapshot as an argument. So I first made code moving changes in a cloned snapshot, and then call allImportsFixes on the cloned snapshot.
Chiawen ChenThis seems unnecessarily complex. The required import updates can be computed from the the moved code: (1) copy all the import declarations from the original file, then, (2) for both files (old and new), delete any import declarations that aren't needed.
To compute the necessary imports, iterate over types.Info.Uses map looking for (id, obj) pairs where obj is a PkgName in the old file. Divide them into two buckets based on whether the refactoring moved id or not. Then see which PkgNames in each bucket have no ids and delete its ImportSpec from the corresponding file.
Alternatively, you could omit step 2 and just wait for the user to save both files, triggering goimports.
Sure! I implemented your approach in the new commits. Omitting the import fixes is okay but not ideal because it leaves the files in warnings after taking the code action.
Thanks.
var actions []protocol.CodeAction
extractToNewFileActions, err := getExtractToNewFileCodeActions(pgf, rng, options)
if err != nil {
return nil, err
}
actions = append(actions, extractToNewFileActions...)
Chiawen ChenWe might want to move this step after the Start==End check, so that you have to select some text to get the action. That seems a little more intuitive to me; it also reduces the amount of work in the common case of typing, when Start==End.
This is handy for when a user place cursor on the function name (single click) to get code action.
Another code action that allows zero-width selection in gopls is inline call.
OK, having played with the feature it seems reasonable.
// TODO: handle dot importsWe should at least detect dot imports and abort the operation for now; we (or you if you're willing) can add support for them later.
func ExtractToNewFile(
ctx context.Context,
snapshot *cache.Snapshot,
fh file.Handle,
rng protocol.Range,Join lines.
start, end, filename, ok := selectedToplevelDecls(pgf, start, end)firstSymbol
adds, deletes := findImportEdits(pgf.File, pkg.GetTypesInfo(), start, end)(The "Get" prefix has since been removed.)
importAdds := ""Let's use a bytes.Buffer here, and start by writing
fmt.Fprintf(&buf, "package %s\n", pgf.File.Name.Name)
followed by the imports. Then:
newFileContent, err := format.Source(buf.Bytes())
return &protocol.WorkspaceEdit{See CL 585277, which should make this slightly easier for you. This should simplify to:
```
edits := append(edits, protocol.TextEdit{Range: replaceRange, NewText: ""})
return NewWorkspaceEdit(
protocol.DocumentChangeEdit(fh, edits)
protocol.DocumentChangeCreate(newFileURI),
protocol.DocumentChangeEdit(newFileURI, []protocol.TextEdit{{NewText: string(newFileContent)}), nil
```
(You'll need to define protocol.DocumentChangeCreate.)
// resolveNewFileURI checks that basename.go does not exists in dir, otherwise
// select basename.{1,2,3,4,5}.go as filename."chooses a new filename in dir, based on the name of the first extracted symbol, and if necessary to disambiguate, a numeric suffix."
func resolveNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, basename string) (protocol.DocumentURI, error) {firstSymbol
func resolveNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, basename string) (protocol.DocumentURI, error) {chooseNewFileURI
basename = strings.ToLower(basename)basename := strings.ToLower(firstSymbol)
return "", nilreturn "", err // e.g. cancelled
breakChange this function to return the non-existent file.Handle on success, and return (fh, nil) here. It will make the calling code more convenient.
if count >= 5 {Change the loop to
for count := 1; count < 5; count++ {
and make the final statement return nil, fmt.Errorf("resolveNewFileURI: exceeded retry limit")// should be offered code action."offered a code action to extract the declarations"
if v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||len("type")
ditto below
i := posThis can be simplified to:
i := pgf.Tok.Offset(pos)
rest := pgf.Src[i:]
return pos + len(rest) - len(bytes.TrimLeft(rest, " \t\n"))
Then you don't need the skipWhiteSpaces function. (Inline the call.)
rng, _ := pgf.PosRange(node.Pos(), node.End())Don't discard the error here. Use bug.Report if it's supposedly impossible.
func contain(a, b, c, d token.Pos) bool {These verbs should be indicative not imperative, since they are predicates, not actions, so add an 's'. Let's also add a "posRange" prefix since their scope is very large:
posRangeContains
posRangeIntersects
// todo: rename file to extract_to_new_file.go after code reviewYou can do it during the review. I suggest extracttofile.go.
Acknowledged
Path stringChiawen Chenpkg *types.Package
and use
typesutil.ImportedPkgName(info, spec) at L99, L101
and
pkgName.Imported() (without .Path()) at L106, 108.
I made a big rewrite of the function using typesutil.ImportedPkgName. I believe the rewrite is easier to follow.
Also I found that my implementation does not handle dot imports correctly. If the extracted section contains identifiers defined by a dot import, the dot import should be added to the new file.
I left the bug unfixed in the new commit.
That's fine; just make sure we report an error.
The code is much clearer.
if importSpec == nil {Chiawen ChenHow can this happen? If it was foundInSelection, there must have been an ImportSpec, and PkgName and a Package.
There is a perfect reason this happens but it is moot after I rewrite function with typesutil.ImportedPkgName.
Acknowledged
end = skipWhiteSpaces(pgf, end)Chiawen ChenIs this necessary? I think it would be fine to replace the selected declarations by "\n" in the source file, and let the formatter tidy up the whitespace on the next save.
This is not necessary but nice to have. Otherwise the code action outputs 3 consecutive empty lines in the most common case.
Ah, because we don't reformat until save. That makes sense. Thanks.
return &protocol.WorkspaceEdit{Chiawen ChenIn an ideal world the user's cursor would be moved to the new file---otherwise they may wonder what file name was created. That's possible to achieve, but a little tricky: rather than returning a WorkspaceEdit, we would have to make a Client.ApplyEdit downcall, followed by a ShowDocument downcall featuring the new URI. (The Client.ApplyEdit need contain only the file-creation edit; we could return a WorkspaceEdit for the actual changes to the text.)
In vscode the CreateFile operation will show the created file in an opened tab (not focused) and shown as unsaved, This should be enough for users to take notice about it. For other editors I have no idea.
OK, that sounds sufficient.
parenthesisFreeImports := findParenthesisFreeImports(pgf)Chiawen ChenI wonder whether this is necessary. If we delete the sole ImportSpec from an import GenDecl, won't the formatter eliminate the declaration entirely? Or does it replace it with import ()? Or "import \n", which doesn't compile?
In any case, this section needs more commentary.
I have added the following comment:
```
// 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.
```
Very nice.
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {Chiawen Chen// selectedToplevelDecls returns the lexical extent of the top-level
// declarations enclosing (start, end), along with the name of the
// first declaration. It returns zeros on error.
func selectedToplevelDecls
This description does not agree with what I have in my mind:
1. partial selection of a declaration should not be offered code action.
2. as an exception selecting only function name should be offered code action.I modified the comment to this:
// selectedToplevelDecls returns the lexical extent of the top-level
// declarations enclosed by [start, end), along with the name of the
// first declaration. The returned boolean reports whether the selection
// should be offered code action.
Acknowledged
func findRangeAndFilename(pgf *parsego.File, start, end token.Pos) (token.Pos, token.Pos, string, error) {Chiawen ChenYou don't need to return an error, since it is ignored in the dryrun mode, and it "can't happen" in the real mode. Just return (0, 0, "") on error, and document this.
I changed the error return value to an ok boolean.
Acknowledged
if intersect(start, end, pgf.File.Package, pgf.File.Name.End()) {
return 0, 0, "", errors.New("selection cannot intersect a package declaration")
}Chiawen ChenThis isn't necessary. If only package decl is selected, the loop below will be a no-op and the function will return an error.
I think it's fine to be permissive: the only declarations that matter are the ones that start after the imports. If stuff before that is selected, pretend it isn't.
I think the main concern of this comment is about the unnecessary use of an error. I changed the error return value to an ok boolean. If you are concerned about that the code action should be offered when package decl is selected, let me know.
Acknowledged
if v, ok := node.(*ast.FuncDecl); ok && contain(v.Pos(), v.Name.End(), start, end) {Chiawen ChenI don't think it's necessary that the name be selected.
if ok {...}
I think you read it in reverse, this reads as "function name contains the selection", not the other way.
This is useful when a user want to refactor out a function and double click select the function name.
Indeed. Thanks for clarifying.
if !contain(start, end, node.Pos(), node.End()) {Chiawen ChenWhy not? If I select most of two adjacent functions, it's pretty clear what to do.
There are two reasons not to offer code action for partial selection:
1. There is an ambiguity for when there is a type declaration inside of a function body, and only that declaration is selected. A user might want to extract only that type declaration but instead we extract the ambient function.
Another ambiguity is when a var declaration declared several variables, but only one of them is selected, a user need to guess wether only the selected value specifier is extracted, or the whole variable declaration is extracted.2. The user story is that a user want to refactor codes and select the code section intended to move. If a user only selected part of a function, the user often don't want this code action, then offering it is a noise.
This is really useful information for gopls' documentation (which is rather scant right now but I am working on adding a more complete manual).
In the meantime, would you mind adding a release note to gopls/doc/release/v0.16.0.md that explains how to use the feature, and any other nuggets of design information (such as your explanation above) that might help users understand? The release note can just be in note form; I'll edit them later.
if !contain(start, end, node.Pos(), node.End()) {
return 0, 0, "", errors.New("selection cannot partially intersect a comment")
}Chiawen ChenNo need for an error; reduce start to comment.Pos, or increase end to comment.End.
When a comment is partially selected, does the user want the comment extracted or not? If we choose to extract it, some users will be surprised, if we chose not to extract it, some others user will be surprised. Not offering code action in this case is a fine way to eliminate this ambiguity.
I see. This is good information for the release note I just mentioned.
type DocumentChanges struct {I've extended this to support CreateFile and also DeleteFile in https://go.dev/cl/585277; the change should make it easy for you to use the marker test for create/delete/rename as well. I suggest you rebase once that CL lands.
return nilWe should probably check the Applied flag returned by the client:
```
resp, err := ApplyEdit(...)
...
if !resp.Applied {
return fmt.Errorf("edits not applied: %s", response.FailureReason)
}
```
if change.CreateFile != nil {func compileTemplate(env *Env, text string, dest string) protocol.Location {Chiawen ChenNeat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
I tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
Got it. I wrote https://go.dev/cl/585277 today which should make it easy to use the marker tests (though I haven't actually tested it). Once that CL lands, why not rebase and try it out?
Related details
- Chiawen Chen
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 1 comment![]( "Open in Gerrit")
func compileTemplate(env *Env, text string, dest string) protocol.Location {Chiawen ChenNeat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
Alan DonovanI tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
Got it. I wrote https://go.dev/cl/585277 today which should make it easy to use the marker tests (though I haven't actually tested it). Once that CL lands, why not rebase and try it out?
CL 585277 has landed, so you can rebase this CL on top of it. I hope it was helpful.
Related details
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chiawen Chen (Gerrit)
Chiawen Chen added 29 comments![]( "Open in Gerrit")
Thank you for the review! I have changed the tests to marker tests.
We should at least detect dot imports and abort the operation for now; we (or you if you're willing) can add support for them later.
I have added the abort handling.
func ExtractToNewFile(
ctx context.Context,
snapshot *cache.Snapshot,
fh file.Handle,
rng protocol.Range,Chiawen ChenJoin lines.
Done
start, end, filename, ok := selectedToplevelDecls(pgf, start, end)Chiawen ChenfirstSymbol
Done
adds, deletes := findImportEdits(pgf.File, pkg.GetTypesInfo(), start, end)(The "Get" prefix has since been removed.)
Done
Let's use a bytes.Buffer here, and start by writing
fmt.Fprintf(&buf, "package %s\n", pgf.File.Name.Name)
followed by the imports. Then:newFileContent, err := format.Source(buf.Bytes())
Done
return &protocol.WorkspaceEdit{See CL 585277, which should make this slightly easier for you. This should simplify to:
```
edits := append(edits, protocol.TextEdit{Range: replaceRange, NewText: ""})
return NewWorkspaceEdit(
protocol.DocumentChangeEdit(fh, edits)
protocol.DocumentChangeCreate(newFileURI),
protocol.DocumentChangeEdit(newFileURI, []protocol.TextEdit{{NewText: string(newFileContent)}), nil
```(You'll need to define protocol.DocumentChangeCreate.)
Done
// resolveNewFileURI checks that basename.go does not exists in dir, otherwise
// select basename.{1,2,3,4,5}.go as filename."chooses a new filename in dir, based on the name of the first extracted symbol, and if necessary to disambiguate, a numeric suffix."
Done
func resolveNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, basename string) (protocol.DocumentURI, error) {Chiawen ChenchooseNewFileURI
Done
func resolveNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, basename string) (protocol.DocumentURI, error) {Chiawen ChenfirstSymbol
Done
basename = strings.ToLower(basename)Chiawen Chenbasename := strings.ToLower(firstSymbol)
Done
return "", err // e.g. cancelled
Done
Change this function to return the non-existent file.Handle on success, and return (fh, nil) here. It will make the calling code more convenient.
Done
Change the loop to
for count := 1; count < 5; count++ {
and make the final statementreturn nil, fmt.Errorf("resolveNewFileURI: exceeded retry limit")
Done
"offered a code action to extract the declarations"
Done
if v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||len("type")
ditto below
Done
This can be simplified to:
i := pgf.Tok.Offset(pos)
rest := pgf.Src[i:]
return pos + len(rest) - len(bytes.TrimLeft(rest, " \t\n"))Then you don't need the skipWhiteSpaces function. (Inline the call.)
Done
Don't discard the error here. Use bug.Report if it's supposedly impossible.
Done
These verbs should be indicative not imperative, since they are predicates, not actions, so add an 's'. Let's also add a "posRange" prefix since their scope is very large:
posRangeContains
posRangeIntersects
Done
if v, ok := node.(*ast.GenDecl); ok && (v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||Again I don't think the contains checks are necessary.
Acknowledged
if v, ok := node.(*ast.GenDecl); ok && (v.Tok == token.TYPE && contain(v.Pos(), v.Pos()+4, start, end) ||Simpler: Tok != types.IMPORT
Combine with the case for IMPORT:
case *ast.GenDecl:
if Tok == types.IMPORT { return ... }
...
Acknowledged
if !contain(start, end, node.Pos(), node.End()) {Chiawen ChenWhy not? If I select most of two adjacent functions, it's pretty clear what to do.
Alan DonovanThere are two reasons not to offer code action for partial selection:
1. There is an ambiguity for when there is a type declaration inside of a function body, and only that declaration is selected. A user might want to extract only that type declaration but instead we extract the ambient function.
Another ambiguity is when a var declaration declared several variables, but only one of them is selected, a user need to guess wether only the selected value specifier is extracted, or the whole variable declaration is extracted.2. The user story is that a user want to refactor codes and select the code section intended to move. If a user only selected part of a function, the user often don't want this code action, then offering it is a noise.
This is really useful information for gopls' documentation (which is rather scant right now but I am working on adding a more complete manual).
In the meantime, would you mind adding a release note to gopls/doc/release/v0.16.0.md that explains how to use the feature, and any other nuggets of design information (such as your explanation above) that might help users understand? The release note can just be in note form; I'll edit them later.
Done
if !contain(start, end, node.Pos(), node.End()) {
return 0, 0, "", errors.New("selection cannot partially intersect a comment")
}Chiawen ChenNo need for an error; reduce start to comment.Pos, or increase end to comment.End.
Alan DonovanWhen a comment is partially selected, does the user want the comment extracted or not? If we choose to extract it, some users will be surprised, if we chose not to extract it, some others user will be surprised. Not offering code action in this case is a fine way to eliminate this ambiguity.
I see. This is good information for the release note I just mentioned.
Done
I've extended this to support CreateFile and also DeleteFile in https://go.dev/cl/585277; the change should make it easy for you to use the marker test for create/delete/rename as well. I suggest you rebase once that CL lands.
Done
We should probably check the Applied flag returned by the client:
```
resp, err := ApplyEdit(...)
...
if !resp.Applied {
return fmt.Errorf("edits not applied: %s", response.FailureReason)
}
```
Done
if errors.Is(err, golang.ErrDotImport) {
showMessage(ctx, c.s.client, protocol.Info, err.Error())
return nil
}
return errNot sure if this is a correct implementation to abort the code action.
(This is also taken care of by https://go.dev/cl/585277.)
Done
func compileTemplate(env *Env, text string, dest string) protocol.Location {Chiawen ChenNeat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
Alan DonovanI tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
Alan DonovanGot it. I wrote https://go.dev/cl/585277 today which should make it easy to use the marker tests (though I haven't actually tested it). Once that CL lands, why not rebase and try it out?
CL 585277 has landed, so you can rebase this CL on top of it. I hope it was helpful.
I have changed the tests to marker tests. There are two problem I met:
1. How do I call `@codeactionedit` with a zero-width location? I need this to test when a user try to use the code action by just placing the cursor on a function name without selecting.
2. How do I test that for a given selection, the code action is not offered?
func (e *Env) FileContent(name string) string {
e.T.Helper()
text, ok := e.Editor.BufferText(name)
if ok {
return text
}
content, err := e.Sandbox.Workdir.ReadFile(name)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return ""
} else {
e.T.Fatal(err)
}
}
return string(content)I have to change this otherwise the marker test fails. Previously `FileContent` panics when the file does not exists, now it returns an empty string. The fail happens when `checkDiffs` calls `FileContent` on a newly created file created by code action, which does not exist on disk or buffer.
Involved functions are `codeActionEditMarker`, `checkDiffs`, and `FileContent`.
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 23 comments![]( "Open in Gerrit")
Tests look good; thanks for redoing them.
Lots of superficial comments, but this is functionally ready to go, I think.
which moves selected code sections to a newly created file within themoves the selected declarations
same package. The created filename is chosen as the first {function, type, file's name
const, var} name encountered. In addition, import declarations are added orImport...
same package. The created filename is chosen as the first {function, type,
const, var} name encountered. In addition, import declarations are added orbased on the first symbol
The user can invoke this code action by selecting a function name, the keywords"You can find this action by selecting one or more complete declarations, or just the `func`, `const`, `var`, or `type` token of any top-level declaration."
In order to avoid ambiguity and surprise about what to extract, some kindsI don't think you need to go into such detail; delete this paragraph.
func findImportEdits(file *ast.File, info *types.Info, start, end token.Pos) (adds []*ast.ImportSpec, deletes []*ast.ImportSpec, _ error) {Factor the types: adds, deletes *[]ast.ImportSpec
return nil, nil, ErrDotImport// TODO: support dot imports.
return nil, bug.Errorf("precondition unmet")"invalid selection"
return nil, bug.Errorf("findRangeAndFilename returned invalid range: %v", err)"invalid range: ..."
protocol.DocumentChangeEdit(&uriVersion{uri: newFile.URI(), version: 0}, []protocol.TextEdit{newFile
(then delete uriVersion)
// uriVersion is the simplest struct that implements protocol.fileHandle.Actually, the existing newFile variable is even simpler. :)
func chooseNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, firstSymbol string) (file.Handle, error) {chooseNewFile
firstName = id.Name// may be "_"
(that's ok, I think)
func findParenthesisFreeImports(pgf *parsego.File) map[*ast.ImportSpec]*ast.GenDecl {// unparenthesizedImports returns a map from each unparenthesized ImportSpec
// to its enclosing declaration (which may need to be deleted too).
func unparenthesizedImports
if g, ok := decl.(*ast.GenDecl); ok {Simplify:
if decl, ok := decl.(*ast.GenDecl); ok && decl.Tok == token.IMPORT && !decl.Lparen.IsValid() {
decls[decl.Specs[0]] = decl
} rng, err := pgf.PosRange(node.Pos(), node.End())NodeRange(node)
ExtractToNewFile(context.Context, ExtractToNewFileArgs) errorLocation
(ExtractToNewFileArgs = URI + Range = Location.)
if errors.Is(err, golang.ErrDotImport) {
showMessage(ctx, c.s.client, protocol.Info, err.Error())
return nil
}
return errNot sure if this is a correct implementation to abort the code action.
I think you can just return the error.
(This will cause one test to fail, so just remove that test.)
// editing session: if the file is open, it returns its buffer content,// it returns the buffer content for an open file, the
// on-disk content for an unopened file,
// or "" for a non-existent file.
func (e *Env) FileContent(name string) string {
e.T.Helper()
text, ok := e.Editor.BufferText(name)
if ok {
return text
}
content, err := e.Sandbox.Workdir.ReadFile(name)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return ""
} else {
e.T.Fatal(err)
}
}
return string(content)I have to change this otherwise the marker test fails. Previously `FileContent` panics when the file does not exists, now it returns an empty string. The fail happens when `checkDiffs` calls `FileContent` on a newly created file created by code action, which does not exist on disk or buffer.
Involved functions are `codeActionEditMarker`, `checkDiffs`, and `FileContent`.
I think this is fine; its sole use is from checkDiffs.
-- dot_import.go --
// This case is not yet implemented, it aborts and shows a message to the client.
package main
import . "fmt"
func F() { Println() } //@codeactionedit("func", "refactor.extract", dot_import)
Let's leave this out for now (you can make a note at the top to add it back later).
Related details
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chiawen Chen (Gerrit)
Chiawen Chen added 22 comments![]( "Open in Gerrit")
which moves selected code sections to a newly created file within theChiawen Chenmoves the selected declarations
Done
same package. The created filename is chosen as the first {function, type, Chiawen Chenfile's name
Done
same package. The created filename is chosen as the first {function, type,
const, var} name encountered. In addition, import declarations are added orbased on the first symbol
Done
const, var} name encountered. In addition, import declarations are added orChiawen ChenImport...
Done
The user can invoke this code action by selecting a function name, the keywords"You can find this action by selecting one or more complete declarations, or just the `func`, `const`, `var`, or `type` token of any top-level declaration."
Done
In order to avoid ambiguity and surprise about what to extract, some kindsI don't think you need to go into such detail; delete this paragraph.
Done
func findImportEdits(file *ast.File, info *types.Info, start, end token.Pos) (adds []*ast.ImportSpec, deletes []*ast.ImportSpec, _ error) {Factor the types: adds, deletes *[]ast.ImportSpec
Done
// TODO: support dot imports.
Done
return nil, bug.Errorf("precondition unmet")Chiawen Chen"invalid selection"
Done
return nil, bug.Errorf("findRangeAndFilename returned invalid range: %v", err)Chiawen Chen"invalid range: ..."
Done
protocol.DocumentChangeEdit(&uriVersion{uri: newFile.URI(), version: 0}, []protocol.TextEdit{Chiawen ChennewFile
(then delete uriVersion)
Done
// uriVersion is the simplest struct that implements protocol.fileHandle.Actually, the existing newFile variable is even simpler. :)
:)
func chooseNewFileURI(ctx context.Context, snapshot *cache.Snapshot, dir string, firstSymbol string) (file.Handle, error) {Chiawen ChenchooseNewFile
Done
// may be "_"
(that's ok, I think)
Done
func findParenthesisFreeImports(pgf *parsego.File) map[*ast.ImportSpec]*ast.GenDecl {// unparenthesizedImports returns a map from each unparenthesized ImportSpec
// to its enclosing declaration (which may need to be deleted too).
func unparenthesizedImports
Done
Simplify:
if decl, ok := decl.(*ast.GenDecl); ok && decl.Tok == token.IMPORT && !decl.Lparen.IsValid() {
decls[decl.Specs[0]] = decl
}
Done
rng, err := pgf.PosRange(node.Pos(), node.End())Chiawen ChenNodeRange(node)
Done
ExtractToNewFile(context.Context, ExtractToNewFileArgs) errorLocation
(ExtractToNewFileArgs = URI + Range = Location.)
Done
if errors.Is(err, golang.ErrDotImport) {
showMessage(ctx, c.s.client, protocol.Info, err.Error())
return nil
}
return errAlan DonovanNot sure if this is a correct implementation to abort the code action.
I think you can just return the error.
(This will cause one test to fail, so just remove that test.)
Done
// editing session: if the file is open, it returns its buffer content,// it returns the buffer content for an open file, the
// on-disk content for an unopened file,
// or "" for a non-existent file.
Done
-- dot_import.go --
// This case is not yet implemented, it aborts and shows a message to the client.
package main
import . "fmt"
func F() { Println() } //@codeactionedit("func", "refactor.extract", dot_import)
Let's leave this out for now (you can make a note at the top to add it back later).
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. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
func compileTemplate(env *Env, text string, dest string) protocol.Location {Chiawen ChenNeat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
Alan DonovanI tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
Alan DonovanGot it. I wrote https://go.dev/cl/585277 today which should make it easy to use the marker tests (though I haven't actually tested it). Once that CL lands, why not rebase and try it out?
Chiawen ChenCL 585277 has landed, so you can rebase this CL on top of it. I hope it was helpful.
I have changed the tests to marker tests. There are two problem I met:
1. How do I call `@codeactionedit` with a zero-width location? I need this to test when a user try to use the code action by just placing the cursor on a function name without selecting.
2. How do I test that for a given selection, the code action is not offered?
How do I call @codeactionedit with a zero-width location?
If the regular expression contains a submatch groups, the position of the group is what matters. So you can say "(a)b" to match the region of the letter "a" but only in a context preceded by "b". Thus: "()b" means the zero-width space before the b.
Related details
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Chiawen Chen
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chiawen Chen (Gerrit)
Chiawen Chen added 2 comments![]( "Open in Gerrit")
func compileTemplate(env *Env, text string, dest string) protocol.Location {Chiawen ChenNeat idea! But we usually write our tests in the marker framework, in the form of a standardized marker/testdata/*.txt file. It's possible to express a whole sequence of test cases in a single file; or you can create several files for different groups of tests if you prefer.
One benefit of the marker approach is that you can edit the input file and re-run the test without recompiling anything at all. Another is uniformity of the tests.
Have you tried expressing these tests in that form?
Alan DonovanI tried to write tests in the marker framework, but it does not work for now because the testing infrastructure is missing some implementations around file creation edits. Earlier I decided not to write tests as marker tests because I thought the marker framework cannot express testing of created files (content and filename), but now I think again it probably could.
Should this be a priority I can try to implement the missing testing infrastructure.
Alan DonovanGot it. I wrote https://go.dev/cl/585277 today which should make it easy to use the marker tests (though I haven't actually tested it). Once that CL lands, why not rebase and try it out?
Chiawen ChenCL 585277 has landed, so you can rebase this CL on top of it. I hope it was helpful.
Alan DonovanI have changed the tests to marker tests. There are two problem I met:
1. How do I call `@codeactionedit` with a zero-width location? I need this to test when a user try to use the code action by just placing the cursor on a function name without selecting.
2. How do I test that for a given selection, the code action is not offered?
How do I call @codeactionedit with a zero-width location?
If the regular expression contains a submatch groups, the position of the group is what matters. So you can say "(a)b" to match the region of the letter "a" but only in a context preceded by "b". Thus: "()b" means the zero-width space before the b.
Thanks I have added the tests.
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Commit-Queue | +1 |
1 comment
LGTM. Many thanks again.
Rob, could you +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. |
Alan Donovan (Gerrit)
Alan Donovan added 3 comments![]( "Open in Gerrit")
Two tests are failing but the fixes are simple; see below.
rest := pgf.Src[pgf.Tok.Offset(end):]This should use safetoken instead of calling token.File.Offset directly:
```
offset, err := safetoken.Offset(pgf.Tok, end)
if err != nil {
return nil, err
}
rest := pgf.Src[offset:]
```
ExtractToNewFile,Could you run (cd gopls && go generate ./...) so that the API and docs pick up the new command?
Related details
- 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. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- 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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 3 comments![]( "Open in Gerrit")
This should use safetoken instead of calling token.File.Offset directly:
```
offset, err := safetoken.Offset(pgf.Tok, end)
if err != nil {
return nil, err
}
rest := pgf.Src[offset:]
```
Done
Could you run (cd gopls && go generate ./...) so that the API and docs pick up the new command?
Done
Related details
- 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 voted Commit-Queue+1![]( "Open in Gerrit")
- 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 1 comment![]( "Open in Gerrit")
One final build failure, with go1.19:
internal/golang/extracttofile.go:155:35: pgf.File.FileStart undefined (type *ast.File has no field or method FileStart)
Since this CL isn't going to make it into gopls/v0.16.0, I suggest you just wait for a few days till we drop support for building gopls with go1.{19,20,22,22}, (about which we are unreasonably excited).
Alan Donovan (Gerrit)
Alan Donovan added 2 comments![]( "Open in Gerrit")
### Extract declarations to new fileSince the v0.16.0 release went out last week, this should go in a v0.17.0.md file (create it as needed).
buf.Write(pgf.Src[start-pgf.File.FileStart : end-pgf.File.FileStart])Rather than keep waiting for changes on our side, let's make this small patch and get this CL merged:
fileStart := pgf.Tok.Pos(0) // TODO(adonovan): use go1.20 pgf.File.FileStart
buf.Write(pgf.Src[start-fileStart : end-fileStart])
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. |
Gerrit Bot (Gerrit)
Gerrit Bot 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. |
Chiawen Chen (Gerrit)
Chiawen Chen added 2 comments![]( "Open in Gerrit")
Since the v0.16.0 release went out last week, this should go in a v0.17.0.md file (create it as needed).
Done
buf.Write(pgf.Src[start-pgf.File.FileStart : end-pgf.File.FileStart])Rather than keep waiting for changes on our side, let's make this small patch and get this CL merged:
fileStart := pgf.Tok.Pos(0) // TODO(adonovan): use go1.20 pgf.File.FileStart
buf.Write(pgf.Src[start-fileStart : end-fileStart])
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. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Auto-Submit | +1 |
| Code-Review | +2 |
| Commit-Queue | +1 |
1 comment
of paritial selection of a declration cannot invoke this code action.I'll fix these typos when I next edit the draft release notes. Let's get this CL in.
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 voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +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. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
gopls/internal: add code action "extract declarations to new file"
This code action moves selected code sections to a newly created file within the same package. The created filename is chosen as the first {function, type, const, var} name encountered. In addition, import declarations are added or removed as needed.
Fixes golang/go#65707
- M gopls/doc/commands.md
- M gopls/doc/release/v0.17.0.md
- M gopls/internal/doc/api.json
- M gopls/internal/golang/codeaction.go
- A gopls/internal/golang/extracttofile.go
- M gopls/internal/protocol/command/command_gen.go
- M gopls/internal/protocol/command/interface.go
- M gopls/internal/protocol/edits.go
- M gopls/internal/server/command.go
- M gopls/internal/test/integration/wrappers.go
- A gopls/internal/test/marker/testdata/codeaction/extracttofile.txt
Code-Review: +2 by Alan Donovan, +1 by Robert Findley
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |