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>

internal/lsp/cache/check.go

@@ -152,18 +152,10 @@ func (imp *importer) typeCheck(pkgPath packagePath) (*pkg, error) {
 }
 
 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 @@ func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata)
 			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.

internal/lsp/cache/load.go

@@ -138,6 +138,13 @@ func (v *view) parseImports(ctx context.Context, f *goFile) bool {
 
 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 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack
 	}
 	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
+}

internal/lsp/cache/parse.go

@@ -105,7 +105,7 @@ func parseGo(ctx context.Context, c *cache, fh source.FileHandle, mode source.Pa
 // 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 @@ func (imp *importer) parseFiles(filenames []string, ignoreFuncBodies bool) ([]*a
 	}
 	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 @@ func (imp *importer) parseFiles(filenames []string, ignoreFuncBodies bool) ([]*a
 	}
 	errors = errors[:o]
 
-	return parsed, errors, nil
+	return parsedByFilename, errors, nil
 }
 
 // sameFile returns true if x and y have the same basename and denote

internal/lsp/cache/pkg.go

@@ -23,7 +23,7 @@ type pkg struct {
 	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) GetFilenames() []string {
 }
 
 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
 }

internal/lsp/cache/view.go

@@ -251,6 +251,14 @@ func (f *goFile) invalidateAST() {
 	}
 }
 
+// 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.