From ccae4fb384a35a4a3ec50178abc6887c141e76f4 Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sat, 21 Nov 2020 01:17:56 -0500 Subject: [PATCH] internal/lsp: use mode (GOPATH/modules) oriented error messages We can offer better error messages to users in GOPATH mode. The Go command has clear error messages indicating where it's looking for its dependencies, so we can borrow these messages too. The error message will look like this: https://user-images.githubusercontent.com/5856771/100292862-bde8bb00-2f4f-11eb-9920-d945ee2c7e87.png Change-Id: I888b33804eec7f38c329ae2e4343b82fcc22f1df Reviewed-on: https://go-review.googlesource.com/c/tools/+/272166 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Heschi Kreinick --- gopls/internal/regtest/diagnostics_test.go | 37 ++++++++++++++++++++++ internal/lsp/cache/check.go | 21 +++++++++++- internal/lsp/cache/view.go | 3 +- internal/lsp/cmd/test/check.go | 8 ----- internal/lsp/testdata/bad/badimport.go | 5 --- internal/lsp/testdata/summary.txt.golden | 2 +- internal/lsp/tests/util.go | 4 --- 7 files changed, 60 insertions(+), 20 deletions(-) delete mode 100644 internal/lsp/testdata/bad/badimport.go diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index 8a7008ab2a..74687b81fc 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1557,3 +1557,40 @@ import _ "mod.com/triple/a" ) }) } + +func TestBadImport(t *testing.T) { + testenv.NeedsGo1Point(t, 14) + + const mod = ` +-- go.mod -- +module mod.com + +go 1.12 +-- main.go -- +package main + +import ( + _ "nosuchpkg" +) +` + t.Run("module", func(t *testing.T) { + run(t, mod, func(t *testing.T, env *Env) { + env.Await( + env.DiagnosticAtRegexpWithMessage("main.go", `"nosuchpkg"`, `could not import nosuchpkg (no required module provides package "nosuchpkg"`), + ) + }) + }) + t.Run("GOPATH", func(t *testing.T) { + withOptions( + InGOPATH(), + EditorConfig{ + Env: map[string]string{"GO111MODULE": "off"}, + }, + WithModes(WithoutExperiments), + ).run(t, mod, func(t *testing.T, env *Env) { + env.Await( + env.DiagnosticAtRegexpWithMessage("main.go", `"nosuchpkg"`, `cannot find package "nosuchpkg" in any of`), + ) + }) + }) +} diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 187a39142a..1760d388c9 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -11,6 +11,7 @@ import ( "go/ast" "go/types" "path" + "path/filepath" "sort" "strings" "sync" @@ -407,7 +408,7 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source } dep := resolveImportPath(pkgPath, pkg, deps) if dep == nil { - return nil, errors.Errorf("no package for import %s", pkgPath) + return nil, snapshot.missingPkgError(pkgPath) } if !isValidImport(m.pkgPath, dep.m.pkgPath) { return nil, errors.Errorf("invalid use of internal package %s", pkgPath) @@ -453,6 +454,24 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source return pkg, nil } +// missingPkgError returns an error message for a missing package that varies +// based on the user's workspace mode. +func (s *snapshot) missingPkgError(pkgPath string) error { + if s.workspaceMode()&moduleMode != 0 { + return fmt.Errorf("no required module provides package %q", pkgPath) + } + gorootSrcPkg := filepath.FromSlash(filepath.Join(s.view.goroot, "src", pkgPath)) + + var b strings.Builder + b.WriteString(fmt.Sprintf("cannot find package %q in any of \n\t%s (from $GOROOT)", pkgPath, gorootSrcPkg)) + + for _, gopath := range strings.Split(s.view.gopath, ":") { + gopathSrcPkg := filepath.FromSlash(filepath.Join(gopath, "src", pkgPath)) + b.WriteString(fmt.Sprintf("\n\t%s (from $GOPATH)", gopathSrcPkg)) + } + return errors.New(b.String()) +} + type extendedError struct { primary types.Error secondaries []types.Error diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 8c682bf340..75c62ddb3f 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -119,7 +119,7 @@ type workspaceInformation struct { } type environmentVariables struct { - gocache, gopath, goprivate, gomodcache string + gocache, gopath, goroot, goprivate, gomodcache string } type workspaceMode int @@ -784,6 +784,7 @@ func (s *Session) getGoEnv(ctx context.Context, folder string, configEnv []strin vars := map[string]*string{ "GOCACHE": &envVars.gocache, "GOPATH": &envVars.gopath, + "GOROOT": &envVars.goroot, "GOPRIVATE": &envVars.goprivate, "GOMODCACHE": &envVars.gomodcache, } diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go index 1e07df357b..f0e6d8fefb 100644 --- a/internal/lsp/cmd/test/check.go +++ b/internal/lsp/cmd/test/check.go @@ -50,10 +50,6 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnost expect = fmt.Sprintf("%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Message) } expect = r.NormalizePrefix(expect) - // Skip the badimport test for now, until we do a better job with diagnostic ranges. - if strings.Contains(uri.Filename(), "badimport") { - continue - } _, found := got[expect] if !found { t.Errorf("missing diagnostic %q, %v", expect, got) @@ -62,10 +58,6 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnost } } for extra := range got { - // Skip the badimport test for now, until we do a better job with diagnostic ranges. - if strings.Contains(extra, "badimport") { - continue - } t.Errorf("extra diagnostic %q", extra) } } diff --git a/internal/lsp/testdata/bad/badimport.go b/internal/lsp/testdata/bad/badimport.go deleted file mode 100644 index fefc22e464..0000000000 --- a/internal/lsp/testdata/bad/badimport.go +++ /dev/null @@ -1,5 +0,0 @@ -package bad - -import ( - _ "nosuchpkg" //@diag("_", "compiler", "could not import nosuchpkg (no package for import nosuchpkg)", "error") -) diff --git a/internal/lsp/testdata/summary.txt.golden b/internal/lsp/testdata/summary.txt.golden index 0f327f6ca5..b97325801c 100644 --- a/internal/lsp/testdata/summary.txt.golden +++ b/internal/lsp/testdata/summary.txt.golden @@ -8,7 +8,7 @@ DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 RankedCompletionsCount = 158 CaseSensitiveCompletionsCount = 4 -DiagnosticsCount = 38 +DiagnosticsCount = 37 FoldingRangesCount = 2 FormatCount = 6 ImportCount = 8 diff --git a/internal/lsp/tests/util.go b/internal/lsp/tests/util.go index 0c050d0507..9fed4deb77 100644 --- a/internal/lsp/tests/util.go +++ b/internal/lsp/tests/util.go @@ -129,10 +129,6 @@ func DiffDiagnostics(uri span.URI, want, got []*source.Diagnostic) string { if w.Source != g.Source { return summarizeDiagnostics(i, uri, want, got, "incorrect Source got %v want %v", g.Source, w.Source) } - // Don't check the range on the badimport test. - if strings.Contains(uri.Filename(), "badimport") { - continue - } if protocol.ComparePosition(w.Range.Start, g.Range.Start) != 0 { return summarizeDiagnostics(i, uri, want, got, "incorrect Start got %v want %v", g.Range.Start, w.Range.Start) }