From 706a59cbbbf292be8ba72275a698abbc98bdcd9a Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 5 Feb 2021 22:18:47 -0500 Subject: [PATCH] internal/lsp: apply go.mod/sum changes via workspace edits We currently write directly to go.mod/sum via the go command, expecting that editors will pick up the changes. While that's true for VS Code, vim doesn't necessarily reload unchanged buffers. Change to send explicit edits instead, but only if the file is open. Behavior when using Go versions that don't support -modfile is unchanged. Fixes golang/go#44035. Change-Id: Ie4e5cf60673b860f5dfcbdb726bee0efe6aae405 Reviewed-on: https://go-review.googlesource.com/c/tools/+/290189 Trust: Heschi Kreinick Reviewed-by: Rebecca Stambler Reviewed-by: Robert Findley --- .../regtest/diagnostics/diagnostics_test.go | 3 +- gopls/internal/regtest/misc/vendor_test.go | 17 +- .../internal/regtest/modfile/modfile_test.go | 11 +- internal/lsp/cache/snapshot.go | 41 ++++- internal/lsp/command.go | 148 ++++++++++++++---- internal/lsp/fake/client.go | 14 +- internal/lsp/source/view.go | 6 +- 7 files changed, 186 insertions(+), 54 deletions(-) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 542991a2e2..3a731527ad 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -692,7 +692,7 @@ func main() { env.Await(EmptyDiagnostics("main.go")) env.Await( OnceMet( - env.DiagnosticAtRegexp("go.mod", "require github.com/ardanlabs/conf"), + env.DiagnosticAtRegexpWithMessage("go.mod", "require github.com/ardanlabs/conf", "not used in this module"), ReadDiagnostics("go.mod", &d), ), ) @@ -741,7 +741,6 @@ func main() { ), ) env.ApplyQuickFixes("main.go", d.Diagnostics) - env.CheckForFileChanges() env.Await( EmptyDiagnostics("main.go"), ) diff --git a/gopls/internal/regtest/misc/vendor_test.go b/gopls/internal/regtest/misc/vendor_test.go index 8e76dd82ca..1c9a4eca1f 100644 --- a/gopls/internal/regtest/misc/vendor_test.go +++ b/gopls/internal/regtest/misc/vendor_test.go @@ -48,31 +48,22 @@ func _() { var q int // hardcode a diagnostic } ` - // TODO(rstambler): Remove this when golang/go#41819 is resolved. WithOptions( Modes(Singleton), ProxyFiles(basicProxy), ).Run(t, pkgThatUsesVendoring, func(t *testing.T, env *Env) { env.OpenFile("a/a1.go") + d := &protocol.PublishDiagnosticsParams{} env.Await( - // The editor should pop up a message suggesting that the user - // run `go mod vendor`, along with a button to do so. - // By default, the fake editor always accepts such suggestions, - // so once we see the request, we can assume that `go mod vendor` - // will be executed. OnceMet( - env.DoneWithOpen(), - env.DiagnosticAtRegexp("go.mod", "module mod.com"), + env.DiagnosticAtRegexpWithMessage("go.mod", "module mod.com", "Inconsistent vendoring"), + ReadDiagnostics("go.mod", d), ), ) - // Apply the quickfix associated with the diagnostic. - d := &protocol.PublishDiagnosticsParams{} - env.Await(ReadDiagnostics("go.mod", d)) env.ApplyQuickFixes("go.mod", d.Diagnostics) - // Confirm that there is no longer any inconsistent vendoring. env.Await( - DiagnosticAt("a/a1.go", 6, 5), + env.DiagnosticAtRegexpWithMessage("a/a1.go", `q int`, "not used"), ) }) } diff --git a/gopls/internal/regtest/modfile/modfile_test.go b/gopls/internal/regtest/modfile/modfile_test.go index 8c167cc2c5..0f897e772c 100644 --- a/gopls/internal/regtest/modfile/modfile_test.go +++ b/gopls/internal/regtest/modfile/modfile_test.go @@ -323,7 +323,7 @@ go 1.14 ), ) env.ApplyQuickFixes("a/go.mod", d.Diagnostics) - if got := env.ReadWorkspaceFile("a/go.mod"); got != want { + if got := env.Editor.BufferText("a/go.mod"); got != want { t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(t, want, got)) } }) @@ -502,6 +502,7 @@ require ( example.com/blah/v2 v2.0.0 ) ` + env.SaveBuffer("a/go.mod") env.Await(EmptyDiagnostics("a/main.go")) if got := env.Editor.BufferText("a/go.mod"); got != want { t.Fatalf("suggested fixes failed:\n%s", tests.Diff(t, want, got)) @@ -542,7 +543,7 @@ func main() { env.DiagnosticAtRegexp("a/go.mod", "example.com v1.2.2"), ) env.RegexpReplace("a/go.mod", "v1.2.2", "v1.2.3") - env.Editor.SaveBuffer(env.Ctx, "a/go.mod") // go.mod changes must be on disk + env.SaveBuffer("a/go.mod") // Save to trigger diagnostics. d := protocol.PublishDiagnosticsParams{} env.Await( @@ -552,7 +553,7 @@ func main() { ), ) env.ApplyQuickFixes("a/go.mod", d.Diagnostics) - + env.SaveBuffer("a/go.mod") // Save to trigger diagnostics. env.Await( EmptyDiagnostics("a/go.mod"), env.DiagnosticAtRegexp("a/main.go", "x = "), @@ -826,6 +827,7 @@ func main() { ), ) env.ApplyQuickFixes("go.mod", d.Diagnostics) + env.SaveBuffer("go.mod") // Save to trigger diagnostics. env.Await( EmptyDiagnostics("go.mod"), ) @@ -925,6 +927,7 @@ func main() {} WithOptions( ProxyFiles(proxy), ).Run(t, mod, func(t *testing.T, env *Env) { + env.OpenFile("go.mod") d := &protocol.PublishDiagnosticsParams{} env.Await( OnceMet( @@ -937,7 +940,7 @@ func main() {} go 1.12 ` env.ApplyQuickFixes("go.mod", d.Diagnostics) - if got := env.ReadWorkspaceFile("go.mod"); got != want { + if got := env.Editor.BufferText("go.mod"); got != want { t.Fatalf("unexpected content in go.mod:\n%s", tests.Diff(t, want, got)) } }) diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 2c90492c0a..3bc9be3de7 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -12,6 +12,7 @@ import ( "go/token" "go/types" "io" + "io/ioutil" "os" "path/filepath" "sort" @@ -255,6 +256,44 @@ func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.Invocation return s.view.session.gocmdRunner.RunPiped(ctx, *inv, stdout, stderr) } +func (s *snapshot) RunGoCommands(ctx context.Context, allowNetwork bool, wd string, run func(invoke func(...string) (*bytes.Buffer, error)) error) (bool, []byte, []byte, error) { + var flags source.InvocationFlags + if s.workspaceMode()&tempModfile != 0 { + flags = source.WriteTemporaryModFile + } else { + flags = source.Normal + } + if allowNetwork { + flags |= source.AllowNetwork + } + tmpURI, inv, cleanup, err := s.goCommandInvocation(ctx, flags, &gocommand.Invocation{WorkingDir: wd}) + if err != nil { + return false, nil, nil, err + } + defer cleanup() + invoke := func(args ...string) (*bytes.Buffer, error) { + inv.Verb = args[0] + inv.Args = args[1:] + return s.view.session.gocmdRunner.Run(ctx, *inv) + } + if err := run(invoke); err != nil { + return false, nil, nil, err + } + if flags.Mode() != source.WriteTemporaryModFile { + return false, nil, nil, nil + } + var modBytes, sumBytes []byte + modBytes, err = ioutil.ReadFile(tmpURI.Filename()) + if err != nil && !os.IsNotExist(err) { + return false, nil, nil, err + } + sumBytes, err = ioutil.ReadFile(strings.TrimSuffix(tmpURI.Filename(), ".mod") + ".sum") + if err != nil && !os.IsNotExist(err) { + return false, nil, nil, err + } + return true, modBytes, sumBytes, nil +} + func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.InvocationFlags, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { s.view.optionsMu.Lock() allowModfileModificationOption := s.view.options.AllowModfileModifications @@ -1386,8 +1425,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } } if isGoMod(uri) { - // If the view's go.mod file's contents have changed, invalidate - // the metadata for every known package in the snapshot. delete(result.parseModHandles, uri) } // Handle the invalidated file; it may have new contents or not exist. diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 469ef925d5..8bbb684451 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "io/ioutil" + "os" "path/filepath" "strings" @@ -174,20 +175,19 @@ func (c *commandHandler) UpgradeDependency(ctx context.Context, args command.Dep func (c *commandHandler) GoGetModule(ctx context.Context, args command.DependencyArgs) error { return c.run(ctx, commandConfig{ - requireSave: true, - progress: "Running go get", - forURI: args.URI, + progress: "Running go get", + forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - return runGoGetModule(ctx, deps.snapshot, args.URI.SpanURI(), args.AddRequire, args.GoCmdArgs) + return c.s.runGoModUpdateCommands(ctx, deps.snapshot, args.URI.SpanURI(), func(invoke func(...string) (*bytes.Buffer, error)) error { + return runGoGetModule(invoke, args.AddRequire, args.GoCmdArgs) + }) }) } // TODO(rFindley): UpdateGoSum, Tidy, and Vendor could probably all be one command. - func (c *commandHandler) UpdateGoSum(ctx context.Context, args command.URIArgs) error { return c.run(ctx, commandConfig{ - requireSave: true, - progress: "Updating go.sum", + progress: "Updating go.sum", }, func(ctx context.Context, deps commandDeps) error { for _, uri := range args.URIs { snapshot, fh, ok, release, err := c.s.beginFileRequest(ctx, uri, source.UnknownKind) @@ -195,7 +195,10 @@ func (c *commandHandler) UpdateGoSum(ctx context.Context, args command.URIArgs) if !ok { return err } - if err := runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, fh.URI(), "list", []string{"all"}); err != nil { + if err := c.s.runGoModUpdateCommands(ctx, snapshot, fh.URI(), func(invoke func(...string) (*bytes.Buffer, error)) error { + _, err := invoke("list", "all") + return err + }); err != nil { return err } } @@ -214,7 +217,10 @@ func (c *commandHandler) Tidy(ctx context.Context, args command.URIArgs) error { if !ok { return err } - if err := runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, fh.URI(), "mod", []string{"tidy"}); err != nil { + if err := c.s.runGoModUpdateCommands(ctx, snapshot, fh.URI(), func(invoke func(...string) (*bytes.Buffer, error)) error { + _, err := invoke("mod", "tidy") + return err + }); err != nil { return err } } @@ -228,15 +234,19 @@ func (c *commandHandler) Vendor(ctx context.Context, args command.URIArg) error progress: "Running go mod vendor", forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - return runSimpleGoCommand(ctx, deps.snapshot, source.UpdateUserModFile|source.AllowNetwork, args.URI.SpanURI(), "mod", []string{"vendor"}) + _, err := deps.snapshot.RunGoCommandDirect(ctx, source.Normal|source.AllowNetwork, &gocommand.Invocation{ + Verb: "mod", + Args: []string{"vendor"}, + WorkingDir: filepath.Dir(args.URI.SpanURI().Filename()), + }) + return err }) } func (c *commandHandler) RemoveDependency(ctx context.Context, args command.RemoveDependencyArgs) error { return c.run(ctx, commandConfig{ - requireSave: true, - progress: "Removing dependency", - forURI: args.URI, + progress: "Removing dependency", + forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { // If the module is tidied apart from the one unused diagnostic, we can // run `go get module@none`, and then run `go mod tidy`. Otherwise, we @@ -244,10 +254,13 @@ func (c *commandHandler) RemoveDependency(ctx context.Context, args command.Remo // TODO(rstambler): In Go 1.17+, we will be able to use the go command // without checking if the module is tidy. if args.OnlyDiagnostic { - if err := runGoGetModule(ctx, deps.snapshot, args.URI.SpanURI(), false, []string{args.ModulePath + "@none"}); err != nil { + return c.s.runGoModUpdateCommands(ctx, deps.snapshot, args.URI.SpanURI(), func(invoke func(...string) (*bytes.Buffer, error)) error { + if err := runGoGetModule(invoke, false, []string{args.ModulePath + "@none"}); err != nil { + return err + } + _, err := invoke("mod", "tidy") return err - } - return runSimpleGoCommand(ctx, deps.snapshot, source.UpdateUserModFile|source.AllowNetwork, args.URI.SpanURI(), "mod", []string{"tidy"}) + }) } pm, err := deps.snapshot.ParseMod(ctx, deps.fh) if err != nil { @@ -444,39 +457,114 @@ func (c *commandHandler) GoGetPackage(ctx context.Context, args command.GoGetPac forURI: args.URI, progress: "Running go get", }, func(ctx context.Context, deps commandDeps) error { - uri := args.URI.SpanURI() + // Run on a throwaway go.mod, otherwise it'll write to the real one. stdout, err := deps.snapshot.RunGoCommandDirect(ctx, source.WriteTemporaryModFile|source.AllowNetwork, &gocommand.Invocation{ Verb: "list", Args: []string{"-f", "{{.Module.Path}}@{{.Module.Version}}", args.Pkg}, - WorkingDir: filepath.Dir(uri.Filename()), + WorkingDir: filepath.Dir(args.URI.SpanURI().Filename()), }) if err != nil { return err } ver := strings.TrimSpace(stdout.String()) - return runGoGetModule(ctx, deps.snapshot, uri, args.AddRequire, []string{ver}) + return c.s.runGoModUpdateCommands(ctx, deps.snapshot, args.URI.SpanURI(), func(invoke func(...string) (*bytes.Buffer, error)) error { + return runGoGetModule(invoke, args.AddRequire, []string{ver}) + }) }) } -func runGoGetModule(ctx context.Context, snapshot source.Snapshot, uri span.URI, addRequire bool, args []string) error { +func (s *Server) runGoModUpdateCommands(ctx context.Context, snapshot source.Snapshot, uri span.URI, run func(invoke func(...string) (*bytes.Buffer, error)) error) error { + tmpModfile, newModBytes, newSumBytes, err := snapshot.RunGoCommands(ctx, true, filepath.Dir(uri.Filename()), run) + if err != nil { + return err + } + if !tmpModfile { + return nil + } + modURI := snapshot.GoModForFile(uri) + sumURI := span.URIFromPath(strings.TrimSuffix(modURI.Filename(), ".mod") + ".sum") + modEdits, err := applyFileEdits(ctx, snapshot, modURI, newModBytes) + if err != nil { + return err + } + sumEdits, err := applyFileEdits(ctx, snapshot, sumURI, newSumBytes) + if err != nil { + return err + } + changes := append(sumEdits, modEdits...) + if len(changes) == 0 { + return nil + } + response, err := s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{ + Edit: protocol.WorkspaceEdit{ + DocumentChanges: changes, + }, + }) + if err != nil { + return err + } + if !response.Applied { + return fmt.Errorf("edits not applied because of %s", response.FailureReason) + } + return nil +} + +func applyFileEdits(ctx context.Context, snapshot source.Snapshot, uri span.URI, newContent []byte) ([]protocol.TextDocumentEdit, error) { + fh, err := snapshot.GetVersionedFile(ctx, uri) + if err != nil { + return nil, err + } + oldContent, err := fh.Read() + if err != nil && !os.IsNotExist(err) { + return nil, err + } + if bytes.Equal(oldContent, newContent) { + return nil, nil + } + + // Sending a workspace edit to a closed file causes VS Code to open the + // file and leave it unsaved. We would rather apply the changes directly, + // especially to go.sum, which should be mostly invisible to the user. + if !snapshot.IsOpen(uri) { + err := ioutil.WriteFile(uri.Filename(), newContent, 0666) + return nil, err + } + + m := &protocol.ColumnMapper{ + URI: fh.URI(), + Converter: span.NewContentConverter(fh.URI().Filename(), oldContent), + Content: oldContent, + } + diff, err := snapshot.View().Options().ComputeEdits(uri, string(oldContent), string(newContent)) + if err != nil { + return nil, err + } + edits, err := source.ToProtocolEdits(m, diff) + if err != nil { + return nil, err + } + return []protocol.TextDocumentEdit{{ + TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ + Version: fh.Version(), + TextDocumentIdentifier: protocol.TextDocumentIdentifier{ + URI: protocol.URIFromSpanURI(uri), + }, + }, + Edits: edits, + }}, nil +} + +func runGoGetModule(invoke func(...string) (*bytes.Buffer, error), addRequire bool, args []string) error { if addRequire { // Using go get to create a new dependency results in an // `// indirect` comment we may not want. The only way to avoid it // is to add the require as direct first. Then we can use go get to // update go.sum and tidy up. - if err := runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile, uri, "mod", append([]string{"edit", "-require"}, args...)); err != nil { + if _, err := invoke(append([]string{"mod", "edit", "-require"}, args...)...); err != nil { return err } } - return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, uri, "get", append([]string{"-d"}, args...)) -} - -func runSimpleGoCommand(ctx context.Context, snapshot source.Snapshot, mode source.InvocationFlags, uri span.URI, verb string, args []string) error { - _, err := snapshot.RunGoCommandDirect(ctx, mode, &gocommand.Invocation{ - Verb: verb, - Args: args, - WorkingDir: filepath.Dir(uri.Filename()), - }) + _, err := invoke(append([]string{"get", "-d"}, args...)...) return err } diff --git a/internal/lsp/fake/client.go b/internal/lsp/fake/client.go index 21336216e1..acc4ea5d00 100644 --- a/internal/lsp/fake/client.go +++ b/internal/lsp/fake/client.go @@ -7,6 +7,7 @@ package fake import ( "context" "fmt" + "os" "golang.org/x/tools/internal/lsp/protocol" ) @@ -110,8 +111,7 @@ func (c *Client) WorkDoneProgressCreate(ctx context.Context, params *protocol.Wo return nil } -// ApplyEdit applies edits sent from the server. Note that as of writing gopls -// doesn't use this feature, so it is untested. +// ApplyEdit applies edits sent from the server. func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceEditParams) (*protocol.ApplyWorkspaceEditResponse, error) { if len(params.Edit.Changes) != 0 { return &protocol.ApplyWorkspaceEditResponse{FailureReason: "Edit.Changes is unsupported"}, nil @@ -119,6 +119,16 @@ func (c *Client) ApplyEdit(ctx context.Context, params *protocol.ApplyWorkspaceE for _, change := range params.Edit.DocumentChanges { path := c.editor.sandbox.Workdir.URIToPath(change.TextDocument.URI) edits := convertEdits(change.Edits) + if !c.editor.HasBuffer(path) { + err := c.editor.OpenFile(ctx, path) + if os.IsNotExist(err) { + c.editor.CreateBuffer(ctx, path, "") + err = nil + } + if err != nil { + return nil, err + } + } if err := c.editor.EditBuffer(ctx, path, edits); err != nil { return nil, err } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 964d631140..5c96908ecd 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -94,6 +94,10 @@ type Snapshot interface { // WorkingDir must be specified. RunGoCommandDirect(ctx context.Context, mode InvocationFlags, inv *gocommand.Invocation) (*bytes.Buffer, error) + // RunGoCommands runs a series of `go` commands that updates the go.mod + // and go.sum file for wd, and returns their updated contents. + RunGoCommands(ctx context.Context, allowNetwork bool, wd string, run func(invoke func(...string) (*bytes.Buffer, error)) error) (bool, []byte, []byte, error) + // RunProcessEnvFunc runs fn with the process env for this snapshot's view. // Note: the process env contains cached module and filesystem state. RunProcessEnvFunc(ctx context.Context, fn func(*imports.Options) error) error @@ -185,7 +189,7 @@ const ( // AllowNetwork is a flag bit that indicates the invocation should be // allowed to access the network. - AllowNetwork = 1 << 10 + AllowNetwork InvocationFlags = 1 << 10 ) func (m InvocationFlags) Mode() InvocationFlags {