gopls/internal/test/integration: style tweaks to CodeAction
Details:
- Pass TriggerKind by value, not pointer.
Define nil and zero as equivalent, always.
Name the zero value: UnknownTrigger.
Omit "kind" suffix in var names for brevity.
- Add Workdir.EntireFile(path string) Location helper.
- Env.CodeAction renamed CodeActionForFile.
Env.CodeAction0 unsuffixed.
Editor.CodeAction deleted.
Editor.CodeAction0 unsuffixed.
- Restore lost commentary.
(Sorry, I was late to the code review of CL 590935.)
Change-Id: I3fbe3e4e7567366b0742dcec44dc50539b9e9621
Reviewed-on: https://go-review.googlesource.com/c/tools/+/591176
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go
index 8e8158c..2208822 100644
--- a/gopls/internal/golang/codeaction.go
+++ b/gopls/internal/golang/codeaction.go
@@ -26,9 +26,14 @@
"golang.org/x/tools/internal/imports"
)
-// CodeActions returns all code actions (edits and other commands)
-// available for the selected range.
-func CodeActions(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, rng protocol.Range, diagnostics []protocol.Diagnostic, want map[protocol.CodeActionKind]bool, triggerKind protocol.CodeActionTriggerKind) (actions []protocol.CodeAction, _ error) {
+// CodeActions returns all wanted code actions (edits and other
+// commands) available for the selected range.
+//
+// Depending on how the request was triggered, fewer actions may be
+// offered, e.g. to avoid UI distractions after mere cursor motion.
+//
+// See ../protocol/codeactionkind.go for some code action theory.
+func CodeActions(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, rng protocol.Range, diagnostics []protocol.Diagnostic, want map[protocol.CodeActionKind]bool, trigger protocol.CodeActionTriggerKind) (actions []protocol.CodeAction, _ error) {
// Only compute quick fixes if there are any diagnostics to fix.
wantQuickFixes := want[protocol.QuickFix] && len(diagnostics) > 0
@@ -138,7 +143,9 @@
actions = append(actions, rewrites...)
}
- if want[protocol.RefactorInline] && (triggerKind != protocol.CodeActionAutomatic || rng.Start != rng.End) {
+ // To avoid distraction (e.g. VS Code lightbulb), offer "inline"
+ // only after a selection or explicit menu operation.
+ if want[protocol.RefactorInline] && (trigger != protocol.CodeActionAutomatic || rng.Start != rng.End) {
rewrites, err := getInlineCodeActions(pkg, pgf, rng, snapshot.Options())
if err != nil {
return nil, err
diff --git a/gopls/internal/protocol/codeactionkind.go b/gopls/internal/protocol/codeactionkind.go
index 8acba0e..f3d953f 100644
--- a/gopls/internal/protocol/codeactionkind.go
+++ b/gopls/internal/protocol/codeactionkind.go
@@ -79,6 +79,11 @@
GoFreeSymbols CodeActionKind = "source.freesymbols"
)
+// CodeActionUnknownTrigger indicates that the trigger for a
+// CodeAction request is unknown. A missing
+// CodeActionContext.TriggerKind should be treated as equivalent.
+const CodeActionUnknownTrigger CodeActionTriggerKind = 0
+
// A CodeLensSource identifies an (algorithmic) source of code lenses.
type CodeLensSource string
diff --git a/gopls/internal/server/code_action.go b/gopls/internal/server/code_action.go
index e79ae68..45f230e 100644
--- a/gopls/internal/server/code_action.go
+++ b/gopls/internal/server/code_action.go
@@ -109,7 +109,7 @@
return actions, nil
case file.Go:
- // diagnostic-associated code actions (problematic code)
+ // diagnostic-bundled code actions
//
// The diagnostics already have a UI presence (e.g. squiggly underline);
// the associated action may additionally show (in VS Code) as a lightbulb.
@@ -120,11 +120,13 @@
if err != nil {
return nil, err
}
- var triggerKind protocol.CodeActionTriggerKind
- if k := params.Context.TriggerKind; k != nil {
- triggerKind = *k
+
+ // computed code actions (may include quickfixes from diagnostics)
+ trigger := protocol.CodeActionUnknownTrigger
+ if k := params.Context.TriggerKind; k != nil { // (some clients omit it)
+ trigger = *k
}
- moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want, triggerKind)
+ moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want, trigger)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/test/integration/bench/codeaction_test.go b/gopls/internal/test/integration/bench/codeaction_test.go
index fe89500..679f2d4 100644
--- a/gopls/internal/test/integration/bench/codeaction_test.go
+++ b/gopls/internal/test/integration/bench/codeaction_test.go
@@ -20,7 +20,7 @@
defer closeBuffer(b, env, test.file)
env.AfterChange()
- env.CodeAction(test.file, nil) // pre-warm
+ env.CodeActionForFile(test.file, nil) // pre-warm
b.ResetTimer()
@@ -29,7 +29,7 @@
}
for i := 0; i < b.N; i++ {
- env.CodeAction(test.file, nil)
+ env.CodeActionForFile(test.file, nil)
}
})
}
@@ -44,7 +44,7 @@
env.EditBuffer(test.file, protocol.TextEdit{NewText: "// __TEST_PLACEHOLDER_0__\n"})
env.AfterChange()
- env.CodeAction(test.file, nil) // pre-warm
+ env.CodeActionForFile(test.file, nil) // pre-warm
b.ResetTimer()
@@ -62,7 +62,7 @@
// Increment the placeholder text, to ensure cache misses.
NewText: fmt.Sprintf("// __TEST_PLACEHOLDER_%d__\n", edits),
})
- env.CodeAction(test.file, nil)
+ env.CodeActionForFile(test.file, nil)
}
})
}
diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go
index f5d62b0..487a255 100644
--- a/gopls/internal/test/integration/fake/editor.go
+++ b/gopls/internal/test/integration/fake/editor.go
@@ -933,7 +933,7 @@
// OrganizeImports requests and performs the source.organizeImports codeAction.
func (e *Editor) OrganizeImports(ctx context.Context, path string) error {
- loc := protocol.Location{URI: e.sandbox.Workdir.URI(path)} // zero Range => whole file
+ loc := e.sandbox.Workdir.EntireFile(path)
_, err := e.applyCodeActions(ctx, loc, nil, protocol.SourceOrganizeImports)
return err
}
@@ -1558,11 +1558,9 @@
// CodeAction executes a codeAction request on the server.
// If loc.Range is zero, the whole file is implied.
-func (e *Editor) CodeAction(ctx context.Context, loc protocol.Location, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) {
- return e.CodeAction0(ctx, loc, diagnostics, nil)
-}
-
-func (e *Editor) CodeAction0(ctx context.Context, loc protocol.Location, diagnostics []protocol.Diagnostic, triggerKind *protocol.CodeActionTriggerKind) ([]protocol.CodeAction, error) {
+// To reduce distraction, the trigger action (unknown, automatic, invoked)
+// may affect what actions are offered.
+func (e *Editor) CodeAction(ctx context.Context, loc protocol.Location, diagnostics []protocol.Diagnostic, trigger protocol.CodeActionTriggerKind) ([]protocol.CodeAction, error) {
if e.Server == nil {
return nil, nil
}
@@ -1577,7 +1575,7 @@
TextDocument: e.TextDocumentIdentifier(path),
Context: protocol.CodeActionContext{
Diagnostics: diagnostics,
- TriggerKind: triggerKind,
+ TriggerKind: &trigger,
},
Range: loc.Range, // may be zero
}
diff --git a/gopls/internal/test/integration/fake/workdir.go b/gopls/internal/test/integration/fake/workdir.go
index 4d21554..977bf54 100644
--- a/gopls/internal/test/integration/fake/workdir.go
+++ b/gopls/internal/test/integration/fake/workdir.go
@@ -149,6 +149,11 @@
return w.RelPath(uri.Path())
}
+// EntireFile returns the entire extent of the file named by the workdir-relative path.
+func (w *Workdir) EntireFile(path string) protocol.Location {
+ return protocol.Location{URI: w.URI(path)}
+}
+
// ReadFile reads a text file specified by a workdir-relative path.
func (w *Workdir) ReadFile(path string) ([]byte, error) {
backoff := 1 * time.Millisecond
diff --git a/gopls/internal/test/integration/misc/codeactions_test.go b/gopls/internal/test/integration/misc/codeactions_test.go
index a6c2a8b..1a169c6 100644
--- a/gopls/internal/test/integration/misc/codeactions_test.go
+++ b/gopls/internal/test/integration/misc/codeactions_test.go
@@ -40,7 +40,7 @@
check := func(filename string, wantKind ...protocol.CodeActionKind) {
env.OpenFile(filename)
loc := env.RegexpSearch(filename, `g\(\)`)
- actions, err := env.Editor.CodeAction(env.Ctx, loc, nil)
+ actions, err := env.Editor.CodeAction(env.Ctx, loc, nil, protocol.CodeActionUnknownTrigger)
if err != nil {
t.Fatal(err)
}
@@ -87,23 +87,21 @@
Run(t, "", func(t *testing.T, env *Env) {
env.CreateBuffer("main.go", vim1)
- for _, triggerKind := range []protocol.CodeActionTriggerKind{0, protocol.CodeActionInvoked, protocol.CodeActionAutomatic} {
- triggerKindPtr := &triggerKind
- if triggerKind == 0 {
- triggerKindPtr = nil
- }
- t.Run(fmt.Sprintf("trigger=%v", triggerKind), func(t *testing.T) {
+ for _, trigger := range []protocol.CodeActionTriggerKind{
+ protocol.CodeActionUnknownTrigger,
+ protocol.CodeActionInvoked,
+ protocol.CodeActionAutomatic,
+ } {
+ t.Run(fmt.Sprintf("trigger=%v", trigger), func(t *testing.T) {
for _, selectedRange := range []bool{false, true} {
t.Run(fmt.Sprintf("range=%t", selectedRange), func(t *testing.T) {
- pattern := env.RegexpSearch("main.go", "Func")
- rng := pattern.Range
+ loc := env.RegexpSearch("main.go", "Func")
if !selectedRange {
// assume the cursor is placed at the beginning of `Func`, so end==start.
- rng.End = rng.Start
+ loc.Range.End = loc.Range.Start
}
- loc := protocol.Location{URI: pattern.URI, Range: rng}
- actions := env.CodeAction0("main.go", loc, nil, triggerKindPtr)
- want := triggerKind != protocol.CodeActionAutomatic || selectedRange
+ actions := env.CodeAction(loc, nil, trigger)
+ want := trigger != protocol.CodeActionAutomatic || selectedRange
if got := slices.ContainsFunc(actions, func(act protocol.CodeAction) bool {
return act.Kind == protocol.RefactorInline
}); got != want {
diff --git a/gopls/internal/test/integration/misc/extract_test.go b/gopls/internal/test/integration/misc/extract_test.go
index 86afb45..ec13856 100644
--- a/gopls/internal/test/integration/misc/extract_test.go
+++ b/gopls/internal/test/integration/misc/extract_test.go
@@ -30,7 +30,7 @@
Run(t, files, func(t *testing.T, env *Env) {
env.OpenFile("main.go")
loc := env.RegexpSearch("main.go", `a := 5\n.*return a`)
- actions, err := env.Editor.CodeAction(env.Ctx, loc, nil)
+ actions, err := env.Editor.CodeAction(env.Ctx, loc, nil, protocol.CodeActionUnknownTrigger)
if err != nil {
t.Fatal(err)
}
diff --git a/gopls/internal/test/integration/misc/fix_test.go b/gopls/internal/test/integration/misc/fix_test.go
index b3d86e1..acf896a 100644
--- a/gopls/internal/test/integration/misc/fix_test.go
+++ b/gopls/internal/test/integration/misc/fix_test.go
@@ -115,7 +115,7 @@
ReadDiagnostics("main.go", &d),
)
var quickFixes []*protocol.CodeAction
- for _, act := range env.CodeAction("main.go", d.Diagnostics) {
+ for _, act := range env.CodeActionForFile("main.go", d.Diagnostics) {
if act.Kind == protocol.QuickFix {
act := act // remove in go1.22
quickFixes = append(quickFixes, &act)
@@ -152,7 +152,7 @@
`
Run(t, files, func(t *testing.T, env *Env) {
env.OpenFile("external.go")
- _, err := env.Editor.CodeAction(env.Ctx, env.RegexpSearch("external.go", "z"), nil)
+ _, err := env.Editor.CodeAction(env.Ctx, env.RegexpSearch("external.go", "z"), nil, protocol.CodeActionUnknownTrigger)
if err != nil {
t.Fatal(err)
}
diff --git a/gopls/internal/test/integration/misc/imports_test.go b/gopls/internal/test/integration/misc/imports_test.go
index 0e85a96..ebc1c0d 100644
--- a/gopls/internal/test/integration/misc/imports_test.go
+++ b/gopls/internal/test/integration/misc/imports_test.go
@@ -149,7 +149,7 @@
env.OrganizeImports("main.go")
// Assert no quick fixes.
- for _, act := range env.CodeAction("main.go", nil) {
+ for _, act := range env.CodeActionForFile("main.go", nil) {
if act.Kind == protocol.QuickFix {
t.Errorf("unexpected quick fix action: %#v", act)
}
@@ -187,7 +187,7 @@
env.OrganizeImports("main.go")
// Assert no quick fixes.
- for _, act := range env.CodeAction("main.go", nil) {
+ for _, act := range env.CodeActionForFile("main.go", nil) {
if act.Kind == protocol.QuickFix {
t.Errorf("unexpected quick-fix action: %#v", act)
}
diff --git a/gopls/internal/test/integration/misc/vuln_test.go b/gopls/internal/test/integration/misc/vuln_test.go
index 6ba1ec8..1ed54a7 100644
--- a/gopls/internal/test/integration/misc/vuln_test.go
+++ b/gopls/internal/test/integration/misc/vuln_test.go
@@ -519,7 +519,7 @@
for pattern, want := range wantVulncheckDiagnostics {
modPathDiagnostics := testVulnDiagnostics(t, env, pattern, want, gotDiagnostics)
- gotActions := env.CodeAction("go.mod", modPathDiagnostics)
+ gotActions := env.CodeActionForFile("go.mod", modPathDiagnostics)
if diff := diffCodeActions(gotActions, want.codeActions); diff != "" {
t.Errorf("code actions for %q do not match, got %v, want %v\n%v\n", pattern, gotActions, want.codeActions, diff)
continue
@@ -716,7 +716,7 @@
modPathDiagnostics := testVulnDiagnostics(t, env, mod, want, gotDiagnostics)
// Check that the actions we get when including all diagnostics at a location return the same result
- gotActions := env.CodeAction("go.mod", modPathDiagnostics)
+ gotActions := env.CodeActionForFile("go.mod", modPathDiagnostics)
if diff := diffCodeActions(gotActions, want.codeActions); diff != "" {
t.Errorf("code actions for %q do not match, expected %v, got %v\n%v\n", mod, want.codeActions, gotActions, diff)
continue
@@ -839,7 +839,7 @@
for mod, want := range wantDiagnostics {
modPathDiagnostics := testVulnDiagnostics(t, env, mod, want, gotDiagnostics)
// Check that the actions we get when including all diagnostics at a location return the same result
- gotActions := env.CodeAction("go.mod", modPathDiagnostics)
+ gotActions := env.CodeActionForFile("go.mod", modPathDiagnostics)
allActions = append(allActions, gotActions...)
if diff := diffCodeActions(gotActions, want.codeActions); diff != "" {
t.Errorf("code actions for %q do not match, expected %v, got %v\n%v\n", mod, want.codeActions, gotActions, diff)
@@ -889,7 +889,7 @@
t.Errorf("incorrect (severity, source) for %q, want (%s, %s) got (%s, %s)\n", w.msg, w.severity, w.source, diag.Severity, diag.Source)
}
// Check expected code actions appear.
- gotActions := env.CodeAction("go.mod", []protocol.Diagnostic{*diag})
+ gotActions := env.CodeActionForFile("go.mod", []protocol.Diagnostic{*diag})
if diff := diffCodeActions(gotActions, w.codeActions); diff != "" {
t.Errorf("code actions for %q do not match, want %v, got %v\n%v\n", w.msg, w.codeActions, gotActions, diff)
continue
diff --git a/gopls/internal/test/integration/misc/webserver_test.go b/gopls/internal/test/integration/misc/webserver_test.go
index c4af449..851cc68 100644
--- a/gopls/internal/test/integration/misc/webserver_test.go
+++ b/gopls/internal/test/integration/misc/webserver_test.go
@@ -198,7 +198,7 @@
// Invoke the "View package documentation" code
// action to start the server.
var docAction *protocol.CodeAction
- actions := env.CodeAction(filename, nil)
+ actions := env.CodeActionForFile(filename, nil)
for _, act := range actions {
if act.Title == "View package documentation" {
docAction = &act
@@ -254,7 +254,7 @@
// Invoke the "Show free symbols" code
// action to start the server.
loc := env.RegexpSearch("a/a.go", "«((?:.|\n)*)»")
- actions, err := env.Editor.CodeAction(env.Ctx, loc, nil)
+ actions, err := env.Editor.CodeAction(env.Ctx, loc, nil, protocol.CodeActionUnknownTrigger)
if err != nil {
t.Fatalf("CodeAction: %v", err)
}
diff --git a/gopls/internal/test/integration/wrappers.go b/gopls/internal/test/integration/wrappers.go
index 0982f8a..8c5e35e 100644
--- a/gopls/internal/test/integration/wrappers.go
+++ b/gopls/internal/test/integration/wrappers.go
@@ -207,7 +207,7 @@
// ApplyQuickFixes processes the quickfix codeAction, calling t.Fatal on any error.
func (e *Env) ApplyQuickFixes(path string, diagnostics []protocol.Diagnostic) {
e.T.Helper()
- loc := protocol.Location{URI: e.Sandbox.Workdir.URI(path)} // zero Range => whole file
+ loc := e.Sandbox.Workdir.EntireFile(path)
if err := e.Editor.ApplyQuickFixes(e.Ctx, loc, diagnostics); err != nil {
e.T.Fatal(err)
}
@@ -224,7 +224,7 @@
// GetQuickFixes returns the available quick fix code actions.
func (e *Env) GetQuickFixes(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction {
e.T.Helper()
- loc := protocol.Location{URI: e.Sandbox.Workdir.URI(path)} // zero Range => whole file
+ loc := e.Sandbox.Workdir.EntireFile(path)
actions, err := e.Editor.GetQuickFixes(e.Ctx, loc, diagnostics)
if err != nil {
e.T.Fatal(err)
@@ -533,16 +533,17 @@
}
}
-// CodeAction calls textDocument/codeAction for the given path, and calls
-// t.Fatal if there are errors.
-func (e *Env) CodeAction(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction {
- loc := protocol.Location{URI: e.Sandbox.Workdir.URI(path)} // no Range => whole file
- return e.CodeAction0(path, loc, diagnostics, nil)
+// CodeActionForFile calls textDocument/codeAction for the entire
+// file, and calls t.Fatal if there were errors.
+func (e *Env) CodeActionForFile(path string, diagnostics []protocol.Diagnostic) []protocol.CodeAction {
+ return e.CodeAction(e.Sandbox.Workdir.EntireFile(path), diagnostics, protocol.CodeActionUnknownTrigger)
}
-func (e *Env) CodeAction0(path string, loc protocol.Location, diagnostics []protocol.Diagnostic, triggerKind *protocol.CodeActionTriggerKind) []protocol.CodeAction {
+// CodeAction calls textDocument/codeAction for a selection,
+// and calls t.Fatal if there were errors.
+func (e *Env) CodeAction(loc protocol.Location, diagnostics []protocol.Diagnostic, trigger protocol.CodeActionTriggerKind) []protocol.CodeAction {
e.T.Helper()
- actions, err := e.Editor.CodeAction0(e.Ctx, loc, diagnostics, triggerKind)
+ actions, err := e.Editor.CodeAction(e.Ctx, loc, diagnostics, trigger)
if err != nil {
e.T.Fatal(err)
}
