Skip to content

Commit

Permalink
gopls/internal/lsp: move options into the snapshot
Browse files Browse the repository at this point in the history 
Snapshots should be idempotent, and the fact that configuration changes
do not cause snapshots to increment has been a long-standing source of
bugs.

After a fair bit of setup, this CL finally moves options onto the
snapshot. The only remaining use of the options stored on the View is
for "minorOptionsChange" detection. This is of questionable value, but I
opted not to delete it in this CL (Chesterton's fence).

There is still more cleanup to do (and tests to update), but that is
deferred to later CLs.

Fixes golang/go#61325
Fixes golang/go#42814

Change-Id: If82dc199af13ddaa3464b2a67a8bab2013161f26
Reviewed-on: https://go-review.googlesource.com/c/tools/+/526159
LUCI-TryBot-Result: Go LUCI <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
  • Loading branch information
@findleyr
findleyr committed Sep 8, 2023
1 parent fb4bd11 commit 0a9721c
Show file tree
Hide file tree
Showing 17 changed files with 145 additions and 136 deletions.
4 changes: 2 additions & 2 deletions gopls/internal/lsp/cache/analysis.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit,
toSrc := make(map[*analysis.Analyzer]*source.Analyzer)
var enabled []*analysis.Analyzer // enabled subset + transitive requirements
for _, a := range analyzers {
if a.IsEnabled(snapshot.view.Options()) {
if a.IsEnabled(snapshot.options) {
toSrc[a.Analyzer] = a
enabled = append(enabled, a.Analyzer)
}
Expand Down Expand Up @@ -309,7 +309,7 @@ func (snapshot *snapshot) Analyze(ctx context.Context, pkgs map[PackageID]unit,
// Now that we have read all files,
// we no longer need the snapshot.
// (but options are needed for progress reporting)
options := snapshot.view.Options()
options := snapshot.options
snapshot = nil

// Progress reporting. If supported, gopls reports progress on analysis
Expand Down
4 changes: 2 additions & 2 deletions gopls/internal/lsp/cache/check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1345,8 +1345,8 @@ func (s *snapshot) typeCheckInputs(ctx context.Context, m *source.Metadata) (typ
depsByImpPath: m.DepsByImpPath,
goVersion: goVersion,

relatedInformation: s.view.Options().RelatedInformationSupported,
linkTarget: s.view.Options().LinkTarget,
relatedInformation: s.options.RelatedInformationSupported,
linkTarget: s.options.LinkTarget,
moduleMode: s.view.moduleMode(),
}, nil
}
Expand Down
14 changes: 6 additions & 8 deletions gopls/internal/lsp/cache/imports.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,13 @@ func (s *importsState) runProcessEnvFunc(ctx context.Context, snapshot *snapshot

// view.goEnv is immutable -- changes make a new view. Options can change.
// We can't compare build flags directly because we may add -modfile.
snapshot.view.optionsMu.Lock()
localPrefix := snapshot.view.options.Local
currentBuildFlags := snapshot.view.options.BuildFlags
currentDirectoryFilters := snapshot.view.options.DirectoryFilters
localPrefix := snapshot.options.Local
currentBuildFlags := snapshot.options.BuildFlags
currentDirectoryFilters := snapshot.options.DirectoryFilters
changed := !reflect.DeepEqual(currentBuildFlags, s.cachedBuildFlags) ||
snapshot.view.options.VerboseOutput != (s.processEnv.Logf != nil) ||
snapshot.options.VerboseOutput != (s.processEnv.Logf != nil) ||
modFileHash != s.cachedModFileHash ||
!reflect.DeepEqual(snapshot.view.options.DirectoryFilters, s.cachedDirectoryFilters)
snapshot.view.optionsMu.Unlock()
!reflect.DeepEqual(snapshot.options.DirectoryFilters, s.cachedDirectoryFilters)

// If anything relevant to imports has changed, clear caches and
// update the processEnv. Clearing caches blocks on any background
Expand Down Expand Up @@ -120,7 +118,7 @@ func populateProcessEnvFromSnapshot(ctx context.Context, pe *imports.ProcessEnv,
ctx, done := event.Start(ctx, "cache.populateProcessEnvFromSnapshot")
defer done()

if snapshot.view.Options().VerboseOutput {
if snapshot.options.VerboseOutput {
pe.Logf = func(format string, args ...interface{}) {
event.Log(ctx, fmt.Sprintf(format, args...))
}
Expand Down
12 changes: 6 additions & 6 deletions gopls/internal/lsp/cache/load.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
if err != nil {
continue
}
if isStandaloneFile(contents, s.view.Options().StandaloneTags) {
if isStandaloneFile(contents, s.options.StandaloneTags) {
standalone = true
query = append(query, uri.Filename())
} else {
Expand Down Expand Up @@ -160,7 +160,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
}

moduleErrs := make(map[string][]packages.Error) // module path -> errors
filterFunc := s.view.filterFunc()
filterFunc := s.filterFunc()
newMetadata := make(map[PackageID]*source.Metadata)
for _, pkg := range pkgs {
// The Go command returns synthetic list results for module queries that
Expand All @@ -178,7 +178,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
continue
}

if !containsDir || s.view.Options().VerboseOutput {
if !containsDir || s.options.VerboseOutput {
event.Log(ctx, eventName, append(
source.SnapshotLabels(s),
tag.Package.Of(pkg.ID),
Expand Down Expand Up @@ -359,7 +359,7 @@ func (s *snapshot) applyCriticalErrorToFiles(ctx context.Context, msg string, fi
for _, fh := range files {
// Place the diagnostics on the package or module declarations.
var rng protocol.Range
switch s.view.FileKind(fh) {
switch s.FileKind(fh) {
case source.Go:
if pgf, err := s.ParseGo(ctx, fh, source.ParseHeader); err == nil {
// Check that we have a valid `package foo` range to use for positioning the error.
Expand Down Expand Up @@ -616,7 +616,7 @@ func containsPackageLocked(s *snapshot, m *source.Metadata) bool {
uris[uri] = struct{}{}
}

filterFunc := s.view.filterFunc()
filterFunc := s.filterFunc()
for uri := range uris {
// Don't use view.contains here. go.work files may include modules
// outside of the workspace folder.
Expand Down Expand Up @@ -671,7 +671,7 @@ func containsFileInWorkspaceLocked(s *snapshot, m *source.Metadata) bool {

// The package's files are in this view. It may be a workspace package.
// Vendored packages are not likely to be interesting to the user.
if !strings.Contains(string(uri), "/vendor/") && s.view.contains(uri) {
if !strings.Contains(string(uri), "/vendor/") && s.contains(uri) {
return true
}
}
Expand Down
17 changes: 12 additions & 5 deletions gopls/internal/lsp/cache/session.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,9 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
v := &View{
id: strconv.FormatInt(index, 10),
gocmdRunner: s.gocmdRunner,
lastOptions: options,
initialWorkspaceLoad: make(chan struct{}),
initializationSema: make(chan struct{}, 1),
options: options,
baseCtx: baseCtx,
name: name,
moduleUpgrades: map[span.URI]map[string]string{},
Expand Down Expand Up @@ -167,6 +167,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
workspaceModFiles: wsModFiles,
workspaceModFilesErr: wsModFilesErr,
pkgIndex: typerefs.NewPackageIndex(),
options: options,
}
// Save one reference in the view.
v.releaseSnapshot = v.snapshot.Acquire()
Expand Down Expand Up @@ -255,9 +256,15 @@ func bestViewForURI(uri span.URI, views []*View) *View {
}
// TODO(rfindley): this should consider the workspace layout (i.e.
// go.work).
if view.contains(uri) {
snapshot, release, err := view.getSnapshot()
if err != nil {
// view is shutdown
continue
}
if snapshot.contains(uri) {
longest = view
}
release()
}
if longest != nil {
return longest
Expand Down Expand Up @@ -420,7 +427,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
// synchronously to change processing? Can we assume that the env did not
// change, and derive go.work using a combination of the configured
// GOWORK value and filesystem?
info, err := s.getWorkspaceInformation(ctx, view.folder, view.Options())
info, err := s.getWorkspaceInformation(ctx, view.folder, view.lastOptions)
if err != nil {
// Catastrophic failure, equivalent to a failure of session
// initialization and therefore should almost never happen. One
Expand All @@ -434,7 +441,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
}

if info != view.workspaceInformation {
if err := s.updateViewLocked(ctx, view, view.Options()); err != nil {
if err := s.updateViewLocked(ctx, view, view.lastOptions); err != nil {
// More catastrophic failure. The view may or may not still exist.
// The best we can do is log and move on.
event.Error(ctx, "recreating view", err)
Expand Down Expand Up @@ -492,7 +499,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif
var releases []func()
viewToSnapshot := map[*View]*snapshot{}
for view, changed := range views {
snapshot, release := view.invalidateContent(ctx, changed, forceReloadMetadata)
snapshot, release := view.invalidateContent(ctx, changed, nil, forceReloadMetadata)
releases = append(releases, release)
viewToSnapshot[view] = snapshot
}
Expand Down
73 changes: 51 additions & 22 deletions gopls/internal/lsp/cache/snapshot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,10 @@ type snapshot struct {
// active packages. Running type checking batches in parallel after an
// invalidation can cause redundant calculation of this shared state.
typeCheckMu sync.Mutex

// options holds the user configuration at the time this snapshot was
// created.
options *source.Options
}

var globalSnapshotID uint64
Expand Down Expand Up @@ -275,12 +279,39 @@ func (s *snapshot) View() source.View {
return s.view
}

func (s *snapshot) FileKind(h source.FileHandle) source.FileKind {
return s.view.FileKind(h)
func (s *snapshot) FileKind(fh source.FileHandle) source.FileKind {
// The kind of an unsaved buffer comes from the
// TextDocumentItem.LanguageID field in the didChange event,
// not from the file name. They may differ.
if o, ok := fh.(*Overlay); ok {
if o.kind != source.UnknownKind {
return o.kind
}
}

fext := filepath.Ext(fh.URI().Filename())
switch fext {
case ".go":
return source.Go
case ".mod":
return source.Mod
case ".sum":
return source.Sum
case ".work":
return source.Work
}
exts := s.options.TemplateExtensions
for _, ext := range exts {
if fext == ext || fext == "."+ext {
return source.Tmpl
}
}
// and now what? This should never happen, but it does for cgo before go1.15
return source.Go
}

func (s *snapshot) Options() *source.Options {
return s.view.Options() // temporarily return view options.
return s.options // temporarily return view options.
}

func (s *snapshot) BackgroundContext() context.Context {
Expand All @@ -306,7 +337,7 @@ func (s *snapshot) Templates() map[span.URI]source.FileHandle {

tmpls := map[span.URI]source.FileHandle{}
s.files.Range(func(k span.URI, fh source.FileHandle) {
if s.view.FileKind(fh) == source.Tmpl {
if s.FileKind(fh) == source.Tmpl {
tmpls[k] = fh
}
})
Expand Down Expand Up @@ -354,8 +385,7 @@ func (s *snapshot) workspaceMode() workspaceMode {
return mode
}
mode |= moduleMode
options := s.view.Options()
if options.TempModfile {
if s.options.TempModfile {
mode |= tempModfile
}
return mode
Expand All @@ -368,9 +398,6 @@ func (s *snapshot) workspaceMode() workspaceMode {
// multiple modules in on config, so buildOverlay needs to filter overlays by
// module.
func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packages.Config {
s.view.optionsMu.Lock()
verboseOutput := s.view.options.VerboseOutput
s.view.optionsMu.Unlock()

cfg := &packages.Config{
Context: ctx,
Expand All @@ -393,7 +420,7 @@ func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packa
panic("go/packages must not be used to parse files")
},
Logf: func(format string, args ...interface{}) {
if verboseOutput {
if s.options.VerboseOutput {
event.Log(ctx, fmt.Sprintf(format, args...))
}
},
Expand Down Expand Up @@ -475,18 +502,16 @@ func (s *snapshot) RunGoCommands(ctx context.Context, allowNetwork bool, wd stri
// it used only after call to tempModFile. Clarify that it is only
// non-nil on success.
func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.InvocationFlags, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) {
s.view.optionsMu.Lock()
allowModfileModificationOption := s.view.options.AllowModfileModifications
allowNetworkOption := s.view.options.AllowImplicitNetworkAccess
allowModfileModificationOption := s.options.AllowModfileModifications
allowNetworkOption := s.options.AllowImplicitNetworkAccess

// TODO(rfindley): this is very hard to follow, and may not even be doing the
// right thing: should inv.Env really trample view.options? Do we ever invoke
// this with a non-empty inv.Env?
//
// We should refactor to make it clearer that the correct env is being used.
inv.Env = append(append(append(os.Environ(), s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.GO111MODULE())
inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...)
s.view.optionsMu.Unlock()
inv.Env = append(append(append(os.Environ(), s.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.GO111MODULE())
inv.BuildFlags = append([]string{}, s.options.BuildFlags...)
cleanup = func() {} // fallback

// All logic below is for module mode.
Expand Down Expand Up @@ -993,8 +1018,7 @@ func (s *snapshot) workspaceDirs(ctx context.Context) []string {
// Code) that do not send notifications for individual files in a directory
// when the entire directory is deleted.
func (s *snapshot) watchSubdirs() bool {
opts := s.view.Options()
switch p := opts.SubdirWatchPatterns; p {
switch p := s.options.SubdirWatchPatterns; p {
case source.SubdirWatchPatternsOn:
return true
case source.SubdirWatchPatternsOff:
Expand All @@ -1007,7 +1031,7 @@ func (s *snapshot) watchSubdirs() bool {
// requirements that client names do not change. We should update the VS
// Code extension to set a default value of "subdirWatchPatterns" to "on",
// so that this workaround is only temporary.
if opts.ClientInfo != nil && opts.ClientInfo.Name == "Visual Studio Code" {
if s.options.ClientInfo != nil && s.options.ClientInfo.Name == "Visual Studio Code" {
return true
}
return false
Expand Down Expand Up @@ -1505,7 +1529,7 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error {
var files []*Overlay
for _, o := range open {
uri := o.URI()
if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go {
if s.IsBuiltin(uri) || s.FileKind(o) != source.Go {
continue
}
if len(meta.ids[uri]) == 0 {
Expand Down Expand Up @@ -1606,7 +1630,7 @@ func (s *snapshot) OrphanedFileDiagnostics(ctx context.Context) (map[span.URI]*s
searchOverlays:
for _, o := range s.overlays() {
uri := o.URI()
if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go {
if s.IsBuiltin(uri) || s.FileKind(o) != source.Go {
continue
}
md, err := s.MetadataForFile(ctx, uri)
Expand Down Expand Up @@ -1805,7 +1829,7 @@ 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, forceReloadMetadata bool) (*snapshot, func()) {
func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source.FileHandle, newOptions *source.Options, forceReloadMetadata bool) (*snapshot, func()) {
ctx, done := event.Start(ctx, "cache.snapshot.clone")
defer done()

Expand Down Expand Up @@ -1838,6 +1862,11 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]source
workspaceModFilesErr: s.workspaceModFilesErr,
importGraph: s.importGraph,
pkgIndex: s.pkgIndex,
options: s.options,
}

if newOptions != nil {
result.options = newOptions
}

// Create a lease on the new snapshot.
Expand Down
Loading

0 comments on commit 0a9721c

Please sign in to comment.