internal/lsp: fix race condition in type-checking

Change-Id: Idff7cc3a28b4a03ce0d374c6c5035b51330143b1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/178724
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Cottrell <[email protected]>

Author: stamblerre

internal/lsp/cache/check.go

@@ -51,7 +51,7 @@ func (v *view) parse(ctx context.Context, f *goFile) ([]packages.Error, error) {
 		go imp.Import(importPath)
 	}
 	// Type-check package.
-	pkg, err := imp.typeCheck(f.meta.pkgPath)
+	pkg, err := imp.getPkg(f.meta.pkgPath)
 	if pkg == nil || pkg.GetTypes() == nil {
 		return nil, err
 	}
@@ -181,6 +181,14 @@ type importer struct {
 }
 
 func (imp *importer) Import(pkgPath string) (*types.Package, error) {
+	pkg, err := imp.getPkg(pkgPath)
+	if err != nil {
+		return nil, err
+	}
+	return pkg.types, nil
+}
+
+func (imp *importer) getPkg(pkgPath string) (*pkg, error) {
 	if _, ok := imp.seen[pkgPath]; ok {
 		return nil, fmt.Errorf("circular import detected")
 	}
@@ -205,7 +213,7 @@ func (imp *importer) Import(pkgPath string) (*types.Package, error) {
 	if e.err != nil {
 		return nil, e.err
 	}
-	return e.pkg.types, nil
+	return e.pkg, nil
 }
 
 func (imp *importer) typeCheck(pkgPath string) (*pkg, error) {
@@ -266,32 +274,32 @@ func (imp *importer) typeCheck(pkgPath string) (*pkg, error) {
 	check.Files(pkg.syntax)
 
 	// Add every file in this package to our cache.
-	imp.view.cachePackage(imp.ctx, pkg, meta)
+	imp.cachePackage(imp.ctx, pkg, meta)
 
 	return pkg, nil
 }
 
-func (v *view) cachePackage(ctx context.Context, pkg *pkg, meta *metadata) {
+func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata) {
 	for _, file := range pkg.GetSyntax() {
 		// TODO: If a file is in multiple packages, which package do we store?
 		if !file.Pos().IsValid() {
-			v.Session().Logger().Errorf(ctx, "invalid position for file %v", file.Name)
+			imp.view.Session().Logger().Errorf(ctx, "invalid position for file %v", file.Name)
 			continue
 		}
-		tok := v.Session().Cache().FileSet().File(file.Pos())
+		tok := imp.view.Session().Cache().FileSet().File(file.Pos())
 		if tok == nil {
-			v.Session().Logger().Errorf(ctx, "no token.File for %v", file.Name)
+			imp.view.Session().Logger().Errorf(ctx, "no token.File for %v", file.Name)
 			continue
 		}
 		fURI := span.FileURI(tok.Name())
-		f, err := v.getFile(fURI)
+		f, err := imp.view.getFile(fURI)
 		if err != nil {
-			v.Session().Logger().Errorf(ctx, "no file: %v", err)
+			imp.view.Session().Logger().Errorf(ctx, "no file: %v", err)
 			continue
 		}
 		gof, ok := f.(*goFile)
 		if !ok {
-			v.Session().Logger().Errorf(ctx, "not a go file: %v", f.URI())
+			imp.view.Session().Logger().Errorf(ctx, "not a go file: %v", f.URI())
 			continue
 		}
 		gof.token = tok
@@ -300,26 +308,15 @@ func (v *view) cachePackage(ctx context.Context, pkg *pkg, meta *metadata) {
 		gof.pkg = pkg
 	}
 
-	v.pcache.mu.Lock()
-	defer v.pcache.mu.Unlock()
-
-	// Cache the entry for this package.
-	// All dependencies are cached through calls to *imp.Import.
-	e := &entry{
-		pkg:   pkg,
-		err:   nil,
-		ready: make(chan struct{}),
-	}
-	close(e.ready)
-	v.pcache.packages[pkg.pkgPath] = e
-
 	// Set imports of package to correspond to cached packages.
 	// We lock the package cache, but we shouldn't get any inconsistencies
 	// because we are still holding the lock on the view.
 	for importPath := range meta.children {
-		if importEntry, ok := v.pcache.packages[importPath]; ok {
-			pkg.imports[importPath] = importEntry.pkg
+		importPkg, err := imp.getPkg(importPath)
+		if err != nil {
+			continue
 		}
+		pkg.imports[importPath] = importPkg
 	}
 }