From eb48d3f608bba06c3bb4f5627f9fc2562cc84dd2 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Sun, 31 Jan 2021 15:13:38 -0500 Subject: [PATCH] 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 Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley Reviewed-by: Rebecca Stambler --- go/packages/golist.go | 2 +- .../regtest/codelens/codelens_test.go | 9 +- .../regtest/diagnostics/diagnostics_test.go | 43 ++++---- internal/lsp/cache/check.go | 98 +++++++++++-------- internal/lsp/cache/load.go | 11 ++- internal/lsp/cache/pkg.go | 32 +++--- internal/lsp/diagnostics.go | 8 +- internal/lsp/source/diagnostics.go | 84 ++++------------ internal/lsp/source/view.go | 2 + 9 files changed, 137 insertions(+), 152 deletions(-) diff --git a/go/packages/golist.go b/go/packages/golist.go index d6ff75d86b..0e1e7f11fe 100644 --- a/go/packages/golist.go +++ b/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, }) } diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go index 9a2cce85e0..88d0e047bc 100644 --- a/gopls/internal/regtest/codelens/codelens_test.go +++ b/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) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 84b8b3dba2..49860005c7 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/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'"), ) }) } diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 0c50d610eb..e0c7cee0bd 100644 --- a/internal/lsp/cache/check.go +++ b/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:] diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 8914d0dc04..f958a56063 100644 --- a/internal/lsp/cache/load.go +++ b/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) diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 66f3e1a21f..5badb33e8e 100644 --- a/internal/lsp/cache/pkg.go +++ b/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 +} diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 59e6070766..479d948449 100644 --- a/internal/lsp/diagnostics.go +++ b/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 diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index f2b382dea6..f9b82de845 100644 --- a/internal/lsp/source/diagnostics.go +++ b/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() { diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 5c96908ecd..1d2c01de17 100644 --- a/internal/lsp/source/view.go +++ b/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 {