internal/lsp/cache: invalidate metadata when parsing issues resolve

Package loading (at least via go list) fails when the import header
doesn't parse, so we need to invalidate metadata upon a state change
from non parsing->parsing. Refactor metadata invalidation to implement
this feature, add additional documentation, and avoid unnecessary work.

This change revealed a latent bug (via TestDeleteDirectory): when
statting a deleted directory failed, we could fail to invalidate any
package IDs, even those we already knew about. This bug was masked by
the somewhat complicated semantics of pkgNameChanged. The semantics of
pkgNameChanged are simplified to encapsulate any change to a
package->file association.

Also refactor the parsing API somewhat, and add a bug report if we don't
get a ParseGoFile while inspecting for metadata changes.

Update most regtests to panic upon receiving bug reports.

Fixes golang/go#52981

Change-Id: I1838963ecc9c01e316f887aa9d8f1260662281ab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/407501
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
This commit is contained in:
Robert Findley 2022-05-20 11:35:51 -04:00
parent 904e24e9fc
commit ccb10502d1
15 changed files with 267 additions and 122 deletions

View File

@ -12,6 +12,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/fake"
. "golang.org/x/tools/internal/lsp/regtest"
@ -19,6 +20,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -11,6 +11,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp/command"
@ -21,6 +22,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -10,6 +10,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp/fake"
@ -18,6 +19,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -11,6 +11,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp"
@ -20,6 +21,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -8,9 +8,11 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/regtest"
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
regtest.Main(m, hooks.Options)
}

View File

