diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go index 0b1fa6a9b0f..31d8f033bea 100644 --- a/gopls/internal/lsp/cache/analysis.go +++ b/gopls/internal/lsp/cache/analysis.go @@ -16,7 +16,6 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/tools/go/analysis" "golang.org/x/tools/gopls/internal/lsp/source" - "golang.org/x/tools/gopls/internal/span" "golang.org/x/tools/internal/bug" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" @@ -465,45 +464,3 @@ func factType(fact analysis.Fact) reflect.Type { } return t } - -func (s *snapshot) DiagnosePackage(ctx context.Context, id PackageID) (map[span.URI][]*source.Diagnostic, error) { - pkgs, err := s.TypeCheck(ctx, source.TypecheckFull, id) - if err != nil { - return nil, err - } - pkg := pkgs[0].(*pkg) - var errorAnalyzerDiag []*source.Diagnostic - if pkg.HasTypeErrors() { - // Apply type error analyzers. - // They augment type error diagnostics with their own fixes. - var analyzers []*source.Analyzer - for _, a := range s.View().Options().TypeErrorAnalyzers { - analyzers = append(analyzers, a) - } - var err error - errorAnalyzerDiag, err = s.Analyze(ctx, pkg.ID(), analyzers) - if err != nil { - // Keep going: analysis failures should not block diagnostics. - event.Error(ctx, "type error analysis failed", err, tag.Package.Of(string(pkg.ID()))) - } - } - diags := map[span.URI][]*source.Diagnostic{} - for _, diag := range pkg.diagnostics { - for _, eaDiag := range errorAnalyzerDiag { - if eaDiag.URI == diag.URI && eaDiag.Range == diag.Range && eaDiag.Message == diag.Message { - // Type error analyzers just add fixes and tags. Make a copy, - // since we don't own either, and overwrite. - // The analyzer itself can't do this merge because - // analysis.Diagnostic doesn't have all the fields, and Analyze - // can't because it doesn't have the type error, notably its code. - clone := *diag - clone.SuggestedFixes = eaDiag.SuggestedFixes - clone.Tags = eaDiag.Tags - clone.Analyzer = eaDiag.Analyzer - diag = &clone - } - } - diags[diag.URI] = append(diags[diag.URI], diag) - } - return diags, nil -} diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 8adec22a17d..beec6593ba6 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -605,6 +605,11 @@ func parseCompiledGoFiles(ctx context.Context, compiledGoFiles []source.FileHand // Optionally remove parts that don't affect the exported API. if mode == source.ParseExported { + // TODO(adonovan): opt: experiment with pre-parser + // trimming, either a scanner-based implementation + // such as https://go.dev/play/p/KUrObH1YkX8 (~31% + // speedup), or a byte-oriented implementation (2x + // speedup). if astFilter != nil { // aggressive pruning based on reachability var files []*ast.File diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go index 5b927a5f6d2..6d138bea15c 100644 --- a/gopls/internal/lsp/cache/pkg.go +++ b/gopls/internal/lsp/cache/pkg.go @@ -161,3 +161,13 @@ func (p *pkg) HasListOrParseErrors() bool { func (p *pkg) HasTypeErrors() bool { return len(p.typeErrors) != 0 } + +func (p *pkg) DiagnosticsForFile(uri span.URI) []*source.Diagnostic { + var res []*source.Diagnostic + for _, diag := range p.diagnostics { + if diag.URI == uri { + res = append(res, diag) + } + } + return res +} diff --git a/gopls/internal/lsp/cmd/test/check.go b/gopls/internal/lsp/cmd/test/check.go index ea9747cae79..35153c2700d 100644 --- a/gopls/internal/lsp/cmd/test/check.go +++ b/gopls/internal/lsp/cmd/test/check.go @@ -15,8 +15,8 @@ import ( "golang.org/x/tools/gopls/internal/span" ) -// Diagnostics runs the gopls command on a single file, parses its -// diagnostics, and compares against the expectations defined by +// Diagnostics runs the "gopls check" command on a single file, parses +// its diagnostics, and compares against the expectations defined by // markers in the source file. func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnostic) { out, _ := r.runGoplsCmd(t, "check", uri.Filename()) diff --git a/gopls/internal/lsp/code_action.go b/gopls/internal/lsp/code_action.go index 63c0d67c6a5..3b333ffa641 100644 --- a/gopls/internal/lsp/code_action.go +++ b/gopls/internal/lsp/code_action.go @@ -155,23 +155,19 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if ctx.Err() != nil { return nil, ctx.Err() } - metas, err := snapshot.MetadataForFile(ctx, fh.URI()) - if err != nil { - return nil, err - } - if len(metas) == 0 { - return nil, fmt.Errorf("no package containing file %q", fh.URI()) - } - id := metas[len(metas)-1].ID // last => widest package - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, id) + + // Type-check the package and also run analysis, + // then combine their diagnostics. + pkg, err := snapshot.PackageForFile(ctx, fh.URI(), source.TypecheckFull, source.WidestPackage) if err != nil { return nil, err } - analysisDiags, err := source.Analyze(ctx, snapshot, id, true) + analysisDiags, err := source.Analyze(ctx, snapshot, pkg.ID(), true) if err != nil { return nil, err } - fileDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...) + var fileDiags []*source.Diagnostic + source.CombineDiagnostics(pkg, fh.URI(), analysisDiags, &fileDiags, &fileDiags) // Split diagnostics into fixes, which must match incoming diagnostics, // and non-fixes, which must match the requested range. Build actions diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go index addfa96aca8..445a81914b3 100644 --- a/gopls/internal/lsp/diagnostics.go +++ b/gopls/internal/lsp/diagnostics.go @@ -178,6 +178,9 @@ func (s *Server) diagnoseChangedFiles(ctx context.Context, snapshot source.Snaps ctx, done := event.Start(ctx, "Server.diagnoseChangedFiles", source.SnapshotLabels(snapshot)...) defer done() + // TODO(adonovan): safety: refactor so that group.Go is called + // in a second loop, so that if we should later add an early + // return to the first loop, we don't leak goroutines. var group errgroup.Group seen := make(map[*source.Metadata]bool) for _, uri := range uris { @@ -366,27 +369,36 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, m *s return } - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, m.ID) + pkgs, err := snapshot.TypeCheck(ctx, source.TypecheckFull, m.ID) if err != nil { - event.Error(ctx, "warning: diagnosing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...) + event.Error(ctx, "warning: typecheck failed", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...) return } - for _, uri := range m.CompiledGoFiles { - // builtin.go exists only for documentation purposes, and is not valid Go code. - // Don't report distracting errors - if !snapshot.IsBuiltin(ctx, uri) { - s.storeDiagnostics(snapshot, uri, typeCheckSource, pkgDiagnostics[uri], true) - } - } + pkg := pkgs[0] + + // Get diagnostics from analysis framework. + // This includes type-error analyzers, which suggest fixes to compiler errors. + var analysisDiags map[span.URI][]*source.Diagnostic if includeAnalysis { - reports, err := source.Analyze(ctx, snapshot, m.ID, false) + diags, err := source.Analyze(ctx, snapshot, m.ID, false) if err != nil { event.Error(ctx, "warning: analyzing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...) return } - for _, uri := range m.CompiledGoFiles { - s.storeDiagnostics(snapshot, uri, analysisSource, reports[uri], true) + analysisDiags = diags + } + + // For each file, update the server's diagnostics state. + for _, cgf := range pkg.CompiledGoFiles() { + // builtin.go exists only for documentation purposes and + // is not valid Go code. Don't report distracting errors. + if snapshot.IsBuiltin(ctx, cgf.URI) { + continue } + var tdiags, adiags []*source.Diagnostic + source.CombineDiagnostics(pkg, cgf.URI, analysisDiags, &tdiags, &adiags) + s.storeDiagnostics(snapshot, cgf.URI, typeCheckSource, tdiags, true) + s.storeDiagnostics(snapshot, cgf.URI, analysisSource, adiags, true) } // If gc optimization details are requested, add them to the diff --git a/gopls/internal/lsp/mod/diagnostics.go b/gopls/internal/lsp/mod/diagnostics.go index a0e42720a8f..65b3786f10b 100644 --- a/gopls/internal/lsp/mod/diagnostics.go +++ b/gopls/internal/lsp/mod/diagnostics.go @@ -11,11 +11,9 @@ import ( "fmt" "sort" "strings" - "sync" "golang.org/x/mod/modfile" "golang.org/x/mod/semver" - "golang.org/x/sync/errgroup" "golang.org/x/tools/gopls/internal/govulncheck" "golang.org/x/tools/gopls/internal/lsp/command" "golang.org/x/tools/gopls/internal/lsp/protocol" @@ -89,7 +87,7 @@ func ModDiagnostics(ctx context.Context, snapshot source.Snapshot, fh source.Fil } // Packages in the workspace can contribute diagnostics to go.mod files. - // TODO(rfindley): Try to avoid calling DiagnosePackage on all packages in the workspace here, + // TODO(rfindley): Try to avoid type checking all packages in the workspace here, // for every go.mod file. If gc_details is enabled, it looks like this could lead to extra // go command invocations (as gc details is not memoized). active, err := snapshot.ActiveMetadata(ctx) @@ -97,27 +95,20 @@ func ModDiagnostics(ctx context.Context, snapshot source.Snapshot, fh source.Fil event.Error(ctx, fmt.Sprintf("workspace packages: diagnosing %s", pm.URI), err) } if err == nil { - // Diagnose packages in parallel. - // (It's possible this is the first operation after the initial - // metadata load to demand type-checking of all the active packages.) - var group errgroup.Group - var mu sync.Mutex - for _, m := range active { - m := m - group.Go(func() error { - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, m.ID) - if err != nil { - return err - } - mu.Lock() - diagnostics = append(diagnostics, pkgDiagnostics[fh.URI()]...) - mu.Unlock() - return nil - }) - } - if err := group.Wait(); err != nil { + // Type-check packages in parallel and gather list/parse/type errors. + // (This may be the first operation after the initial metadata load + // to demand type-checking of all active packages.) + ids := make([]source.PackageID, len(active)) + for i, meta := range active { + ids[i] = meta.ID + } + pkgs, err := snapshot.TypeCheck(ctx, source.TypecheckFull, ids...) + if err != nil { return nil, err } + for _, pkg := range pkgs { + diagnostics = append(diagnostics, pkg.DiagnosticsForFile(fh.URI())...) + } } tidied, err := snapshot.ModTidy(ctx, pm) diff --git a/gopls/internal/lsp/source/diagnostics.go b/gopls/internal/lsp/source/diagnostics.go index 60ef7e69604..8101a043256 100644 --- a/gopls/internal/lsp/source/diagnostics.go +++ b/gopls/internal/lsp/source/diagnostics.go @@ -6,7 +6,6 @@ package source import ( "context" - "fmt" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/span" @@ -37,6 +36,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon categories := []map[string]*Analyzer{ options.DefaultAnalyzers, options.StaticcheckAnalyzers, + options.TypeErrorAnalyzers, } if includeConvenience { // e.g. for codeAction categories = append(categories, options.ConvenienceAnalyzers) // e.g. fillstruct @@ -55,14 +55,15 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon } // Report diagnostics and errors from root analyzers. - reports := map[span.URI][]*Diagnostic{} + reports := make(map[span.URI][]*Diagnostic) for _, diag := range analysisDiagnostics { reports[diag.URI] = append(reports[diag.URI], diag) } return reports, nil } -// FileDiagnostics reports diagnostics in the specified file. +// FileDiagnostics reports diagnostics in the specified file, +// as used by the "gopls check" command. // // TODO(adonovan): factor in common with (*Server).codeAction, which // executes { PackageForFile; DiagnosePackage; Analyze } too? @@ -76,22 +77,66 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers if err != nil { return VersionedFileIdentity{}, nil, err } - metas, err := snapshot.MetadataForFile(ctx, uri) + pkg, err := snapshot.PackageForFile(ctx, uri, TypecheckFull, NarrowestPackage) if err != nil { return VersionedFileIdentity{}, nil, err } - if len(metas) == 0 { - return VersionedFileIdentity{}, nil, fmt.Errorf("no package containing file %q", uri) - } - id := metas[0].ID // 0 => narrowest package - diagnostics, err := snapshot.DiagnosePackage(ctx, id) + adiags, err := Analyze(ctx, snapshot, pkg.ID(), false) if err != nil { return VersionedFileIdentity{}, nil, err } - analysisDiags, err := Analyze(ctx, snapshot, id, false) - if err != nil { - return VersionedFileIdentity{}, nil, err - } - fileDiags := append(diagnostics[fh.URI()], analysisDiags[fh.URI()]...) + var fileDiags []*Diagnostic // combine load/parse/type + analysis diagnostics + CombineDiagnostics(pkg, fh.URI(), adiags, &fileDiags, &fileDiags) return fh.VersionedFileIdentity(), fileDiags, nil } + +// CombineDiagnostics combines and filters list/parse/type diagnostics +// from pkg.DiagnosticsForFile(uri) with analysisDiagnostics[uri], and +// appends the two lists to *outT and *outA, respectively. +// +// Type-error analyzers produce diagnostics that are redundant +// with type checker diagnostics, but more detailed (e.g. fixes). +// Rather than report two diagnostics for the same problem, +// we combine them by augmenting the type-checker diagnostic +// and discarding the analyzer diagnostic. +// +// If an analysis diagnostic has the same range and message as +// a list/parse/type diagnostic, the suggested fix information +// (et al) of the latter is merged into a copy of the former. +// This handles the case where a type-error analyzer suggests +// a fix to a type error, and avoids duplication. +// +// The use of out-slices, though irregular, allows the caller to +// easily choose whether to keep the results separate or combined. +// +// The arguments are not modified. +func CombineDiagnostics(pkg Package, uri span.URI, analysisDiagnostics map[span.URI][]*Diagnostic, outT, outA *[]*Diagnostic) { + + // Build index of (list+parse+)type errors. + type key struct { + Range protocol.Range + message string + } + index := make(map[key]int) // maps (Range,Message) to index in tdiags slice + tdiags := pkg.DiagnosticsForFile(uri) + for i, diag := range tdiags { + index[key{diag.Range, diag.Message}] = i + } + + // Filter out analysis diagnostics that match type errors, + // retaining their suggested fix (etc) fields. + for _, diag := range analysisDiagnostics[uri] { + if i, ok := index[key{diag.Range, diag.Message}]; ok { + copy := *tdiags[i] + copy.SuggestedFixes = diag.SuggestedFixes + copy.Tags = diag.Tags + copy.Analyzer = diag.Analyzer + tdiags[i] = © + continue + } + + *outA = append(*outA, diag) + } + + *outT = append(*outT, tdiags...) +} diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 39a9a3ab2dc..e484e3a752d 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -107,41 +107,6 @@ type Snapshot interface { // Position information is added to FileSet(). ParseGo(ctx context.Context, fh FileHandle, mode ParseMode) (*ParsedGoFile, error) - // DiagnosePackage returns basic diagnostics, including list, - // parse, and type errors for pkg, grouped by file. - // - // It may include suggested fixes for type errors, created by - // running the analysis framework. - // - // TODO(adonovan): this operation does a mix of checks that - // range from cheap (list errors, parse errors) to expensive - // (type errors, type-error-analyzer results). In particular, - // type-error analyzers (as currently implemented) depend on - // the full analyzer machinery, and in the near future that - // will be completely separate from regular type checking. - // So, we must choose between: - // - // (1) rewriting them as ad-hoc functions that operate on - // type-checked packages. That's a fair amount of work - // since they are fairly deeply enmeshed in the framework, - // (some have horizontal dependencies such as Inspector), - // and quite reasonably so. So we reject this in favor of: - // - // (2) separating the generation of type errors (which happens - // at the lowest latency) from full analysis, which is - // slower, although hopefully eventually only on the order - // of seconds. In this case, type error analyzers are - // basically not special; the only special part would be - // the postprocessing step to merge the - // type-error-analyzer fixes into the ordinary diagnostics - // produced by type checking. (Not yet sure how to do that.) - // - // So then the role of this function is "report fast - // diagnostics, up to type-checking", and the role of - // Analyze() is "run the analysis framework, included - // suggested fixes for type errors" - DiagnosePackage(ctx context.Context, id PackageID) (map[span.URI][]*Diagnostic, error) - // Analyze runs the specified analyzers on the given package at this snapshot. Analyze(ctx context.Context, id PackageID, analyzers []*Analyzer) ([]*Diagnostic, error) @@ -711,6 +676,8 @@ type Analyzer struct { Severity protocol.DiagnosticSeverity } +func (a *Analyzer) String() string { return a.Analyzer.String() } + // Enabled reports whether this analyzer is enabled by the given options. func (a Analyzer) IsEnabled(options *Options) bool { // Staticcheck analyzers can only be enabled when staticcheck is on. @@ -762,6 +729,7 @@ type Package interface { ResolveImportPath(path ImportPath) (Package, error) Imports() []Package // new slice of all direct dependencies, unordered HasTypeErrors() bool + DiagnosticsForFile(uri span.URI) []*Diagnostic // new array of list/parse/type errors } // A CriticalError is a workspace-wide error that generally prevents gopls from diff --git a/gopls/internal/lsp/testdata/badstmt/badstmt.go.in b/gopls/internal/lsp/testdata/badstmt/badstmt.go.in index 8f0654a8a52..81aee201d7f 100644 --- a/gopls/internal/lsp/testdata/badstmt/badstmt.go.in +++ b/gopls/internal/lsp/testdata/badstmt/badstmt.go.in @@ -4,10 +4,13 @@ import ( "golang.org/lsptests/foo" ) -func _() { +// The nonewvars expectation asserts that the go/analysis framework ran. +// See comments in noparse. + +func _(x int) { defer foo.F //@complete(" //", Foo),diag(" //", "syntax", "function must be invoked in defer statement|expression in defer must be function call", "error") - y := 1 defer foo.F //@complete(" //", Foo) + x := 123 //@diag(":=", "nonewvars", "no new variables", "warning") } func _() { diff --git a/gopls/internal/lsp/testdata/noparse/noparse.go.in b/gopls/internal/lsp/testdata/noparse/noparse.go.in index 7dc23e02562..8b0bfaa035c 100644 --- a/gopls/internal/lsp/testdata/noparse/noparse.go.in +++ b/gopls/internal/lsp/testdata/noparse/noparse.go.in @@ -1,11 +1,24 @@ package noparse +// The type error was chosen carefully to exercise a type-error analyzer. +// We use the 'nonewvars' analyzer because the other candidates are tricky: +// +// - The 'unusedvariable' analyzer is disabled by default, so it is not +// consistently enabled across Test{LSP,CommandLine} tests, which +// both process this file. +// - The 'undeclaredname' analyzer depends on the text of the go/types +// "undeclared name" error, which changed in go1.20. +// - The 'noresultvalues' analyzer produces a diagnostic containing newlines, +// which breaks the parser used by TestCommandLine. +// +// This comment is all that remains of my afternoon. + func bye(x int) { - hi() + x := 123 //@diag(":=", "nonewvars", "no new variables", "warning") } func stuff() { - x := 5 + } func .() {} //@diag(".", "syntax", "expected 'IDENT', found '.'", "error") diff --git a/gopls/internal/lsp/testdata/noparse_format/noparse_format.go.in b/gopls/internal/lsp/testdata/noparse_format/noparse_format.go.in index 4fc3824d9b8..311a99aafb3 100644 --- a/gopls/internal/lsp/testdata/noparse_format/noparse_format.go.in +++ b/gopls/internal/lsp/testdata/noparse_format/noparse_format.go.in @@ -2,8 +2,13 @@ package noparse_format //@format("package") +// The nonewvars expectation asserts that the go/analysis framework ran. +// See comments in badstmt. + func what() { - var b int + var hi func() if { hi() //@diag("{", "syntax", "missing condition in if statement", "error") } -} \ No newline at end of file + hi := nil //@diag(":=", "nonewvars", "no new variables", "warning") +} + diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden index c4e047b5827..075fdc21193 100644 --- a/gopls/internal/lsp/testdata/summary.txt.golden +++ b/gopls/internal/lsp/testdata/summary.txt.golden @@ -8,7 +8,7 @@ DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 RankedCompletionsCount = 164 CaseSensitiveCompletionsCount = 4 -DiagnosticsCount = 39 +DiagnosticsCount = 42 FoldingRangesCount = 2 FormatCount = 6 ImportCount = 8 diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden index 17df1010a76..c26b928aba9 100644 --- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden @@ -8,7 +8,7 @@ DeepCompletionsCount = 5 FuzzyCompletionsCount = 8 RankedCompletionsCount = 174 CaseSensitiveCompletionsCount = 4 -DiagnosticsCount = 39 +DiagnosticsCount = 42 FoldingRangesCount = 2 FormatCount = 6 ImportCount = 8