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 {