diff --git a/gopls/internal/regtest/watch/watch_test.go b/gopls/internal/regtest/watch/watch_test.go index 436c09153a..ae43beaf0c 100644 --- a/gopls/internal/regtest/watch/watch_test.go +++ b/gopls/internal/regtest/watch/watch_test.go @@ -370,6 +370,7 @@ func _() { // Tests golang/go#38498. Delete a file and then force a reload. // Assert that we no longer try to load the file. func TestDeleteFiles(t *testing.T) { + testenv.NeedsGo1Point(t, 13) // Poor overlay support causes problems on 1.12. const pkg = ` -- go.mod -- module mod.com diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index 69ee199505..9b76f3ff4d 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -69,7 +69,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if err != nil { return nil, err } - quickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, diags) + quickFixes, err := codeActionsMatchingDiagnostics(ctx, snapshot, diagnostics, diags) if err != nil { return nil, err } @@ -128,75 +128,68 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara if err != nil { return nil, err } - if (wanted[protocol.QuickFix] || wanted[protocol.SourceFixAll]) && len(diagnostics) > 0 { - analysisDiags, err := source.Analyze(ctx, snapshot, pkg) + + pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) + if err != nil { + return nil, err + } + analysisDiags, err := source.Analyze(ctx, snapshot, pkg, true) + if err != nil { + return nil, err + } + fileDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...) + modURI := snapshot.GoModForFile(fh.URI()) + if modURI != "" { + modFH, err := snapshot.GetVersionedFile(ctx, modURI) if err != nil { return nil, err } - - if wanted[protocol.QuickFix] { - pkgDiagnostics, err := snapshot.DiagnosePackage(ctx, pkg) - if err != nil { - return nil, err - } - quickFixDiags := append(pkgDiagnostics[uri], analysisDiags[uri]...) - modURI := snapshot.GoModForFile(fh.URI()) - if modURI != "" { - modFH, err := snapshot.GetVersionedFile(ctx, modURI) - if err != nil { - return nil, err - } - modDiags, err := mod.DiagnosticsForMod(ctx, snapshot, modFH) - if err != nil && !source.IsNonFatalGoModError(err) { - // Not a fatal error. - event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder())) - } - quickFixDiags = append(quickFixDiags, modDiags...) - } - quickFixes, err := quickFixesForDiagnostics(ctx, snapshot, diagnostics, quickFixDiags) - if err != nil { - return nil, err - } - codeActions = append(codeActions, quickFixes...) - + modDiags, err := mod.DiagnosticsForMod(ctx, snapshot, modFH) + if err != nil && !source.IsNonFatalGoModError(err) { + // Not a fatal error. + event.Error(ctx, "module suggested fixes failed", err, tag.Directory.Of(snapshot.View().Folder())) } - if wanted[protocol.SourceFixAll] { - var fixAllEdits []protocol.TextDocumentEdit - for _, ad := range analysisDiags[uri] { - if ad.Analyzer == nil || !ad.Analyzer.HighConfidence { - continue - } - for _, fix := range ad.SuggestedFixes { - edits := fix.Edits[fh.URI()] - if len(edits) == 0 { - continue - } - fixAllEdits = append(fixAllEdits, documentChanges(fh, edits)...) - } + fileDiags = append(fileDiags, modDiags...) + } - } - if len(fixAllEdits) != 0 { - codeActions = append(codeActions, protocol.CodeAction{ - Title: "Simplifications", - Kind: protocol.SourceFixAll, - Edit: protocol.WorkspaceEdit{ - DocumentChanges: fixAllEdits, - }, - }) - } + // Split diagnostics into fixes, which must match incoming diagnostics, + // and non-fixes, which must match the requested range. Build actions + // for all of them. + var fixDiags, nonFixDiags []*source.Diagnostic + for _, d := range fileDiags { + if len(d.SuggestedFixes) == 0 { + continue + } + kind := protocol.QuickFix + if d.Analyzer != nil && d.Analyzer.ActionKind != "" { + kind = d.Analyzer.ActionKind + } + if kind == protocol.QuickFix || kind == protocol.SourceFixAll { + fixDiags = append(fixDiags, d) + } else { + nonFixDiags = append(nonFixDiags, d) } } - if ctx.Err() != nil { - return nil, ctx.Err() + + fixActions, err := codeActionsMatchingDiagnostics(ctx, snapshot, diagnostics, fixDiags) + if err != nil { + return nil, err } - // Add any suggestions that do not necessarily fix any diagnostics. - if wanted[protocol.RefactorRewrite] { - fixes, err := convenienceFixes(ctx, snapshot, pkg, uri, params.Range) + codeActions = append(codeActions, fixActions...) + + for _, nonfix := range nonFixDiags { + // For now, only show diagnostics for matching lines. Maybe we should + // alter this behavior in the future, depending on the user experience. + if !protocol.Intersect(nonfix.Range, params.Range) { + continue + } + actions, err := codeActionsForDiagnostic(ctx, snapshot, nonfix, nil) if err != nil { return nil, err } - codeActions = append(codeActions, fixes...) + codeActions = append(codeActions, actions...) } + if wanted[protocol.RefactorExtract] { fixes, err := extractionFixes(ctx, snapshot, pkg, uri, params.Range) if err != nil { @@ -217,7 +210,14 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara // Unsupported file kind for a code action. return nil, nil } - return codeActions, nil + + var filtered []protocol.CodeAction + for _, action := range codeActions { + if wanted[action.Kind] { + filtered = append(filtered, action) + } + } + return filtered, nil } func (s *Server) getSupportedCodeActions() []protocol.CodeActionKind { @@ -278,94 +278,6 @@ func importDiagnostics(fix *imports.ImportFix, diagnostics []protocol.Diagnostic return results } -// diagnosticToAnalyzer return the analyzer associated with a given diagnostic. -// It assumes that the diagnostic's source will be the name of the analyzer. -// If this changes, this approach will need to be reworked. -func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer *source.Analyzer) { - // Make sure that the analyzer we found is enabled. - defer func() { - if analyzer != nil && !analyzer.IsEnabled(snapshot.View()) { - analyzer = nil - } - }() - if a, ok := snapshot.View().Options().DefaultAnalyzers[src]; ok { - return a - } - if a, ok := snapshot.View().Options().StaticcheckAnalyzers[src]; ok { - return a - } - if a, ok := snapshot.View().Options().ConvenienceAnalyzers[src]; ok { - return a - } - return nil -} - -func convenienceFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) { - var analyzers []*source.Analyzer - for _, a := range snapshot.View().Options().ConvenienceAnalyzers { - if !a.IsEnabled(snapshot.View()) { - continue - } - if a.Fix == "" { - event.Error(ctx, "convenienceFixes", fmt.Errorf("no suggested fixes for convenience analyzer %s", a.Analyzer.Name)) - continue - } - analyzers = append(analyzers, a) - } - diagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) - if err != nil { - return nil, err - } - var codeActions []protocol.CodeAction - for _, d := range diagnostics { - // For now, only show diagnostics for matching lines. Maybe we should - // alter this behavior in the future, depending on the user experience. - if d.URI != uri { - continue - } - - if !protocol.Intersect(d.Range, rng) { - continue - } - action, err := diagnosticToCommandCodeAction(ctx, snapshot, d, nil, protocol.RefactorRewrite) - if err != nil { - return nil, err - } - codeActions = append(codeActions, *action) - } - return codeActions, nil -} - -func diagnosticToCommandCodeAction(ctx context.Context, snapshot source.Snapshot, sd *source.Diagnostic, pd *protocol.Diagnostic, kind protocol.CodeActionKind) (*protocol.CodeAction, error) { - // The fix depends on the category of the analyzer. The diagnostic may be - // nil, so use the error's category. - analyzer := diagnosticToAnalyzer(snapshot, string(sd.Source), sd.Message) - if analyzer == nil { - return nil, fmt.Errorf("no convenience analyzer for source %s", sd.Source) - } - if analyzer.Fix == "" { - return nil, fmt.Errorf("no fix for convenience analyzer %s", analyzer.Analyzer.Name) - } - cmd, err := command.NewApplyFixCommand(sd.Message, command.ApplyFixArgs{ - URI: protocol.URIFromSpanURI(sd.URI), - Range: sd.Range, - Fix: analyzer.Fix, - }) - if err != nil { - return nil, err - } - var diagnostics []protocol.Diagnostic - if pd != nil { - diagnostics = append(diagnostics, *pd) - } - return &protocol.CodeAction{ - Title: sd.Message, - Kind: kind, - Diagnostics: diagnostics, - Command: &cmd, - }, nil -} - func extractionFixes(ctx context.Context, snapshot source.Snapshot, pkg source.Package, uri span.URI, rng protocol.Range) ([]protocol.CodeAction, error) { if rng.Start == rng.End { return nil, nil @@ -431,47 +343,63 @@ func documentChanges(fh source.VersionedFileHandle, edits []protocol.TextEdit) [ } } -func quickFixesForDiagnostics(ctx context.Context, snapshot source.Snapshot, pdiags []protocol.Diagnostic, sdiags []*source.Diagnostic) ([]protocol.CodeAction, error) { - var quickFixes []protocol.CodeAction - for _, e := range sdiags { +func codeActionsMatchingDiagnostics(ctx context.Context, snapshot source.Snapshot, pdiags []protocol.Diagnostic, sdiags []*source.Diagnostic) ([]protocol.CodeAction, error) { + var actions []protocol.CodeAction + for _, sd := range sdiags { var diag *protocol.Diagnostic - for _, d := range pdiags { - if sameDiagnostic(d, e) { - diag = &d + for _, pd := range pdiags { + if sameDiagnostic(pd, sd) { + diag = &pd break } } if diag == nil { continue } - for _, fix := range e.SuggestedFixes { - action := protocol.CodeAction{ - Title: fix.Title, - Kind: protocol.QuickFix, - Diagnostics: []protocol.Diagnostic{*diag}, - Edit: protocol.WorkspaceEdit{}, - Command: fix.Command, - } - - for uri, edits := range fix.Edits { - fh, err := snapshot.GetVersionedFile(ctx, uri) - if err != nil { - return nil, err - } - action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, protocol.TextDocumentEdit{ - TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ - Version: fh.Version(), - TextDocumentIdentifier: protocol.TextDocumentIdentifier{ - URI: protocol.URIFromSpanURI(uri), - }, - }, - Edits: edits, - }) - } - quickFixes = append(quickFixes, action) + diagActions, err := codeActionsForDiagnostic(ctx, snapshot, sd, diag) + if err != nil { + return nil, err } + actions = append(actions, diagActions...) + } - return quickFixes, nil + return actions, nil +} + +func codeActionsForDiagnostic(ctx context.Context, snapshot source.Snapshot, sd *source.Diagnostic, pd *protocol.Diagnostic) ([]protocol.CodeAction, error) { + var actions []protocol.CodeAction + for _, fix := range sd.SuggestedFixes { + action := protocol.CodeAction{ + Title: fix.Title, + Kind: protocol.QuickFix, + Edit: protocol.WorkspaceEdit{}, + Command: fix.Command, + } + if pd != nil { + action.Diagnostics = []protocol.Diagnostic{*pd} + } + if sd.Analyzer != nil && sd.Analyzer.ActionKind != "" { + action.Kind = sd.Analyzer.ActionKind + } + + for uri, edits := range fix.Edits { + fh, err := snapshot.GetVersionedFile(ctx, uri) + if err != nil { + return nil, err + } + action.Edit.DocumentChanges = append(action.Edit.DocumentChanges, protocol.TextDocumentEdit{ + TextDocument: protocol.OptionalVersionedTextDocumentIdentifier{ + Version: fh.Version(), + TextDocumentIdentifier: protocol.TextDocumentIdentifier{ + URI: protocol.URIFromSpanURI(uri), + }, + }, + Edits: edits, + }) + } + actions = append(actions, action) + } + return actions, nil } func sameDiagnostic(pd protocol.Diagnostic, sd *source.Diagnostic) bool { diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 036877163b..6b75febbbc 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -265,7 +265,7 @@ func (s *Server) diagnosePkg(ctx context.Context, snapshot source.Snapshot, pkg s.storeDiagnostics(snapshot, cgf.URI, typeCheckSource, pkgDiagnostics[cgf.URI]) } if includeAnalysis && !pkg.HasListOrParseErrors() { - reports, err := source.Analyze(ctx, snapshot, pkg) + reports, err := source.Analyze(ctx, snapshot, pkg, false) if err != nil { event.Error(ctx, "warning: analyzing 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 e1a74160d7..1948381d5a 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -23,39 +23,19 @@ type RelatedInformation struct { Message string } -func Analyze(ctx context.Context, snapshot Snapshot, pkg Package) (map[span.URI][]*Diagnostic, error) { +func Analyze(ctx context.Context, snapshot Snapshot, pkg Package, includeConvenience bool) (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, pkg.HasTypeErrors()) - analysisDiagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) - if err != nil { - return nil, err - } - reports := map[span.URI][]*Diagnostic{} - // Report diagnostics and errors from root analyzers. - for _, diag := range analysisDiagnostics { - // If the diagnostic comes from a "convenience" analyzer, it is not - // meant to provide diagnostics, but rather only suggested fixes. - // Skip these types of errors in diagnostics; we will use their - // suggested fixes when providing code actions. - if isConvenienceAnalyzer(string(diag.Source)) { - continue - } - reports[diag.URI] = append(reports[diag.URI], diag) + categories := []map[string]*Analyzer{} + if includeConvenience { + categories = append(categories, snapshot.View().Options().ConvenienceAnalyzers) } - return reports, nil -} - -func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) []*Analyzer { - // Always run convenience analyzers. - categories := []map[string]*Analyzer{snapshot.View().Options().ConvenienceAnalyzers} // If we had type errors, don't run any other analyzers. - if !hadTypeErrors { + if !pkg.HasTypeErrors() { categories = append(categories, snapshot.View().Options().DefaultAnalyzers, snapshot.View().Options().StaticcheckAnalyzers) } var analyzers []*Analyzer @@ -64,7 +44,18 @@ func pickAnalyzers(snapshot Snapshot, hadTypeErrors bool) []*Analyzer { analyzers = append(analyzers, a) } } - return analyzers + + analysisDiagnostics, err := snapshot.Analyze(ctx, pkg.ID(), analyzers) + if err != nil { + return nil, err + } + + reports := map[span.URI][]*Diagnostic{} + // Report diagnostics and errors from root analyzers. + for _, diag := range analysisDiagnostics { + reports[diag.URI] = append(reports[diag.URI], diag) + } + return reports, nil } func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (VersionedFileIdentity, []*Diagnostic, error) { @@ -82,7 +73,7 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (Vers } fileDiags := diagnostics[fh.URI()] if !pkg.HasListOrParseErrors() { - analysisDiags, err := Analyze(ctx, snapshot, pkg) + analysisDiags, err := Analyze(ctx, snapshot, pkg, false) if err != nil { return VersionedFileIdentity{}, nil, err } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index d36d12949f..0f8dda1e40 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -97,6 +97,7 @@ func DefaultOptions() *Options { }, Mod: { protocol.SourceOrganizeImports: true, + protocol.QuickFix: true, }, Sum: {}, }, @@ -1078,9 +1079,9 @@ func EnabledAnalyzers(snapshot Snapshot) (analyzers []*Analyzer) { func typeErrorAnalyzers() map[string]*Analyzer { return map[string]*Analyzer{ fillreturns.Analyzer.Name: { - Analyzer: fillreturns.Analyzer, - HighConfidence: true, - Enabled: true, + Analyzer: fillreturns.Analyzer, + ActionKind: protocol.SourceFixAll, + Enabled: true, }, nonewvars.Analyzer.Name: { Analyzer: nonewvars.Analyzer, @@ -1101,9 +1102,10 @@ func typeErrorAnalyzers() map[string]*Analyzer { func convenienceAnalyzers() map[string]*Analyzer { return map[string]*Analyzer{ fillstruct.Analyzer.Name: { - Analyzer: fillstruct.Analyzer, - Fix: FillStruct, - Enabled: true, + Analyzer: fillstruct.Analyzer, + Fix: FillStruct, + Enabled: true, + ActionKind: protocol.RefactorRewrite, }, } } @@ -1148,9 +1150,9 @@ func defaultAnalyzers() map[string]*Analyzer { unusedwrite.Analyzer.Name: {Analyzer: unusedwrite.Analyzer, Enabled: false}, // gofmt -s suite: - simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, HighConfidence: true}, - simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, HighConfidence: true}, - simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, HighConfidence: true}, + simplifycompositelit.Analyzer.Name: {Analyzer: simplifycompositelit.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, + simplifyrange.Analyzer.Name: {Analyzer: simplifyrange.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, + simplifyslice.Analyzer.Name: {Analyzer: simplifyslice.Analyzer, Enabled: true, ActionKind: protocol.SourceFixAll}, } } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 57712b6d3d..9bc5eca3b9 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -520,9 +520,9 @@ type Analyzer struct { // the analyzer's suggested fixes through a Command, not a TextEdit. Fix string - // If this is true, then we can apply the suggested fixes - // as part of a source.FixAll codeaction. - HighConfidence bool + // ActionKind is the kind of code action this analyzer produces. If + // unspecified the type defaults to quickfix. + ActionKind protocol.CodeActionKind } func (a Analyzer) IsEnabled(view View) bool {