diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index d0ee9753..b56be5f6 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -192,8 +192,8 @@ func (imp *importer) typeCheck(id packageID) (*pkg, error) { return pkg, nil } -func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, mode source.ParseMode) { - for _, file := range p.files { +func (imp *importer) cachePackage(ctx context.Context, pkg *pkg, meta *metadata, mode source.ParseMode) { + for _, file := range pkg.files { f, err := imp.view.getFile(file.uri) if err != nil { imp.view.session.log.Errorf(ctx, "no file: %v", err) @@ -204,35 +204,9 @@ func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, m imp.view.session.log.Errorf(ctx, "%v is not a Go file", file.uri) continue } - // Set the package even if we failed to parse the file. - if gof.pkgs == nil { - gof.pkgs = make(map[packageID]*pkg) + if err := imp.cachePerFile(gof, file, pkg); err != nil { + imp.view.session.log.Errorf(ctx, "failed to cache file %s: %v", gof.URI(), err) } - gof.pkgs[p.id] = p - - // Get the AST for the file. - gof.ast = file - if gof.ast == nil { - imp.view.session.log.Errorf(ctx, "no AST information for %s", file.uri) - continue - } - if gof.ast.file == nil { - imp.view.session.log.Errorf(ctx, "no AST for %s: %v", file.uri, err) - continue - } - // Get the *token.File directly from the AST. - pos := gof.ast.file.Pos() - if !pos.IsValid() { - imp.view.session.log.Errorf(ctx, "AST for %s has an invalid position", file.uri) - continue - } - tok := imp.view.session.cache.FileSet().File(pos) - if tok == nil { - imp.view.session.log.Errorf(ctx, "no *token.File for %s", file.uri) - continue - } - gof.token = tok - gof.imports = gof.ast.file.Imports } // Set imports of package to correspond to cached packages. @@ -243,10 +217,42 @@ func (imp *importer) cachePackage(ctx context.Context, p *pkg, meta *metadata, m if err != nil { continue } - p.imports[importPkg.pkgPath] = importPkg + pkg.imports[importPkg.pkgPath] = importPkg } } +func (imp *importer) cachePerFile(gof *goFile, file *astFile, p *pkg) error { + gof.mu.Lock() + defer gof.mu.Unlock() + + // Set the package even if we failed to parse the file. + if gof.pkgs == nil { + gof.pkgs = make(map[packageID]*pkg) + } + gof.pkgs[p.id] = p + + // Get the AST for the file. + gof.ast = file + if gof.ast == nil { + return fmt.Errorf("no AST information for %s", file.uri) + } + if gof.ast.file == nil { + return fmt.Errorf("no AST for %s", file.uri) + } + // Get the *token.File directly from the AST. + pos := gof.ast.file.Pos() + if !pos.IsValid() { + return fmt.Errorf("AST for %s has an invalid position", file.uri) + } + tok := imp.view.session.cache.FileSet().File(pos) + if tok == nil { + return fmt.Errorf("no *token.File for %s", file.uri) + } + gof.token = tok + gof.imports = gof.ast.file.Imports + return nil +} + func (c *cache) appendPkgError(pkg *pkg, err error) { if err == nil { return diff --git a/internal/lsp/cache/gofile.go b/internal/lsp/cache/gofile.go index 36ec12f9..0351aa68 100644 --- a/internal/lsp/cache/gofile.go +++ b/internal/lsp/cache/gofile.go @@ -8,6 +8,7 @@ import ( "context" "go/ast" "go/token" + "sync" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" @@ -17,7 +18,9 @@ import ( type goFile struct { fileBase - ast *astFile + // mu protects all mutable state of the Go file, + // 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. @@ -28,9 +31,11 @@ type goFile struct { // that we know about all of their packages. justOpened bool - pkgs map[packageID]*pkg - meta map[packageID]*metadata imports []*ast.ImportSpec + + ast *astFile + pkgs map[packageID]*pkg + meta map[packageID]*metadata } type astFile struct { @@ -130,13 +135,16 @@ func (f *goFile) GetPackage(ctx context.Context) source.Package { } 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()) return true } // If the AST comes back trimmed, something has gone wrong. - if f.astIsTrimmed() { + if f.ast.isTrimmed { f.View().Session().Logger().Errorf(ctx, "expected full AST for %s, returned trimmed", f.URI()) return true } @@ -146,6 +154,9 @@ func unexpectedAST(ctx context.Context, f *goFile) bool { // 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() bool { + f.mu.Lock() + defer f.mu.Unlock() + // If the the file has just been opened, // it may be part of more packages than we are aware of. // @@ -166,6 +177,9 @@ func (f *goFile) isDirty() bool { } func (f *goFile) astIsTrimmed() bool { + f.mu.Lock() + defer f.mu.Unlock() + return f.ast != nil && f.ast.isTrimmed } diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 1368d56a..913ef4a3 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -20,59 +20,43 @@ func (v *view) loadParseTypecheck(ctx context.Context, f *goFile) ([]packages.Er // If the AST for this file is trimmed, and we are explicitly type-checking it, // don't ignore function bodies. if f.astIsTrimmed() { + v.pcache.mu.Lock() f.invalidateAST() + v.pcache.mu.Unlock() } - // Save the metadata's current missing imports, if any. - originalMissingImports := f.missingImports - // Check if we need to run go/packages.Load for this file's package. - if errs, err := v.checkMetadata(ctx, f); err != nil { + // Get the metadata for the file. + meta, errs, err := v.checkMetadata(ctx, f) + if err != nil { return errs, err } - - // If `go list` failed to get data for the file in question (this should never happen). - if len(f.meta) == 0 { - return nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename()) - } - - // If we have already seen these missing imports before, and we still have type information, - // there is no need to continue. - if sameSet(originalMissingImports, f.missingImports) && len(f.pkgs) > 0 { + if meta == nil { return nil, nil } - - for id, meta := range f.meta { - if _, ok := f.pkgs[id]; ok { - continue - } + for id, m := range meta { imp := &importer{ view: v, seen: make(map[packageID]struct{}), ctx: ctx, - fset: f.FileSet(), - topLevelPkgID: meta.id, + fset: v.session.cache.FileSet(), + topLevelPkgID: id, } // Start prefetching direct imports. - for importID := range meta.children { + for importID := range m.children { go imp.getPkg(importID) } // Type-check package. - pkg, err := imp.getPkg(meta.id) + pkg, err := imp.getPkg(imp.topLevelPkgID) if err != nil { return nil, err } if pkg == nil || pkg.IsIllTyped() { - return nil, fmt.Errorf("loadParseTypecheck: %s is ill typed", meta.pkgPath) - } - // If we still have not found the package for the file, something is wrong. - if f.pkgs[id] == nil { - v.Session().Logger().Errorf(ctx, "failed to type-check package %s", meta.pkgPath) + return nil, fmt.Errorf("loadParseTypecheck: %s is ill typed", m.pkgPath) } } if len(f.pkgs) == 0 { - return nil, fmt.Errorf("loadParseTypeCheck: no packages found for %v", f.filename()) + return nil, fmt.Errorf("no packages found for %s", f.URI()) } - return nil, nil } @@ -90,9 +74,15 @@ func sameSet(x, y map[packagePath]struct{}) bool { // 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) ([]packages.Error, error) { +func (v *view) checkMetadata(ctx context.Context, f *goFile) (map[packageID]*metadata, []packages.Error, error) { + f.mu.Lock() + defer f.mu.Unlock() + + // Save the metadata's current missing imports, if any. + originalMissingImports := f.missingImports + if !v.parseImports(ctx, f) { - return nil, nil + return f.meta, nil, nil } // Reset the file's metadata and type information if we are re-running `go list`. @@ -109,7 +99,7 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, err = fmt.Errorf("go/packages.Load: no packages found for %s", f.filename()) } // Return this error as a diagnostic to the user. - return []packages.Error{ + return nil, []packages.Error{ { Msg: err.Error(), Kind: packages.UnknownError, @@ -125,7 +115,7 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, // If the package comes back with errors from `go list`, // don't bother type-checking it. if len(pkg.Errors) > 0 { - return pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath) + return nil, pkg.Errors, fmt.Errorf("package %s has errors, skipping type-checking", pkg.PkgPath) } for importPath, importPkg := range pkg.Imports { if len(importPkg.Errors) > 0 { @@ -138,7 +128,19 @@ func (v *view) checkMetadata(ctx context.Context, f *goFile) ([]packages.Error, // Build the import graph for this package. v.link(ctx, packagePath(pkg.PkgPath), pkg, nil) } - return nil, nil + + // If `go list` failed to get data for the file in question (this should never happen). + if len(f.meta) == 0 { + return nil, nil, fmt.Errorf("loadParseTypecheck: no metadata found for %v", f.filename()) + } + + // If we have already seen these missing imports before, and we still have type information, + // there is no need to continue. + if sameSet(originalMissingImports, f.missingImports) && len(f.pkgs) != 0 { + return nil, nil, nil + } + + return f.meta, nil, nil } // reparseImports reparses a file's package and import declarations to @@ -158,6 +160,7 @@ func (v *view) parseImports(ctx context.Context, f *goFile) bool { if len(f.imports) != len(parsed.Imports) { return true } + for i, importSpec := range f.imports { if importSpec.Path.Value != parsed.Imports[i].Path.Value { return true @@ -173,7 +176,9 @@ func (v *view) link(ctx context.Context, pkgPath packagePath, pkg *packages.Pack // 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(id) + v.pcache.mu.Lock() + v.remove(id, make(map[packageID]struct{})) + v.pcache.mu.Unlock() } // If we haven't seen this package before. @@ -234,18 +239,15 @@ 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/session.go b/internal/lsp/cache/session.go index 90880539..16a928ea 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -203,7 +203,9 @@ func (s *session) DidOpen(ctx context.Context, uri span.URI) { return } // Mark file as open. + gof.mu.Lock() gof.justOpened = true + gof.mu.Unlock() } } } diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 76fade1a..ec848a0c 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -228,6 +228,12 @@ func (f *goFile) invalidateContent() { f.handleMu.Lock() defer f.handleMu.Unlock() + f.view.mcache.mu.Lock() + defer f.view.mcache.mu.Unlock() + + f.view.pcache.mu.Lock() + defer f.view.pcache.mu.Unlock() + f.invalidateAST() f.handle = nil } @@ -235,29 +241,20 @@ func (f *goFile) invalidateContent() { // invalidateAST invalidates the AST of a Go file, // including any position and type information that depends on it. func (f *goFile) invalidateAST() { - f.view.pcache.mu.Lock() - defer f.view.pcache.mu.Unlock() - + f.mu.Lock() f.ast = nil f.token = nil + pkgs := f.pkgs + f.mu.Unlock() // Remove the package and all of its reverse dependencies from the cache. - for id, pkg := range f.pkgs { + for id, pkg := range pkgs { if pkg != nil { f.view.remove(id, map[packageID]struct{}{}) } } } -// invalidatePackage removes the specified package and dependents from the -// package cache. -func (v *view) invalidatePackage(id packageID) { - v.pcache.mu.Lock() - defer v.pcache.mu.Unlock() - - v.remove(id, make(map[packageID]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. @@ -278,7 +275,9 @@ func (v *view) remove(id packageID, seen map[packageID]struct{}) { for _, filename := range m.files { if f, _ := v.findFile(span.FileURI(filename)); f != nil { if gof, ok := f.(*goFile); ok { + gof.mu.Lock() delete(gof.pkgs, id) + gof.mu.Unlock() } } } @@ -341,7 +340,11 @@ func (v *view) getFile(uri span.URI) (viewFile, error) { }, } v.session.filesWatchMap.Watch(uri, func() { - f.(*goFile).invalidateContent() + gof, ok := f.(*goFile) + if !ok { + return + } + gof.invalidateContent() }) } v.mapFile(uri, f)