gopls/internal/lsp: add code actions to remove unused parameters

Use the new inlining technology to implement a first change-signature
refactoring, by rewriting the declaration to be a trivial wrapper around
a declaration with modified signature, inlining the wrapper, and then
performing string substitution to replace references to the synthetic
delegate.

This demonstrates the power of inlining, which can be seen as a more
general tool for rewriting source code: express old code as new code,
recognize instances of old code (in this case, calls), and inline.

However, this CL was still rather difficult, primarily because of (1)
the problem of manipulating syntax without breaking formatting and
comments, and (2) the problem of composing multiple refactoring
operations, which in general requires iterative type checking.

Neither of those difficulties have general solutions: any form of
nontrivial AST manipulation tends to result in an unacceptable movement
of comments, and iterative type checking is difficult because it may
involve an arbitrarily modified package graph, and because it is
difficult to correlate references in the previous version of the package
with references in the new version of the package.

But in the case of removing a parameter, these problems are solvable: We
can avoid most AST mangling by restricting the scope of rewriting to the
function signature. We can type check the new package using the imports
of the old package. We can find the next reference in the new package by
counting.

Fixes golang/go#63534

Change-Id: Iba5fa6b0da503b7723bea1b43fd2c4151576a672
Reviewed-on: https://go-review.googlesource.com/c/tools/+/532495
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

Author: findleyr

gopls/doc/commands.md

@@ -84,6 +84,26 @@ Args:
 }
 ```
 
+### **performs a "change signature" refactoring.**
+Identifier: `gopls.change_signature`
+
+This command is experimental, currently only supporting parameter removal.
+Its signature will certainly change in the future (pun intended).
+
+Args:
+
+```
+{
+	"RemoveParameter": {
+		"uri": string,
+		"range": {
+			"start": { ... },
+			"end": { ... },
+		},
+	},
+}
+```
+
 ### **Check for upgrades**
 Identifier: `gopls.check_upgrades`
 

gopls/internal/lsp/analysis/unusedparams/cmd/main.go

@@ -0,0 +1,13 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// The stringintconv command runs the stringintconv analyzer.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/singlechecker"
+	"golang.org/x/tools/gopls/internal/lsp/analysis/unusedparams"
+)
+
+func main() { singlechecker.Main(unusedparams.Analyzer) }

gopls/internal/lsp/analysis/unusedparams/unusedparams.go

@@ -28,11 +28,20 @@ To reduce false positives it ignores:
 - functions in test files
 - functions with empty bodies or those with just a return stmt`
 