@ -11,6 +11,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp/protocol"
@ -19,6 +20,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -9,11 +9,13 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/protocol"
. "golang.org/x/tools/internal/lsp/regtest"
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -8,6 +8,7 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/bug"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp/fake"
@ -16,6 +17,7 @@ import (
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -0,0 +1,43 @@
// Copyright 2022 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
package workspace
import (
"testing"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/testenv"
)
// TODO(rfindley): move workspace tests related to metadata bugs into this
// file.
func TestFixImportDecl(t *testing.T) {
// It appears that older Go versions don't even see p.go from the initial
// workspace load.
testenv.NeedsGo1Point(t, 15)
const src = `
-- go.mod --
module mod.test
go 1.12
-- p.go --
package p
import (
_ "fmt"
const C = 42
`
Run(t, src, func(t *testing.T, env *Env) {
env.OpenFile("p.go")
env.RegexpReplace("p.go", "\"fmt\"", "\"fmt\"\n)")
env.Await(OnceMet(
env.DoneWithChange(),
EmptyDiagnostics("p.go"),
))
})
}

View File

@ -12,15 +12,17 @@ import (
"testing"
"golang.org/x/tools/gopls/internal/hooks"
. "golang.org/x/tools/internal/lsp/regtest"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/fake"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/source"
"golang.org/x/tools/internal/testenv"
. "golang.org/x/tools/internal/lsp/regtest"
)
func TestMain(m *testing.M) {
bug.PanicOnBugs = true
Main(m, hooks.Options)
}

View File

@ -19,6 +19,11 @@ import (
"sync"
)
// PanicOnBugs controls whether to panic when bugs are reported.
//
// It may be set to true during testing.
var PanicOnBugs = false
var (
mu sync.Mutex
exemplars map[string]Bug
@ -39,6 +44,11 @@ type Bug struct {
// Data is additional metadata to record for a bug.
type Data map[string]interface{}
// Reportf reports a formatted bug message.
func Reportf(format string, args ...interface{}) {
Report(fmt.Sprintf(format, args...), nil)
}
// Report records a new bug encountered on the server.
// It uses reflection to report the position of the immediate caller.
func Report(description string, data Data) {
@ -49,6 +59,10 @@ func Report(description string, data Data) {
key = fmt.Sprintf("%s:%d", file, line)
}
if PanicOnBugs {
panic(fmt.Sprintf("%s: %s", key, description))
}
bug := Bug{
File: file,
Line: line,

View File

@ -570,8 +570,7 @@ func parseCompiledGoFiles(ctx context.Context, snapshot *snapshot, mode source.P
// Only parse Full through the cache -- we need to own Exported ASTs
// to prune them.
if mode == source.ParseFull {
pgh := snapshot.parseGoHandle(ctx, fh, mode)
pgf, fixed, err = snapshot.parseGo(ctx, pgh)
pgf, fixed, err = snapshot.parseGo(ctx, fh, mode)
} else {
d := parseGo(ctx, snapshot.FileSet(), fh, mode)
pgf, fixed, err = d.parsed, d.fixed, d.err

View File

@ -72,27 +72,19 @@ func (s *snapshot) parseGoHandle(ctx context.Context, fh source.FileHandle, mode
}
func (pgh *parseGoHandle) String() string {
return pgh.File().URI().Filename()
}
func (pgh *parseGoHandle) File() source.FileHandle {
return pgh.file
}
func (pgh *parseGoHandle) Mode() source.ParseMode {
return pgh.mode
return pgh.file.URI().Filename()
}
func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) {
pgh := s.parseGoHandle(ctx, fh, mode)
pgf, _, err := s.parseGo(ctx, pgh)
pgf, _, err := s.parseGo(ctx, fh, mode)
return pgf, err
}
func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.ParsedGoFile, bool, error) {
if pgh.mode == source.ParseExported {
func (s *snapshot) parseGo(ctx context.Context, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, bool, error) {
if mode == source.ParseExported {
panic("only type checking should use Exported")
}
pgh := s.parseGoHandle(ctx, fh, mode)
d, err := pgh.handle.Get(ctx, s.generation, s)
if err != nil {
return nil, false, err
@ -101,6 +93,22 @@ func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.Par
return data.parsed, data.fixed, data.err
}
// cachedPGF returns the cached ParsedGoFile for the given ParseMode, if it
// has already been computed. Otherwise, it returns nil.
func (s *snapshot) cachedPGF(fh source.FileHandle, mode source.ParseMode) *source.ParsedGoFile {
key := parseKey{file: fh.FileIdentity(), mode: mode}
if pgh := s.getGoFile(key); pgh != nil {
cached := pgh.handle.Cached(s.generation)
if cached != nil {
cached := cached.(*parseGoData)
if cached.parsed != nil {
return cached.parsed
}
}
}
return nil
}
type astCacheKey struct {
pkg packageHandleKey
uri span.URI

View File

@ -29,6 +29,7 @@ import (
"golang.org/x/tools/go/packages"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/gocommand"
"golang.org/x/tools/internal/lsp/bug"
"golang.org/x/tools/internal/lsp/debug/log"
"golang.org/x/tools/internal/lsp/debug/tag"
"golang.org/x/tools/internal/lsp/source"
@ -1817,7 +1818,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
}
}
changedPkgNames := map[PackageID]struct{}{}
changedPkgFiles := map[PackageID]struct{}{} // packages whose file set may have changed
anyImportDeleted := false
for uri, change := range changes {
// Maybe reinitialize the view if we see a change in the vendor
@ -1829,23 +1830,26 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
// The original FileHandle for this URI is cached on the snapshot.
originalFH := s.files[uri]
// Check if the file's package name or imports have changed,
// and if so, invalidate this file's packages' metadata.
var shouldInvalidateMetadata, pkgNameChanged, importDeleted bool
if !isGoMod(uri) {
shouldInvalidateMetadata, pkgNameChanged, importDeleted = checkForMetadataChanges(ctx, originalFH, change.fileHandle)
// If uri is a Go file, check if it has changed in a way that would
// invalidate metadata. Note that we can't use s.view.FileKind here,
// because the file type that matters is not what the *client* tells us,
// but what the Go command sees.
var invalidateMetadata, pkgFileChanged, importDeleted bool
if strings.HasSuffix(uri.Filename(), ".go") {
invalidateMetadata, pkgFileChanged, importDeleted = metadataChanges(ctx, s, originalFH, change.fileHandle)
}
invalidateMetadata := forceReloadMetadata || workspaceReload || shouldInvalidateMetadata
invalidateMetadata = invalidateMetadata || forceReloadMetadata || workspaceReload
anyImportDeleted = anyImportDeleted || importDeleted
// Mark all of the package IDs containing the given file.
filePackageIDs := invalidatedPackageIDs(uri, s.ids, originalFH == nil || pkgNameChanged)
if pkgNameChanged {
for _, id := range filePackageIDs {
changedPkgNames[id] = struct{}{}
filePackageIDs := invalidatedPackageIDs(uri, s.ids, pkgFileChanged)
if pkgFileChanged {
for id := range filePackageIDs {
changedPkgFiles[id] = struct{}{}
}
}
for _, id := range filePackageIDs {
for id := range filePackageIDs {
directIDs[id] = directIDs[id] || invalidateMetadata
}
@ -2050,7 +2054,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
// possible that the package ID may no longer exist. Delete it from
// the set of workspace packages, on the assumption that we will add it
// back when the relevant files are reloaded.
if _, ok := changedPkgNames[id]; ok {
if _, ok := changedPkgFiles[id]; ok {
continue
}
@ -2093,20 +2097,25 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
// directory. This is of course incorrect in build systems where packages are
// not organized by directory.
//
// If newPackageFile is set, the file is either a new file, or has a new
// If packageFileChanged is set, the file is either a new file, or has a new
// package name. In this case, all known packages in the directory will be
// invalidated.
func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, newPackageFile bool) []PackageID {
func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, packageFileChanged bool) map[PackageID]struct{} {
invalidated := make(map[PackageID]struct{})
// At a minimum, we invalidate packages known to contain uri.
for _, id := range known[uri] {
invalidated[id] = struct{}{}
}
// If the file didn't move to a new package, we should only invalidate the
// packages it is currently contained inside.
if !newPackageFile {
if packages := known[uri]; len(packages) > 0 {
// We've seen this file before.
return packages
}
if !packageFileChanged && len(invalidated) > 0 {
return invalidated
}
// This is a file we don't yet know about. Guess relevant packages by
// considering files in the same directory.
// This is a file we don't yet know about, or which has moved packages. Guess
// relevant packages by considering files in the same directory.
// Cache of FileInfo to avoid unnecessary stats for multiple files in the
// same directory.
@ -2127,23 +2136,22 @@ func invalidatedPackageIDs(uri span.URI, known map[span.URI][]PackageID, newPack
}
dir := filepath.Dir(uri.Filename())
fi, err := getInfo(dir)
if err != nil {
return nil
}
// Aggregate all possibly relevant package IDs.
var found []PackageID
for knownURI, ids := range known {
knownDir := filepath.Dir(knownURI.Filename())
knownFI, err := getInfo(knownDir)
if err != nil {
continue
}
if os.SameFile(fi, knownFI) {
found = append(found, ids...)
if err == nil {
// Aggregate all possibly relevant package IDs.
for knownURI, ids := range known {
knownDir := filepath.Dir(knownURI.Filename())
knownFI, err := getInfo(knownDir)
if err != nil {
continue
}
if os.SameFile(fi, knownFI) {
for _, id := range ids {
invalidated[id] = struct{}{}
}
}
}
}
return found
return invalidated
}
// fileWasSaved reports whether the FileHandle passed in has been saved. It
@ -2162,66 +2170,117 @@ func fileWasSaved(originalFH, currentFH source.FileHandle) bool {
return !o.saved && c.saved
}
// checkForMetadataChanges reparses the full file's AST to determine
// if the file requires a metadata reload.
func checkForMetadataChanges(ctx context.Context, originalFH, currentFH source.FileHandle) (invalidate, pkgNameChanged, importDeleted bool) {
if originalFH == nil {
return true, false, false
// metadataChanges detects features of the change from oldFH->newFH that may
// affect package metadata.
//
// It uses lockedSnapshot to access cached parse information. lockedSnapshot
// must be locked.
//
// The result parameters have the following meaning:
// - invalidate means that package metadata for packages containing the file
// should be invalidated.
// - pkgFileChanged means that the file->package associates for the file have
// changed (possibly because the file is new, or because its package name has
// changed).
// - importDeleted means that an import has been deleted, or we can't
// determine if an import was deleted due to errors.
func metadataChanges(ctx context.Context, lockedSnapshot *snapshot, oldFH, newFH source.FileHandle) (invalidate, pkgFileChanged, importDeleted bool) {
if oldFH == nil || newFH == nil { // existential changes
changed := (oldFH == nil) != (newFH == nil)
return changed, changed, (newFH == nil) // we don't know if an import was deleted
}
// If the file hasn't changed, there's no need to reload.
if originalFH.FileIdentity() == currentFH.FileIdentity() {
if oldFH.FileIdentity() == newFH.FileIdentity() {
return false, false, false
}
// Get the original and current parsed files in order to check package name
// and imports. Don't parse through the cache as we don't know if we want to
// own the resulting file handle.
fset := token.NewFileSet() // We don't need positions, so can use a new file set.
original := parseGo(ctx, fset, originalFH, source.ParseFull)
current := parseGo(ctx, fset, currentFH, source.ParseFull)
if original.err != nil || current.err != nil {
return (original.err == nil) != (current.err == nil), false, (current.err != nil) // we don't know if an import was deleted
}
// Check if the package's metadata has changed. The cases handled are:
// 1. A package's name has changed
// 2. A file's imports have changed
if original.parsed.File.Name.Name != current.parsed.File.Name.Name {
invalidate = true
pkgNameChanged = true
}
origImportSet := make(map[string]struct{})
for _, importSpec := range original.parsed.File.Imports {
origImportSet[importSpec.Path.Value] = struct{}{}
}
curImportSet := make(map[string]struct{})
for _, importSpec := range current.parsed.File.Imports {
curImportSet[importSpec.Path.Value] = struct{}{}
}
// If any of the current imports were not in the original imports.
for path := range curImportSet {
if _, ok := origImportSet[path]; ok {
delete(origImportSet, path)
continue
}
// If the import path is obviously not valid, we can skip reloading
// metadata. For now, valid means properly quoted and without a
// terminal slash.
if isBadImportPath(path) {
continue
}
invalidate = true
// Parse headers to compare package names and imports.
oldHead, oldErr := peekOrParse(ctx, lockedSnapshot, oldFH, source.ParseHeader)
newHead, newErr := peekOrParse(ctx, lockedSnapshot, newFH, source.ParseHeader)
if oldErr != nil || newErr != nil {
// TODO(rfindley): we can get here if newFH does not exists. There is
// asymmetry here, in that newFH may be non-nil even if the underlying file
// does not exist.
//
// We should not produce a non-nil filehandle for a file that does not exist.
errChanged := (oldErr == nil) != (newErr == nil)
return errChanged, errChanged, (newErr != nil) // we don't know if an import was deleted
}
for path := range origImportSet {
if !isBadImportPath(path) {
invalidate = true
importDeleted = true
// `go list` fails completely if the file header cannot be parsed. If we go
// from a non-parsing state to a parsing state, we should reload.
if oldHead.ParseErr != nil && newHead.ParseErr == nil {
return true, true, true // We don't know what changed, so fall back on full invalidation.
}
// If a package name has changed, the set of package imports may have changed
// in ways we can't detect here. Assume an import has been deleted.
if oldHead.File.Name.Name != newHead.File.Name.Name {
return true, true, true
}
// Check whether package imports have changed. Only consider potentially
// valid imports paths.
oldImports := validImports(oldHead.File.Imports)
newImports := validImports(newHead.File.Imports)
for path := range newImports {
if _, ok := oldImports[path]; ok {
delete(oldImports, path)
} else {
invalidate = true // a new, potentially valid import was added
}
}
if len(oldImports) > 0 {
invalidate = true
importDeleted = true
}
// If the change does not otherwise invalidate metadata, get the full ASTs in
// order to check magic comments.
//
// Note: if this affects performance we can probably avoid parsing in the
// common case by first scanning the source for potential comments.
if !invalidate {
invalidate = magicCommentsChanged(original.parsed.File, current.parsed.File)
origFull, oldErr := peekOrParse(ctx, lockedSnapshot, oldFH, source.ParseFull)
currFull, newErr := peekOrParse(ctx, lockedSnapshot, newFH, source.ParseFull)
if oldErr == nil && newErr == nil {
invalidate = magicCommentsChanged(origFull.File, currFull.File)
} else {
// At this point, we shouldn't ever fail to produce a ParsedGoFile, as
// we're already past header parsing.
bug.Reportf("metadataChanges: unparseable file %v (old error: %v, new error: %v)", oldFH.URI(), oldErr, newErr)
}
}
return invalidate, pkgNameChanged, importDeleted
return invalidate, pkgFileChanged, importDeleted
}
// peekOrParse returns the cached ParsedGoFile if it exists, otherwise parses
// without caching.
//
// It returns an error if the file could not be read (note that parsing errors
// are stored in ParsedGoFile.ParseErr).
//
// lockedSnapshot must be locked.
func peekOrParse(ctx context.Context, lockedSnapshot *snapshot, fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) {
key := parseKey{file: fh.FileIdentity(), mode: mode}
if pgh := lockedSnapshot.goFiles[key]; pgh != nil {
cached := pgh.handle.Cached(lockedSnapshot.generation)
if cached != nil {
cached := cached.(*parseGoData)
if cached.parsed != nil {
return cached.parsed, nil
}
}
}
fset := token.NewFileSet()
data := parseGo(ctx, fset, fh, mode)
return data.parsed, data.err
}
func magicCommentsChanged(original *ast.File, current *ast.File) bool {
@ -2238,18 +2297,29 @@ func magicCommentsChanged(original *ast.File, current *ast.File) bool {
return false
}
func isBadImportPath(path string) bool {
// validImports extracts the set of valid import paths from imports.
func validImports(imports []*ast.ImportSpec) map[string]struct{} {
m := make(map[string]struct{})
for _, spec := range imports {
if path := spec.Path.Value; validImportPath(path) {
m[path] = struct{}{}
}
}
return m
}
func validImportPath(path string) bool {
path, err := strconv.Unquote(path)
if err != nil {
return true
return false
}
if path == "" {
return true
return false
}
if path[len(path)-1] == '/' {
return true
return false
}
return false
return true
}
var buildConstraintOrEmbedRe = regexp.MustCompile(`^//(go:embed|go:build|\s*\+build).*`)

View File

@ -40,10 +40,10 @@ func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle)
return h
}
key := symbolHandleKey(fh.FileIdentity().Hash)
h := s.generation.Bind(key, func(ctx context.Context, arg memoize.Arg) interface{} {
h := s.generation.Bind(key, func(_ context.Context, arg memoize.Arg) interface{} {
snapshot := arg.(*snapshot)
data := &symbolData{}
data.symbols, data.err = symbolize(ctx, snapshot, fh)
data.symbols, data.err = symbolize(snapshot, fh)
return data
}, nil)
@ -57,7 +57,7 @@ func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle)
// symbolize extracts symbols from a file. It uses a parsed file already
// present in the cache but otherwise does not populate the cache.
func symbolize(ctx context.Context, snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) {
func symbolize(snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) {
src, err := fh.Read()
if err != nil {
return nil, err
@ -70,16 +70,9 @@ func symbolize(ctx context.Context, snapshot *snapshot, fh source.FileHandle) ([
// If the file has already been fully parsed through the cache, we can just
// use the result.
key := parseKey{file: fh.FileIdentity(), mode: source.ParseFull}
if pgh := snapshot.getGoFile(key); pgh != nil {
cached := pgh.handle.Cached(snapshot.generation)
if cached != nil {
cached := cached.(*parseGoData)
if cached.parsed != nil {
file = cached.parsed.File
fileDesc = cached.parsed.Tok
}
}
if pgf := snapshot.cachedPGF(fh, source.ParseFull); pgf != nil {
file = pgf.File
fileDesc = pgf.Tok
}
// Otherwise, we parse the file ourselves. Notably we don't use parseGo here,