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>
diff --git a/go/packages/golist.go b/go/packages/golist.go
index d6ff75d..0e1e7f1 100644
--- a/go/packages/golist.go
+++ b/go/packages/golist.go
@@ -584,7 +584,7 @@
// 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,
})
}
diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go
index 9a2cce8..88d0e04 100644
--- a/gopls/internal/regtest/codelens/codelens_test.go
+++ b/gopls/internal/regtest/codelens/codelens_test.go
@@ -256,14 +256,17 @@
}
`
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)
diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go
index 84b8b3d..4986000 100644
--- a/gopls/internal/regtest/diagnostics/diagnostics_test.go
+++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go
@@ -8,7 +8,6 @@
"context"
"fmt"
"log"
- "os"
"testing"
. "golang.org/x/tools/gopls/internal/regtest"
@@ -585,8 +584,9 @@
}
}
-// 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 @@
}
`
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 @@
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 @@
}
`
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 @@
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'"),
)
})
}
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 0c50d61..e0c7cee 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -271,11 +271,6 @@
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 @@
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 @@
}
}
+ 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 @@
// 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 @@
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 @@
}
// 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
+ event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID()))
+ continue
}
- pkg.diagnostics = append(pkg.diagnostics, depsErrors...)
- if err := addGoGetFixes(ctx, snapshot, pkg); err != nil {
- return nil, err
- }
+ 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 @@
//
// 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:]
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 8914d0d..f958a56 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -372,12 +372,21 @@
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)
diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go
index 66f3e1a..5badb33 100644
--- a/internal/lsp/cache/pkg.go
+++ b/internal/lsp/cache/pkg.go
@@ -16,17 +16,19 @@
// 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) Version() *module.Version {
return p.version
}
+
+func (p *pkg) HasListOrParseErrors() bool {
+ return p.hasListOrParseErrors
+}
+
+func (p *pkg) HasTypeErrors() bool {
+ return p.hasTypeErrors
+}
diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go
index 59e6070..479d948 100644
--- a/internal/lsp/diagnostics.go
+++ b/internal/lsp/diagnostics.go
@@ -262,12 +262,12 @@
}
}
- 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
diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go
index f2b382d..f9b82de 100644
--- a/internal/lsp/source/diagnostics.go
+++ b/internal/lsp/source/diagnostics.go
@@ -8,8 +8,6 @@
"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 @@
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 @@
}
// 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 @@
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 @@
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() {
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 5c96908..1d2c01d 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -557,6 +557,8 @@
MissingDependencies() []string
Imports() []Package
Version() *module.Version
+ HasListOrParseErrors() bool
+ HasTypeErrors() bool
}
type CriticalError struct {
