internal/modindex: fix nil panic, and other bugs

ReadIndex: when it fails to read a non-existent index,
it must return an error, since it did not complete its
task; (nil, nil) is not acceptable. Document this,
and fix all callers to handle the ErrNotExist case
specially if necessary.

Also:
- use a collision-resistant hash (SHA256, not CRC32) when
  choosing a unique filename for each GOMODCACHE value.

- Remove the special test-only logic in modcacheState.getIndex
  to call modindex.Create and retry. That's the tests' problem.
- Three integration tests needed rather subtle changes to add
  "happens before" edges to populate GOMODCACHE before
  building the index, and building the index before
  invoking OrganizeImports (since it is fundamentally
  asynchronous):
  1 misc.TestGOMODCACHE
  2 misc.TestRelativeReplace (renamed to TestIssue67156
    since the old name was a red herring)
  3 diagnostics.Test_issue38211
  The purpose of each of the steps in the three tests
  was also clarified.
- Fix a bug in WithProxy: if the module cache is derived
  from a test's TempDir, it may contain "#" which has
  special meaning in GOPROXY. Now we report an error.
  The workaround is to use a fresh subtest, whose name
  will not contain "#".
- Fix a bug in Sandbox.Close when it is called on a
  partially constructed sandbox.
- Move CleanModCache into env package so it can be
  shared. Also flag a couple of related problems in
  comments that refer to it.
- Link to new issues for existing unsolved mysteries
  encountered during testing.

- Eliminate Create from public API; it's not needed.
- Make Update and Read return the current index; this
  avoids the need for redundant Update+Read sequences.
- modcacheindex no longer returns (nil, nil).
- len(Entries)==0 is no longer a special case.
- fix bug in modcacheTimed where len(new.Entries)==0
  was erroneously used to mean "no change".
- Rename Index.Changed to ValidAt, since it doesn't
  mean anything was actually changed at that time.
- Record only the "best" directory for each import path,
  instead of a list, which must then be sorted.
- Don't store symbols in "directory" type; it's now
  just an immutable triple value.
- Ensure that modcacheState.getIndex updates s.index.
  The lack of updates in the previous code looked like an
  oversight.
- Simplify "region" type away.
- Log unexpected ReadIndex failures.
- Rename dir, cachedir -> gomodcache throughout for clarity.
- Eliminate an unnecessary path segment from IndexDir, and
  initialize it inline.
- Better error hygiene.
- Improve variable names, and eliminate unnecessary ones.
- Improve commentary.
- Make tests external, expressed using public API.

Updates golang/go#74055

Change-Id: I3648d2cbd0dc3e169564f6c0dc122d223966b4c0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/682717
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/internal/cache/imports.go

