From 1f28ee682075ddd58383833c7340c421a395e93e Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Tue, 13 Oct 2020 00:50:48 -0400 Subject: [PATCH] internal/lsp: change `go mod vendor` warning into a diagnostic This CL uses the approach of a source.ErrorList for the `go mod vendor` error--rather than a ShowMessageRequest. The suggested fix is a command that runs `go mod vendor`, so I added a CommandFix to the source.Error type. I'm not sure if a diagnostic is better than a ShowMessageRequest-- perhaps it should be both? Fixes golang/go#41819 Fixes golang/go#42152 Change-Id: I4ba2d7af0233f13583395e871166caed83454d6e Reviewed-on: https://go-review.googlesource.com/c/tools/+/261737 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley --- gopls/internal/regtest/vendor_test.go | 19 +++++++--- internal/lsp/cache/load.go | 7 ---- internal/lsp/cache/mod_tidy.go | 47 +++++++++++++++++++++++ internal/lsp/cache/snapshot.go | 25 +++++++++++++ internal/lsp/cache/view_test.go | 24 ++++++++++++ internal/lsp/code_action.go | 12 +++++- internal/lsp/command.go | 9 ++++- internal/lsp/diagnostics.go | 54 +-------------------------- internal/lsp/mod/diagnostics.go | 27 +++++++++----- internal/lsp/source/view.go | 3 +- 10 files changed, 148 insertions(+), 79 deletions(-) diff --git a/gopls/internal/regtest/vendor_test.go b/gopls/internal/regtest/vendor_test.go index 7f11d4a206..7e754be843 100644 --- a/gopls/internal/regtest/vendor_test.go +++ b/gopls/internal/regtest/vendor_test.go @@ -4,6 +4,8 @@ import ( "testing" "golang.org/x/tools/internal/lsp" + "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/testenv" ) @@ -20,6 +22,7 @@ var Goodbye error func TestInconsistentVendoring(t *testing.T) { testenv.NeedsGo1Point(t, 14) + const pkgThatUsesVendoring = ` -- go.mod -- module mod.com @@ -52,15 +55,21 @@ func _() { // will be executed. OnceMet( CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1), - ShowMessageRequest("go mod vendor"), + env.DiagnosticAtRegexp("go.mod", "module mod.com"), ), ) + // Apply the quickfix associated with the diagnostic. + d := &protocol.PublishDiagnosticsParams{} + env.Await(ReadDiagnostics("go.mod", d)) + env.ApplyQuickFixes("go.mod", d.Diagnostics) + + // Check for file changes when the command completes. + env.Await(CompletedWork(source.CommandVendor.Title, 1)) env.CheckForFileChanges() + + // Confirm that there is no longer any inconsistent vendoring. env.Await( - OnceMet( - CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1), - DiagnosticAt("a/a1.go", 6, 5), - ), + DiagnosticAt("a/a1.go", 6, 5), ) }) } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index ccf0c1a162..5a67a1732d 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -92,8 +92,6 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query)) defer done() - cleanup := func() {} - _, inv, cleanup, err := s.goCommandInvocation(ctx, source.ForTypeChecking, &gocommand.Invocation{ WorkingDir: s.view.rootURI.Filename(), }) @@ -111,11 +109,6 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { return ctx.Err() } if err != nil { - // Match on common error messages. This is really hacky, but I'm not sure - // of any better way. This can be removed when golang/go#39164 is resolved. - if strings.Contains(err.Error(), "inconsistent vendoring") { - return source.InconsistentVendoring - } event.Error(ctx, "go/packages.Load", err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) } else { event.Log(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs))) diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 5f735b9abd..e4934f4e6d 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -69,6 +69,9 @@ func (s *snapshot) ModTidy(ctx context.Context, fh source.FileHandle) (*source.T } workspacePkgs, err := s.WorkspacePackages(ctx) if err != nil { + if tm, ok := s.parseModErrors(ctx, fh, err); ok { + return tm, nil + } return nil, err } importHash, err := hashImports(ctx, workspacePkgs) @@ -160,6 +163,50 @@ func (s *snapshot) ModTidy(ctx context.Context, fh source.FileHandle) (*source.T return mth.tidy(ctx, s) } +func (s *snapshot) parseModErrors(ctx context.Context, fh source.FileHandle, err error) (*source.TidiedModule, bool) { + if err == nil { + return nil, false + } + switch { + // Match on common error messages. This is really hacky, but I'm not sure + // of any better way. This can be removed when golang/go#39164 is resolved. + case strings.Contains(err.Error(), "inconsistent vendoring"): + pmf, err := s.ParseMod(ctx, fh) + if err != nil { + return nil, false + } + if pmf.File.Module == nil || pmf.File.Module.Syntax == nil { + return nil, false + } + rng, err := rangeFromPositions(pmf.Mapper, pmf.File.Module.Syntax.Start, pmf.File.Module.Syntax.End) + if err != nil { + return nil, false + } + args, err := source.MarshalArgs(protocol.URIFromSpanURI(fh.URI())) + if err != nil { + return nil, false + } + return &source.TidiedModule{ + Parsed: pmf, + Errors: []source.Error{{ + URI: fh.URI(), + Range: rng, + Kind: source.ListError, + Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". +See https://github.com/golang/go/issues/39164 for more detail on this issue.`, + SuggestedFixes: []source.SuggestedFix{{ + Command: &protocol.Command{ + Command: source.CommandVendor.ID(), + Title: source.CommandVendor.Title, + Arguments: args, + }, + }}, + }}, + }, true + } + return nil, false +} + func hashImports(ctx context.Context, wsPackages []source.Package) (string, error) { results := make(map[string]bool) var imports []string diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 83bb6652fe..55884bbf9d 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -946,6 +946,11 @@ func (s *snapshot) orphanedFileScopes() []interface{} { if !contains(s.view.session.viewsOf(uri), s.view) { continue } + // If the file is not open and is in a vendor directory, don't treat it + // like a workspace package. + if _, ok := fh.(*overlay); !ok && inVendor(uri) { + continue + } // Don't reload metadata for files we've already deemed unloadable. if _, ok := s.unloadableFiles[uri]; ok { continue @@ -970,6 +975,20 @@ func contains(views []*View, view *View) bool { return false } +func inVendor(uri span.URI) bool { + toSlash := filepath.ToSlash(uri.Filename()) + if !strings.Contains(toSlash, "/vendor/") { + return false + } + // Only packages in _subdirectories_ of /vendor/ are considered vendored + // (/vendor/a/foo.go is vendored, /vendor/foo.go is not). + split := strings.Split(toSlash, "/vendor/") + if len(split) < 2 { + return false + } + return strings.Contains(split[1], "/") +} + func generationName(v *View, snapshotID uint64) string { return fmt.Sprintf("v%v/%v", v.id, snapshotID) } @@ -1069,6 +1088,12 @@ func (s *snapshot) clone(ctx context.Context, changes map[span.URI]*fileChange, } for uri, change := range changes { + // Maybe reinitialize the view if we see a change in the vendor + // directory. + if inVendor(uri) { + reinitialize = maybeReinit + } + // The original FileHandle for this URI is cached on the snapshot. originalFH := s.files[uri] diff --git a/internal/lsp/cache/view_test.go b/internal/lsp/cache/view_test.go index 87774e6714..39f2e86ea5 100644 --- a/internal/lsp/cache/view_test.go +++ b/internal/lsp/cache/view_test.go @@ -94,3 +94,27 @@ module de } } } + +func TestInVendor(t *testing.T) { + for _, tt := range []struct { + path string + inVendor bool + }{ + { + path: "foo/vendor/x.go", + inVendor: false, + }, + { + path: "foo/vendor/x/x.go", + inVendor: true, + }, + { + path: "foo/x.go", + inVendor: false, + }, + } { + if got := inVendor(span.URIFromPath(tt.path)); got != tt.inVendor { + t.Errorf("expected %s inVendor %v, got %v", tt.path, tt.inVendor, got) + } + } +} diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 4068935c5f..af7eeb3d10 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -14,6 +14,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/debug/tag" + "golang.org/x/tools/internal/lsp/mod" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" @@ -486,12 +487,12 @@ func moduleQuickFixes(ctx context.Context, snapshot source.Snapshot, fh source.V return nil, err } } - tidied, err := snapshot.ModTidy(ctx, modFH) + errors, err := mod.ErrorsForMod(ctx, snapshot, modFH) if err != nil { return nil, err } var quickFixes []protocol.CodeAction - for _, e := range tidied.Errors { + for _, e := range errors { var diag *protocol.Diagnostic for _, d := range diagnostics { if sameDiagnostic(d, e) { @@ -524,6 +525,13 @@ func moduleQuickFixes(ctx context.Context, snapshot source.Snapshot, fh source.V Edits: edits, }) } + if fix.Command != nil { + action.Command = &protocol.Command{ + Command: fix.Command.Command, + Title: fix.Command.Title, + Arguments: fix.Command.Arguments, + } + } quickFixes = append(quickFixes, action) } } diff --git a/internal/lsp/command.go b/internal/lsp/command.go index d51a98b4c9..ce7b1cde0e 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -55,8 +55,13 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom } if unsaved { switch params.Command { - case source.CommandTest.ID(), source.CommandGenerate.ID(), source.CommandToggleDetails.ID(), - source.CommandAddDependency.ID(), source.CommandUpgradeDependency.ID(), source.CommandRemoveDependency.ID(): + case source.CommandTest.ID(), + source.CommandGenerate.ID(), + source.CommandToggleDetails.ID(), + source.CommandAddDependency.ID(), + source.CommandUpgradeDependency.ID(), + source.CommandRemoveDependency.ID(), + source.CommandVendor.ID(): // TODO(PJW): for Toggle, not an error if it is being disabled err := errors.New("all files must be saved first") s.showCommandError(ctx, command.Title, err) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index ca560d3c4b..92f2e46c38 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -344,7 +344,7 @@ func errorsToDiagnostic(ctx context.Context, snapshot source.Snapshot, errors [] if err != nil { return err } - reports.add(fh.VersionedFileIdentity(), false, diagnostic) + reports.add(fh.VersionedFileIdentity(), true, diagnostic) } return nil } @@ -469,55 +469,5 @@ func (s *Server) handleFatalErrors(ctx context.Context, snapshot source.Snapshot hasGo = true return errors.New("done") }) - if !hasGo { - return true - } - - // All other workarounds are for errors associated with modules. - if len(snapshot.ModFiles()) == 0 { - return false - } - - switch loadErr { - case source.InconsistentVendoring: - item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{ - Type: protocol.Error, - Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". -See https://github.com/golang/go/issues/39164 for more detail on this issue.`, - Actions: []protocol.MessageActionItem{ - {Title: "go mod vendor"}, - }, - }) - // If the user closes the pop-up, don't show them further errors. - if item == nil { - return true - } - if err != nil { - event.Error(ctx, "go mod vendor ShowMessageRequest failed", err, tag.Directory.Of(snapshot.View().Folder().Filename())) - return true - } - // Right now, we don't have a good way of mapping the error message - // to a specific module, so this will re-run `go mod vendor` in every - // known module with a vendor directory. - // TODO(rstambler): Only re-run `go mod vendor` in the relevant module. - for _, uri := range snapshot.ModFiles() { - // Check that there is a vendor directory in the module before - // running `go mod vendor`. - if info, _ := os.Stat(filepath.Join(filepath.Dir(uri.Filename()), "vendor")); info == nil { - continue - } - if err := s.directGoModCommand(ctx, protocol.URIFromSpanURI(uri), "mod", []string{"vendor"}...); err != nil { - if err := s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: fmt.Sprintf(`"go mod vendor" failed with %v`, err), - }); err != nil { - if err != nil { - event.Error(ctx, "go mod vendor ShowMessage failed", err, tag.Directory.Of(snapshot.View().Folder().Filename())) - } - } - } - } - return true - } - return false + return !hasGo } diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index 8904ba2fe1..41a462c6ed 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -26,30 +26,39 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.Vers return nil, err } reports[fh.VersionedFileIdentity()] = []*source.Diagnostic{} - tidied, err := snapshot.ModTidy(ctx, fh) - if err == source.ErrTmpModfileUnsupported { - return nil, nil - } + errors, err := ErrorsForMod(ctx, snapshot, fh) if err != nil { return nil, err } - for _, e := range tidied.Errors { - diag := &source.Diagnostic{ + for _, e := range errors { + d := &source.Diagnostic{ Message: e.Message, Range: e.Range, Source: e.Category, } if e.Category == "syntax" { - diag.Severity = protocol.SeverityError + d.Severity = protocol.SeverityError } else { - diag.Severity = protocol.SeverityWarning + d.Severity = protocol.SeverityWarning } fh, err := snapshot.GetVersionedFile(ctx, e.URI) if err != nil { return nil, err } - reports[fh.VersionedFileIdentity()] = append(reports[fh.VersionedFileIdentity()], diag) + reports[fh.VersionedFileIdentity()] = append(reports[fh.VersionedFileIdentity()], d) } } return reports, nil } + +func ErrorsForMod(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) ([]source.Error, error) { + tidied, err := snapshot.ModTidy(ctx, fh) + + if source.IsNonFatalGoModError(err) { + return nil, nil + } + if err != nil { + return nil, err + } + return tidied.Errors, nil +} diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index ccac54db9d..390102eee6 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -576,8 +576,7 @@ func (e *Error) Error() string { } var ( - InconsistentVendoring = errors.New("inconsistent vendoring") - PackagesLoadError = errors.New("packages.Load error") + PackagesLoadError = errors.New("packages.Load error") ) // WorkspaceModuleVersion is the nonexistent pseudoversion suffix used in the