Google Git
Sign in
go/tools/a35b054a189a7451ce4fb8067f293285b3f02500^!/.
commit
a35b054a189a7451ce4fb8067f293285b3f02500
[log]
author
Alan Donovan <adonovan@google.com>
Thu Jun 06 16:48:10 2024 -0400
committer
Gopher Robot <gobot@golang.org>
Fri Jun 07 15:21:21 2024 +0000
tree
806c70efaa6fee6efad8e6bfd87effd7c79eed5f
parent
fc82f4e5a0172290db048466b14783f620daeae4 [diff]
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)
 	}