gopls/internal/lsp/cache: pre-compute load diagnostics

Process go/packages errors immediately after loading, to eliminate a
dependency on syntax packages.

This simplifies the cache.Package type, which is now just a simple join
of metadata and type-checked syntax.

For golang/go#57987

Change-Id: I3d120fb4cb5687df2a376f8d8617e23e16ddaa92
Reviewed-on: https://go-review.googlesource.com/c/tools/+/468776
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/check.go

@@ -636,12 +636,12 @@ func parseCompiledGoFiles(ctx context.Context, compiledGoFiles []source.FileHand
 // These may be attached to import declarations in the transitive source files
 // of pkg, or to 'requires' declarations in the package's go.mod file.
 //
-// TODO(rfindley): move this to errors.go
-func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsErrors []*packagesinternal.PackageError) ([]*source.Diagnostic, error) {
+// TODO(rfindley): move this to load.go
+func depsErrors(ctx context.Context, m *source.Metadata, meta *metadataGraph, fs source.FileSource, workspacePackages map[PackageID]PackagePath) ([]*source.Diagnostic, error) {
 	// Select packages that can't be found, and were imported in non-workspace packages.
 	// Workspace packages already show their own errors.
 	var relevantErrors []*packagesinternal.PackageError
-	for _, depsError := range depsErrors {
+	for _, depsError := range m.DepsErrors {
 		// Up to Go 1.15, the missing package was included in the stack, which
 		// was presumably a bug. We want the next one up.
 		directImporterIdx := len(depsError.ImportStack) - 1
@@ -650,7 +650,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 		}
 
 		directImporter := depsError.ImportStack[directImporterIdx]
-		if s.isWorkspacePackage(PackageID(directImporter)) {
+		if _, ok := workspacePackages[PackageID(directImporter)]; ok {
 			continue
 		}
 		relevantErrors = append(relevantErrors, depsError)
@@ -661,21 +661,31 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 		return nil, nil
 	}
 
+	// Subsequent checks require Go files.
+	if len(m.CompiledGoFiles) == 0 {
+		return nil, nil
+	}
+
 	// Build an index of all imports in the package.
 	type fileImport struct {
 		cgf *source.ParsedGoFile
 		imp *ast.ImportSpec
 	}
 	allImports := map[string][]fileImport{}
-	for _, cgf := range pkg.compiledGoFiles {
+	for _, uri := range m.CompiledGoFiles {
+		pgf, err := parseGoURI(ctx, fs, uri, source.ParseHeader)
+		if err != nil {
+			return nil, err
+		}
+		fset := source.SingletonFileSet(pgf.Tok)
 		// TODO(adonovan): modify Imports() to accept a single token.File (cgf.Tok).
-		for _, group := range astutil.Imports(pkg.fset, cgf.File) {
+		for _, group := range astutil.Imports(fset, pgf.File) {
 			for _, imp := range group {
 				if imp.Path == nil {
 					continue
 				}
 				path := strings.Trim(imp.Path.Value, `"`)
-				allImports[path] = append(allImports[path], fileImport{cgf, imp})
+				allImports[path] = append(allImports[path], fileImport{pgf, imp})
 			}
 		}
 	}
@@ -686,7 +696,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 	for _, depErr := range relevantErrors {
 		for i := len(depErr.ImportStack) - 1; i >= 0; i-- {
 			item := depErr.ImportStack[i]
-			if s.isWorkspacePackage(PackageID(item)) {
+			if _, ok := workspacePackages[PackageID(item)]; ok {
 				break
 			}
 
@@ -695,7 +705,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 				if err != nil {
 					return nil, err
 				}
-				fixes, err := goGetQuickFixes(s, imp.cgf.URI, item)
+				fixes, err := goGetQuickFixes(m.Module != nil, imp.cgf.URI, item)
 				if err != nil {
 					return nil, err
 				}
@@ -711,18 +721,11 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 		}
 	}
 
-	if len(pkg.compiledGoFiles) == 0 {
-		return errors, nil
-	}
-	mod := s.GoModForFile(pkg.compiledGoFiles[0].URI)
-	if mod == "" {
-		return errors, nil
-	}
-	fh, err := s.GetFile(ctx, mod)
+	modFile, err := nearestModFile(ctx, m.CompiledGoFiles[0], fs)
 	if err != nil {
 		return nil, err
 	}
-	pm, err := s.ParseMod(ctx, fh)
+	pm, err := parseModURI(ctx, fs, modFile)
 	if err != nil {
 		return nil, err
 	}
@@ -732,7 +735,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 	for _, depErr := range relevantErrors {
 		for i := len(depErr.ImportStack) - 1; i >= 0; i-- {
 			item := depErr.ImportStack[i]
-			m := s.Metadata(PackageID(item))
+			m := meta.metadata[PackageID(item)]
 			if m == nil || m.Module == nil {
 				continue
 			}
@@ -745,7 +748,7 @@ func (s *snapshot) depsErrors(ctx context.Context, pkg *syntaxPackage, depsError
 			if err != nil {
 				return nil, err
 			}
-			fixes, err := goGetQuickFixes(s, pm.URI, item)
+			fixes, err := goGetQuickFixes(true, pm.URI, item)
 			if err != nil {
 				return nil, err
 			}

gopls/internal/lsp/cache/debug.go

@@ -30,6 +30,8 @@ func debugf(format string, args ...interface{}) {
 
 // If debugEnabled is true, dumpWorkspace prints a summary of workspace
 // packages to stderr. If debugEnabled is false, it is a no-op.
+//
+// TODO(rfindley): this has served its purpose. Delete.
 func (s *snapshot) dumpWorkspace(context string) {
 	if !debugEnabled {
 		return

gopls/internal/lsp/cache/errors.go

@@ -9,8 +9,10 @@ package cache
 // source.Diagnostic form, and suggesting quick fixes.
 
 import (
+	"context"
 	"fmt"
 	"go/scanner"
+	"go/token"
 	"go/types"
 	"log"
 	"regexp"
@@ -28,20 +30,26 @@ import (
 	"golang.org/x/tools/internal/typesinternal"
 )
 
-func goPackagesErrorDiagnostics(e packages.Error, pkg *syntaxPackage, fromDir string) (diags []*source.Diagnostic, rerr error) {
-	if diag, ok := parseGoListImportCycleError(e, pkg); ok {
+// goPackagesErrorDiagnostics translates the given go/packages Error into a
+// diagnostic, using the provided metadata and filesource.
+//
+// The slice of diagnostics may be empty.
+func goPackagesErrorDiagnostics(ctx context.Context, e packages.Error, m *source.Metadata, fs source.FileSource) ([]*source.Diagnostic, error) {
+	if diag, err := parseGoListImportCycleError(ctx, e, m, fs); err != nil {
+		return nil, err
+	} else if diag != nil {
 		return []*source.Diagnostic{diag}, nil
 	}
 
 	var spn span.Span
 	if e.Pos == "" {
-		spn = parseGoListError(e.Msg, fromDir)
+		spn = parseGoListError(e.Msg, m.LoadDir)
 		// We may not have been able to parse a valid span. Apply the errors to all files.
-		if _, err := spanToRange(pkg, spn); err != nil {
+		if _, err := spanToRange(ctx, fs, spn); err != nil {
 			var diags []*source.Diagnostic
-			for _, pgf := range pkg.compiledGoFiles {
+			for _, uri := range m.CompiledGoFiles {
 				diags = append(diags, &source.Diagnostic{
-					URI:      pgf.URI,
+					URI:      uri,
 					Severity: protocol.SeverityError,
 					Source:   source.ListError,
 					Message:  e.Msg,
@@ -50,7 +58,7 @@ func goPackagesErrorDiagnostics(e packages.Error, pkg *syntaxPackage, fromDir st
 			return diags, nil
 		}
 	} else {
-		spn = span.ParseInDir(e.Pos, fromDir)
+		spn = span.ParseInDir(e.Pos, m.LoadDir)
 	}
 
 	// TODO(rfindley): in some cases the go command outputs invalid spans, for
@@ -64,7 +72,7 @@ func goPackagesErrorDiagnostics(e packages.Error, pkg *syntaxPackage, fromDir st
 	// likely because *token.File lacks information about newline termination.
 	//
 	// We could do better here by handling that case.
-	rng, err := spanToRange(pkg, spn)
+	rng, err := spanToRange(ctx, fs, spn)
 	if err != nil {
 		return nil, err
 	}
@@ -136,7 +144,7 @@ func typeErrorDiagnostics(snapshot *snapshot, pkg *syntaxPackage, e extendedErro
 	}
 
 	if match := importErrorRe.FindStringSubmatch(e.primary.Msg); match != nil {
-		diag.SuggestedFixes, err = goGetQuickFixes(snapshot, loc.URI.SpanURI(), match[1])
+		diag.SuggestedFixes, err = goGetQuickFixes(snapshot.moduleMode(), loc.URI.SpanURI(), match[1])
 		if err != nil {
 			return nil, err
 		}
@@ -150,9 +158,8 @@ func typeErrorDiagnostics(snapshot *snapshot, pkg *syntaxPackage, e extendedErro
 	return []*source.Diagnostic{diag}, nil
 }
 
-func goGetQuickFixes(snapshot *snapshot, uri span.URI, pkg string) ([]source.SuggestedFix, error) {
-	// Go get only supports module mode for now.
-	if snapshot.workspaceMode()&moduleMode == 0 {
+func goGetQuickFixes(moduleMode bool, uri span.URI, pkg string) ([]source.SuggestedFix, error) {
+	if !moduleMode {
 		return nil, nil
 	}
 	title := fmt.Sprintf("go get package %v", pkg)
@@ -312,14 +319,20 @@ func typeErrorData(pkg *syntaxPackage, terr types.Error) (typesinternal.ErrorCod
 	return ecode, loc, err
 }
 
-// spanToRange converts a span.Span to a protocol.Range,
-// assuming that the span belongs to the package whose diagnostics are being computed.
-func spanToRange(pkg *syntaxPackage, spn span.Span) (protocol.Range, error) {
-	pgf, err := pkg.File(spn.URI())
+// spanToRange converts a span.Span to a protocol.Range, by mapping content
+// contained in the provided FileSource.
+func spanToRange(ctx context.Context, fs source.FileSource, spn span.Span) (protocol.Range, error) {
+	uri := spn.URI()
+	fh, err := fs.GetFile(ctx, uri)
+	if err != nil {
+		return protocol.Range{}, err
+	}
+	content, err := fh.Read()
 	if err != nil {
 		return protocol.Range{}, err
 	}
-	return pgf.Mapper.SpanRange(spn)
+	mapper := protocol.NewMapper(uri, content)
+	return mapper.SpanRange(spn)
 }
 
 // parseGoListError attempts to parse a standard `go list` error message
@@ -338,28 +351,36 @@ func parseGoListError(input, wd string) span.Span {
 	return span.ParseInDir(input[:msgIndex], wd)
 }
 
-func parseGoListImportCycleError(e packages.Error, pkg *syntaxPackage) (*source.Diagnostic, bool) {
+// parseGoListImportCycleError attempts to parse the given go/packages error as
+// an import cycle, returning a diagnostic if successful.
+//
+// If the error is not detected as an import cycle error, it returns nil, nil.
+func parseGoListImportCycleError(ctx context.Context, e packages.Error, m *source.Metadata, fs source.FileSource) (*source.Diagnostic, error) {
 	re := regexp.MustCompile(`(.*): import stack: \[(.+)\]`)
 	matches := re.FindStringSubmatch(strings.TrimSpace(e.Msg))
 	if len(matches) < 3 {
-		return nil, false
+		return nil, nil
 	}
 	msg := matches[1]
 	importList := strings.Split(matches[2], " ")
 	// Since the error is relative to the current package. The import that is causing
 	// the import cycle error is the second one in the list.
 	if len(importList) < 2 {
-		return nil, false
+		return nil, nil
 	}
 	// Imports have quotation marks around them.
 	circImp := strconv.Quote(importList[1])
-	for _, pgf := range pkg.compiledGoFiles {
+	for _, uri := range m.CompiledGoFiles {
+		pgf, err := parseGoURI(ctx, fs, uri, source.ParseHeader)
+		if err != nil {
+			return nil, err
+		}
 		// Search file imports for the import that is causing the import cycle.
 		for _, imp := range pgf.File.Imports {
 			if imp.Path.Value == circImp {
 				rng, err := pgf.NodeMappedRange(imp)
 				if err != nil {
-					return nil, false
+					return nil, nil
 				}
 
 				return &source.Diagnostic{
@@ -368,9 +389,35 @@ func parseGoListImportCycleError(e packages.Error, pkg *syntaxPackage) (*source.
 					Severity: protocol.SeverityError,
 					Source:   source.ListError,
 					Message:  msg,
-				}, true
+				}, nil
 			}
 		}
 	}
-	return nil, false
+	return nil, nil
+}
+
+// parseGoURI is a helper to parse the Go file at the given URI from the file
+// source fs. The resulting syntax and token.File belong to an ephemeral,
+// encapsulated FileSet, so this file stands only on its own: it's not suitable
+// to use in a list of file of a package, for example.
+//
+// It returns an error if the file could not be read.
+func parseGoURI(ctx context.Context, fs source.FileSource, uri span.URI, mode source.ParseMode) (*source.ParsedGoFile, error) {
+	fh, err := fs.GetFile(ctx, uri)
+	if err != nil {
+		return nil, err
+	}
+	return parseGoImpl(ctx, token.NewFileSet(), fh, source.ParseHeader)
+}
+
+// parseModURI is a helper to parse the Mod file at the given URI from the file
+// source fs.
+//
+// It returns an error if the file could not be read.
+func parseModURI(ctx context.Context, fs source.FileSource, uri span.URI) (*source.ParsedModule, error) {
+	fh, err := fs.GetFile(ctx, uri)
+	if err != nil {
+		return nil, err
+	}
+	return parseModImpl(ctx, fh)
 }

gopls/internal/lsp/cache/load.go

@@ -229,10 +229,20 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 
 	event.Log(ctx, fmt.Sprintf("%s: updating metadata for %d packages", eventName, len(updates)))
 
-	s.meta = s.meta.Clone(updates)
-	s.resetIsActivePackageLocked()
+	// Before mutating the snapshot, ensure that we compute load diagnostics
+	// successfully. This could fail if the context is cancelled, and we don't
+	// want to leave the snapshot metadata in a partial state.
+	meta := s.meta.Clone(updates)
+	workspacePackages := computeWorkspacePackagesLocked(s, meta)
+	for _, update := range updates {
+		if err := computeLoadDiagnostics(ctx, update, meta, lockedSnapshot{s}, workspacePackages); err != nil {
+			return err
+		}
+	}
+	s.meta = meta
+	s.workspacePackages = workspacePackages
 
-	s.workspacePackages = computeWorkspacePackagesLocked(s, s.meta)
+	s.resetIsActivePackageLocked()
 	s.dumpWorkspace("load")
 	s.mu.Unlock()
 
@@ -549,6 +559,46 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con
 	m.DepsByImpPath = depsByImpPath
 	m.DepsByPkgPath = depsByPkgPath
 
+	// m.Diagnostics is set later in the loading pass, using
+	// computeLoadDiagnostics.
+
+	return nil
+}
+
+// computeLoadDiagnostics computes and sets m.Diagnostics for the given metadata m.
+//
+// It should only be called during metadata construction in snapshot.load.
+func computeLoadDiagnostics(ctx context.Context, m *source.Metadata, meta *metadataGraph, fs source.FileSource, workspacePackages map[PackageID]PackagePath) error {
+	for _, packagesErr := range m.Errors {
+		// Filter out parse errors from go list. We'll get them when we
+		// actually parse, and buggy overlay support may generate spurious
+		// errors. (See TestNewModule_Issue38207.)
+		if strings.Contains(packagesErr.Msg, "expected '") {
+			continue
+		}
+		pkgDiags, err := goPackagesErrorDiagnostics(ctx, packagesErr, m, fs)
+		if err != nil {
+			// There are certain cases where the go command returns invalid
+			// positions, so we cannot panic or even bug.Reportf here.
+			event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(string(m.ID)))
+			continue
+		}
+		m.Diagnostics = append(m.Diagnostics, pkgDiags...)
+	}
+
+	// TODO(rfindley): this is buggy: an insignificant change to a modfile
+	// (or an unsaved modfile) could affect the position of deps errors,
+	// without invalidating the package.
+	depsDiags, err := depsErrors(ctx, m, meta, fs, workspacePackages)
+	if err != nil {
+		if ctx.Err() == nil {
+			// TODO(rfindley): consider making this a bug.Reportf. depsErrors should
+			// not normally fail.
+			event.Error(ctx, "unable to compute deps errors", err, tag.Package.Of(string(m.ID)))
+		}
+		return nil
+	}
+	m.Diagnostics = append(m.Diagnostics, depsDiags...)
 	return nil
 }