internal/lsp/cache: refactor diagnostic suppression

In typeCheckDiagnostics, we have logic to suppress go list and type
checking errors based on the presence of other errors. Nobody seems to
know why the logic is exactly what it is, and suppressing list errors
means that bad //go:embed patterns don't show up.

Intuitively, it makes sense to me that we don't want to type check if
listing or parsing fails: list errors mean that whole files may be
missing, and parsing errors may wipe out arbitrary chunks of files.
There's little point reporting errors from type checking that. However,
list errors and parse errors should be mostly orthogonal: go list
parses very little of the file and in practice only reports errors
parsing the package statement. So, at least for now, we report both
parse and list errors, and stop there if there are any.

Finally, move the suppression logic to the actual typeCheck function
that generates the diagnostics. typeCheckDiagnostics is the primary
consumer, but I think it's better to do the suppression at the source.

Because we are now showing list errors, and they are prone to getting
stuck due to bad overlay support, a couple of tests now require 1.16.

Change-Id: I7801e44c4d3da30bda61e0c1710a2f52de6215f5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295414
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

go/packages/golist.go

@@ -584,7 +584,7 @@ func (state *golistState) createDriverResponse(words ...string) (*driverResponse
 				// golang/go#38990: go list silently fails to do cgo processing
 				pkg.CompiledGoFiles = nil
 				pkg.Errors = append(pkg.Errors, Error{
-					Msg:  "go list failed to return CompiledGoFiles; https://golang.org/issue/38990?",
+					Msg:  "go list failed to return CompiledGoFiles. This may indicate failure to perform cgo processing; try building at the command line. See https://golang.org/issue/38990.",
 					Kind: ListError,
 				})
 			}

gopls/internal/regtest/codelens/codelens_test.go

@@ -256,14 +256,17 @@ func Foo() {
 }
 `
 	Run(t, workspace, func(t *testing.T, env *Env) {
-		// Open the file. We should have a nonexistant symbol.
+		// Open the file. We have a nonexistant symbol that will break cgo processing.
 		env.OpenFile("cgo.go")
-		env.Await(env.DiagnosticAtRegexp("cgo.go", `C\.(fortytwo)`)) // could not determine kind of name for C.fortytwo
+		env.Await(env.DiagnosticAtRegexpWithMessage("cgo.go", ``, "go list failed to return CompiledGoFiles"))
 
 		// Fix the C function name. We haven't regenerated cgo, so nothing should be fixed.
 		env.RegexpReplace("cgo.go", `int fortythree`, "int fortytwo")
 		env.SaveBuffer("cgo.go")
-		env.Await(env.DiagnosticAtRegexp("cgo.go", `C\.(fortytwo)`))
+		env.Await(OnceMet(
+			env.DoneWithSave(),
+			env.DiagnosticAtRegexpWithMessage("cgo.go", ``, "go list failed to return CompiledGoFiles"),
+		))
 
 		// Regenerate cgo, fixing the diagnostic.
 		env.ExecuteCodeLensCommand("cgo.go", command.RegenerateCgo)

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"os"
 	"testing"
 
 	. "golang.org/x/tools/gopls/internal/regtest"
@@ -585,8 +584,9 @@ hi mom
 	}
 }
 
-// Tests golang/go#38602.
-func TestNonexistentFileDiagnostics_Issue38602(t *testing.T) {
+// Tests the repro case from golang/go#38602. Diagnostics are now handled properly,
+// which blocks type checking.
+func TestConflictingMainPackageErrors(t *testing.T) {
 	const collision = `
 -- x/x.go --
 package x
