diff --git a/gopls/internal/immutable/immutable.go b/gopls/internal/immutable/immutable.go index a88133fe92f..b0b14c16404 100644 --- a/gopls/internal/immutable/immutable.go +++ b/gopls/internal/immutable/immutable.go @@ -20,6 +20,15 @@ func MapOf[K comparable, V any](m map[K]V) Map[K, V] { return Map[K, V]{m} } +// Keys returns all keys present in the map. +func (m Map[K, V]) Keys() []K { + var keys []K + for k := range m.m { + keys = append(keys, k) + } + return keys +} + // Value returns the mapped value for k. // It is equivalent to the commaok form of an ordinary go map, and returns // (zero, false) if the key is not present. diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index 57b5f85be4f..01d7f474d14 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -128,8 +128,6 @@ func (s *Session) createView(ctx context.Context, info *workspaceInformation, fo initialWorkspaceLoad: make(chan struct{}), initializationSema: make(chan struct{}, 1), baseCtx: baseCtx, - moduleUpgrades: map[span.URI]map[string]string{}, - vulns: map[span.URI]*vulncheck.Result{}, parseCache: s.parseCache, fs: s.overlayFS, workspaceInformation: info, @@ -173,6 +171,8 @@ func (s *Session) createView(ctx context.Context, info *workspaceInformation, fo workspaceModFiles: wsModFiles, workspaceModFilesErr: wsModFilesErr, pkgIndex: typerefs.NewPackageIndex(), + moduleUpgrades: new(persistent.Map[span.URI, map[string]string]), + vulns: new(persistent.Map[span.URI, *vulncheck.Result]), } // Save one reference in the view. v.releaseSnapshot = v.snapshot.Acquire() @@ -501,9 +501,9 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif } var releases []func() - viewToSnapshot := map[*View]*snapshot{} + viewToSnapshot := make(map[*View]source.Snapshot) for view, changed := range views { - snapshot, release := view.invalidateContent(ctx, changed) + snapshot, release := view.Invalidate(ctx, source.StateChange{Files: changed}) releases = append(releases, release) viewToSnapshot[view] = snapshot } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index c40fe0a9b32..ef204aa4a6b 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -40,6 +40,8 @@ import ( "golang.org/x/tools/gopls/internal/lsp/source/xrefs" "golang.org/x/tools/gopls/internal/persistent" "golang.org/x/tools/gopls/internal/span" + "golang.org/x/tools/gopls/internal/vulncheck" + "golang.org/x/tools/internal/constraints" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/gocommand" @@ -177,6 +179,13 @@ type snapshot struct { // detect ignored files. ignoreFilterOnce sync.Once ignoreFilter *ignoreFilter + + // moduleUpgrades tracks known upgrades for module paths in each modfile. + // Each modfile has a map of module name to upgrade version. + moduleUpgrades *persistent.Map[span.URI, map[string]string] + + // vulns maps each go.mod file's URI to its known vulnerabilities. + vulns *persistent.Map[span.URI, *vulncheck.Result] } var globalSnapshotID uint64 @@ -251,6 +260,8 @@ func (s *snapshot) destroy(destroyedBy string) { s.modVulnHandles.Destroy() s.modWhyHandles.Destroy() s.unloadableFiles.Destroy() + s.moduleUpgrades.Destroy() + s.vulns.Destroy() } func (s *snapshot) SequenceID() uint64 { @@ -1814,7 +1825,8 @@ func inVendor(uri span.URI) bool { return found && strings.Contains(after, "/") } -func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source.FileHandle) (*snapshot, func()) { +func (s *snapshot) clone(ctx, bgCtx context.Context, changed source.StateChange) (*snapshot, func()) { + changedFiles := changed.Files ctx, done := event.Start(ctx, "cache.snapshot.clone") defer done() @@ -1834,20 +1846,22 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source initializedErr: s.initializedErr, packages: s.packages.Clone(), activePackages: s.activePackages.Clone(), - files: s.files.Clone(changes), - symbolizeHandles: cloneWithout(s.symbolizeHandles, changes), + files: s.files.Clone(changedFiles), + symbolizeHandles: cloneWithout(s.symbolizeHandles, changedFiles), workspacePackages: s.workspacePackages, shouldLoad: s.shouldLoad.Clone(), // not cloneWithout: shouldLoad is cleared on loads unloadableFiles: s.unloadableFiles.Clone(), // not cloneWithout: typing in a file doesn't necessarily make it loadable - parseModHandles: cloneWithout(s.parseModHandles, changes), - parseWorkHandles: cloneWithout(s.parseWorkHandles, changes), - modTidyHandles: cloneWithout(s.modTidyHandles, changes), - modWhyHandles: cloneWithout(s.modWhyHandles, changes), - modVulnHandles: cloneWithout(s.modVulnHandles, changes), + parseModHandles: cloneWithout(s.parseModHandles, changedFiles), + parseWorkHandles: cloneWithout(s.parseWorkHandles, changedFiles), + modTidyHandles: cloneWithout(s.modTidyHandles, changedFiles), + modWhyHandles: cloneWithout(s.modWhyHandles, changedFiles), + modVulnHandles: cloneWithout(s.modVulnHandles, changedFiles), workspaceModFiles: s.workspaceModFiles, workspaceModFilesErr: s.workspaceModFilesErr, importGraph: s.importGraph, pkgIndex: s.pkgIndex, + moduleUpgrades: cloneWith(s.moduleUpgrades, changed.ModuleUpgrades), + vulns: cloneWith(s.vulns, changed.Vulns), } // Create a lease on the new snapshot. @@ -1864,7 +1878,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source // vendor tree after 'go mod vendor' or 'rm -fr vendor/'. // // TODO(rfindley): revisit the location of this check. - for uri := range changes { + for uri := range changedFiles { if inVendor(uri) && s.initializedErr != nil || strings.HasSuffix(string(uri), "/vendor/modules.txt") { reinit = true @@ -1877,7 +1891,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source // where a file is added on disk; we don't want to read the newly added file // into the old snapshot, as that will break our change detection below. oldFiles := make(map[span.URI]source.FileHandle) - for uri := range changes { + for uri := range changedFiles { if fh, ok := s.files.Get(uri); ok { oldFiles[uri] = fh } @@ -1898,7 +1912,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source } if workURI, _ := s.view.GOWORK(); workURI != "" { - if newFH, ok := changes[workURI]; ok { + if newFH, ok := changedFiles[workURI]; ok { result.workspaceModFiles, result.workspaceModFilesErr = computeWorkspaceModFiles(ctx, s.view.gomod, workURI, s.view.effectiveGO111MODULE(), result) if changedOnDisk(oldFiles[workURI], newFH) { reinit = true @@ -1907,14 +1921,14 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source } // Reinitialize if any workspace mod file has changed on disk. - for uri, newFH := range changes { + for uri, newFH := range changedFiles { if _, ok := result.workspaceModFiles[uri]; ok && changedOnDisk(oldFiles[uri], newFH) { reinit = true } } // Finally, process sumfile changes that may affect loading. - for uri, newFH := range changes { + for uri, newFH := range changedFiles { if !changedOnDisk(oldFiles[uri], newFH) { continue // like with go.mod files, we only reinit when things change on disk } @@ -1955,7 +1969,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source anyFileOpenedOrClosed := false // opened files affect workspace packages anyFileAdded := false // adding a file can resolve missing dependencies - for uri, newFH := range changes { + for uri, newFH := range changedFiles { // The original FileHandle for this URI is cached on the snapshot. oldFH, _ := oldFiles[uri] // may be nil _, oldOpen := oldFH.(*Overlay) @@ -2160,7 +2174,8 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source return result, release } -func cloneWithout[V any](m *persistent.Map[span.URI, V], changes map[span.URI]source.FileHandle) *persistent.Map[span.URI, V] { +// cloneWithout clones m then deletes from it the keys of changes. +func cloneWithout[K constraints.Ordered, V1, V2 any](m *persistent.Map[K, V1], changes map[K]V2) *persistent.Map[K, V1] { m2 := m.Clone() for k := range changes { m2.Delete(k) @@ -2168,6 +2183,15 @@ func cloneWithout[V any](m *persistent.Map[span.URI, V], changes map[span.URI]so return m2 } +// cloneWith clones m then inserts the changes into it. +func cloneWith[K constraints.Ordered, V any](m *persistent.Map[K, V], changes map[K]V) *persistent.Map[K, V] { + m2 := m.Clone() + for k, v := range changes { + m2.Set(k, v, nil) + } + return m2 +} + // deleteMostRelevantModFile deletes the mod file most likely to be the mod // file for the changed URI, if it exists. // diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go index 29af9b4ab76..6361735d04b 100644 --- a/gopls/internal/lsp/cache/view.go +++ b/gopls/internal/lsp/cache/view.go @@ -65,15 +65,6 @@ type View struct { importsState *importsState - // moduleUpgrades tracks known upgrades for module paths in each modfile. - // Each modfile has a map of module name to upgrade version. - moduleUpgradesMu sync.Mutex - moduleUpgrades map[span.URI]map[string]string - - // vulns maps each go.mod file's URI to its known vulnerabilities. - vulnsMu sync.Mutex - vulns map[span.URI]*vulncheck.Result - // parseCache holds an LRU cache of recently parsed files. parseCache *parseCache @@ -864,14 +855,13 @@ func (s *snapshot) loadWorkspace(ctx context.Context, firstAttempt bool) (loadEr return loadErr } -// invalidateContent invalidates the content of a Go file, -// including any position and type information that depends on it. +// Invalidate processes the provided state change, invalidating any derived +// results that depend on the changed state. // -// invalidateContent returns a non-nil snapshot for the new content, along with -// a callback which the caller must invoke to release that snapshot. -// -// newOptions may be nil, in which case options remain unchanged. -func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]source.FileHandle) (*snapshot, func()) { +// The resulting snapshot is non-nil, representing the outcome of the state +// change. The second result is a function that must be called to release the +// snapshot when the snapshot is no longer needed. +func (v *View) Invalidate(ctx context.Context, changed source.StateChange) (source.Snapshot, func()) { // Detach the context so that content invalidation cannot be canceled. ctx = xcontext.Detach(ctx) @@ -893,7 +883,7 @@ func (v *View) invalidateContent(ctx context.Context, changes map[span.URI]sourc prevSnapshot.AwaitInitialized(ctx) // Save one lease of the cloned snapshot in the view. - v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changes) + v.snapshot, v.releaseSnapshot = prevSnapshot.clone(ctx, v.baseCtx, changed) prevReleaseSnapshot() v.destroy(prevSnapshot, "View.invalidateContent") @@ -1037,61 +1027,37 @@ func (v *View) IsGoPrivatePath(target string) bool { return globsMatchPath(v.goprivate, target) } -func (v *View) ModuleUpgrades(modfile span.URI) map[string]string { - v.moduleUpgradesMu.Lock() - defer v.moduleUpgradesMu.Unlock() - +func (s *snapshot) ModuleUpgrades(modfile span.URI) map[string]string { + s.mu.Lock() + defer s.mu.Unlock() upgrades := map[string]string{} - for mod, ver := range v.moduleUpgrades[modfile] { + orig, _ := s.moduleUpgrades.Get(modfile) + for mod, ver := range orig { upgrades[mod] = ver } return upgrades } -func (v *View) RegisterModuleUpgrades(modfile span.URI, upgrades map[string]string) { - // Return early if there are no upgrades. - if len(upgrades) == 0 { - return - } - - v.moduleUpgradesMu.Lock() - defer v.moduleUpgradesMu.Unlock() - - m := v.moduleUpgrades[modfile] - if m == nil { - m = make(map[string]string) - v.moduleUpgrades[modfile] = m - } - for mod, ver := range upgrades { - m[mod] = ver - } -} - -func (v *View) ClearModuleUpgrades(modfile span.URI) { - v.moduleUpgradesMu.Lock() - defer v.moduleUpgradesMu.Unlock() - - delete(v.moduleUpgrades, modfile) -} - -const maxGovulncheckResultAge = 1 * time.Hour // Invalidate results older than this limit. -var timeNow = time.Now // for testing +// MaxGovulncheckResultsAge defines the maximum vulnerability age considered +// valid by gopls. +// +// Mutable for testing. +var MaxGovulncheckResultAge = 1 * time.Hour -func (v *View) Vulnerabilities(modfiles ...span.URI) map[span.URI]*vulncheck.Result { +// TODO(rfindley): move to snapshot.go +func (s *snapshot) Vulnerabilities(modfiles ...span.URI) map[span.URI]*vulncheck.Result { m := make(map[span.URI]*vulncheck.Result) - now := timeNow() - v.vulnsMu.Lock() - defer v.vulnsMu.Unlock() + now := time.Now() + + s.mu.Lock() + defer s.mu.Unlock() if len(modfiles) == 0 { // empty means all modfiles - for modfile := range v.vulns { - modfiles = append(modfiles, modfile) - } + modfiles = s.vulns.Keys() } for _, modfile := range modfiles { - vuln := v.vulns[modfile] - if vuln != nil && now.Sub(vuln.AsOf) > maxGovulncheckResultAge { - v.vulns[modfile] = nil // same as SetVulnerabilities(modfile, nil) + vuln, _ := s.vulns.Get(modfile) + if vuln != nil && now.Sub(vuln.AsOf) > MaxGovulncheckResultAge { vuln = nil } m[modfile] = vuln @@ -1099,13 +1065,6 @@ func (v *View) Vulnerabilities(modfiles ...span.URI) map[span.URI]*vulncheck.Res return m } -func (v *View) SetVulnerabilities(modfile span.URI, vulns *vulncheck.Result) { - v.vulnsMu.Lock() - defer v.vulnsMu.Unlock() - - v.vulns[modfile] = vulns -} - func (v *View) GoVersion() int { return v.workspaceInformation.goversion } diff --git a/gopls/internal/lsp/cache/view_test.go b/gopls/internal/lsp/cache/view_test.go index 2b7249b69ab..69e0ddc5fd9 100644 --- a/gopls/internal/lsp/cache/view_test.go +++ b/gopls/internal/lsp/cache/view_test.go @@ -9,13 +9,10 @@ import ( "os" "path/filepath" "testing" - "time" - "github.com/google/go-cmp/cmp" "golang.org/x/tools/gopls/internal/lsp/fake" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/span" - "golang.org/x/tools/gopls/internal/vulncheck" ) func TestCaseInsensitiveFilesystem(t *testing.T) { @@ -209,65 +206,6 @@ func TestSuffixes(t *testing.T) { } } -func TestView_Vulnerabilities(t *testing.T) { - // TODO(hyangah): use t.Cleanup when we get rid of go1.13 legacy CI. - defer func() { timeNow = time.Now }() - - now := time.Now() - - view := &View{ - vulns: make(map[span.URI]*vulncheck.Result), - } - file1, file2 := span.URIFromPath("f1/go.mod"), span.URIFromPath("f2/go.mod") - - vuln1 := &vulncheck.Result{AsOf: now.Add(-(maxGovulncheckResultAge * 3) / 4)} // already ~3/4*maxGovulncheckResultAge old - view.SetVulnerabilities(file1, vuln1) - - vuln2 := &vulncheck.Result{AsOf: now} // fresh. - view.SetVulnerabilities(file2, vuln2) - - t.Run("fresh", func(t *testing.T) { - got := view.Vulnerabilities() - want := map[span.URI]*vulncheck.Result{ - file1: vuln1, - file2: vuln2, - } - - if diff := cmp.Diff(toJSON(want), toJSON(got)); diff != "" { - t.Errorf("view.Vulnerabilities() mismatch (-want +got):\n%s", diff) - } - }) - - // maxGovulncheckResultAge/2 later - timeNow = func() time.Time { return now.Add(maxGovulncheckResultAge / 2) } - t.Run("after30min", func(t *testing.T) { - got := view.Vulnerabilities() - want := map[span.URI]*vulncheck.Result{ - file1: nil, // expired. - file2: vuln2, - } - - if diff := cmp.Diff(toJSON(want), toJSON(got)); diff != "" { - t.Errorf("view.Vulnerabilities() mismatch (-want +got):\n%s", diff) - } - }) - - // maxGovulncheckResultAge later - timeNow = func() time.Time { return now.Add(maxGovulncheckResultAge + time.Minute) } - - t.Run("after1hr", func(t *testing.T) { - got := view.Vulnerabilities() - want := map[span.URI]*vulncheck.Result{ - file1: nil, - file2: nil, - } - - if diff := cmp.Diff(toJSON(want), toJSON(got)); diff != "" { - t.Errorf("view.Vulnerabilities() mismatch (-want +got):\n%s", diff) - } - }) -} - func toJSON(x interface{}) string { b, _ := json.MarshalIndent(x, "", " ") return string(b) diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go index 2d5804afc6f..5d814e0d996 100644 --- a/gopls/internal/lsp/command.go +++ b/gopls/internal/lsp/command.go @@ -216,37 +216,44 @@ func (c *commandHandler) RegenerateCgo(ctx context.Context, args command.URIArg) return c.run(ctx, commandConfig{ progress: "Regenerating Cgo", }, func(ctx context.Context, _ commandDeps) error { - var wg sync.WaitGroup // tracks work done on behalf of this function, incl. diagnostics - wg.Add(1) - defer wg.Done() - - // Track progress on this operation for testing. - if c.s.Options().VerboseWorkDoneProgress { - work := c.s.progress.Start(ctx, DiagnosticWorkTitle(FromRegenerateCgo), "Calculating file diagnostics...", nil, nil) - go func() { - wg.Wait() - work.End(ctx, "Done.") - }() - } - - // Resetting the view causes cgo to be regenerated via `go list`. - v, err := c.s.session.ResetView(ctx, args.URI.SpanURI()) - if err != nil { - return err - } + return c.modifyState(ctx, FromRegenerateCgo, func() (source.Snapshot, func(), error) { + // Resetting the view causes cgo to be regenerated via `go list`. + v, err := c.s.session.ResetView(ctx, args.URI.SpanURI()) + if err != nil { + return nil, nil, err + } + return v.Snapshot() + }) + }) +} - snapshot, release, err := v.Snapshot() - if err != nil { - return err - } - wg.Add(1) +// modifyState performs an operation that modifies the snapshot state. +// +// It causes a snapshot diagnosis for the provided ModificationSource. +func (c *commandHandler) modifyState(ctx context.Context, source ModificationSource, work func() (source.Snapshot, func(), error)) error { + var wg sync.WaitGroup // tracks work done on behalf of this function, incl. diagnostics + wg.Add(1) + defer wg.Done() + + // Track progress on this operation for testing. + if c.s.Options().VerboseWorkDoneProgress { + work := c.s.progress.Start(ctx, DiagnosticWorkTitle(source), "Calculating file diagnostics...", nil, nil) go func() { - c.s.diagnoseSnapshot(snapshot, nil, true, 0) - release() - wg.Done() + wg.Wait() + work.End(ctx, "Done.") }() - return nil - }) + } + snapshot, release, err := work() + if err != nil { + return err + } + wg.Add(1) + go func() { + c.s.diagnoseSnapshot(snapshot, nil, true, 0) + release() + wg.Done() + }() + return nil } func (c *commandHandler) CheckUpgrades(ctx context.Context, args command.CheckUpgradesArgs) error { @@ -254,14 +261,16 @@ func (c *commandHandler) CheckUpgrades(ctx context.Context, args command.CheckUp forURI: args.URI, progress: "Checking for upgrades", }, func(ctx context.Context, deps commandDeps) error { - upgrades, err := c.s.getUpgrades(ctx, deps.snapshot, args.URI.SpanURI(), args.Modules) - if err != nil { - return err - } - deps.snapshot.View().RegisterModuleUpgrades(args.URI.SpanURI(), upgrades) - // Re-diagnose the snapshot to publish the new module diagnostics. - c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0) - return nil + return c.modifyState(ctx, FromCheckUpgrades, func() (source.Snapshot, func(), error) { + upgrades, err := c.s.getUpgrades(ctx, deps.snapshot, args.URI.SpanURI(), args.Modules) + if err != nil { + return nil, nil, err + } + snapshot, release := deps.snapshot.View().Invalidate(ctx, source.StateChange{ + ModuleUpgrades: map[span.URI]map[string]string{args.URI.SpanURI(): upgrades}, + }) + return snapshot, release, nil + }) }) } @@ -277,22 +286,17 @@ func (c *commandHandler) ResetGoModDiagnostics(ctx context.Context, args command return c.run(ctx, commandConfig{ forURI: args.URI, }, func(ctx context.Context, deps commandDeps) error { - // Clear all diagnostics coming from the upgrade check source and vulncheck. - // This will clear the diagnostics in all go.mod files, but they - // will be re-calculated when the snapshot is diagnosed again. - if args.DiagnosticSource == "" || args.DiagnosticSource == string(source.UpgradeNotification) { - deps.snapshot.View().ClearModuleUpgrades(args.URI.SpanURI()) - c.s.clearDiagnosticSource(modCheckUpgradesSource) - } - - if args.DiagnosticSource == "" || args.DiagnosticSource == string(source.Govulncheck) { - deps.snapshot.View().SetVulnerabilities(args.URI.SpanURI(), nil) - c.s.clearDiagnosticSource(modVulncheckSource) - } - - // Re-diagnose the snapshot to remove the diagnostics. - c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0) - return nil + return c.modifyState(ctx, FromResetGoModDiagnostics, func() (source.Snapshot, func(), error) { + snapshot, release := deps.snapshot.View().Invalidate(ctx, source.StateChange{ + ModuleUpgrades: map[span.URI]map[string]string{ + deps.fh.URI(): nil, + }, + Vulns: map[span.URI]*vulncheck.Result{ + deps.fh.URI(): nil, + }, + }) + return snapshot, release, nil + }) }) } @@ -734,6 +738,7 @@ func addModuleRequire(invoke func(...string) (*bytes.Buffer, error), args []stri return err } +// TODO(rfindley): inline. func (s *server) getUpgrades(ctx context.Context, snapshot source.Snapshot, uri span.URI, modules []string) (map[string]string, error) { stdout, err := snapshot.RunGoCommandDirect(ctx, source.Normal|source.AllowNetwork, &gocommand.Invocation{ Verb: "list", @@ -950,7 +955,7 @@ func (c *commandHandler) FetchVulncheckResult(ctx context.Context, arg command.U } } // Overwrite if there is any govulncheck-based result. - for modfile, result := range deps.snapshot.View().Vulnerabilities() { + for modfile, result := range deps.snapshot.Vulnerabilities() { ret[protocol.URIFromSpanURI(modfile)] = result } return nil @@ -986,8 +991,11 @@ func (c *commandHandler) RunGovulncheck(ctx context.Context, args command.Vulnch return err } - deps.snapshot.View().SetVulnerabilities(args.URI.SpanURI(), result) - c.s.diagnoseSnapshot(deps.snapshot, nil, false, 0) + snapshot, release := deps.snapshot.View().Invalidate(ctx, source.StateChange{ + Vulns: map[span.URI]*vulncheck.Result{args.URI.SpanURI(): result}, + }) + defer release() + c.s.diagnoseSnapshot(snapshot, nil, false, 0) affecting := make(map[string]bool, len(result.Entries)) for _, finding := range result.Findings { diff --git a/gopls/internal/lsp/mod/diagnostics.go b/gopls/internal/lsp/mod/diagnostics.go index 9f901206988..55002f6435e 100644 --- a/gopls/internal/lsp/mod/diagnostics.go +++ b/gopls/internal/lsp/mod/diagnostics.go @@ -140,7 +140,7 @@ func ModUpgradeDiagnostics(ctx context.Context, snapshot source.Snapshot, fh sou return nil, err } - upgrades := snapshot.View().ModuleUpgrades(fh.URI()) + upgrades := snapshot.ModuleUpgrades(fh.URI()) for _, req := range pm.File.Require { ver, ok := upgrades[req.Mod.Path] if !ok || req.Mod.Version == ver { @@ -189,7 +189,7 @@ func ModVulnerabilityDiagnostics(ctx context.Context, snapshot source.Snapshot, } diagSource := source.Govulncheck - vs := snapshot.View().Vulnerabilities(fh.URI())[fh.URI()] + vs := snapshot.Vulnerabilities(fh.URI())[fh.URI()] if vs == nil && snapshot.Options().Vulncheck == source.ModeVulncheckImports { vs, err = snapshot.ModVuln(ctx, fh.URI()) if err != nil { diff --git a/gopls/internal/lsp/mod/hover.go b/gopls/internal/lsp/mod/hover.go index b39993b2924..fc1705b3798 100644 --- a/gopls/internal/lsp/mod/hover.go +++ b/gopls/internal/lsp/mod/hover.go @@ -83,7 +83,7 @@ func hoverOnRequireStatement(ctx context.Context, pm *source.ParsedModule, offse // Get the vulnerability info. fromGovulncheck := true - vs := snapshot.View().Vulnerabilities(fh.URI())[fh.URI()] + vs := snapshot.Vulnerabilities(fh.URI())[fh.URI()] if vs == nil && snapshot.Options().Vulncheck == source.ModeVulncheckImports { var err error vs, err = snapshot.ModVuln(ctx, fh.URI()) @@ -140,7 +140,7 @@ func hoverOnModuleStatement(ctx context.Context, pm *source.ParsedModule, offset return nil, false } fromGovulncheck := true - vs := snapshot.View().Vulnerabilities(fh.URI())[fh.URI()] + vs := snapshot.Vulnerabilities(fh.URI())[fh.URI()] if vs == nil && snapshot.Options().Vulncheck == source.ModeVulncheckImports { vs, err = snapshot.ModVuln(ctx, fh.URI()) diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 46c2eeeb609..3e9578d3395 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -257,6 +257,18 @@ type Snapshot interface { // If these indexes cannot be loaded from cache, the requested packages may // be type-checked. MethodSets(ctx context.Context, ids ...PackageID) ([]*methodsets.Index, error) + + // ModuleUpgrades returns known module upgrades for the dependencies of + // modfile. + ModuleUpgrades(modfile span.URI) map[string]string + + // Vulnerabilities returns known vulnerabilities for the given modfile. + // + // Results more than an hour old are excluded. + // + // TODO(suzmue): replace command.Vuln with a different type, maybe + // https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck/govulnchecklib#Summary? + Vulnerabilities(modfile ...span.URI) map[span.URI]*vulncheck.Result } // NarrowestMetadataForFile returns metadata for the narrowest package @@ -400,30 +412,14 @@ type View interface { // release function need not be called. Snapshot() (Snapshot, func(), error) + // Invalidate is documented at its sole implementation in the cache package. + // (this interface is to be deleted). + Invalidate(context.Context, StateChange) (Snapshot, func()) + // IsGoPrivatePath reports whether target is a private import path, as identified // by the GOPRIVATE environment variable. IsGoPrivatePath(path string) bool - // ModuleUpgrades returns known module upgrades for the dependencies of - // modfile. - ModuleUpgrades(modfile span.URI) map[string]string - - // RegisterModuleUpgrades registers that upgrades exist for the given modules - // required by modfile. - RegisterModuleUpgrades(modfile span.URI, upgrades map[string]string) - - // ClearModuleUpgrades clears all upgrades for the modules in modfile. - ClearModuleUpgrades(modfile span.URI) - - // Vulnerabilities returns known vulnerabilities for the given modfile. - // TODO(suzmue): replace command.Vuln with a different type, maybe - // https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck/govulnchecklib#Summary? - Vulnerabilities(modfile ...span.URI) map[span.URI]*vulncheck.Result - - // SetVulnerabilities resets the list of vulnerabilities that exists for the given modules - // required by modfile. - SetVulnerabilities(modfile span.URI, vulncheckResult *vulncheck.Result) - // GoVersion returns the configured Go version for this view. GoVersion() int @@ -432,6 +428,16 @@ type View interface { GoVersionString() string } +// A StateChange describes external state changes that may affect a snapshot. +// +// By far the most common of these is a change to file state, but a query of +// module upgrade information or vulnerabilities also affects gopls' behavior. +type StateChange struct { + Files map[span.URI]FileHandle + ModuleUpgrades map[span.URI]map[string]string + Vulns map[span.URI]*vulncheck.Result +} + // A FileSource maps URIs to FileHandles. type FileSource interface { // ReadFile returns the FileHandle for a given URI, either by diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go index 4d35d8424e4..1daaeae14c5 100644 --- a/gopls/internal/lsp/text_synchronization.go +++ b/gopls/internal/lsp/text_synchronization.go @@ -49,6 +49,14 @@ const ( // FromInitialWorkspaceLoad refers to the loading of all packages in the // workspace when the view is first created. FromInitialWorkspaceLoad + + // FromCheckUpgrades refers to state changes resulting from the CheckUpgrades + // command, which queries module upgrades. + FromCheckUpgrades + + // FromResetGoModDiagnostics refers to state changes resulting from the + // ResetGoModDiagnostics command. + FromResetGoModDiagnostics ) func (m ModificationSource) String() string { @@ -67,6 +75,10 @@ func (m ModificationSource) String() string { return "regenerate cgo" case FromInitialWorkspaceLoad: return "initial workspace load" + case FromCheckUpgrades: + return "from check upgrades" + case FromResetGoModDiagnostics: + return "from resetting go.mod diagnostics" default: return "unknown file modification" } diff --git a/gopls/internal/persistent/map.go b/gopls/internal/persistent/map.go index 64cd500c65a..c168aa6a9da 100644 --- a/gopls/internal/persistent/map.go +++ b/gopls/internal/persistent/map.go @@ -146,6 +146,15 @@ func (pm *Map[K, V]) Clear() { pm.root = nil } +// Keys returns all keys present in the map. +func (pm *Map[K, V]) Keys() []K { + var keys []K + pm.root.forEach(func(k, _ any) { + keys = append(keys, k.(K)) + }) + return keys +} + // Range calls f sequentially in ascending key order for all entries in the map. func (pm *Map[K, V]) Range(f func(key K, value V)) { pm.root.forEach(func(k, v any) { diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index db222b31725..c162e7b3e8e 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/gopls/internal/regtest/codelens/codelens_test.go @@ -208,18 +208,10 @@ require golang.org/x/hello v1.2.3 env.OpenFile("a/go.mod") env.OpenFile("b/go.mod") - // Await the diagnostics resulting from opening the modfiles, because - // otherwise they may cause races when running asynchronously to the - // explicit re-diagnosing below. - // - // TODO(golang/go#58750): there is still a race here, inherent to - // accessing state on the View; we should create a new snapshot when - // the view diagnostics change. - env.AfterChange() - env.ExecuteCodeLensCommand("a/go.mod", command.CheckUpgrades, nil) d := &protocol.PublishDiagnosticsParams{} env.OnceMet( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromCheckUpgrades), 1, true), Diagnostics(env.AtRegexp("a/go.mod", `require`), WithMessage("can be upgraded")), ReadDiagnostics("a/go.mod", d), // We do not want there to be a diagnostic for b/go.mod, @@ -230,9 +222,15 @@ require golang.org/x/hello v1.2.3 ) // Check for upgrades in b/go.mod and then clear them. env.ExecuteCodeLensCommand("b/go.mod", command.CheckUpgrades, nil) - env.Await(Diagnostics(env.AtRegexp("b/go.mod", `require`), WithMessage("can be upgraded"))) + env.OnceMet( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromCheckUpgrades), 2, true), + Diagnostics(env.AtRegexp("b/go.mod", `require`), WithMessage("can be upgraded")), + ) env.ExecuteCodeLensCommand("b/go.mod", command.ResetGoModDiagnostics, nil) - env.Await(NoDiagnostics(ForFile("b/go.mod"))) + env.OnceMet( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromResetGoModDiagnostics), 1, true), + NoDiagnostics(ForFile("b/go.mod")), + ) // Apply the diagnostics to a/go.mod. env.ApplyQuickFixes("a/go.mod", d.Diagnostics) @@ -293,7 +291,7 @@ func main() { WithOptions(ProxyFiles(proxy)).Run(t, shouldRemoveDep, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.ExecuteCodeLensCommand("go.mod", command.Tidy, nil) - env.Await(env.DoneWithChangeWatchedFiles()) + env.AfterChange() got := env.BufferText("go.mod") const wantGoMod = `module mod.com diff --git a/gopls/internal/regtest/misc/vuln_test.go b/gopls/internal/regtest/misc/vuln_test.go index bb15e7b9d89..68b664983a5 100644 --- a/gopls/internal/regtest/misc/vuln_test.go +++ b/gopls/internal/regtest/misc/vuln_test.go @@ -13,9 +13,11 @@ import ( "sort" "strings" "testing" + "time" "github.com/google/go-cmp/cmp" + "golang.org/x/tools/gopls/internal/lsp/cache" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" @@ -205,28 +207,9 @@ func main() { ).Run(t, files, func(t *testing.T, env *Env) { env.OpenFile("go.mod") - // Test CodeLens is present. - lenses := env.CodeLens("go.mod") - - const wantCommand = "gopls." + string(command.RunGovulncheck) - var gotCodelens = false - var lens protocol.CodeLens - for _, l := range lenses { - if l.Command.Command == wantCommand { - gotCodelens = true - lens = l - break - } - } - if !gotCodelens { - t.Fatal("got no vulncheck codelens") - } // Run Command included in the codelens. var result command.RunVulncheckResult - env.ExecuteCommand(&protocol.ExecuteCommandParams{ - Command: lens.Command.Command, - Arguments: lens.Command.Arguments, - }, &result) + env.ExecuteCodeLensCommand("go.mod", command.RunGovulncheck, &result) env.OnceMet( CompletedProgress(result.Token, nil), @@ -237,7 +220,6 @@ func main() { "go.mod": {IDs: []string{"GOSTDLIB"}, Mode: vulncheck.ModeGovulncheck}}) }) } - func TestFetchVulncheckResultStd(t *testing.T) { const files = ` -- go.mod -- @@ -617,6 +599,41 @@ func TestRunVulncheckPackageDiagnostics(t *testing.T) { } } +// TestRunGovulncheck_Expiry checks that govulncheck results expire after a +// certain amount of time. +func TestRunGovulncheck_Expiry(t *testing.T) { + // For this test, set the max age to a duration smaller than the sleep below. + defer func(prev time.Duration) { + cache.MaxGovulncheckResultAge = prev + }(cache.MaxGovulncheckResultAge) + cache.MaxGovulncheckResultAge = 99 * time.Millisecond + + db, opts0, err := vulnTestEnv(vulnsData, proxy1) + if err != nil { + t.Fatal(err) + } + defer db.Clean() + + WithOptions(opts0...).Run(t, workspace1, func(t *testing.T, env *Env) { + env.OpenFile("go.mod") + env.OpenFile("x/x.go") + + var result command.RunVulncheckResult + env.ExecuteCodeLensCommand("go.mod", command.RunGovulncheck, &result) + env.OnceMet( + CompletedProgress(result.Token, nil), + ShownMessage("Found"), + ) + // Sleep long enough for the results to expire. + time.Sleep(100 * time.Millisecond) + // Make an arbitrary edit to force re-diagnosis of the workspace. + env.RegexpReplace("x/x.go", "package x", "package x ") + env.AfterChange( + NoDiagnostics(env.AtRegexp("go.mod", "golang.org/bmod")), + ) + }) +} + func stringify(a interface{}) string { data, _ := json.Marshal(a) return string(data) diff --git a/gopls/internal/vulncheck/scan/command.go b/gopls/internal/vulncheck/scan/command.go index 89d24e08b71..ee05dae1581 100644 --- a/gopls/internal/vulncheck/scan/command.go +++ b/gopls/internal/vulncheck/scan/command.go @@ -48,6 +48,9 @@ func Main(ctx context.Context, args ...string) error { // RunGovulncheck implements the codelens "Run Govulncheck" // that runs 'gopls vulncheck' and converts the output to gopls's internal data // used for diagnostics and hover message construction. +// +// TODO(rfindley): this should accept a *View (which exposes) Options, rather +// than a snapshot. func RunGovulncheck(ctx context.Context, pattern string, snapshot source.Snapshot, dir string, log io.Writer) (*vulncheck.Result, error) { vulncheckargs := []string{ "vulncheck", "--",