internal/lsp: refactor codeAction

As much as possible, try to unify the codeAction code paths. We always
run analysis now. And rather than assuming certain categories of
analyzers will generate certain kinds of code actions, mark them
explicitly and use that information to filter the actions afterward.

Change-Id: I8154cd67aa8b59b2a6c8aa9c3ea811de2e190db4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/300170
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: Rebecca Stambler <rstambler@golang.org>

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

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 {

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

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
 		}

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},
 	}
 }
 

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 {