-var Analyzer = &analysis.Analyzer{
-	Name:     "unusedparams",
-	Doc:      Doc,
-	Requires: []*analysis.Analyzer{inspect.Analyzer},
-	Run:      run,
+var (
+	Analyzer = &analysis.Analyzer{
+		Name:     "unusedparams",
+		Doc:      Doc,
+		Requires: []*analysis.Analyzer{inspect.Analyzer},
+		Run:      run,
+	}
+	inspectLits     bool
+	inspectWrappers bool
+)
+
+func init() {
+	Analyzer.Flags.BoolVar(&inspectLits, "lits", true, "inspect function literals")
+	Analyzer.Flags.BoolVar(&inspectWrappers, "wrappers", false, "inspect functions whose body consists of a single return statement")
 }
 
 type paramData struct {
@@ -45,7 +54,9 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	nodeFilter := []ast.Node{
 		(*ast.FuncDecl)(nil),
-		(*ast.FuncLit)(nil),
+	}
+	if inspectLits {
+		nodeFilter = append(nodeFilter, (*ast.FuncLit)(nil))
 	}
 
 	inspect.Preorder(nodeFilter, func(n ast.Node) {
@@ -62,6 +73,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			if f.Recv != nil {
 				return
 			}
+
 			// Ignore functions in _test.go files to reduce false positives.
 			if file := pass.Fset.File(n.Pos()); file != nil && strings.HasSuffix(file.Name(), "_test.go") {
 				return
@@ -76,8 +88,10 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		switch expr := body.List[0].(type) {
 		case *ast.ReturnStmt:
-			// Ignore functions that only contain a return statement to reduce false positives.
-			return
+			if !inspectWrappers {
+				// Ignore functions that only contain a return statement to reduce false positives.
+				return
+			}
 		case *ast.ExprStmt:
 			callExpr, ok := expr.X.(*ast.CallExpr)
 			if !ok || len(body.List) > 1 {

gopls/internal/lsp/cache/check.go

@@ -1487,6 +1487,7 @@ func typeCheckImpl(ctx context.Context, b *typeCheckBatch, inputs typeCheckInput
 	return pkg, nil
 }
 
+// TODO(golang/go#63472): this looks wrong with the new Go version syntax.
 var goVersionRx = regexp.MustCompile(`^go([1-9][0-9]*)\.(0|[1-9][0-9]*)$`)
 
 func doTypeCheck(ctx context.Context, b *typeCheckBatch, inputs typeCheckInputs) (*syntaxPackage, error) {

gopls/internal/lsp/code_action.go

@@ -430,6 +430,26 @@ func refactorRewrite(ctx context.Context, snapshot source.Snapshot, pkg source.P
 			rerr = bug.Errorf("refactor.rewrite code actions panicked: %v", r)
 		}
 	}()
+
+	var actions []protocol.CodeAction
+
+	if canRemoveParameter(pkg, pgf, rng) {
+		cmd, err := command.NewChangeSignatureCommand("remove unused parameter", command.ChangeSignatureArgs{
+			RemoveParameter: protocol.Location{
+				URI:   protocol.URIFromSpanURI(pgf.URI),
+				Range: rng,
+			},
+		})
+		if err != nil {
+			return nil, err
+		}
+		actions = append(actions, protocol.CodeAction{
+			Title:   "Refactor: remove unused parameter",
+			Kind:    protocol.RefactorRewrite,
+			Command: &cmd,
+		})
+	}
+
 	start, end, err := pgf.RangePos(rng)
 	if err != nil {
 		return nil, err
@@ -471,7 +491,6 @@ func refactorRewrite(ctx context.Context, snapshot source.Snapshot, pkg source.P
 		}
 	}
 
-	var actions []protocol.CodeAction
 	for i := range commands {
 		actions = append(actions, protocol.CodeAction{
 			Title:   commands[i].Title,
@@ -510,6 +529,43 @@ func refactorRewrite(ctx context.Context, snapshot source.Snapshot, pkg source.P
 	return actions, nil
 }
 
+// canRemoveParameter reports whether we can remove the function parameter
+// indicated by the given [start, end) range.
+//
+// This is true if:
+//   - [start, end) is contained within an unused field or parameter name
+//   - ... of a non-method function declaration.
+func canRemoveParameter(pkg source.Package, pgf *source.ParsedGoFile, rng protocol.Range) bool {
+	info := source.FindParam(pgf, rng)
+	if info.Decl == nil || info.Field == nil {
+		return false
+	}
+
+	if len(info.Field.Names) == 0 {
+		return true // no names => field is unused
+	}
+	if info.Name == nil {
+		return false // no name is indicated
+	}
+	if info.Name.Name == "_" {
+		return true // trivially unused
+	}
+
+	obj := pkg.GetTypesInfo().Defs[info.Name]
+	if obj == nil {
+		return false // something went wrong
+	}
+
+	used := false
+	ast.Inspect(info.Decl.Body, func(node ast.Node) bool {
+		if n, ok := node.(*ast.Ident); ok && pkg.GetTypesInfo().Uses[n] == obj {
+			used = true
+		}
+		return !used // keep going until we find a use
+	})
+	return !used
+}
+
 // refactorInline returns inline actions available at the specified range.
 func refactorInline(ctx context.Context, snapshot source.Snapshot, pkg source.Package, pgf *source.ParsedGoFile, fh source.FileHandle, rng protocol.Range) ([]protocol.CodeAction, error) {
 	var commands []protocol.Command

gopls/internal/lsp/command.go

@@ -1210,3 +1210,25 @@ func showDocumentImpl(ctx context.Context, cli protocol.Client, url protocol.URI
 		event.Log(ctx, fmt.Sprintf("client declined to open document %v", url))
 	}
 }
+
+func (c *commandHandler) ChangeSignature(ctx context.Context, args command.ChangeSignatureArgs) error {
+	return c.run(ctx, commandConfig{
+		forURI: args.RemoveParameter.URI,
+	}, func(ctx context.Context, deps commandDeps) error {
+		// For now, gopls only supports removing unused parameters.
+		changes, err := source.RemoveUnusedParameter(ctx, deps.fh, args.RemoveParameter.Range, deps.snapshot)
+		if err != nil {
+			return err
+		}
+		r, err := c.s.client.ApplyEdit(ctx, &protocol.ApplyWorkspaceEditParams{
+			Edit: protocol.WorkspaceEdit{
+				DocumentChanges: changes,
+			},
+		})
+		if !r.Applied {
+			return fmt.Errorf("failed to apply edits: %v", r.FailureReason)
+		}
+
+		return nil
+	})
+}

gopls/internal/lsp/command/command_gen.go

@@ -201,6 +201,12 @@ type Interface interface {
 	// the user to ask if they want to enable Go telemetry uploading. If the user
 	// responds 'Yes', the telemetry mode is set to "on".
 	MaybePromptForTelemetry(context.Context) error
+
+	// ChangeSignature: performs a "change signature" refactoring.
+	//
+	// This command is experimental, currently only supporting parameter removal.
+	// Its signature will certainly change in the future (pun intended).
+	ChangeSignature(context.Context, ChangeSignatureArgs) error
 }
 
 type RunTestsArgs struct {
@@ -519,3 +525,8 @@ type AddTelemetryCountersArgs struct {
 	Names  []string // Name of counters.
 	Values []int64  // Values added to the corresponding counters. Must be non-negative.
 }
+
+// ChangeSignatureArgs specifies a "change signature" refactoring to perform.
+type ChangeSignatureArgs struct {
+	RemoveParameter protocol.Location
+}

gopls/internal/lsp/command/interface.go

@@ -1001,6 +1001,9 @@ func formatTest(test *markerTest) ([]byte, error) {
 	// ...followed by any new golden files.
 	var newGoldenFiles []txtar.File
 	for filename, data := range updatedGolden {
+		// TODO(rfindley): it looks like this implicitly removes trailing newlines
+		// from golden content. Is there any way to fix that? Perhaps we should
+		// just make the diff tolerant of missing newlines?
 		newGoldenFiles = append(newGoldenFiles, txtar.File{Name: filename, Data: data})
 	}
 	// Sort new golden files lexically.
@@ -2047,7 +2050,7 @@ func codeAction(env *Env, uri protocol.DocumentURI, rng protocol.Range, actionKi
 			Command:   action.Command.Command,
 			Arguments: action.Command.Arguments,
 		}); err != nil {
-			env.T.Fatalf("error converting command %q to edits: %v", action.Command.Command, err)
+			return nil, err
 		}
 
 		if err := applyDocumentChanges(env, env.Awaiter.takeDocumentChanges(), fileChanges); err != nil {