internal/lsp: fix errors when adding new file to existing package
Previously when you added a new file to an existing package, the new
file would get stuck with the "no package for file" error until you
saved the file and then made changed a different file in the
package. There were two changes required to fix the errors:
First, we need to invalidate the package cache when a new file is
added to a package so that the package will actually re-parse and
re-type check. We now notice if file names changed when updating a
package's metadata and invalidate the package cache accordingly.
Second, when dealing with overlay (unsaved) files, we need to map
the *goFile to the package even if we fail to parse the
file (e.g. the new file fails to parse when it is empty). If we don't
map it to the package, the package won't get refreshed as the file is
changed.
Fixes golang/go#32341
Change-Id: I1a728fbedc79da7d5fe69554a5893efcd1e1d902
GitHub-Last-Rev: e7c3d4c1f8f73b12c87ee76d868cc04893e55808
GitHub-Pull-Request: golang/tools#111
Reviewed-on: https://go-review.googlesource.com/c/tools/+/181417
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index c31d3d1..99a4f67 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -152,18 +152,10 @@
}
func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata) {
- for _, fAST := range pkg.syntax {
+ for _, filename := range pkg.files {
// TODO: If a file is in multiple packages, which package do we store?
- if !fAST.file.Pos().IsValid() {
- imp.view.Session().Logger().Errorf(ctx, "invalid position for file %v", fAST.file.Name)
- continue
- }
- tok := imp.view.Session().Cache().FileSet().File(fAST.file.Pos())
- if tok == nil {
- imp.view.Session().Logger().Errorf(ctx, "no token.File for %v", fAST.file.Name)
- continue
- }
- fURI := span.FileURI(tok.Name())
+
+ fURI := span.FileURI(filename)
f, err := imp.view.getFile(fURI)
if err != nil {
imp.view.Session().Logger().Errorf(ctx, "no file: %v", err)
@@ -174,10 +166,28 @@
imp.view.Session().Logger().Errorf(ctx, "%v is not a Go file", f.URI())
continue
}
+
+ // Set the package even if we failed to parse the file otherwise
+ // future updates to this file won't refresh the package.
+ gof.pkg = pkg
+
+ fAST := pkg.syntax[filename]
+ if fAST == nil {
+ continue
+ }
+
+ if !fAST.file.Pos().IsValid() {
+ imp.view.Session().Logger().Errorf(ctx, "invalid position for file %v", fAST.file.Name)
+ continue
+ }
+ tok := imp.view.Session().Cache().FileSet().File(fAST.file.Pos())
+ if tok == nil {
+ imp.view.Session().Logger().Errorf(ctx, "no token.File for %v", fAST.file.Name)
+ continue
+ }
gof.token = tok
gof.ast = fAST
gof.imports = fAST.file.Imports
- gof.pkg = pkg
}
// Set imports of package to correspond to cached packages.
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index ea7f7bc..71da94c 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -138,6 +138,13 @@
func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Package, parent *metadata) *metadata {
m, ok := v.mcache.packages[pkgPath]
+
+ // If a file was added or deleted we need to invalidate the package cache
+ // so relevant packages get parsed and type checked again.
+ if ok && !filenamesIdentical(m.files, pkg.CompiledGoFiles) {
+ v.invalidatePackage(pkgPath)
+ }
+
if !ok {
m = &metadata{
pkgPath: pkgPath,
@@ -186,3 +193,24 @@
}
return m
}
+
+// filenamesIdentical reports whether two sets of file names are identical.
+func filenamesIdentical(oldFiles, newFiles []string) bool {
+ if len(oldFiles) != len(newFiles) {
+ return false
+ }
+
+ oldByName := make(map[string]struct{}, len(oldFiles))
+ for _, filename := range oldFiles {
+ oldByName[filename] = struct{}{}
+ }
+
+ for _, newFilename := range newFiles {
+ if _, found := oldByName[newFilename]; !found {
+ return false
+ }
+ delete(oldByName, newFilename)
+ }
+
+ return len(oldByName) == 0
+}
diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go
index 8fb8548..f339229 100644
--- a/internal/lsp/cache/parse.go
+++ b/internal/lsp/cache/parse.go
@@ -105,7 +105,7 @@
// Because files are scanned in parallel, the token.Pos
// positions of the resulting ast.Files are not ordered.
//
-func (imp *importer) parseFiles(filenames []string, ignoreFuncBodies bool) ([]*astFile, []error, error) {
+func (imp *importer) parseFiles(filenames []string, ignoreFuncBodies bool) (map[string]*astFile, []error, error) {
var (
wg sync.WaitGroup
n = len(filenames)
@@ -140,17 +140,15 @@
}
wg.Wait()
- // Eliminate nils, preserving order.
- var o int
- for _, f := range parsed {
+ parsedByFilename := make(map[string]*astFile)
+
+ for i, f := range parsed {
if f.file != nil {
- parsed[o] = f
- o++
+ parsedByFilename[filenames[i]] = f
}
}
- parsed = parsed[:o]
- o = 0
+ var o int
for _, err := range errors {
if err != nil {
errors[o] = err
@@ -159,7 +157,7 @@
}
errors = errors[:o]
- return parsed, errors, nil
+ return parsedByFilename, errors, nil
}
// sameFile returns true if x and y have the same basename and denote
diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go
index 9c3b71d..7a84785 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -23,7 +23,7 @@
pkgPath packagePath
files []string
- syntax []*astFile
+ syntax map[string]*astFile
errors []packages.Error
imports map[packagePath]*pkg
types *types.Package
@@ -146,9 +146,9 @@
}
func (pkg *pkg) GetSyntax() []*ast.File {
- syntax := make([]*ast.File, len(pkg.syntax))
- for i := range pkg.syntax {
- syntax[i] = pkg.syntax[i].file
+ syntax := make([]*ast.File, 0, len(pkg.syntax))
+ for _, astFile := range pkg.syntax {
+ syntax = append(syntax, astFile.file)
}
return syntax
}
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 2fb70ac..c7ef828 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -251,6 +251,14 @@
}
}
+// invalidatePackage removes the specified package and dependents from the
+// package cache.
+func (v *view) invalidatePackage(pkgPath packagePath) {
+ v.pcache.mu.Lock()
+ defer v.pcache.mu.Unlock()
+ v.remove(pkgPath, make(map[packagePath]struct{}))
+}
+
// remove invalidates a package and its reverse dependencies in the view's
// package cache. It is assumed that the caller has locked both the mutexes
// of both the mcache and the pcache.
