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)) +}