diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index f1b94f1a94..2f8020869a 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -158,6 +158,14 @@ func (cph *checkPackageHandle) ID() string { return string(cph.m.id) } +func (cph *checkPackageHandle) MissingDependencies() []string { + var md []string + for i := range cph.m.missingDeps { + md = append(md, string(i)) + } + return md +} + func (cph *checkPackageHandle) Cached(ctx context.Context) (source.Package, error) { return cph.cached(ctx) } diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index a08cb1935b..1102cec943 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -23,10 +23,6 @@ type goFile struct { // which can be modified during type-checking. mu sync.Mutex - // missingImports is the set of unresolved imports for this package. - // It contains any packages with `go list` errors. - missingImports map[packagePath]struct{} - imports []*ast.ImportSpec } @@ -35,21 +31,47 @@ type packageKey struct { mode source.ParseMode } -func (f *goFile) CheckPackageHandles(ctx context.Context) (cphs []source.CheckPackageHandle, err error) { +func (f *goFile) CheckPackageHandles(ctx context.Context) (results []source.CheckPackageHandle, err error) { ctx = telemetry.File.With(ctx, f.URI()) fh := f.Handle(ctx) - cphs = f.isDirty(ctx, fh) - if len(cphs) == 0 { - cphs, err = f.view.loadParseTypecheck(ctx, f, fh) + var dirty bool + meta, pkgs := f.view.getSnapshot(f.URI()) + if len(meta) == 0 { + dirty = true + } + for _, m := range meta { + if len(m.missingDeps) != 0 { + dirty = true + } + } + for _, cph := range pkgs { + // If we're explicitly checking if a file needs to be type-checked, + // we need it to be fully parsed. + if cph.mode() != source.ParseFull { + continue + } + // Check if there is a fully-parsed package to which this file belongs. + for _, file := range cph.Files() { + if file.File().Identity() == fh.Identity() { + results = append(results, cph) + } + } + } + if dirty || len(results) == 0 { + cphs, err := f.view.loadParseTypecheck(ctx, f, fh) if err != nil { return nil, err } + if cphs == nil { + return results, nil + } + results = cphs } - if len(cphs) == 0 { + if len(results) == 0 { return nil, errors.Errorf("no CheckPackageHandles for %s", f.URI()) } - return cphs, nil + return results, nil } func (v *view) GetActiveReverseDeps(ctx context.Context, uri span.URI) (results []source.CheckPackageHandle) { @@ -78,27 +100,3 @@ func (v *view) GetActiveReverseDeps(ctx context.Context, uri span.URI) (results } return results } - -// isDirty is true if the file needs to be type-checked. -// It assumes that the file's view's mutex is held by the caller. -func (f *goFile) isDirty(ctx context.Context, fh source.FileHandle) []source.CheckPackageHandle { - meta, cphs := f.view.getSnapshot(f.URI()) - if len(meta) == 0 { - return nil - } - var results []source.CheckPackageHandle - for key, cph := range cphs { - // If we're explicitly checking if a file needs to be type-checked, - // we need it to be fully parsed. - if key.mode != source.ParseFull { - continue - } - // Check if there is a fully-parsed package to which this file belongs. - for _, file := range cph.Files() { - if file.File().Identity() == fh.Identity() { - results = append(results, cph) - } - } - } - return results -} diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index d70e091c85..9d99eff9f8 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -27,6 +27,11 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile, fh source.File if err != nil { return nil, err } + // If load has explicitly returns nil metadata and no error, + // it means that we should not re-typecheck the packages. + if meta == nil { + return nil, nil + } var ( cphs []*checkPackageHandle results []source.CheckPackageHandle @@ -129,7 +134,15 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile, fh source.FileHandl // Return this error as a diagnostic to the user. return nil, err } - return v.updateMetadata(ctx, f.URI(), pkgs) + m, prevMissingImports, err := v.updateMetadata(ctx, f.URI(), pkgs) + if err != nil { + return nil, err + } + meta, err := validateMetadata(ctx, m, prevMissingImports) + if err != nil { + return nil, err + } + return meta, nil } // shouldRunGopackages reparses a file's package and import declarations to @@ -161,3 +174,36 @@ func (v *view) shouldRunGopackages(ctx context.Context, f *goFile, file *ast.Fil } return false } + +func validateMetadata(ctx context.Context, metadata []*metadata, prevMissingImports map[packageID]map[packagePath]struct{}) ([]*metadata, error) { + // If we saw incorrect metadata for this package previously, don't both rechecking it. + for _, m := range metadata { + if len(m.missingDeps) > 0 { + prev, ok := prevMissingImports[m.id] + // There are missing imports that we previously hadn't seen before. + if !ok { + return metadata, nil + } + // The set of missing imports has changed. + if !sameSet(prev, m.missingDeps) { + return metadata, nil + } + } else { + // There are no missing imports. + return metadata, nil + } + } + return nil, nil +} + +func sameSet(x, y map[packagePath]struct{}) bool { + if len(x) != len(y) { + return false + } + for k := range x { + if _, ok := y[k]; !ok { + return false + } + } + return true +} diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 5cf1660850..e0fc01ad92 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -24,9 +24,8 @@ type pkg struct { view *view // ID and package path have their own types to avoid being used interchangeably. - id packageID - pkgPath packagePath - + id packageID + pkgPath packagePath files []source.ParseGoHandle errors []packages.Error imports map[packagePath]*pkg diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 38312ec4f9..a998fa7fdd 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -20,17 +20,18 @@ type snapshot struct { } type metadata struct { - id packageID - pkgPath packagePath - name string - files []span.URI - typesSizes types.Sizes - parents map[packageID]bool - children map[packageID]*metadata - errors []packages.Error + id packageID + pkgPath packagePath + name string + files []span.URI + typesSizes types.Sizes + parents map[packageID]bool + children map[packageID]*metadata + errors []packages.Error + missingDeps map[packagePath]struct{} } -func (v *view) getSnapshot(uri span.URI) ([]*metadata, map[packageKey]*checkPackageHandle) { +func (v *view) getSnapshot(uri span.URI) ([]*metadata, []*checkPackageHandle) { v.snapshotMu.Lock() defer v.snapshotMu.Unlock() @@ -38,7 +39,11 @@ func (v *view) getSnapshot(uri span.URI) ([]*metadata, map[packageKey]*checkPack for _, id := range v.snapshot.ids[uri] { m = append(m, v.snapshot.metadata[id]) } - return m, v.snapshot.packages[uri] + var cphs []*checkPackageHandle + for _, cph := range v.snapshot.packages[uri] { + cphs = append(cphs, cph) + } + return m, cphs } func (v *view) getMetadata(uri span.URI) []*metadata { @@ -59,11 +64,17 @@ func (v *view) getPackages(uri span.URI) map[packageKey]*checkPackageHandle { return v.snapshot.packages[uri] } -func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*packages.Package) ([]*metadata, error) { +func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*packages.Package) ([]*metadata, map[packageID]map[packagePath]struct{}, error) { v.snapshotMu.Lock() defer v.snapshotMu.Unlock() // Clear metadata since we are re-running go/packages. + prevMissingImports := make(map[packageID]map[packagePath]struct{}) + for _, id := range v.snapshot.ids[uri] { + if m, ok := v.snapshot.metadata[id]; ok && len(m.missingDeps) > 0 { + prevMissingImports[id] = m.missingDeps + } + } without := make(map[span.URI]struct{}) for _, id := range v.snapshot.ids[uri] { v.remove(id, without, map[packageID]struct{}{}) @@ -80,11 +91,14 @@ func (v *view) updateMetadata(ctx context.Context, uri span.URI, pkgs []*package pkg: pkg, parent: nil, }); err != nil { - return nil, err + return nil, nil, err } results = append(results, v.snapshot.metadata[packageID(pkg.ID)]) } - return results, nil + if len(results) == 0 { + return nil, nil, errors.Errorf("no metadata for %s", uri) + } + return results, prevMissingImports, nil } type importGraph struct { @@ -134,6 +148,10 @@ func (v *view) updateImportGraph(ctx context.Context, g *importGraph) error { } // Don't remember any imports with significant errors. if importPkgPath != "unsafe" && len(importPkg.CompiledGoFiles) == 0 { + if m.missingDeps == nil { + m.missingDeps = make(map[packagePath]struct{}) + } + m.missingDeps[importPkgPath] = struct{}{} continue } if _, ok := m.children[packageID(importPkg.ID)]; !ok { diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index f6a42b74a5..5176ca5e28 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -47,6 +47,16 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ return nil, "", err } cph := WidestCheckPackageHandle(cphs) + + // If we are missing dependencies, it may because the user's workspace is + // not correctly configured. Report errors, if possible. + var warningMsg string + if len(cph.MissingDependencies()) > 0 { + warningMsg, err = checkCommonErrors(ctx, view, f.URI()) + if err != nil { + log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI)) + } + } pkg, err := cph.Check(ctx) if err != nil { log.Error(ctx, "no package for file", err) @@ -59,15 +69,6 @@ func Diagnostics(ctx context.Context, view View, f GoFile, disabledAnalyses map[ clearReports(view, reports, fh.File().Identity().URI) } - // If we have `go list` errors, we may want to offer a warning message to the user. - var warningMsg string - if hasListErrors(pkg.GetErrors()) { - warningMsg, err = checkCommonErrors(ctx, view, f.URI()) - if err != nil { - log.Error(ctx, "error checking common errors", err, telemetry.File.Of(f.URI)) - } - } - // Prepare any additional reports for the errors in this package. for _, err := range pkg.GetErrors() { if err.Kind != packages.ListError { diff --git a/internal/lsp/source/errors.go b/internal/lsp/source/errors.go index 9b740fe645..634e287562 100644 --- a/internal/lsp/source/errors.go +++ b/internal/lsp/source/errors.go @@ -12,7 +12,20 @@ import ( "golang.org/x/tools/internal/span" ) +const ( + // TODO(rstambler): We should really be able to point to a link on the website. + modulesWiki = "https://github.com/golang/go/wiki/Modules" +) + func checkCommonErrors(ctx context.Context, view View, uri span.URI) (string, error) { + // Unfortunately, we probably can't have go/packages expose a function like this. + // Since we only really understand the `go` command, check the user's GOPACKAGESDRIVER + // and, if they are using `go list`, consider the possible error cases. + gopackagesdriver := os.Getenv("GOPACKAGESDRIVER") + if gopackagesdriver != "" && gopackagesdriver != "off" { + return "", nil + } + // Some cases we should be able to detect: // // 1. The user is in GOPATH mode and is working outside their GOPATH @@ -49,7 +62,7 @@ func checkCommonErrors(ctx context.Context, view View, uri span.URI) (string, er msg = "You are in module mode, but you are not inside of a module. Please create a module." } } else { - msg = "You are neither in a module nor in your GOPATH. Please see X for information on how to set up your Go project." + msg = fmt.Sprintf("You are neither in a module nor in your GOPATH. Please see %s for information on how to set up your Go project.", modulesWiki) } return msg, nil } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 20191866e6..77556afc53 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -122,6 +122,8 @@ type CheckPackageHandle interface { // Cached returns the Package for the CheckPackageHandle if it has already been stored. Cached(ctx context.Context) (Package, error) + + MissingDependencies() []string } // Cache abstracts the core logic of dealing with the environment from the