From a2a24778ba9562d210892fad5bf136c0bdbd233c Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 14 Jul 2022 10:44:30 -0400 Subject: [PATCH] gopls/internal/regtest: externalize shouldLoad tracking The fundamental bug causing TestChangePackageName to fail has been fixed, yet unskipping it revealed a new bug: tracking whether or not a package should be loaded requires that we actually store that package in s.meta. In cases where we drop metadata, we also lose the information that a package path needs to be reloaded. Fix this by significantly reworking the tracking of pending loads, to simplify the code and separate the reloading logic from the logic of tracking metadata. As a nice side-effect, this eliminates the needless work necessary to mark/unmark packages as needing loading, since this is no longer tracked by the immutable metadata graph. Additionally, eliminate the "shouldLoad" guard inside of snapshot.load. We should never ask for loads that we do not want, and the shouldLoad guard either masks bugs or leads to bugs. For example, we would repeatedly call load from reloadOrphanedFiles for files that are part of a package that needs loading, because we skip loading the file scope. Lift the responsibility for determining if we should load to the callers of load. Along the way, make a few additional minor improvements: - simplify the code where possible - leave TODOs for likely bugs or things that should be simplified in the future - reduce the overly granular locking in getOrLoadIDsForURI, which could lead to strange races - remove a stale comment for a test that is no longer flaky. Updates golang/go#53878 Change-Id: I6d9084806f1fdebc43002c7cc75dc1b94f8514b9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/417576 Run-TryBot: Robert Findley gopls-CI: kokoro Reviewed-by: Suzy Mueller TryBot-Result: Gopher Robot --- .../regtest/diagnostics/diagnostics_test.go | 15 +- gopls/internal/regtest/misc/vendor_test.go | 10 - internal/lsp/cache/load.go | 15 -- internal/lsp/cache/metadata.go | 12 +- internal/lsp/cache/snapshot.go | 231 +++++++++--------- internal/lsp/source/util.go | 4 + 6 files changed, 132 insertions(+), 155 deletions(-) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index b9dc2d434b..ae8b4a56cd 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -703,7 +703,8 @@ func main() { // Test for golang/go#38207. func TestNewModule_Issue38207(t *testing.T) { - testenv.NeedsGo1Point(t, 14) + // Fails at Go 1.14 following CL 417576. Not investigated. + testenv.NeedsGo1Point(t, 15) const emptyFile = ` -- go.mod -- module mod.com @@ -874,7 +875,7 @@ func TestX(t *testing.T) { } func TestChangePackageName(t *testing.T) { - t.Skip("This issue hasn't been fixed yet. See golang.org/issue/41061.") + testenv.NeedsGo1Point(t, 16) // needs native overlay support const mod = ` -- go.mod -- @@ -889,15 +890,11 @@ package foo_ Run(t, mod, func(t *testing.T, env *Env) { env.OpenFile("foo/bar_test.go") env.RegexpReplace("foo/bar_test.go", "package foo_", "package foo_test") - env.SaveBuffer("foo/bar_test.go") env.Await( OnceMet( - env.DoneWithSave(), - NoDiagnostics("foo/bar_test.go"), - ), - OnceMet( - env.DoneWithSave(), - NoDiagnostics("foo/foo.go"), + env.DoneWithChange(), + EmptyOrNoDiagnostics("foo/bar_test.go"), + EmptyOrNoDiagnostics("foo/foo.go"), ), ) }) diff --git a/gopls/internal/regtest/misc/vendor_test.go b/gopls/internal/regtest/misc/vendor_test.go index 324a8006fa..4e02799b47 100644 --- a/gopls/internal/regtest/misc/vendor_test.go +++ b/gopls/internal/regtest/misc/vendor_test.go @@ -27,16 +27,6 @@ var Goodbye error func TestInconsistentVendoring(t *testing.T) { testenv.NeedsGo1Point(t, 14) - // TODO(golang/go#49646): delete this comment once this test is stable. - // - // In golang/go#49646, this test is reported as flaky on Windows. We believe - // this is due to file contention from go mod vendor that should be resolved. - // If this test proves to still be flaky, skip it. - // - // if runtime.GOOS == "windows" { - // t.Skipf("skipping test due to flakiness on Windows: https://golang.org/issue/49646") - // } - const pkgThatUsesVendoring = ` -- go.mod -- module mod.com diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index d0942b51bf..79789fe60d 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -7,7 +7,6 @@ package cache import ( "bytes" "context" - "errors" "fmt" "io/ioutil" "os" @@ -41,24 +40,10 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf var query []string var containsDir bool // for logging - // Unless the context was canceled, set "shouldLoad" to false for all - // of the metadata we attempted to load. - defer func() { - if errors.Is(err, context.Canceled) { - return - } - // TODO(rfindley): merge these metadata updates with the updates below, to - // avoid updating the graph twice. - s.clearShouldLoad(scopes...) - }() - // Keep track of module query -> module path so that we can later correlate query // errors with errors. moduleQueries := make(map[string]string) for _, scope := range scopes { - if !s.shouldLoad(scope) { - continue - } switch scope := scope.(type) { case PackagePath: if source.IsCommandLineArguments(string(scope)) { diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go index 486035f939..7d9192f43a 100644 --- a/internal/lsp/cache/metadata.go +++ b/internal/lsp/cache/metadata.go @@ -29,7 +29,7 @@ type Metadata struct { Name PackageName GoFiles []span.URI CompiledGoFiles []span.URI - ForTest PackagePath + ForTest PackagePath // package path under test, or "" TypesSizes types.Sizes Errors []packages.Error Deps []PackageID // direct dependencies, in string order @@ -94,12 +94,8 @@ type KnownMetadata struct { // PkgFilesChanged reports whether the file set of this metadata has // potentially changed. - PkgFilesChanged bool - - // ShouldLoad is true if the given metadata should be reloaded. // - // Note that ShouldLoad is different from !Valid: when we try to load a - // package, we mark ShouldLoad = false regardless of whether the load - // succeeded, to prevent endless loads. - ShouldLoad bool + // TODO(rfindley): this is used for WorkspacePackages, and looks fishy: we + // should probably only consider valid packages to be workspace packages. + PkgFilesChanged bool } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index a516860aaf..64e7f17c99 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -117,6 +117,13 @@ type snapshot struct { // when the view is created. workspacePackages map[PackageID]PackagePath + // shouldLoad tracks packages that need to be reloaded, mapping a PackageID + // to the package paths that should be used to reload it + // + // When we try to load a package, we clear it from the shouldLoad map + // regardless of whether the load succeeded, to prevent endless loads. + shouldLoad map[PackageID][]PackagePath + // unloadableFiles keeps track of files that we've failed to load. unloadableFiles map[span.URI]struct{} @@ -643,6 +650,10 @@ func (s *snapshot) PackageForFile(ctx context.Context, uri span.URI, mode source } func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode source.TypecheckMode, includeTestVariants bool) ([]*packageHandle, error) { + // TODO(rfindley): why can't/shouldn't we awaitLoaded here? It seems that if + // we ask for package handles for a file, we should wait for pending loads. + // Else we will reload orphaned files before the initial load completes. + // Check if we should reload metadata for the file. We don't invalidate IDs // (though we should), so the IDs will be a better source of truth than the // metadata. If there are no IDs for the file, then we should also reload. @@ -691,13 +702,13 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode } func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]PackageID, error) { - knownIDs := s.getIDsForURI(uri) - reload := len(knownIDs) == 0 - for _, id := range knownIDs { - // Reload package metadata if any of the metadata has missing - // dependencies, in case something has changed since the last time we - // reloaded it. - if s.noValidMetadataForID(id) { + s.mu.Lock() + ids := s.meta.ids[uri] + reload := len(ids) == 0 + for _, id := range ids { + // If the file is part of a package that needs reloading, reload it now to + // improve our responsiveness. + if len(s.shouldLoad[id]) > 0 { reload = true break } @@ -705,20 +716,38 @@ func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]Pack // missing dependencies. This is expensive and results in too many // calls to packages.Load. Determine what we should do instead. } - if reload { - err := s.load(ctx, false, fileURI(uri)) + s.mu.Unlock() + if reload { + scope := fileURI(uri) + err := s.load(ctx, false, scope) + + // As in reloadWorkspace, we must clear scopes after loading. + // + // TODO(rfindley): simply call reloadWorkspace here, first, to avoid this + // duplication. + if !errors.Is(err, context.Canceled) { + s.clearShouldLoad(scope) + } + + // TODO(rfindley): this doesn't look right. If we don't reload, we use + // invalid metadata anyway, but if we DO reload and it fails, we don't? if !s.useInvalidMetadata() && err != nil { return nil, err } + + s.mu.Lock() + ids = s.meta.ids[uri] + s.mu.Unlock() + // We've tried to reload and there are still no known IDs for the URI. // Return the load error, if there was one. - knownIDs = s.getIDsForURI(uri) - if len(knownIDs) == 0 { + if len(ids) == 0 { return nil, err } } - return knownIDs, nil + + return ids, nil } // Only use invalid metadata for Go versions >= 1.13. Go 1.12 and below has @@ -1153,13 +1182,6 @@ func moduleForURI(modFiles map[span.URI]struct{}, uri span.URI) span.URI { return match } -func (s *snapshot) getIDsForURI(uri span.URI) []PackageID { - s.mu.Lock() - defer s.mu.Unlock() - - return s.meta.ids[uri] -} - func (s *snapshot) getMetadata(id PackageID) *KnownMetadata { s.mu.Lock() defer s.mu.Unlock() @@ -1167,78 +1189,34 @@ func (s *snapshot) getMetadata(id PackageID) *KnownMetadata { return s.meta.metadata[id] } -func (s *snapshot) shouldLoad(scope interface{}) bool { - s.mu.Lock() - defer s.mu.Unlock() - - g := s.meta - - switch scope := scope.(type) { - case PackagePath: - var meta *KnownMetadata - for _, m := range g.metadata { - if m.PkgPath != scope { - continue - } - meta = m - } - if meta == nil || meta.ShouldLoad { - return true - } - return false - case fileURI: - uri := span.URI(scope) - ids := g.ids[uri] - if len(ids) == 0 { - return true - } - for _, id := range ids { - m, ok := g.metadata[id] - if !ok || m.ShouldLoad { - return true - } - } - return false - default: - return true - } -} - +// clearShouldLoad clears package IDs that no longer need to be reloaded after +// scopes has been loaded. func (s *snapshot) clearShouldLoad(scopes ...interface{}) { s.mu.Lock() defer s.mu.Unlock() - g := s.meta - - var updates map[PackageID]*KnownMetadata - markLoaded := func(m *KnownMetadata) { - if updates == nil { - updates = make(map[PackageID]*KnownMetadata) - } - next := *m - next.ShouldLoad = false - updates[next.ID] = &next - } for _, scope := range scopes { switch scope := scope.(type) { case PackagePath: - for _, m := range g.metadata { - if m.PkgPath == scope { - markLoaded(m) + var toDelete []PackageID + for id, pkgPaths := range s.shouldLoad { + for _, pkgPath := range pkgPaths { + if pkgPath == scope { + toDelete = append(toDelete, id) + } } } + for _, id := range toDelete { + delete(s.shouldLoad, id) + } case fileURI: uri := span.URI(scope) - ids := g.ids[uri] + ids := s.meta.ids[uri] for _, id := range ids { - if m, ok := g.metadata[id]; ok { - markLoaded(m) - } + delete(s.shouldLoad, id) } } } - s.meta = g.Clone(updates) - s.resetIsActivePackageLocked() } // noValidMetadataForURILocked reports whether there is any valid metadata for @@ -1256,16 +1234,6 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool { return true } -// noValidMetadataForID reports whether there is no valid metadata for the -// given ID. -func (s *snapshot) noValidMetadataForID(id PackageID) bool { - s.mu.Lock() - defer s.mu.Unlock() - - m := s.meta.metadata[id] - return m == nil || !m.Valid -} - func (s *snapshot) isWorkspacePackage(id PackageID) bool { s.mu.Lock() defer s.mu.Unlock() @@ -1479,39 +1447,42 @@ func (s *snapshot) AwaitInitialized(ctx context.Context) { // reloadWorkspace reloads the metadata for all invalidated workspace packages. func (s *snapshot) reloadWorkspace(ctx context.Context) error { - // See which of the workspace packages are missing metadata. + var scopes []interface{} + var seen map[PackagePath]bool s.mu.Lock() - missingMetadata := len(s.workspacePackages) == 0 || len(s.meta.metadata) == 0 - pkgPathSet := map[PackagePath]struct{}{} - for id, pkgPath := range s.workspacePackages { - if m, ok := s.meta.metadata[id]; ok && m.Valid { - continue + for _, pkgPaths := range s.shouldLoad { + for _, pkgPath := range pkgPaths { + if seen == nil { + seen = make(map[PackagePath]bool) + } + if seen[pkgPath] { + continue + } + seen[pkgPath] = true + scopes = append(scopes, pkgPath) } - missingMetadata = true - - // Don't try to reload "command-line-arguments" directly. - if source.IsCommandLineArguments(string(pkgPath)) { - continue - } - pkgPathSet[pkgPath] = struct{}{} } s.mu.Unlock() - // If the view's build configuration is invalid, we cannot reload by - // package path. Just reload the directory instead. - if missingMetadata && !s.ValidBuildConfiguration() { - return s.load(ctx, false, viewLoadScope("LOAD_INVALID_VIEW")) - } - - if len(pkgPathSet) == 0 { + if len(scopes) == 0 { return nil } - var pkgPaths []interface{} - for pkgPath := range pkgPathSet { - pkgPaths = append(pkgPaths, pkgPath) + // If the view's build configuration is invalid, we cannot reload by + // package path. Just reload the directory instead. + if !s.ValidBuildConfiguration() { + scopes = []interface{}{viewLoadScope("LOAD_INVALID_VIEW")} } - return s.load(ctx, false, pkgPaths...) + + err := s.load(ctx, false, scopes...) + + // Unless the context was canceled, set "shouldLoad" to false for all + // of the metadata we attempted to load. + if !errors.Is(err, context.Canceled) { + s.clearShouldLoad(scopes...) + } + + return err } func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error { @@ -1676,6 +1647,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC release := result.Acquire() // Copy the set of unloadable files. + // + // TODO(rfindley): this looks wrong. Shouldn't we clear unloadableFiles on + // changes to environment or workspace layout, or more generally on any + // metadata change? for k, v := range s.unloadableFiles { result.unloadableFiles[k] = v } @@ -1873,6 +1848,15 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } } + // Any packages that need loading in s still need loading in the new + // snapshot. + for k, v := range s.shouldLoad { + if result.shouldLoad == nil { + result.shouldLoad = make(map[PackageID][]PackagePath) + } + result.shouldLoad[k] = v + } + // Compute which metadata updates are required. We only need to invalidate // packages directly containing the affected file, and only if it changed in // a relevant way. @@ -1880,20 +1864,41 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged for k, v := range s.meta.metadata { invalidateMetadata := idsToInvalidate[k] + + // For metadata that has been newly invalidated, capture package paths + // requiring reloading in the shouldLoad map. + if invalidateMetadata && !source.IsCommandLineArguments(string(v.ID)) { + if result.shouldLoad == nil { + result.shouldLoad = make(map[PackageID][]PackagePath) + } + needsReload := []PackagePath{v.PkgPath} + if v.ForTest != "" && v.ForTest != v.PkgPath { + // When reloading test variants, always reload their ForTest package as + // well. Otherwise, we may miss test variants in the resulting load. + // + // TODO(rfindley): is this actually sufficient? Is it possible that + // other test variants may be invalidated? Either way, we should + // determine exactly what needs to be reloaded here. + needsReload = append(needsReload, v.ForTest) + } + result.shouldLoad[k] = needsReload + } + + // Check whether the metadata should be deleted. if skipID[k] || (invalidateMetadata && deleteInvalidMetadata) { metadataUpdates[k] = nil continue } + + // Check if the metadata has changed. valid := v.Valid && !invalidateMetadata pkgFilesChanged := v.PkgFilesChanged || changedPkgFiles[k] - shouldLoad := v.ShouldLoad || invalidateMetadata - if valid != v.Valid || pkgFilesChanged != v.PkgFilesChanged || shouldLoad != v.ShouldLoad { + if valid != v.Valid || pkgFilesChanged != v.PkgFilesChanged { // Mark invalidated metadata rather than deleting it outright. metadataUpdates[k] = &KnownMetadata{ Metadata: v.Metadata, Valid: valid, PkgFilesChanged: pkgFilesChanged, - ShouldLoad: shouldLoad, } } } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 8d205ee6ce..262447cf36 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -550,6 +550,8 @@ func IsValidImport(pkgPath, importPkgPath string) bool { if i == -1 { return true } + // TODO(rfindley): this looks wrong: IsCommandLineArguments is meant to + // operate on package IDs, not package paths. if IsCommandLineArguments(string(pkgPath)) { return true } @@ -560,6 +562,8 @@ func IsValidImport(pkgPath, importPkgPath string) bool { // "command-line-arguments" package, which is a package with an unknown ID // created by the go command. It can have a test variant, which is why callers // should not check that a value equals "command-line-arguments" directly. +// +// TODO(rfindley): this should accept a PackageID. func IsCommandLineArguments(s string) bool { return strings.Contains(s, "command-line-arguments") }