internal/lsp: move the missing imports handling into the metadata
This change uses the missing imports detection to return diagnostics and
warning messages to the user.
Updates golang/go#34484
Change-Id: If7bb7e702b8bdbd7e1ad5e26f93acc50d16209b0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196985
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index f1b94f1..2f80208 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -158,6 +158,14 @@
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 a08cb19..1102cec 100644
--- a/internal/lsp/cache/gofile.go
+++ b/internal/lsp/cache/gofile.go
@@ -23,10 +23,6 @@
// 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 @@
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 @@
}
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 d70e091..9d99eff 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -27,6 +27,11 @@
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 @@
// 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 @@
}
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 5cf1660..e0fc01a 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -24,9 +24,8 @@
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 38312ec..a998fa7 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -20,17 +20,18 @@
}
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 @@
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 @@
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 @@
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 @@
}
// 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 f6a42b7..5176ca5 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -47,6 +47,16 @@
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 @@
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 9b740fe..634e287 100644
--- a/internal/lsp/source/errors.go
+++ b/internal/lsp/source/errors.go
@@ -12,7 +12,20 @@
"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 @@
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 2019186..77556af 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -122,6 +122,8 @@
// 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
