From e652b2f42cc723c83eedb31cfd097d44fbd0809c Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sun, 6 Dec 2020 21:05:58 -0500 Subject: [PATCH] internal/lsp: show multiple modules error message with GO111MODULE=auto When GO111MODULE=auto and the workspace is outside of $GOPATH/src, we should still show diagnostics about how to correctly configure multi-module workspaces. We should not show these warnings for workspaces outside of a module within GOPATH. This adds an extra piece to the WorkspacePackages error logic--we may still need to show the errors even if WorkspacePackages returned results. We should eventually consolidate all of this logic to be more cohesive, but for now I think it's more important to cover all of the different cases and add tests. Updates golang/go#42109 Change-Id: I673a03c9840cdaaf7f058de1cda3bf36b96fa7d3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/275553 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler TryBot-Result: Go Bot gopls-CI: kokoro Reviewed-by: Robert Findley --- gopls/internal/regtest/diagnostics_test.go | 56 ++++++++++++++++------ internal/lsp/cache/load.go | 11 ++--- internal/lsp/diagnostics.go | 19 ++++++++ internal/lsp/source/view.go | 5 ++ 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index 8453a51500..b03dc9c071 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1651,7 +1651,7 @@ import ( }) } -func TestMultipleModules_GO111MODULE_on(t *testing.T) { +func TestMultipleModules_Warning(t *testing.T) { const modules = ` -- a/go.mod -- module a.com @@ -1666,21 +1666,47 @@ go 1.12 -- b/b.go -- package b ` - withOptions( - WithModes(Singleton), - EditorConfig{ - Env: map[string]string{ - "GO111MODULE": "on", + for _, go111module := range []string{"on", "auto"} { + t.Run("GO111MODULE="+go111module, func(t *testing.T) { + withOptions( + WithModes(Singleton), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": go111module, + }, + }, + ).run(t, modules, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.OpenFile("b/go.mod") + env.Await( + env.DiagnosticAtRegexp("a/a.go", "package a"), + env.DiagnosticAtRegexp("b/go.mod", "module b.com"), + OutstandingWork("Error loading workspace", "gopls requires a module at the root of your workspace."), + ) + }) + }) + } + + // Expect no warning if GO111MODULE=auto in a directory in GOPATH. + t.Run("GOPATH_GO111MODULE_auto", func(t *testing.T) { + withOptions( + WithModes(Singleton), + EditorConfig{ + Env: map[string]string{ + "GO111MODULE": "auto", + }, }, - }, - ).run(t, modules, func(t *testing.T, env *Env) { - env.OpenFile("a/a.go") - env.OpenFile("b/go.mod") - env.Await( - env.DiagnosticAtRegexp("a/a.go", "package a"), - env.DiagnosticAtRegexp("b/go.mod", "module b.com"), - OutstandingWork("Error loading workspace", "gopls requires a module at the root of your workspace."), - ) + InGOPATH(), + ).run(t, modules, func(t *testing.T, env *Env) { + env.OpenFile("a/a.go") + env.Await( + OnceMet( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1), + NoDiagnostics("a/a.go"), + ), + NoOutstandingWork(), + ) + }) }) } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index c8843731e5..530c156c10 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -175,10 +175,8 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf } func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) *source.CriticalError { - // The error may be a result of the user's workspace layout. Check for - // a valid workspace configuration first. - if criticalErr := s.workspaceLayoutErrors(ctx, loadErr); criticalErr != nil { - return criticalErr + if strings.Contains(loadErr.Error(), "cannot find main module") { + return s.WorkspaceLayoutError(ctx) } criticalErr := &source.CriticalError{ MainError: loadErr, @@ -200,7 +198,7 @@ func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) *source.Cr // workspaceLayoutErrors returns a diagnostic for every open file, as well as // an error message if there are no open files. -func (s *snapshot) workspaceLayoutErrors(ctx context.Context, err error) *source.CriticalError { +func (s *snapshot) WorkspaceLayoutError(ctx context.Context) *source.CriticalError { // Assume the workspace is misconfigured only if we've detected an invalid // build configuration. Currently, a valid build configuration is either a // module at the root of the view or a GOPATH workspace. @@ -210,8 +208,7 @@ func (s *snapshot) workspaceLayoutErrors(ctx context.Context, err error) *source if len(s.workspace.getKnownModFiles()) == 0 { return nil } - // TODO(rstambler): Handle GO111MODULE=auto. - if s.view.userGo111Module != on { + if s.view.userGo111Module == off { return nil } if s.workspace.moduleSource != legacyWorkspace { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 7e6acb123e..f334a279f2 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -182,6 +182,16 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn return false } + // Even if workspace packages were returned, there still may be an error + // with the user's workspace layout. Workspace packages that only have the + // ID "command-line-arguments" are usually a symptom of a bad workspace + // configuration. + if onlyCommandLineArguments(wsPkgs) { + if criticalErr := snapshot.WorkspaceLayoutError(ctx); criticalErr != nil { + err = criticalErr + } + } + // Show the error as a progress error report so that it appears in the // status bar. If a client doesn't support progress reports, the error // will still be shown as a ShowMessage. If there is no error, any running @@ -577,3 +587,12 @@ func (s *Server) shouldIgnoreError(ctx context.Context, snapshot source.Snapshot }) return !hasGo } + +func onlyCommandLineArguments(pkgs []source.Package) bool { + for _, pkg := range pkgs { + if pkg.ID() != "command-line-arguments" { + return false + } + } + return true +} diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index ff9dade5fc..b440d23d5c 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -146,6 +146,11 @@ type Snapshot interface { // WorkspacePackages returns the snapshot's top-level packages. WorkspacePackages(ctx context.Context) ([]Package, error) + + // WorkspaceLayoutError reports whether there might be any problems with + // the user's workspace configuration, which would cause bad or incorrect + // diagnostics. + WorkspaceLayoutError(ctx context.Context) *CriticalError } // PackageFilter sets how a package is filtered out from a set of packages