diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index f9e57ccf9a..5e0c1dbd5c 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -16,6 +16,7 @@ import ( "golang.org/x/mod/modfile" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/debug/tag" + "golang.org/x/tools/internal/lsp/diff" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/memoize" @@ -76,9 +77,8 @@ func (s *snapshot) ModTidy(ctx context.Context) (*source.TidiedModule, error) { s.mu.Unlock() var ( - modURI = s.view.modURI - cfg = s.config(ctx) - options = s.view.Options() + modURI = s.view.modURI + cfg = s.config(ctx) ) key := modTidyKey{ sessionID: s.view.session.id, @@ -127,46 +127,17 @@ func (s *snapshot) ModTidy(ctx context.Context) (*source.TidiedModule, error) { // since it has been "tidied". return &modTidyData{err: err} } - // Get the dependencies that are different between the original and - // ideal go.mod files. - unusedDeps := make(map[string]*modfile.Require, len(pm.File.Require)) - missingDeps := make(map[string]*modfile.Require, len(ideal.Require)) - for _, req := range pm.File.Require { - unusedDeps[req.Mod.Path] = req - } - for _, req := range ideal.Require { - origDep := unusedDeps[req.Mod.Path] - if origDep != nil && origDep.Indirect == req.Indirect { - delete(unusedDeps, req.Mod.Path) - } else { - missingDeps[req.Mod.Path] = req - } - } - // First, compute any errors specific to the go.mod file. These include - // unused dependencies and modules with incorrect // indirect comments. - /// Both the diagnostic and the fix will appear on the go.mod file. - modRequireErrs, err := modRequireErrors(pm.Mapper, missingDeps, unusedDeps, options) - if err != nil { - return &modTidyData{err: err} - } - for _, req := range missingDeps { - if unusedDeps[req.Mod.Path] != nil { - delete(missingDeps, req.Mod.Path) - } - } - // Next, compute any diagnostics for modules that are missing from the - // go.mod file. The fixes will be for the go.mod file, but the - // diagnostics should appear on the import statements in the Go or - // go.mod files. - missingModuleErrs, err := missingModuleErrors(ctx, snapshot, pm.Mapper, workspacePkgs, ideal.Require, missingDeps, pm.File, options) + // Compare the original and tidied go.mod files to compute errors and + // suggested fixes. + errors, err := modTidyErrors(ctx, snapshot, pm, ideal, workspacePkgs) if err != nil { return &modTidyData{err: err} } return &modTidyData{ tidied: &source.TidiedModule{ + Errors: errors, Parsed: pm, TidiedContent: tempContents, - Errors: append(modRequireErrs, missingModuleErrs...), }, } }) @@ -196,58 +167,164 @@ func hashImports(ctx context.Context, wsPackages []source.Package) (string, erro return hashContents([]byte(hashed)), nil } -// modRequireErrors extracts the errors that occur on the require directives. -// It checks for directness issues and unused dependencies. -func modRequireErrors(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 { +// modTidyErrors computes the differences between the original and tidied +// go.mod files to produce diagnostic and suggested fixes. Some diagnostics +// may appear on the Go files that import packages from missing modules. +func modTidyErrors(ctx context.Context, snapshot source.Snapshot, pm *source.ParsedModule, ideal *modfile.File, workspacePkgs []source.Package) (errors []source.Error, err error) { + // First, determine which modules are unused and which are missing from the + // original go.mod file. + var ( + unused = make(map[string]*modfile.Require, len(pm.File.Require)) + missing = make(map[string]*modfile.Require, len(ideal.Require)) + wrongDirectness = make(map[string]*modfile.Require, len(pm.File.Require)) + ) + for _, req := range pm.File.Require { + unused[req.Mod.Path] = req + } + for _, req := range ideal.Require { + origReq := unused[req.Mod.Path] + if origReq == nil { + missing[req.Mod.Path] = req + continue + } else if origReq.Indirect != req.Indirect { + wrongDirectness[req.Mod.Path] = origReq + } + delete(unused, req.Mod.Path) + } + for _, req := range unused { + srcErr, err := unusedError(pm.Mapper, req, snapshot.View().Options().ComputeEdits) + if err != nil { + return nil, err + } + errors = append(errors, srcErr) + } + for _, req := range wrongDirectness { + // Handle dependencies that are incorrectly labeled indirect and + // vice versa. + srcErr, err := directnessError(pm.Mapper, req, snapshot.View().Options().ComputeEdits) + if err != nil { + return nil, err + } + errors = append(errors, srcErr) + } + // Next, compute any diagnostics for modules that are missing from the + // go.mod file. The fixes will be for the go.mod file, but the + // diagnostics should also appear in both the go.mod file and the import + // statements in the Go files in which the dependencies are used. + missingModuleFixes := map[*modfile.Require][]source.SuggestedFix{} + for _, req := range missing { + srcErr, err := missingModuleError(snapshot, pm, req) + if err != nil { + return nil, err + } + missingModuleFixes[req] = srcErr.SuggestedFixes + errors = append(errors, srcErr) + } + // Add diagnostics for missing modules anywhere they are imported in the + // workspace. + for _, pkg := range workspacePkgs { + missingImports := map[string]*modfile.Require{} + for _, imp := range pkg.Imports() { + if req, ok := missing[imp.PkgPath()]; ok { + missingImports[imp.PkgPath()] = req + break + } + // If the import is a package of the dependency, then add the + // package to the map, this will eliminate the need to do this + // prefix package search on each import for each file. + // Example: + // + // import ( + // "golang.org/x/tools/go/expect" + // "golang.org/x/tools/go/packages" + // ) + // They both are related to the same module: "golang.org/x/tools". + var match string + for _, req := range ideal.Require { + if strings.HasPrefix(imp.PkgPath(), req.Mod.Path) && len(req.Mod.Path) > len(match) { + match = req.Mod.Path + } + } + if req, ok := missing[match]; ok { + missingImports[imp.PkgPath()] = req + } + } + // None of this package's imports are from missing modules. + if len(missingImports) == 0 { continue } - // Handle dependencies that are incorrectly labeled indirect and vice versa. - if missingDeps[dep] != nil && req.Indirect != missingDeps[dep].Indirect { - directErr, err := modDirectnessErrors(m, req, options) - if err != nil { - return nil, err + for _, pgf := range pkg.CompiledGoFiles() { + file, m := pgf.File, pgf.Mapper + if file == nil || m == nil { + continue } - errors = append(errors, directErr) - } - // Handle unused dependencies. - if missingDeps[dep] == nil { - rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End) - if err != nil { - return nil, err + imports := make(map[string]*ast.ImportSpec) + for _, imp := range file.Imports { + if imp.Path == nil { + continue + } + if target, err := strconv.Unquote(imp.Path.Value); err == nil { + imports[target] = imp + } } - edits, err := dropDependencyEdits(m, req, options) - if err != nil { - return nil, err + if len(imports) == 0 { + continue + } + for importPath, req := range missingImports { + imp, ok := imports[importPath] + if !ok { + continue + } + fixes, ok := missingModuleFixes[req] + if !ok { + return nil, fmt.Errorf("no missing module fix for %q (%q)", importPath, req.Mod.Path) + } + srcErr, err := missingModuleForImport(snapshot, m, imp, req, fixes) + if err != nil { + return nil, err + } + errors = append(errors, srcErr) } - errors = append(errors, source.Error{ - Category: ModTidyError, - Message: fmt.Sprintf("%s is not used in this module.", dep), - Range: rng, - URI: m.URI, - SuggestedFixes: []source.SuggestedFix{{ - Title: fmt.Sprintf("Remove dependency: %s", dep), - Edits: map[span.URI][]protocol.TextEdit{ - m.URI: edits, - }, - }}, - }) } } return errors, nil } -const ModTidyError = "go mod tidy" - -// modDirectnessErrors extracts errors when a dependency is labeled indirect when it should be direct and vice versa. -func modDirectnessErrors(m *protocol.ColumnMapper, req *modfile.Require, options source.Options) (source.Error, error) { +// unusedError returns a source.Error for an unused require. +func unusedError(m *protocol.ColumnMapper, req *modfile.Require, computeEdits diff.ComputeEdits) (source.Error, error) { rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End) if err != nil { return source.Error{}, err } + edits, err := dropDependency(req, m, computeEdits) + if err != nil { + return source.Error{}, err + } + return source.Error{ + Category: source.GoModTidy, + Message: fmt.Sprintf("%s is not used in this module", req.Mod.Path), + Range: rng, + URI: m.URI, + SuggestedFixes: []source.SuggestedFix{{ + Title: fmt.Sprintf("Remove dependency: %s", req.Mod.Path), + Edits: map[span.URI][]protocol.TextEdit{ + m.URI: edits, + }, + }}, + }, nil +} + +// directnessError extracts errors when a dependency is labeled indirect when +// it should be direct and vice versa. +func directnessError(m *protocol.ColumnMapper, req *modfile.Require, computeEdits diff.ComputeEdits) (source.Error, error) { + rng, err := rangeFromPositions(m, req.Syntax.Start, req.Syntax.End) + if err != nil { + return source.Error{}, err + } + direction := "indirect" if req.Indirect { + direction = "direct" + // If the dependency should be direct, just highlight the // indirect. if comments := req.Syntax.Comment(); comments != nil && len(comments.Suffix) > 0 { end := comments.Suffix[0].Start @@ -258,35 +335,19 @@ func modDirectnessErrors(m *protocol.ColumnMapper, req *modfile.Require, options return source.Error{}, err } } - edits, err := changeDirectnessEdits(m, req, false, options) - if err != nil { - return source.Error{}, err - } - return source.Error{ - Category: ModTidyError, - Message: fmt.Sprintf("%s should be a direct dependency", req.Mod.Path), - Range: rng, - URI: m.URI, - SuggestedFixes: []source.SuggestedFix{{ - Title: fmt.Sprintf("Make %s direct", req.Mod.Path), - Edits: map[span.URI][]protocol.TextEdit{ - m.URI: edits, - }, - }}, - }, nil } // If the dependency should be indirect, add the // indirect. - edits, err := changeDirectnessEdits(m, req, true, options) + edits, err := switchDirectness(req, m, computeEdits) if err != nil { return source.Error{}, err } return source.Error{ - Category: ModTidyError, - Message: fmt.Sprintf("%s should be an indirect dependency", req.Mod.Path), + Message: fmt.Sprintf("%s should be %s", req.Mod.Path, direction), Range: rng, URI: m.URI, + Category: source.GoModTidy, SuggestedFixes: []source.SuggestedFix{{ - Title: fmt.Sprintf("Make %s indirect", req.Mod.Path), + Title: fmt.Sprintf("Change %s to %s", req.Mod.Path, direction), Edits: map[span.URI][]protocol.TextEdit{ m.URI: edits, }, @@ -294,19 +355,35 @@ func modDirectnessErrors(m *protocol.ColumnMapper, req *modfile.Require, options }, nil } -// dropDependencyEdits gets the edits needed to remove the dependency from the go.mod file. -// As an example, this function will codify the edits needed to convert the before go.mod file to the after. -// Before: -// module t -// -// go 1.11 -// -// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee -// After: -// module t -// -// go 1.11 -func dropDependencyEdits(m *protocol.ColumnMapper, req *modfile.Require, options source.Options) ([]protocol.TextEdit, error) { +func missingModuleError(snapshot source.Snapshot, pm *source.ParsedModule, req *modfile.Require) (source.Error, error) { + start, end := pm.File.Module.Syntax.Span() + rng, err := rangeFromPositions(pm.Mapper, start, end) + if err != nil { + return source.Error{}, err + } + edits, err := addRequireFix(pm.Mapper, req, snapshot.View().Options().ComputeEdits) + if err != nil { + return source.Error{}, err + } + fix := &source.SuggestedFix{ + Title: "Add %s to your go.mod file", + Edits: map[span.URI][]protocol.TextEdit{ + pm.Mapper.URI: edits, + }, + } + return source.Error{ + URI: pm.Mapper.URI, + Range: rng, + Message: fmt.Sprintf("%s is not in your go.mod file", req.Mod.Path), + Category: source.GoModTidy, + Kind: source.ModTidyError, + SuggestedFixes: []source.SuggestedFix{*fix}, + }, nil +} + +// dropDependency returns the edits to remove the given require from the go.mod +// file. +func dropDependency(req *modfile.Require, m *protocol.ColumnMapper, computeEdits diff.ComputeEdits) ([]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) @@ -322,29 +399,13 @@ func dropDependencyEdits(m *protocol.ColumnMapper, req *modfile.Require, options return nil, err } // Calculate the edits to be made due to the change. - diff := options.ComputeEdits(m.URI, string(m.Content), string(newContent)) - edits, err := source.ToProtocolEdits(m, diff) - if err != nil { - return nil, err - } - return edits, nil + diff := computeEdits(m.URI, string(m.Content), string(newContent)) + return source.ToProtocolEdits(m, diff) } -// changeDirectnessEdits gets the edits needed to change an indirect dependency to direct and vice versa. -// As an example, this function will codify the edits needed to convert the before go.mod file to the after. -// Before: -// module t -// -// go 1.11 -// -// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee -// After: -// module t -// -// go 1.11 -// -// require golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee // indirect -func changeDirectnessEdits(m *protocol.ColumnMapper, req *modfile.Require, indirect bool, options source.Options) ([]protocol.TextEdit, error) { +// switchDirectness gets the edits needed to change an indirect dependency to +// direct and vice versa. +func switchDirectness(req *modfile.Require, m *protocol.ColumnMapper, computeEdits diff.ComputeEdits) ([]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) @@ -359,7 +420,7 @@ func changeDirectnessEdits(m *protocol.ColumnMapper, req *modfile.Require, indir requires = append(requires, &modfile.Require{ Mod: r.Mod, Syntax: r.Syntax, - Indirect: indirect, + Indirect: !r.Indirect, }) continue } @@ -371,12 +432,54 @@ func changeDirectnessEdits(m *protocol.ColumnMapper, req *modfile.Require, indir return nil, err } // Calculate the edits to be made due to the change. - diff := options.ComputeEdits(m.URI, string(m.Content), string(newContent)) - edits, err := source.ToProtocolEdits(m, diff) + diff := computeEdits(m.URI, string(m.Content), string(newContent)) + return source.ToProtocolEdits(m, diff) +} + +// missingModuleForImport creates an error for a given import path that comes +// from a missing module. +func missingModuleForImport(snapshot source.Snapshot, m *protocol.ColumnMapper, imp *ast.ImportSpec, req *modfile.Require, fixes []source.SuggestedFix) (source.Error, error) { + if req.Syntax == nil { + return source.Error{}, fmt.Errorf("no syntax for %v", req) + } + spn, err := span.NewRange(snapshot.FileSet(), imp.Path.Pos(), imp.Path.End()).Span() + if err != nil { + return source.Error{}, err + } + rng, err := m.Range(spn) + if err != nil { + return source.Error{}, err + } + return source.Error{ + Category: source.GoModTidy, + URI: m.URI, + Range: rng, + Message: fmt.Sprintf("%s is not in your go.mod file", req.Mod.Path), + Kind: source.ModTidyError, + SuggestedFixes: fixes, + }, nil +} + +// addRequireFix creates edits for adding a given require to a go.mod file. +func addRequireFix(m *protocol.ColumnMapper, req *modfile.Require, computeEdits diff.ComputeEdits) ([]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 } - return edits, 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 := computeEdits(m.URI, string(m.Content), string(newContents)) + return source.ToProtocolEdits(m, diff) } func rangeFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (protocol.Range, error) { @@ -397,158 +500,3 @@ func rangeFromPositions(m *protocol.ColumnMapper, s, e modfile.Position) (protoc } return m.Range(span.New(m.URI, start, end)) } - -// missingModuleErrors returns diagnostics for each file in each workspace -// package that has dependencies that are not reflected in the go.mod file. -func missingModuleErrors(ctx context.Context, snapshot *snapshot, modMapper *protocol.ColumnMapper, pkgs []source.Package, modules []*modfile.Require, missingMods map[string]*modfile.Require, original *modfile.File, options source.Options) ([]source.Error, error) { - var moduleErrs []source.Error - matchedMissingMods := make(map[*modfile.Require]struct{}) - for _, pkg := range pkgs { - missingPkgs := map[string]*modfile.Require{} - for _, imp := range pkg.Imports() { - found := func(req *modfile.Require) { - missingPkgs[imp.PkgPath()] = req - matchedMissingMods[req] = struct{}{} - } - if req, ok := missingMods[imp.PkgPath()]; ok { - found(req) - break - } - // If the import is a package of the dependency, then add the - // package to the map, this will eliminate the need to do this - // prefix package search on each import for each file. - // Example: - // - // import ( - // "golang.org/x/tools/go/expect" - // "golang.org/x/tools/go/packages" - // ) - // They both are related to the same module: "golang.org/x/tools". - var match string - for _, mod := range modules { - if strings.HasPrefix(imp.PkgPath(), mod.Mod.Path) && len(mod.Mod.Path) > len(match) { - match = mod.Mod.Path - } - } - if req, ok := missingMods[match]; ok { - found(req) - } - } - if len(missingPkgs) > 0 { - errs, err := missingModules(ctx, snapshot, modMapper, pkg, missingPkgs, options) - if err != nil { - return nil, err - } - moduleErrs = append(moduleErrs, errs...) - } - } - for _, req := range missingMods { - if _, ok := matchedMissingMods[req]; ok { - continue - } - s, e := original.Module.Syntax.Span() - rng, err := rangeFromPositions(modMapper, s, e) - if err != nil { - return nil, err - } - edits, err := addRequireFix(modMapper, req, options) - if err != nil { - return nil, err - } - moduleErrs = append(moduleErrs, source.Error{ - URI: modMapper.URI, - Range: rng, - Message: fmt.Sprintf("%s is not in your go.mod file", req.Mod.Path), - Category: "go mod tidy", - Kind: source.ModTidyError, - SuggestedFixes: []source.SuggestedFix{ - { - Title: "Add %s to your go.mod file", - Edits: edits, - }, - }, - }) - } - return moduleErrs, nil -} - -func missingModules(ctx context.Context, snapshot *snapshot, modMapper *protocol.ColumnMapper, pkg source.Package, missing map[string]*modfile.Require, options source.Options) ([]source.Error, error) { - var errors []source.Error - for _, pgf := range pkg.CompiledGoFiles() { - imports := make(map[string]*ast.ImportSpec) - for _, imp := range pgf.File.Imports { - if imp.Path == nil { - continue - } - if target, err := strconv.Unquote(imp.Path.Value); err == nil { - imports[target] = imp - } - } - if len(imports) == 0 { - continue - } - for mod, req := range missing { - if req.Syntax == nil { - continue - } - imp, ok := imports[mod] - if !ok { - continue - } - spn, err := span.NewRange(snapshot.view.session.cache.fset, imp.Path.Pos(), imp.Path.End()).Span() - if err != nil { - return nil, err - } - rng, err := pgf.Mapper.Range(spn) - if err != nil { - return nil, err - } - edits, err := addRequireFix(modMapper, req, options) - if err != nil { - return nil, err - } - errors = append(errors, source.Error{ - URI: pgf.URI, - Range: rng, - Message: fmt.Sprintf("%s is not in your go.mod file", req.Mod.Path), - Category: "go mod tidy", - Kind: source.ModTidyError, - SuggestedFixes: []source.SuggestedFix{ - { - Title: "Add %s to your go.mod file", - Edits: edits, - }, - }, - }) - } - } - return errors, nil -} - -// addRequireFix creates edits for adding a given require to a go.mod file. -func addRequireFix(m *protocol.ColumnMapper, req *modfile.Require, options source.Options) (map[span.URI][]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 - } - // 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 := options.ComputeEdits(m.URI, string(m.Content), string(newContents)) - edits, err := source.ToProtocolEdits(m, diff) - if err != nil { - return nil, err - } - return map[span.URI][]protocol.TextEdit{ - m.URI: edits, - }, nil -} diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 76495c5c5f..37c72c6edc 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -459,6 +459,9 @@ type Error struct { Related []RelatedInformation } +// GoModTidy is the source for a diagnostic computed by running `go mod tidy`. +const GoModTidy = "go mod tidy" + type ErrorKind int const ( diff --git a/internal/lsp/testdata/unused/primarymod/go.mod b/internal/lsp/testdata/unused/primarymod/go.mod index 78035bdb6e..12e9ade5e8 100644 --- a/internal/lsp/testdata/unused/primarymod/go.mod +++ b/internal/lsp/testdata/unused/primarymod/go.mod @@ -2,4 +2,4 @@ module unused go 1.12 -require example.com/extramodule v1.0.0 //@diag("require example.com/extramodule v1.0.0", "go mod tidy", "example.com/extramodule is not used in this module.", "warning"),suggestedfix("require example.com/extramodule v1.0.0", "quickfix") +require example.com/extramodule v1.0.0 //@diag("require example.com/extramodule v1.0.0", "go mod tidy", "example.com/extramodule is not used in this module", "warning"),suggestedfix("require example.com/extramodule v1.0.0", "quickfix")