@@ -7,8 +7,8 @@ package cache
 import (
 	"context"
 	"fmt"
+	"log"
 	"sync"
-	"testing"
 	"time"
 
 	"golang.org/x/tools/gopls/internal/file"
@@ -150,66 +150,55 @@ func newImportsState(backgroundCtx context.Context, modCache *sharedModCache, en
 
 // modcacheState holds a modindex.Index and controls its updates
 type modcacheState struct {
-	dir          string // GOMODCACHE
+	gomodcache   string
 	refreshTimer *refreshTimer
-	mu           sync.Mutex
-	index        *modindex.Index
+
+	// (index, indexErr) is zero in the initial state.
+	// Thereafter they hold the memoized result pair for getIndex.
+	mu       sync.Mutex
+	index    *modindex.Index
+	indexErr error
 }
 
 // newModcacheState constructs a new modcacheState for goimports.
 // The returned state is automatically updated until [modcacheState.stopTimer] is called.
-func newModcacheState(dir string) *modcacheState {
+func newModcacheState(gomodcache string) *modcacheState {
 	s := &modcacheState{
-		dir: dir,
+		gomodcache: gomodcache,
 	}
-	s.index, _ = modindex.ReadIndex(dir)
 	s.refreshTimer = newRefreshTimer(s.refreshIndex)
 	go s.refreshIndex()
 	return s
 }
 
-// getIndex reads the module cache index. It might not exist yet
-// inside tests. It might contain no Entries if the cache
-// is empty.
+// getIndex reads the module cache index.
 func (s *modcacheState) getIndex() (*modindex.Index, error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	ix := s.index
-	if ix == nil || len(ix.Entries) == 0 {
-		var err error
-		// this should only happen near the beginning of a session
-		// (or in tests)
-		ix, err = modindex.ReadIndex(s.dir)
-		if err != nil {
-			return nil, fmt.Errorf("ReadIndex %w", err)
-		}
-		if !testing.Testing() {
-			return ix, nil
-		}
-		if ix == nil || len(ix.Entries) == 0 {
-			err = modindex.Create(s.dir)
-			if err != nil {
-				return nil, fmt.Errorf("creating index %w", err)
-			}
-			ix, err = modindex.ReadIndex(s.dir)
-			if err != nil {
-				return nil, fmt.Errorf("read index after create %w", err)
-			}
-			s.index = ix
-		}
+
+	if s.index == nil && s.indexErr == nil {
+		// getIndex was called before refreshIndex finished.
+		// Read, but don't update, whatever index is present.
+		s.index, s.indexErr = modindex.Read(s.gomodcache)
 	}
-	return s.index, nil
+
+	return s.index, s.indexErr
 }
 
 func (s *modcacheState) refreshIndex() {
-	ok, err := modindex.Update(s.dir)
-	if err != nil || !ok {
-		return
-	}
-	// read the new index
+	index, err := modindex.Update(s.gomodcache)
 	s.mu.Lock()
-	defer s.mu.Unlock()
-	s.index, _ = modindex.ReadIndex(s.dir)
+	if err != nil {
+		if s.indexErr != nil {
+			s.indexErr = err // prefer most recent error
+		} else {
+			// Keep using stale s.index (if any).
+			log.Printf("modcacheState.refreshIndex: %v", err)
+		}
+	} else {
+		s.index, s.indexErr = index, nil // success
+	}
+	s.mu.Unlock()
 }
 
 func (s *modcacheState) stopTimer() {

gopls/internal/cache/source.go

@@ -21,7 +21,7 @@ import (
 // goplsSource is an imports.Source that provides import information using
 // gopls and the module cache index.
 type goplsSource struct {
-	S         *Snapshot
+	snapshot  *Snapshot
 	envSource *imports.ProcessEnvSource
 
 	// set by each invocation of ResolveReferences
@@ -30,7 +30,7 @@ type goplsSource struct {
 
 func (s *Snapshot) NewGoplsSource(is *imports.ProcessEnvSource) *goplsSource {
 	return &goplsSource{
-		S:         s,
+		snapshot:  s,
 		envSource: is,
 	}
 }
@@ -110,7 +110,7 @@ func (s *goplsSource) ResolveReferences(ctx context.Context, filename string, mi
 
 		dbgpr("fromWS", fromWS)
 		dbgpr("old", old)
-		for k, v := range s.S.workspacePackages.All() {
+		for k, v := range s.snapshot.workspacePackages.All() {
 			log.Printf("workspacePackages[%s]=%s", k, v)
 		}
 		// anything in ans with >1 matches?
@@ -134,7 +134,7 @@ func (s *goplsSource) ResolveReferences(ctx context.Context, filename string, mi
 }
 
 func (s *goplsSource) resolveCacheReferences(missing imports.References) ([]*result, error) {
-	ix, err := s.S.view.ModcacheIndex()
+	ix, err := s.snapshot.view.ModcacheIndex()
 	if err != nil {
 		return nil, err
 	}
@@ -176,7 +176,7 @@ type found struct {
 
 func (s *goplsSource) resolveWorkspaceReferences(filename string, missing imports.References) ([]*imports.Result, error) {
 	uri := protocol.URIFromPath(filename)
-	mypkgs, err := s.S.MetadataForFile(s.ctx, uri)
+	mypkgs, err := s.snapshot.MetadataForFile(s.ctx, uri)
 	if err != nil {
 		return nil, err
 	}
@@ -185,7 +185,7 @@ func (s *goplsSource) resolveWorkspaceReferences(filename string, missing import
 	}
 	mypkg := mypkgs[0] // narrowest package
 	// search the metadata graph for package ids correstponding to missing
-	g := s.S.MetadataGraph()
+	g := s.snapshot.MetadataGraph()
 	var ids []metadata.PackageID
 	var pkgs []*metadata.Package
 	for pid, pkg := range g.Packages {
@@ -200,7 +200,7 @@ func (s *goplsSource) resolveWorkspaceReferences(filename string, missing import
 	}
 	// find the symbols in those packages
 	// the syms occur in the same order as the ids and the pkgs
-	syms, err := s.S.Symbols(s.ctx, ids...)
+	syms, err := s.snapshot.Symbols(s.ctx, ids...)
 	if err != nil {
 		return nil, err
 	}
@@ -331,12 +331,12 @@ func (s *goplsSource) bestCache(nm string, got []*result) *imports.Result {
 func (s *goplsSource) fromGoMod(got []*result) *imports.Result {
 	// should we use s.S.view.worsspaceModFiles, and the union of their requires?
 	// (note that there are no tests where it contains more than one)
-	modURI := s.S.view.gomod
-	modfh, ok := s.S.files.get(modURI)
+	modURI := s.snapshot.view.gomod
+	modfh, ok := s.snapshot.files.get(modURI)
 	if !ok {
 		return nil
 	}
-	parsed, err := s.S.ParseMod(s.ctx, modfh)
+	parsed, err := s.snapshot.ParseMod(s.ctx, modfh)
 	if err != nil {
 		return nil
 	}

gopls/internal/golang/completion/unimported.go

@@ -224,9 +224,6 @@ func (c *completer) modcacheMatches(pkg metadata.PackageName, prefix string) ([]
 	if err != nil {
 		return nil, err
 	}
-	if ix == nil || len(ix.Entries) == 0 { // in tests ix might always be nil
-		return nil, fmt.Errorf("no index %w", err)
-	}
 	// retrieve everything and let usefulCompletion() and the matcher sort them out
 	cands := ix.Lookup(string(pkg), "", true)
 	lx := len(cands)

gopls/internal/golang/format.go

@@ -149,8 +149,8 @@ func computeImportEdits(ctx context.Context, pgf *parsego.File, snapshot *cache.
 		source = isource
 	}
 	// imports require a current metadata graph
-	// TODO(rfindlay) improve the API
-	snapshot.WorkspaceMetadata(ctx)
+	// TODO(rfindley): improve the API
+	snapshot.WorkspaceMetadata(ctx) // ignore error
 	allFixes, err := imports.FixImports(ctx, filename, pgf.Src, goroot, options.Env.Logf, source)
 	if err != nil {
 		return nil, nil, err

gopls/internal/test/integration/bench/unimported_test.go

@@ -123,15 +123,12 @@ func findSym(t testing.TB) (pkg, name, gomodcache string) {
 		t.Fatal(err)
 	}
 	modcache := strings.TrimSpace(string(out))
-	ix, err := modindex.ReadIndex(modcache)
+	ix, err := modindex.Read(modcache)
 	if err != nil {
 		t.Fatal(err)
 	}
 	if ix == nil {
-		t.Fatal("no index")
-	}
-	if len(ix.Entries) == 0 {
-		t.Fatal("no entries")
+		t.Fatal("nil index")
 	}
 	nth := 100 // or something
 	for _, e := range ix.Entries {

gopls/internal/test/integration/diagnostics/diagnostics_test.go

@@ -681,7 +681,7 @@ var ErrHelpWanted error
 `
 
 // Test for golang/go#38211.
-func Test_Issue38211(t *testing.T) {
+func Test_issue38211(t *testing.T) {
 	const ardanLabs = `
 -- go.mod --
 module mod.com
@@ -696,46 +696,89 @@ func main() {
 	_ = conf.ErrHelpWanted
 }
 `
-	WithOptions(
+
+	modcache := t.TempDir()
+	defer CleanModCache(t, modcache)
+
+	opts := []RunOption{
+		EnvVars{"GOMODCACHE": modcache},
 		ProxyFiles(ardanLabsProxy),
-	).Run(t, ardanLabs, func(t *testing.T, env *Env) {
-		// Expect a diagnostic with a suggested fix to add
-		// "github.com/ardanlabs/conf" to the go.mod file.
+		//WriteGoSum("."), // TODO(golang/go#74594): uncommenting this causes mysterious failure; investigate and make the error clearer (go list?)
+	}
+
+	t.Run("setup", func(t *testing.T) {
+		// Forcibly populate GOMODCACHE
+		// so OrganizeImports can later rely on it.
+		WithOptions(opts...).Run(t, ardanLabs, func(t *testing.T, env *Env) {
+			// TODO(adonovan): why doesn't RunGoCommand respect EnvVars??
+			// (That was the motivation to use Sandbox.RunGoCommand
+			// rather than execute go mod download directly!)
+			// See comment at CleanModCache and golang/go#74595.
+			environ := []string{"GOMODCACHE=" + modcache}
+			_, err := env.Sandbox.RunGoCommand(env.Ctx, "", "get", []string{"github.com/ardanlabs/[email protected]"}, environ, false)
+			if err != nil {
+				t.Error(err)
+			}
+		})
+	})
+
+	WithOptions(opts...).Run(t, ardanLabs, func(t *testing.T, env *Env) {
+		// Expect a "no module provides package" diagnostic.
 		env.OpenFile("go.mod")
 		env.OpenFile("main.go")
 		var d protocol.PublishDiagnosticsParams
 		env.AfterChange(
-			Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)),
+			Diagnostics(
+				env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`),
+				WithMessage("no required module provides package")),
 			ReadDiagnostics("main.go", &d),
 		)
+
+		// Apply the suggested fix to make the go.mod file
+		// require "github.com/ardanlabs/conf".
 		env.ApplyQuickFixes("main.go", d.Diagnostics)
 		env.SaveBuffer("go.mod")
 		env.AfterChange(
 			NoDiagnostics(ForFile("main.go")),
 		)
-		// Comment out the line that depends on conf and expect a
-		// diagnostic and a fix to remove the import.
+
+		// Comment out the sole use of conf,
+		// causing an "unused import" diagnostic.
 		env.RegexpReplace("main.go", "_ = conf.ErrHelpWanted", "//_ = conf.ErrHelpWanted")
 		env.AfterChange(
-			Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)),
+			Diagnostics(
+				env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`),
+				WithMessage("imported and not used")),
 		)
-		env.SaveBuffer("main.go")
-		// Expect a diagnostic and fix to remove the dependency in the go.mod.
+
+		// Remove the import using OrganizeImports, leading
+		// to an "unused require" diagnostic in the go.mod.
+		env.SaveBuffer("main.go") // => OrganizeImports
 		env.AfterChange(
 			NoDiagnostics(ForFile("main.go")),
-			Diagnostics(env.AtRegexp("go.mod", "require github.com/ardanlabs/conf"), WithMessage("not used in this module")),
+			Diagnostics(
+				env.AtRegexp("go.mod", "require github.com/ardanlabs/conf"),
+				WithMessage("not used in this module")),
 			ReadDiagnostics("go.mod", &d),
 		)
+
+		// Apply the suggested fix to remove the "require" directive.
 		env.ApplyQuickFixes("go.mod", d.Diagnostics)
 		env.SaveBuffer("go.mod")
 		env.AfterChange(
 			NoDiagnostics(ForFile("go.mod")),
 		)
-		// Uncomment the lines and expect a new diagnostic for the import.
+
+		// Uncomment the use of the import.
+		// OrganizeImports should add the import.
+		// Expect another "no required module provides package"
+		// diagnostic, bringing us full circle.
 		env.RegexpReplace("main.go", "//_ = conf.ErrHelpWanted", "_ = conf.ErrHelpWanted")
-		env.SaveBuffer("main.go")
+		env.SaveBuffer("main.go") // => OrganizeImports
 		env.AfterChange(
-			Diagnostics(env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`)),
+			Diagnostics(
+				env.AtRegexp("main.go", `"github.com/ardanlabs/conf"`),
+				WithMessage("no required module provides package")),
 		)
 	})
 }

gopls/internal/test/integration/env.go

@@ -8,6 +8,8 @@ import (
 	"context"
 	"fmt"
 	"net/http/httptest"
+	"os"
+	"os/exec"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -378,3 +380,20 @@ func indent(msg string) string {
 	const prefix = "  "
 	return prefix + strings.ReplaceAll(msg, "\n", "\n"+prefix)
 }
+
+// CleanModCache cleans the specified GOMODCACHE.
+//
+// TODO(golang/go#74595): this is only necessary as the module cache cleaning of the
+// sandbox does not respect GOMODCACHE set via EnvVars. We should fix this, but
+// that is probably part of a larger refactoring of the sandbox that I'm not
+// inclined to undertake. --rfindley.
+//
+// (For similar problems caused by the same bug, see Test_issue38211; see also
+// comment in Sandbox.Env.)
+func CleanModCache(t *testing.T, modcache string) {
+	cmd := exec.Command("go", "clean", "-modcache")
+	cmd.Env = append(os.Environ(), "GOMODCACHE="+modcache, "GOTOOLCHAIN=local")
+	if output, err := cmd.CombinedOutput(); err != nil {
+		t.Errorf("cleaning modcache: %v\noutput:\n%s", err, string(output))
+	}
+}

gopls/internal/test/integration/fake/proxy.go

@@ -6,6 +6,7 @@ package fake
 
 import (
 	"fmt"
+	"strings"
 
 	"golang.org/x/tools/internal/proxydir"
 )
@@ -27,6 +28,11 @@ func WriteProxy(tmpdir string, files map[string][]byte) (string, error) {
 		filesByModule[mv][suffix] = data
 	}
 	for mv, files := range filesByModule {
+		// Don't hoist this check out of the loop:
+		// the problem is benign if filesByModule is empty.
+		if strings.Contains(tmpdir, "#") {
+			return "", fmt.Errorf("WriteProxy's tmpdir contains '#', which is unsuitable for GOPROXY. (If tmpdir was derived from testing.T.Name, use t.Run to ensure that each subtest has a unique name.)")
+		}
 		if err := proxydir.WriteModuleVersion(tmpdir, mv.modulePath, mv.version, files); err != nil {
 			return "", fmt.Errorf("error writing %s@%s: %v", mv.modulePath, mv.version, err)
 		}