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 <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Robert Findley <rfindley@google.com>
golang · Nov 2, 2020 · 1f28ee6 · 1f28ee6
1 parent 582c62e
commit 1f28ee6
Show file tree

Hide file tree

Showing 10 changed files with 148 additions and 79 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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