From 125cc70a4027cda5812e28e63f852aea8df190d5 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 9 Jul 2020 02:02:35 -0400 Subject: [PATCH] internal/lsp: add new go.mod requires to files with unused require This change addresses an underlying issue with the go.mod code, which is that it was modifying go.mod files without cloning them. This could've resulted in some ugly race conditions. We also handle the fact that new dependencies weren't being added cleanly to files that already had unused dependencies. Fixes golang/go#39041 Change-Id: I96ee0052d8d29a25e24f0bda9688e780a0fa7442 Reviewed-on: https://go-review.googlesource.com/c/tools/+/241443 Run-TryBot: Rebecca Stambler TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/cache/mod_tidy.go | 79 +++++++++++++++------------- internal/lsp/mod/diagnostics.go | 20 +++---- internal/lsp/regtest/modfile_test.go | 67 +++++++++++++++++++++++ 3 files changed, 120 insertions(+), 46 deletions(-) diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index d57b616497..4227982861 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -150,7 +150,7 @@ func (s *snapshot) ModTidyHandle(ctx context.Context) (source.ModTidyHandle, err missingDeps[req.Mod.Path] = req } } - diagnostics, err := modRequireErrors(pmh.Mod().URI(), original, m, missingDeps, unusedDeps, options) + diagnostics, err := modRequireErrors(pmh.Mod().URI(), m, missingDeps, unusedDeps, options) if err != nil { return &modTidyData{err: err} } @@ -175,7 +175,7 @@ func (s *snapshot) ModTidyHandle(ctx context.Context) (source.ModTidyHandle, err // modRequireErrors extracts the errors that occur on the require directives. // It checks for directness issues and unused dependencies. -func modRequireErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapper, missingDeps, unusedDeps map[string]*modfile.Require, options source.Options) ([]source.Error, error) { +func modRequireErrors(uri span.URI, m *protocol.ColumnMapper, missingDeps, unusedDeps map[string]*modfile.Require, options source.Options) ([]source.Error, error) { var errors []source.Error for dep, req := range unusedDeps { if req.Syntax == nil { @@ -183,7 +183,7 @@ func modRequireErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapp } // Handle dependencies that are incorrectly labeled indirect and vice versa. if missingDeps[dep] != nil && req.Indirect != missingDeps[dep].Indirect { - directErr, err := modDirectnessErrors(uri, parsed, m, req, options) + directErr, err := modDirectnessErrors(uri, m, req, options) if err != nil { return nil, err } @@ -195,7 +195,7 @@ func modRequireErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapp if err != nil { return nil, err } - edits, err := dropDependencyEdits(uri, parsed, m, req, options) + edits, err := dropDependencyEdits(uri, m, req, options) if err != nil { return nil, err } @@ -219,7 +219,7 @@ func modRequireErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapp const ModTidyError = "go mod tidy" // modDirectnessErrors extracts errors when a dependency is labeled indirect when it should be direct and vice versa. -func modDirectnessErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapper, req *modfile.Require, options source.Options) (source.Error, error) { +func modDirectnessErrors(uri span.URI, m *protocol.ColumnMapper, req *modfile.Require, options source.Options) (source.Error, error) { rng, err := rangeFromPositions(uri, m, req.Syntax.Start, req.Syntax.End) if err != nil { return source.Error{}, err @@ -235,7 +235,7 @@ func modDirectnessErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnM return source.Error{}, err } } - edits, err := changeDirectnessEdits(uri, parsed, m, req, false, options) + edits, err := changeDirectnessEdits(uri, m, req, false, options) if err != nil { return source.Error{}, err } @@ -253,7 +253,7 @@ func modDirectnessErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnM }, nil } // If the dependency should be indirect, add the // indirect. - edits, err := changeDirectnessEdits(uri, parsed, m, req, true, options) + edits, err := changeDirectnessEdits(uri, m, req, true, options) if err != nil { return source.Error{}, err } @@ -283,19 +283,23 @@ func modDirectnessErrors(uri span.URI, parsed *modfile.File, m *protocol.ColumnM // module t // // go 1.11 -func dropDependencyEdits(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapper, req *modfile.Require, options source.Options) ([]protocol.TextEdit, error) { - if err := parsed.DropRequire(req.Mod.Path); err != nil { - return nil, err - } - parsed.Cleanup() - newContents, err := parsed.Format() +func dropDependencyEdits(uri span.URI, m *protocol.ColumnMapper, req *modfile.Require, options source.Options) ([]protocol.TextEdit, error) { + // We need a private copy of the parsed go.mod file, since we're going to + // modify it. + copied, err := modfile.Parse("", m.Content, nil) + if err != nil { + return nil, err + } + if err := copied.DropRequire(req.Mod.Path); err != nil { + return nil, err + } + copied.Cleanup() + newContent, err := copied.Format() if err != nil { return nil, err } - // Reset the *modfile.File back to before we dropped the dependency. - parsed.AddNewRequire(req.Mod.Path, req.Mod.Version, req.Indirect) // Calculate the edits to be made due to the change. - diff := options.ComputeEdits(uri, string(m.Content), string(newContents)) + diff := options.ComputeEdits(uri, string(m.Content), string(newContent)) edits, err := source.ToProtocolEdits(m, diff) if err != nil { return nil, err @@ -317,33 +321,34 @@ func dropDependencyEdits(uri span.URI, parsed *modfile.File, m *protocol.ColumnM // go 1.11 // // require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee // indirect -func changeDirectnessEdits(uri span.URI, parsed *modfile.File, m *protocol.ColumnMapper, req *modfile.Require, indirect bool, options source.Options) ([]protocol.TextEdit, error) { - var newReq []*modfile.Require - prevIndirect := false - // Change the directness in the matching require statement. - for _, r := range parsed.Require { - if req.Mod.Path == r.Mod.Path { - prevIndirect = req.Indirect - req.Indirect = indirect - } - newReq = append(newReq, r) - } - parsed.SetRequire(newReq) - parsed.Cleanup() - newContents, err := parsed.Format() +func changeDirectnessEdits(uri span.URI, m *protocol.ColumnMapper, req *modfile.Require, indirect bool, options source.Options) ([]protocol.TextEdit, error) { + // We need a private copy of the parsed go.mod file, since we're going to + // modify it. + copied, err := modfile.Parse("", m.Content, nil) if err != nil { return nil, err } - // Change the dependency back to the way it was before we got the newContents. - for _, r := range parsed.Require { - if req.Mod.Path == r.Mod.Path { - req.Indirect = prevIndirect + // Change the directness in the matching require statement. To avoid + // reordering the require statements, rewrite all of them. + var requires []*modfile.Require + for _, r := range copied.Require { + if r.Mod.Path == req.Mod.Path { + requires = append(requires, &modfile.Require{ + Mod: r.Mod, + Syntax: r.Syntax, + Indirect: indirect, + }) + continue } - newReq = append(newReq, r) + requires = append(requires, r) + } + copied.SetRequire(requires) + newContent, err := copied.Format() + if err != nil { + return nil, err } - parsed.SetRequire(newReq) // Calculate the edits to be made due to the change. - diff := options.ComputeEdits(uri, string(m.Content), string(newContents)) + diff := options.ComputeEdits(uri, string(m.Content), string(newContent)) edits, err := source.ToProtocolEdits(m, diff) if err != nil { return nil, err diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index b07d7b2c25..184b216149 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -143,7 +143,7 @@ func SuggestedGoFixes(ctx context.Context, snapshot source.Snapshot) (map[string if err != nil { return nil, err } - file, m, _, err := pmh.Parse(ctx) + _, m, _, err := pmh.Parse(ctx) if err != nil { return nil, err } @@ -154,17 +154,19 @@ func SuggestedGoFixes(ctx context.Context, snapshot source.Snapshot) (map[string } textDocumentEdits := make(map[string]protocol.TextDocumentEdit) for dep, req := range missingDeps { - // Calculate the quick fix edits that need to be made to the go.mod file. - if err := file.AddRequire(req.Mod.Path, req.Mod.Version); err != nil { - return nil, err - } - file.Cleanup() - newContents, err := file.Format() + // We need a private copy of the parsed go.mod file, since we're going to + // modify it. + copied, err := modfile.Parse("", oldContents, nil) if err != nil { return nil, err } - // Reset the *modfile.File back to before we added the dependency. - if err := file.DropRequire(req.Mod.Path); err != nil { + // Calculate the quick fix edits that need to be made to the go.mod file. + if err := copied.AddRequire(req.Mod.Path, req.Mod.Version); err != nil { + return nil, err + } + copied.SortBlocks() + newContents, err := copied.Format() + if err != nil { return nil, err } // Calculate the edits to be made due to the change. diff --git a/internal/lsp/regtest/modfile_test.go b/internal/lsp/regtest/modfile_test.go index c0dcfe9154..5caf8758b8 100644 --- a/internal/lsp/regtest/modfile_test.go +++ b/internal/lsp/regtest/modfile_test.go @@ -103,3 +103,70 @@ require example.com v1.2.3 } }, WithProxy(proxy)) } + +// Test to reproduce golang/go#39041. It adds a new require to a go.mod file +// that already has an unused require. +func TestNewDepWithUnusedDep(t *testing.T) { + testenv.NeedsGo1Point(t, 14) + + const proxy = ` +-- github.com/esimov/caire@v1.2.5/go.mod -- +module github.com/esimov/caire + +go 1.12 +-- github.com/esimov/caire@v1.2.5/caire.go -- +package caire + +func RemoveTempImage() {} +-- google.golang.org/protobuf@v1.20.0/go.mod -- +module google.golang.org/protobuf + +go 1.12 +-- google.golang.org/protobuf@v1.20.0/hello/hello.go -- +package hello +` + const repro = ` +-- go.mod -- +module mod.com + +go 1.14 + +require google.golang.org/protobuf v1.20.0 +-- main.go -- +package main + +import ( + "github.com/esimov/caire" +) + +func _() { + caire.RemoveTempImage() +}` + runner.Run(t, repro, func(t *testing.T, env *Env) { + env.OpenFile("go.mod") + env.OpenFile("main.go") + d := env.Await( + env.DiagnosticAtRegexp("main.go", `"github.com/esimov/caire"`), + ) + if len(d) == 0 { + t.Fatalf("no diagnostics") + } + params, ok := d[0].(*protocol.PublishDiagnosticsParams) + if !ok { + t.Fatalf("expected diagnostic of type PublishDiagnosticParams, got %T", d[0]) + } + env.ApplyQuickFixes("main.go", params.Diagnostics) + want := `module mod.com + +go 1.14 + +require ( + github.com/esimov/caire v1.2.5 + google.golang.org/protobuf v1.20.0 +) +` + if got := env.Editor.BufferText("go.mod"); got != want { + t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(want, got)) + } + }, WithProxy(proxy)) +}