internal/lsp: handle unknown revision in go.mod file

This change ensures that, when the initial workspace load fails, we
re-run it if the go.mod file changes. Previously, if a user opened a
workspace with a corrupt go.mod file, we never recovered.

To reinitialize the workspace on-demand, we use the initializeOnce field
as an indicator of whether or not we should reinitialize. Every call to
awaitInitialized (which is called by all functions that need the IWL),
passes through the initialization code. If a retry isn't necessary,
this is a no-op, but if it is, we will call the initialization logic.
Only the first attempt uses a detached context; subsequent attempts can
be canceled by their contexts.

To indicate that we should reinitialize, we call maybeReinitialize.
Right now, we only call this when the go.mod file changes. In the
future, we may need it in other cases.

Fixes golang/go#38232

Change-Id: I77eefebb0ac38fbd0fe2c7da09c864eba45b075f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/242159
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>

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,
+		session:            s,
-		initialized:   make(chan struct{}),
+		initialized:        make(chan struct{}),
-		id:            strconv.FormatInt(index, 10),
+		initializationSema: make(chan struct{}, 1),
-		options:       options,
+		initializeOnce:     &sync.Once{},
-		baseCtx:       baseCtx,
+		id:                 strconv.FormatInt(index, 10),
-		backgroundCtx: backgroundCtx,
+		options:            options,
-		cancel:        cancel,
+		baseCtx:            baseCtx,
-		name:          name,
+		backgroundCtx:      backgroundCtx,
-		folder:        folder,
+		cancel:             cancel,
-		filesByURI:    make(map[span.URI]*fileBase),
+		name:               name,
-		filesByBase:   make(map[string][]*fileBase),
+		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
+	v.initCancelFirstAttempt = initCancel
-	go v.initialize(initCtx, v.snapshot)
+	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 {
+			var (
-				views[view][c.URI] = o
+				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

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.
+	// initialized is closed when the view has been fully initialized. On
-	// On initialization, the view's workspace packages are loaded.
+	// initialization, the view's workspace packages are loaded. All of the
-	// All of the fields below are set as part of initialization.
+	// fields below are set as part of initialization. If we failed to load, we
-	// If we failed to load, we don't re-try to avoid too many go/packages calls.
+	// only retry if the go.mod file changes, to avoid too many go/packages
-	initializeOnce sync.Once
+	// calls.
-	initialized    chan struct{}
+	//
-	initCancel     context.CancelFunc
+	// 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
+	// initializationSema is used as a mutex to guard initializeOnce and
-	// has been set.
+	// initializedErr, which will be updated after each attempt to initialize
-	initializedErr error
+	// 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) {
+func (v *View) initialize(ctx context.Context, s *snapshot, firstAttempt bool) {
-	v.initializeOnce.Do(func() {
+	select {
-		defer close(v.initialized)
+	case <-ctx.Done():
+		return
+	case v.initializationSema <- struct{}{}:
+	}
 
-		if err := s.load(ctx, viewLoadScope("LOAD_VIEW"), packagePath("builtin")); err != nil {
+	defer func() {
-			if ctx.Err() != nil {
+		<-v.initializationSema
-				return
+	}()
+
+	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)

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 {
+	if err != nil {
-		item, err := s.client.ShowMessageRequest(ctx, &protocol.ShowMessageRequestParams{
+		s.handleFatalErrors(ctx, snapshot, err)
-			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()))
 		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()))
+		}
+	}
+}

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 {
+		for _, m := range s.showMessage {
-			return Met, title
+			if strings.Contains(m.Message, title) {
+				return Met, m
+			}
 		}
 		return Unmet, nil
 	}

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))
+	})
+}