internal/lsp: fix bug where gopls hangs on manually typed imports
Fixes golang/go#32797
Change-Id: I0d2975561035c3ac5f5cd460ecdee452c2f6a17f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/184258
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go
index 23ecac6..9155040 100644
--- a/internal/lsp/cache/gofile.go
+++ b/internal/lsp/cache/gofile.go
@@ -56,6 +56,9 @@
return nil
}
}
+ f.mu.Lock()
+ defer f.mu.Unlock()
+
if unexpectedAST(ctx, f) {
return nil
}
@@ -72,6 +75,10 @@
return nil
}
}
+
+ f.mu.Lock()
+ defer f.mu.Unlock()
+
if f.ast == nil {
return nil
}
@@ -88,6 +95,9 @@
return nil
}
}
+ f.mu.Lock()
+ defer f.mu.Unlock()
+
if unexpectedAST(ctx, f) {
return nil
}
@@ -109,6 +119,10 @@
return nil
}
}
+
+ f.mu.Lock()
+ defer f.mu.Unlock()
+
if unexpectedAST(ctx, f) {
return nil
}
@@ -135,9 +149,6 @@
}
func unexpectedAST(ctx context.Context, f *goFile) bool {
- f.mu.Lock()
- defer f.mu.Unlock()
-
// If the AST comes back nil, something has gone wrong.
if f.ast == nil {
f.View().Session().Logger().Errorf(ctx, "expected full AST for %s, returned nil", f.URI())
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 73aeff6..6877650 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -75,8 +75,7 @@
// checkMetadata determines if we should run go/packages.Load for this file.
// If yes, update the metadata for the file and its package.
func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*metadata, []packages.Error, error) {
- filename, ok := v.runGopackages(ctx, f)
- if !ok {
+ if !v.runGopackages(ctx, f) {
return f.meta, nil, nil
}
@@ -85,10 +84,10 @@
return nil, nil, ctx.Err()
}
- pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", filename))
+ pkgs, err := packages.Load(v.buildConfig(), fmt.Sprintf("file=%s", f.filename()))
if len(pkgs) == 0 {
if err == nil {
- err = fmt.Errorf("go/packages.Load: no packages found for %s", filename)
+ err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename())
}
// Return this error as a diagnostic to the user.
return nil, []packages.Error{
@@ -106,14 +105,8 @@
if len(pkg.Errors) > 0 {
return nil, pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath)
}
- for importPath, importPkg := range pkg.Imports {
- // If we encounter a package we cannot import, mark it as missing.
- if importPkg.PkgPath != "unsafe" && len(importPkg.CompiledGoFiles) == 0 {
- missingImports[packagePath(importPath)] = struct{}{}
- }
- }
// Build the import graph for this package.
- if err := v.link(ctx, packagePath(pkg.PkgPath), pkg, nil); err != nil {
+ if err := v.link(ctx, packagePath(pkg.PkgPath), pkg, nil, missingImports); err != nil {
return nil, nil, err
}
}
@@ -145,7 +138,7 @@
// reparseImports reparses a file's package and import declarations to
// determine if they have changed.
-func (v *view) runGopackages(ctx context.Context, f *goFile) (filename string, result bool) {
+func (v *view) runGopackages(ctx context.Context, f *goFile) (result bool) {
f.mu.Lock()
defer func() {
// Clear metadata if we are intending to re-run go/packages.
@@ -163,12 +156,12 @@
}()
if len(f.meta) == 0 || len(f.missingImports) > 0 {
- return f.filename(), true
+ return true
}
// Get file content in case we don't already have it.
parsed, _ := v.session.cache.ParseGoHandle(f.Handle(ctx), source.ParseHeader).Parse(ctx)
if parsed == nil {
- return f.filename(), true
+ return true
}
// Check if the package's name has changed, by checking if this is a filename
// we already know about, and if so, check if its package name has changed.
@@ -176,24 +169,24 @@
for _, filename := range m.files {
if filename == f.URI().Filename() {
if m.name != parsed.Name.Name {
- return f.filename(), true
+ return true
}
}
}
}
// If the package's imports have changed, re-run `go list`.
if len(f.imports) != len(parsed.Imports) {
- return f.filename(), true
+ return true
}
for i, importSpec := range f.imports {
if importSpec.Path.Value != parsed.Imports[i].Path.Value {
- return f.filename(), true
+ return true
}
}
- return f.filename(), false
+ return false
}
-func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) error {
+func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata, missingImports map[packagePath]struct{}) error {
id := packageID(pkg.ID)
m, ok := v.mcache.packages[id]
@@ -223,11 +216,11 @@
for _, filename := range m.files {
f, err := v.getFile(ctx, span.FileURI(filename))
if err != nil {
- return err
+ v.session.log.Errorf(ctx, "no file %s: %v", filename, err)
}
gof, ok := f.(*goFile)
if !ok {
- return fmt.Errorf("not a Go file: %s", f.URI())
+ v.session.log.Errorf(ctx, "not a Go file: %s", f.URI())
}
if gof.meta == nil {
gof.meta = make(map[packageID]*metadata)
@@ -242,12 +235,16 @@
for importPath, importPkg := range pkg.Imports {
importPkgPath := packagePath(importPath)
if importPkgPath == pkgPath {
- v.session.log.Errorf(ctx, "cycle detected in %s", importPath)
- return nil
+ return fmt.Errorf("cycle detected in %s", importPath)
+ }
+ // Don't remember any imports with significant errors.
+ if importPkgPath != "unsafe" && len(pkg.CompiledGoFiles) == 0 {
+ missingImports[importPkgPath] = struct{}{}
+ continue
}
if _, ok := m.children[packageID(importPkg.ID)]; !ok {
- if err := v.link(ctx, importPkgPath, importPkg, m); err != nil {
- return err
+ if err := v.link(ctx, importPkgPath, importPkg, m, missingImports); err != nil {
+ v.session.log.Errorf(ctx, "error in dependency %s: %v", importPkgPath, err)
}
}
}
diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go
index 3a308a7..aa485a5 100644
--- a/internal/lsp/source/format.go
+++ b/internal/lsp/source/format.go
@@ -52,6 +52,9 @@
return nil, err
}
pkg := f.GetPackage(ctx)
+ if pkg == nil || pkg.IsIllTyped() {
+ return nil, fmt.Errorf("no package for file %s", f.URI())
+ }
if hasListErrors(pkg.GetErrors()) {
return nil, fmt.Errorf("%s has list errors, not running goimports", f.URI())
}
