diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index bec8fce39a..3fadd36012 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -128,17 +128,19 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, backgroundCtx, cancel := context.WithCancel(baseCtx) v := &View{ - session: s, - initialized: make(chan struct{}), - id: strconv.FormatInt(index, 10), - options: options, - baseCtx: baseCtx, - backgroundCtx: backgroundCtx, - cancel: cancel, - name: name, - folder: folder, - filesByURI: make(map[span.URI]*fileBase), - filesByBase: make(map[string][]*fileBase), + session: s, + initialized: make(chan struct{}), + initializationSema: make(chan struct{}, 1), + initializeOnce: &sync.Once{}, + id: strconv.FormatInt(index, 10), + options: options, + baseCtx: baseCtx, + backgroundCtx: backgroundCtx, + cancel: cancel, + name: name, + folder: folder, + filesByURI: make(map[span.URI]*fileBase), + filesByBase: make(map[string][]*fileBase), snapshot: &snapshot{ id: snapshotID, packages: make(map[packageKey]*packageHandle), @@ -171,8 +173,8 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, // Initialize the view without blocking. initCtx, initCancel := context.WithCancel(xcontext.Detach(ctx)) - v.initCancel = initCancel - go v.initialize(initCtx, v.snapshot) + v.initCancelFirstAttempt = initCancel + go v.initialize(initCtx, v.snapshot, true) return v, v.snapshot, nil } @@ -324,7 +326,7 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif if err != nil { return nil, err } - forceReloadMetadata := false + var forceReloadMetadata bool for _, c := range changes { if c.Action == source.InvalidateMetadata { forceReloadMetadata = true @@ -344,15 +346,27 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif if _, ok := views[view]; !ok { views[view] = make(map[span.URI]source.FileHandle) } - if o, ok := overlays[c.URI]; ok { - views[view][c.URI] = o + var ( + fh source.FileHandle + ok bool + err error + ) + if fh, ok = overlays[c.URI]; ok { + views[view][c.URI] = fh } else { - fh, err := s.cache.getFile(ctx, c.URI) + fh, err = s.cache.getFile(ctx, c.URI) if err != nil { return nil, err } views[view][c.URI] = fh } + // If the file change is to a go.mod file, and initialization for + // the view has previously failed, we should attempt to retry. + // TODO(rstambler): We can use unsaved contents with -modfile, so + // maybe we should do that and retry on any change? + if fh.Kind() == source.Mod && (c.OnDisk || c.Action == source.Save) { + view.maybeReinitialize() + } } } var snapshots []source.Snapshot diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 95f03b173a..2d2b1e6798 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -86,17 +86,30 @@ type View struct { snapshotMu sync.Mutex snapshot *snapshot - // initialized is closed when the view has been fully initialized. - // On initialization, the view's workspace packages are loaded. - // All of the fields below are set as part of initialization. - // If we failed to load, we don't re-try to avoid too many go/packages calls. - initializeOnce sync.Once - initialized chan struct{} - initCancel context.CancelFunc + // initialized is closed when the view has been fully initialized. On + // initialization, the view's workspace packages are loaded. All of the + // fields below are set as part of initialization. If we failed to load, we + // only retry if the go.mod file changes, to avoid too many go/packages + // calls. + // + // When the view is created, initializeOnce is non-nil, initialized is + // open, and initCancelFirstAttempt can be used to terminate + // initialization. Once initialization completes, initializedErr may be set + // and initializeOnce becomes nil. If initializedErr is non-nil, + // initialization may be retried (depending on how files are changed). To + // indicate that initialization should be retried, initializeOnce will be + // set. The next time a caller requests workspace packages, the + // initialization will retry. + initialized chan struct{} + initCancelFirstAttempt context.CancelFunc - // initializedErr needs no mutex, since any access to it happens after it - // has been set. - initializedErr error + // initializationSema is used as a mutex to guard initializeOnce and + // initializedErr, which will be updated after each attempt to initialize + // the view. We use a channel instead of a mutex to avoid blocking when a + // context is canceled. + initializationSema chan struct{} + initializeOnce *sync.Once + initializedErr error // builtin pins the AST and package for builtin.go in memory. builtin *builtinPackageHandle @@ -639,7 +652,7 @@ func (v *View) Shutdown(ctx context.Context) { func (v *View) shutdown(ctx context.Context) { // Cancel the initial workspace load if it is still running. - v.initCancel() + v.initCancelFirstAttempt() v.mu.Lock() defer v.mu.Unlock() @@ -702,25 +715,48 @@ func (v *View) getSnapshot() *snapshot { return v.snapshot } -func (v *View) initialize(ctx context.Context, s *snapshot) { - v.initializeOnce.Do(func() { - defer close(v.initialized) +func (v *View) initialize(ctx context.Context, s *snapshot, firstAttempt bool) { + select { + case <-ctx.Done(): + return + case v.initializationSema <- struct{}{}: + } - if err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")); err != nil { - if ctx.Err() != nil { - return + defer func() { + <-v.initializationSema + }() + + if v.initializeOnce == nil { + return + } + v.initializeOnce.Do(func() { + defer func() { + v.initializeOnce = nil + if firstAttempt { + close(v.initialized) } - v.initializedErr = err + }() + + err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")) + if ctx.Err() != nil { + return + } + if err != nil { event.Error(ctx, "initial workspace load failed", err) } + v.initializedErr = err }) } func (v *View) awaitInitialized(ctx context.Context) { select { case <-ctx.Done(): + return case <-v.initialized: } + // We typically prefer to run something as intensive as the IWL without + // blocking. I'm not sure if there is a way to do that here. + v.initialize(ctx, v.getSnapshot(), false) } // invalidateContent invalidates the content of a Go file, @@ -756,6 +792,19 @@ func (v *View) cancelBackground() { v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx) } +func (v *View) maybeReinitialize() { + v.initializationSema <- struct{}{} + defer func() { + <-v.initializationSema + }() + + if v.initializedErr == nil { + return + } + var once sync.Once + v.initializeOnce = &once +} + func (v *View) setBuildInformation(ctx context.Context, folder span.URI, env []string, modfileFlagEnabled bool) error { if err := checkPathCase(folder.Filename()); err != nil { return fmt.Errorf("invalid workspace configuration: %w", err) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index b1f9085659..e5a83ad7de 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -28,7 +28,7 @@ func (s *Server) diagnoseDetached(snapshot source.Snapshot) { ctx = xcontext.Detach(ctx) reports, shows := s.diagnose(ctx, snapshot, false) if shows != nil { - // If a view has been created or the configuration changed, warn the user + // If a view has been created or the configuration changed, warn the user. s.client.ShowMessage(ctx, shows) } s.publishReports(ctx, snapshot, reports) @@ -36,7 +36,8 @@ func (s *Server) diagnoseDetached(snapshot source.Snapshot) { func (s *Server) diagnoseSnapshot(snapshot source.Snapshot) { ctx := snapshot.View().BackgroundContext() - // Ignore possible workspace configuration warnings in the normal flow + + // Ignore possible workspace configuration warnings in the normal flow. reports, _ := s.diagnose(ctx, snapshot, false) s.publishReports(ctx, snapshot, reports) } @@ -67,7 +68,6 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA if ctx.Err() != nil { return nil, nil } - modURI := snapshot.View().ModFile() for id, diags := range reports { if id.URI == "" { event.Error(ctx, "missing URI for module diagnostics", fmt.Errorf("empty URI"), tag.Directory.Of(snapshot.View().Folder().Filename())) @@ -82,27 +82,8 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, alwaysA // Diagnose all of the packages in the workspace. wsPackages, err := snapshot.WorkspacePackages(ctx) - if err == source.InconsistentVendoring { - item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{ - Type: protocol.Error, - Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". -See https://github.com/golang/go/issues/39164 for more detail on this issue.`, - Actions: []protocol.MessageActionItem{ - {Title: "go mod vendor"}, - }, - }) - if item == nil || err != nil { - return nil, nil - } - if err := s.directGoModCommand(ctx, protocol.URIFromSpanURI(modURI), "mod", []string{"vendor"}...); err != nil { - return nil, &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: fmt.Sprintf(`"go mod vendor" failed with %v`, err), - } - } - return nil, nil - } else if err != nil { - event.Error(ctx, "failed to load workspace packages, skipping diagnostics", err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) + if err != nil { + s.handleFatalErrors(ctx, snapshot, err) return nil, nil } var shows *protocol.ShowMessageParams @@ -247,3 +228,39 @@ func toProtocolDiagnostics(diagnostics []*source.Diagnostic) []protocol.Diagnost } return reports } + +func (s *Server) handleFatalErrors(ctx context.Context, snapshot source.Snapshot, err error) { + switch err { + case source.InconsistentVendoring: + item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{ + Type: protocol.Error, + Message: `Inconsistent vendoring detected. Please re-run "go mod vendor". +See https://github.com/golang/go/issues/39164 for more detail on this issue.`, + Actions: []protocol.MessageActionItem{ + {Title: "go mod vendor"}, + }, + }) + if item == nil || err != nil { + event.Error(ctx, "go mod vendor ShowMessageRequest failed", err, tag.Directory.Of(snapshot.View().Folder())) + return + } + modURI := snapshot.View().ModFile() + if err := s.directGoModCommand(ctx, protocol.URIFromSpanURI(modURI), "mod", []string{"vendor"}...); err != nil { + if err := s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: fmt.Sprintf(`"go mod vendor" failed with %v`, err), + }); err != nil { + event.Error(ctx, "ShowMessage failed", err) + } + } + default: + msg := "failed to load workspace packages, skipping diagnostics" + event.Error(ctx, msg, err, tag.Snapshot.Of(snapshot.ID()), tag.Directory.Of(snapshot.View().Folder())) + if err := s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Error, + Message: fmt.Sprintf("%s: %v", msg, err), + }); err != nil { + event.Error(ctx, "ShowMessage failed", err, tag.Directory.Of(snapshot.View().Folder())) + } + } +} diff --git a/internal/lsp/regtest/env.go b/internal/lsp/regtest/env.go index b1ef95c2ba..5ce13ca04c 100644 --- a/internal/lsp/regtest/env.go +++ b/internal/lsp/regtest/env.go @@ -364,11 +364,13 @@ func EmptyShowMessage(title string) SimpleExpectation { } } -// SomeShowMessage asserts that the editor has received a ShowMessage. +// SomeShowMessage asserts that the editor has received a ShowMessage with the given title. func SomeShowMessage(title string) SimpleExpectation { check := func(s State) (Verdict, interface{}) { - if len(s.showMessage) > 0 { - return Met, title + for _, m := range s.showMessage { + if strings.Contains(m.Message, title) { + return Met, m + } } return Unmet, nil } diff --git a/internal/lsp/regtest/modfile_test.go b/internal/lsp/regtest/modfile_test.go index 503a789c09..b71fdd3d47 100644 --- a/internal/lsp/regtest/modfile_test.go +++ b/internal/lsp/regtest/modfile_test.go @@ -322,3 +322,78 @@ require ( } }, WithProxy(badModule)) } + +// Reproduces golang/go#38232. +func TestUnknownRevision(t *testing.T) { + const unknown = ` +-- go.mod -- +module mod.com + +require ( + example.com v1.2.2 +) +-- main.go -- +package main + +import "example.com/blah" + +func main() { + var x = blah.Name +} +` + + // Start from a bad state/bad IWL, and confirm that we recover. + t.Run("bad", func(t *testing.T) { + runner.Run(t, unknown, func(t *testing.T, env *Env) { + env.Await( + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromInitialWorkspaceLoad), 1), + ) + env.OpenFile("go.mod") + env.Await( + SomeShowMessage("failed to load workspace packages, skipping diagnostics"), + ) + env.RegexpReplace("go.mod", "v1.2.2", "v1.2.3") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + }, WithProxy(proxy)) + }) + + const known = ` +-- go.mod -- +module mod.com + +require ( + example.com v1.2.3 +) +-- main.go -- +package main + +import "example.com/blah" + +func main() { + var x = blah.Name +} +` + // Start from a good state, transform to a bad state, and confirm that we + // still recover. + t.Run("good", func(t *testing.T) { + runner.Run(t, known, func(t *testing.T, env *Env) { + env.OpenFile("go.mod") + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + env.RegexpReplace("go.mod", "v1.2.3", "v1.2.2") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + SomeShowMessage("failed to load workspace packages, skipping diagnostics"), + ) + env.RegexpReplace("go.mod", "v1.2.2", "v1.2.3") + env.Editor.SaveBufferWithoutActions(env.Ctx, "go.mod") // go.mod changes must be on disk + env.Await( + env.DiagnosticAtRegexp("main.go", "x = "), + ) + }, WithProxy(proxy)) + }) +}