gopls/internal/server: fix regression in organize imports code action

CL 587555 suppressed some code actions if the request was triggered
automatically, but that was too invasive and suppressed useful code
actions like quick fixes and organize imports. Maybe some refactoring
code actions and source.* actions can be also useful enough to be
presented in the light bulb menu.

Partially revert CL 587555, by suppress only refactor.inline code
action if the user's intention is not refactoring
(e.g. cursor move around a function call). However, if the user
selected a code area (function name), that is likely an action for
refactoring, so continue to present refactor.inline in the light bulb
menu.

Fixes golang/go#67823
Fixes golang/go#65167

Change-Id: I2d4a3d92199e501103596d0aed78ece34760149f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/590935
Auto-Submit: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopls/internal/golang/codeaction.go

@@ -28,7 +28,7 @@ import (
 
 // 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) (actions []protocol.CodeAction, _ error) {
+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) {
 	// Only compute quick fixes if there are any diagnostics to fix.
 	wantQuickFixes := want[protocol.QuickFix] && len(diagnostics) > 0
 
@@ -138,7 +138,7 @@ func CodeActions(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 			actions = append(actions, rewrites...)
 		}
 
-		if want[protocol.RefactorInline] {
+		if want[protocol.RefactorInline] && (triggerKind != protocol.CodeActionAutomatic || rng.Start != rng.End) {
 			rewrites, err := getInlineCodeActions(pkg, pgf, rng, snapshot.Options())
 			if err != nil {
 				return nil, err

gopls/internal/server/code_action.go

@@ -113,25 +113,22 @@ func (s *server) CodeAction(ctx context.Context, params *protocol.CodeActionPara
 		//
 		// The diagnostics already have a UI presence (e.g. squiggly underline);
 		// the associated action may additionally show (in VS Code) as a lightbulb.
+		// Note s.codeActionsMatchingDiagnostics returns only fixes
+		// detected during the analysis phase. golang.CodeActions computes
+		// extra changes that can address some diagnostics.
 		actions, err := s.codeActionsMatchingDiagnostics(ctx, uri, snapshot, params.Context.Diagnostics, want)
 		if err != nil {
 			return nil, err
 		}
-
+		var triggerKind protocol.CodeActionTriggerKind
-		// non-diagnostic code actions (non-problematic)
+		if k := params.Context.TriggerKind; k != nil {
-		//
+			triggerKind = *k
-		// Don't report these for mere cursor motion (trigger=Automatic), only
+		}
-		// when the menu is opened, to avoid a distracting lightbulb in VS Code.
+		moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want, triggerKind)
-		// (See protocol/codeactionkind.go for background.)
-		//
-		// Some clients (e.g. eglot) do not set TriggerKind at all.
-		if k := params.Context.TriggerKind; k == nil || *k != protocol.CodeActionAutomatic {
-			moreActions, err := golang.CodeActions(ctx, snapshot, fh, params.Range, params.Context.Diagnostics, want)
 		if err != nil {
 			return nil, err
 		}
 		actions = append(actions, moreActions...)
-		}
 
 		// Don't suggest fixes for generated files, since they are generally
 		// not useful and some editors may apply them automatically on save.

gopls/internal/test/integration/fake/editor.go

@@ -1559,6 +1559,10 @@ func (e *Editor) ChangeWorkspaceFolders(ctx context.Context, folders []string) e
 // 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) {
 	if e.Server == nil {
 		return nil, nil
 	}
@@ -1573,6 +1577,7 @@ func (e *Editor) CodeAction(ctx context.Context, loc protocol.Location, diagnost
 		TextDocument: e.TextDocumentIdentifier(path),
 		Context: protocol.CodeActionContext{
 			Diagnostics: diagnostics,
+			TriggerKind: triggerKind,
 		},
 		Range: loc.Range, // may be zero
 	}

gopls/internal/test/integration/misc/codeactions_test.go

@@ -5,11 +5,13 @@
 package misc
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"golang.org/x/tools/gopls/internal/protocol"
 	. "golang.org/x/tools/gopls/internal/test/integration"
+	"golang.org/x/tools/gopls/internal/util/slices"
 )
 
 // This test exercises the filtering of code actions in generated files.
@@ -70,3 +72,46 @@ func g() {}
 			protocol.GoFreeSymbols)
 	})
 }
+
+// Test refactor.inline is not included in automatically triggered code action
+// unless users want refactoring.
+func TestVSCodeIssue65167(t *testing.T) {
+	const vim1 = `package main
+
+func main() {
+	Func()  // range to be selected
+}
+
+func Func() int { return 0 }
+`
+
+	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 _, 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
+						if !selectedRange {
+							// assume the cursor is placed at the beginning of `Func`, so end==start.
+							rng.End = rng.Start
+						}
+						loc := protocol.Location{URI: pattern.URI, Range: rng}
+						actions := env.CodeAction0("main.go", loc, nil, triggerKindPtr)
+						want := triggerKind != protocol.CodeActionAutomatic || selectedRange
+						if got := slices.ContainsFunc(actions, func(act protocol.CodeAction) bool {
+							return act.Kind == protocol.RefactorInline
+						}); got != want {
+							t.Errorf("got refactor.inline = %t, want %t", got, want)
+						}
+					})
+				}
+			})
+		}
+	})
+}

gopls/internal/test/integration/wrappers.go

@@ -536,9 +536,13 @@ func (e *Env) AcceptCompletion(loc protocol.Location, item protocol.CompletionIt
 // 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 {
-	e.T.Helper()
 	loc := protocol.Location{URI: e.Sandbox.Workdir.URI(path)} // no Range => whole file
-	actions, err := e.Editor.CodeAction(e.Ctx, loc, diagnostics)
+	return e.CodeAction0(path, loc, diagnostics, nil)
+}
+
+func (e *Env) CodeAction0(path string, loc protocol.Location, diagnostics []protocol.Diagnostic, triggerKind *protocol.CodeActionTriggerKind) []protocol.CodeAction {
+	e.T.Helper()
+	actions, err := e.Editor.CodeAction0(e.Ctx, loc, diagnostics, triggerKind)
 	if err != nil {
 		e.T.Fatal(err)
 	}