@@ -604,27 +604,19 @@ func main() {
 }
 `
 	WithOptions(InGOPATH()).Run(t, collision, func(t *testing.T, env *Env) {
-		env.OpenFile("x/main.go")
+		env.OpenFile("x/x.go")
 		env.Await(
-			env.DiagnosticAtRegexp("x/main.go", "fmt.Println"),
+			env.DiagnosticAtRegexpWithMessage("x/x.go", `^`, "found packages main (main.go) and x (x.go)"),
+			env.DiagnosticAtRegexpWithMessage("x/main.go", `^`, "found packages main (main.go) and x (x.go)"),
 		)
-		env.OrganizeImports("x/main.go")
-		// span.Parse misparses the error message when multiple packages are
-		// defined in the same directory, creating a garbage filename.
-		// Previously, we would send diagnostics for this nonexistent file.
-		// This test checks that we don't send diagnostics for this file.
-		dir, err := os.Getwd()
-		if err != nil {
-			t.Fatal(err)
-		}
-		badFile := fmt.Sprintf("%s/found packages main (main.go) and x (x.go) in %s/src/x", dir, env.Sandbox.GOPATH())
-		env.Await(
-			OnceMet(
+
+		// We don't recover cleanly from the errors without good overlay support.
+		if testenv.Go1Point() >= 16 {
+			env.RegexpReplace("x/x.go", `package x`, `package main`)
+			env.Await(OnceMet(
 				env.DoneWithChange(),
-				EmptyDiagnostics("x/main.go"),
-			),
-			NoDiagnostics(badFile),
-		)
+				env.DiagnosticAtRegexpWithMessage("x/main.go", `fmt`, "undeclared name")))
+		}
 	})
 }
 
@@ -717,15 +709,14 @@ go 1.12
 	WithOptions(
 		ProxyFiles(ardanLabsProxy),
 	).Run(t, emptyFile, func(t *testing.T, env *Env) {
-		env.OpenFile("main.go")
-		env.EditBuffer("main.go", fake.NewEdit(0, 0, 0, 0, `package main
+		env.CreateBuffer("main.go", `package main
 
 import "github.com/ardanlabs/conf"
 
 func main() {
 	_ = conf.ErrHelpWanted
 }
-`))
+`)
 		env.SaveBuffer("main.go")
 		var d protocol.PublishDiagnosticsParams
 		env.Await(
@@ -1116,7 +1107,7 @@ func Foo() {
 }
 `
 	Run(t, basic, func(t *testing.T, env *Env) {
-		testenv.NeedsGo1Point(t, 15)
+		testenv.NeedsGo1Point(t, 16) // We can't recover cleanly from this case without good overlay support.
 
 		env.WriteWorkspaceFile("foo/foo_test.go", `package main
 
@@ -1219,7 +1210,7 @@ func main() {}
 	Run(t, pkgDefault, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.Await(
-			env.DiagnosticAtRegexp("main.go", "default"),
+			env.DiagnosticAtRegexpWithMessage("main.go", "default", "expected 'IDENT'"),
 		)
 	})
 }

internal/lsp/cache/check.go

@@ -271,11 +271,6 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 	ctx, done := event.Start(ctx, "cache.importer.typeCheck", tag.Package.Of(string(m.id)))
 	defer done()
 
-	var rawErrors []error
-	for _, err := range m.errors {
-		rawErrors = append(rawErrors, err)
-	}
-
 	fset := snapshot.view.session.cache.fset
 	pkg := &pkg{
 		m:               m,
@@ -332,6 +327,8 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 			pkg.compiledGoFiles[i] = pgf
 			files[i], parseErrors[i], actualErrors[i] = pgf.File, pgf.ParseErr, err
 
+			// If we have fixed parse errors in any of the files,
+			// we should hide type errors, as they may be completely nonsensical.
 			mu.Lock()
 			skipTypeErrors = skipTypeErrors || fixed
 			mu.Unlock()
@@ -357,13 +354,16 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 		}
 	}
 
+	var i int
 	for _, e := range parseErrors {
 		if e != nil {
-			rawErrors = append(rawErrors, e)
+			parseErrors[i] = e
+			i++
 		}
 	}
+	parseErrors = parseErrors[:i]
 
-	var i int
+	i = 0
 	for _, f := range files {
 		if f != nil {
 			files[i] = f
@@ -379,9 +379,9 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 		// race to Unsafe.completed.
 		return pkg, nil
 	} else if len(files) == 0 { // not the unsafe package, no parsed files
-		// Try to attach errors messages to the file as much as possible.
+		// Try to attach error messages to the file as much as possible.
 		var found bool
-		for _, e := range rawErrors {
+		for _, e := range m.errors {
 			srcDiags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
 			if err != nil {
 				continue
@@ -392,19 +392,15 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 		if found {
 			return pkg, nil
 		}
-		return nil, errors.Errorf("no parsed files for package %s, expected: %v, list errors: %v", pkg.m.pkgPath, pkg.compiledGoFiles, rawErrors)
+		return nil, errors.Errorf("no parsed files for package %s, expected: %v, errors: %v", pkg.m.pkgPath, pkg.compiledGoFiles, m.errors)
 	} else {
 		pkg.types = types.NewPackage(string(m.pkgPath), string(m.name))
 	}
 
+	var typeErrors []types.Error
 	cfg := &types.Config{
 		Error: func(e error) {
-			// If we have fixed parse errors in any of the files,
-			// we should hide type errors, as they may be completely nonsensical.
-			if skipTypeErrors {
-				return
-			}
-			rawErrors = append(rawErrors, e)
+			typeErrors = append(typeErrors, e.(types.Error))
 		},
 		Importer: importerFunc(func(pkgPath string) (*types.Package, error) {
 			// If the context was cancelled, we should abort.
@@ -441,28 +437,56 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source
 	}
 
 	// We don't care about a package's errors unless we have parsed it in full.
-	if mode == source.ParseFull {
-		for _, e := range expandErrors(rawErrors) {
-			srcDiags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
+	if mode != source.ParseFull {
+		return pkg, nil
+	}
+
+	if len(m.errors) != 0 {
+		pkg.hasListOrParseErrors = true
+		for _, e := range m.errors {
+			diags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
 			if err != nil {
-				event.Error(ctx, "unable to compute error positions", err, tag.Package.Of(pkg.ID()))
+				event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(pkg.ID()))
 				continue
 			}
-			pkg.diagnostics = append(pkg.diagnostics, srcDiags...)
+			pkg.diagnostics = append(pkg.diagnostics, diags...)
+		}
+	}
 
-			if err, ok := e.(extendedError); ok {
-				pkg.typeErrors = append(pkg.typeErrors, err.primary)
+	if len(parseErrors) != 0 {
+		pkg.hasListOrParseErrors = true
+		for _, e := range parseErrors {
+			diags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
+			if err != nil {
+				event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(pkg.ID()))
+				continue
 			}
+			pkg.diagnostics = append(pkg.diagnostics, diags...)
 		}
+	}
 
-		depsErrors, err := snapshot.depsErrors(ctx, pkg)
+	if pkg.hasListOrParseErrors || skipTypeErrors {
+		return pkg, nil
+	}
+
+	for _, e := range expandErrors(typeErrors) {
+		pkg.hasTypeErrors = true
+		diags, err := sourceDiagnostics(ctx, snapshot, pkg, protocol.SeverityError, e)
 		if err != nil {
-			return nil, err
-		}
-		pkg.diagnostics = append(pkg.diagnostics, depsErrors...)
-		if err := addGoGetFixes(ctx, snapshot, pkg); err != nil {
-			return nil, err
+			event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID()))
+			continue
 		}
+		pkg.diagnostics = append(pkg.diagnostics, diags...)
+		pkg.typeErrors = append(pkg.typeErrors, e.primary)
+	}
+
+	depsErrors, err := snapshot.depsErrors(ctx, pkg)
+	if err != nil {
+		return nil, err
+	}
+	pkg.diagnostics = append(pkg.diagnostics, depsErrors...)
+	if err := addGoGetFixes(ctx, snapshot, pkg); err != nil {
+		return nil, err
 	}
 
 	return pkg, nil
@@ -652,21 +676,15 @@ func (e extendedError) Error() string {
 //
 // This code associates the secondary error with its primary error, which can
 // then be used as RelatedInformation when the error becomes a diagnostic.
-func expandErrors(errs []error) []error {
-	var result []error
+func expandErrors(errs []types.Error) []extendedError {
+	var result []extendedError
 	for i := 0; i < len(errs); {
-		e, ok := errs[i].(types.Error)
-		if !ok {
-			result = append(result, errs[i])
-			i++
-			continue
-		}
 		original := extendedError{
-			primary: e,
+			primary: errs[i],
 		}
 		for i++; i < len(errs); i++ {
-			spl, ok := errs[i].(types.Error)
-			if !ok || len(spl.Msg) == 0 || spl.Msg[0] != '\t' {
+			spl := errs[i]
+			if len(spl.Msg) == 0 || spl.Msg[0] != '\t' {
 				break
 			}
 			spl.Msg = spl.Msg[1:]

internal/lsp/cache/load.go

@@ -372,12 +372,21 @@ func (s *snapshot) setMetadata(ctx context.Context, pkgPath packagePath, pkg *pa
 		name:       packageName(pkg.Name),
 		forTest:    packagePath(packagesinternal.GetForTest(pkg)),
 		typesSizes: pkg.TypesSizes,
-		errors:     pkg.Errors,
 		config:     cfg,
 		module:     pkg.Module,
 		depsErrors: packagesinternal.GetDepsErrors(pkg),
 	}
 
+	for _, err := range pkg.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(err.Msg, "expected '") {
+			continue
+		}
+		m.errors = append(m.errors, err)
+	}
+
 	for _, filename := range pkg.CompiledGoFiles {
 		uri := span.URIFromPath(filename)
 		m.compiledGoFiles = append(m.compiledGoFiles, uri)

internal/lsp/cache/pkg.go

@@ -16,17 +16,19 @@ import (
 
 // pkg contains the type information needed by the source package.
 type pkg struct {
-	m               *metadata
-	mode            source.ParseMode
-	goFiles         []*source.ParsedGoFile
-	compiledGoFiles []*source.ParsedGoFile
-	diagnostics     []*source.Diagnostic
-	imports         map[packagePath]*pkg
-	version         *module.Version
-	typeErrors      []types.Error
-	types           *types.Package
-	typesInfo       *types.Info
-	typesSizes      types.Sizes
+	m                    *metadata
+	mode                 source.ParseMode
+	goFiles              []*source.ParsedGoFile
+	compiledGoFiles      []*source.ParsedGoFile
+	diagnostics          []*source.Diagnostic
+	imports              map[packagePath]*pkg
+	version              *module.Version
+	typeErrors           []types.Error
+	types                *types.Package
+	typesInfo            *types.Info
+	typesSizes           types.Sizes
+	hasListOrParseErrors bool
+	hasTypeErrors        bool
 }
 
 // Declare explicit types for package paths, names, and IDs to ensure that we
@@ -146,3 +148,11 @@ func (p *pkg) Imports() []source.Package {
 func (p *pkg) Version() *module.Version {
 	return p.version
 }
+
+func (p *pkg) HasListOrParseErrors() bool {
+	return p.hasListOrParseErrors
+}
+
+func (p *pkg) HasTypeErrors() bool {
+	return p.hasTypeErrors
+}

internal/lsp/diagnostics.go

@@ -262,12 +262,12 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg
 		}
 	}
 
-	typeCheckResults := source.GetTypeCheckDiagnostics(ctx, snapshot, pkg)
-	for uri, diags := range typeCheckResults.Diagnostics {
+	typeCheckDiagnostics := source.GetTypeCheckDiagnostics(ctx, snapshot, pkg)
+	for uri, diags := range typeCheckDiagnostics {
 		s.storeDiagnostics(snapshot, uri, typeCheckSource, diags)
 	}
-	if includeAnalysis && !typeCheckResults.HasParseOrListErrors {
-		reports, err := source.Analyze(ctx, snapshot, pkg, typeCheckResults)
+	if includeAnalysis && !pkg.HasListOrParseErrors() {
+		reports, err := source.Analyze(ctx, snapshot, pkg, typeCheckDiagnostics)
 		if err != nil {
 			event.Error(ctx, "warning: diagnose package", err, tag.Snapshot.Of(snapshot.ID()), tag.Package.Of(pkg.ID()))
 			return

internal/lsp/source/diagnostics.go

@@ -8,8 +8,6 @@ import (
 	"context"
 
 	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/internal/event"
-	"golang.org/x/tools/internal/lsp/debug/tag"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/span"
 )
@@ -26,25 +24,33 @@ type RelatedInformation struct {
 	Message string
 }
 
-func GetTypeCheckDiagnostics(ctx context.Context, snapshot Snapshot, pkg Package) TypeCheckDiagnostics {
+func GetTypeCheckDiagnostics(ctx context.Context, snapshot Snapshot, pkg Package) map[span.URI][]*Diagnostic {
 	onlyIgnoredFiles := true
 	for _, pgf := range pkg.CompiledGoFiles() {
 		onlyIgnoredFiles = onlyIgnoredFiles && snapshot.IgnoredFile(pgf.URI)
 	}
 	if onlyIgnoredFiles {
-		return TypeCheckDiagnostics{}
+		return nil
 	}
-	return typeCheckDiagnostics(ctx, snapshot, pkg)
+
+	diagSets := emptyDiagnostics(pkg)
+	for _, diag := range pkg.GetDiagnostics() {
+		diagSets[diag.URI] = append(diagSets[diag.URI], diag)
+	}
+	for uri, diags := range diagSets {
+		diagSets[uri] = cloneDiagnostics(diags)
+	}
+	return diagSets
 }
 
-func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckResult TypeCheckDiagnostics) (map[span.URI][]*Diagnostic, error) {
+func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckDiagnostics map[span.URI][]*Diagnostic) (map[span.URI][]*Diagnostic, error) {
 	// Exit early if the context has been canceled. This also protects us
 	// from a race on Options, see golang/go#36699.
 	if ctx.Err() != nil {
 		return nil, ctx.Err()
 	}
 	// If we don't have any list or parse errors, run analyses.
-	analyzers := pickAnalyzers(snapshot, typeCheckResult.HasTypeErrors)
+	analyzers := pickAnalyzers(snapshot, pkg.HasTypeErrors())
 	analysisDiagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers...)
 	if err != nil {
 		return nil, err
@@ -70,7 +76,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, typeCheckResul
 		}
 		// Type error analyzers only alter the tags for existing type errors.
 		if _, ok := snapshot.View().Options().TypeErrorAnalyzers[string(diag.Source)]; ok {
-			existingDiagnostics := typeCheckResult.Diagnostics[diag.URI]
+			existingDiagnostics := typeCheckDiagnostics[diag.URI]
 			for _, existing := range existingDiagnostics {
 				if r := protocol.CompareRange(diag.Range, existing.Range); r != 0 {
 					continue
@@ -128,10 +134,10 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers
 	if err != nil {
 		return VersionedFileIdentity{}, nil, err
 	}
-	typeCheckResults := GetTypeCheckDiagnostics(ctx, snapshot, pkg)
-	diagnostics := typeCheckResults.Diagnostics[fh.URI()]
-	if !typeCheckResults.HasParseOrListErrors {
-		reports, err := Analyze(ctx, snapshot, pkg, typeCheckResults)
+	typeCheckDiagnostics := GetTypeCheckDiagnostics(ctx, snapshot, pkg)
+	diagnostics := typeCheckDiagnostics[fh.URI()]
+	if !pkg.HasListOrParseErrors() {
+		reports, err := Analyze(ctx, snapshot, pkg, typeCheckDiagnostics)
 		if err != nil {
 			return VersionedFileIdentity{}, nil, err
 		}
@@ -140,60 +146,6 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers
 	return fh.VersionedFileIdentity(), diagnostics, nil
 }
 
-type TypeCheckDiagnostics struct {
-	HasTypeErrors        bool
-	HasParseOrListErrors bool
-	Diagnostics          map[span.URI][]*Diagnostic
-}
-
-type diagnosticSet struct {
-	listErrors, parseErrors, typeErrors []*Diagnostic
-}
-
-func typeCheckDiagnostics(ctx context.Context, snapshot Snapshot, pkg Package) TypeCheckDiagnostics {
-	ctx, done := event.Start(ctx, "source.typeCheckDiagnostics", tag.Package.Of(pkg.ID()))
-	_ = ctx // circumvent SA4006
-	defer done()
-
-	diagSets := make(map[span.URI]*diagnosticSet)
-	for _, diag := range pkg.GetDiagnostics() {
-		set, ok := diagSets[diag.URI]
-		if !ok {
-			set = &diagnosticSet{}
-			diagSets[diag.URI] = set
-		}
-		switch diag.Source {
-		case ParseError:
-			set.parseErrors = append(set.parseErrors, diag)
-		case TypeError:
-			set.typeErrors = append(set.typeErrors, diag)
-		case ListError:
-			set.listErrors = append(set.listErrors, diag)
-		}
-	}
-	typecheck := TypeCheckDiagnostics{
-		Diagnostics: emptyDiagnostics(pkg),
-	}
-	for uri, set := range diagSets {
-		// Don't report type errors if there are parse errors or list errors.
-		diags := set.typeErrors
-		switch {
-		case len(set.parseErrors) > 0:
-			typecheck.HasParseOrListErrors = true
-			diags = set.parseErrors
-		case len(set.listErrors) > 0:
-			typecheck.HasParseOrListErrors = true
-			if len(pkg.MissingDependencies()) > 0 {
-				diags = set.listErrors
-			}
-		case len(set.typeErrors) > 0:
-			typecheck.HasTypeErrors = true
-		}
-		typecheck.Diagnostics[uri] = cloneDiagnostics(diags)
-	}
-	return typecheck
-}
-
 func emptyDiagnostics(pkg Package) map[span.URI][]*Diagnostic {
 	diags := map[span.URI][]*Diagnostic{}
 	for _, pgf := range pkg.CompiledGoFiles() {

internal/lsp/source/view.go

@@ -557,6 +557,8 @@ type Package interface {
 	MissingDependencies() []string
 	Imports() []Package
 	Version() *module.Version
+	HasListOrParseErrors() bool
+	HasTypeErrors() bool
 }
 
 type CriticalError struct {