gopls/internal/golang: change signature via renaming 'func'

More for testing and debugging than anything else, allow changing
signatures by invoking a renaming on the 'func' keyword.

For now, this is still restricted to parameter permutation or removal.

For golang/go#38028

Change-Id: I62e387cc7d7f46fc892b7b20050d99fac6800e3f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/631296
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

gopls/internal/golang/change_signature.go

@@ -146,7 +146,6 @@ func removeParam(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle,
 //   - Stream type checking via ForEachPackage.
 //   - Avoid unnecessary additional type checking.
 func ChangeSignature(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.Package, pgf *parsego.File, rng protocol.Range, newParams []int) ([]protocol.DocumentChange, error) {
-
 	// Changes to our heuristics for whether we can remove a parameter must also
 	// be reflected in the canRemoveParameter helper.
 	if perrors, terrors := pkg.ParseErrors(), pkg.TypeErrors(); len(perrors) > 0 || len(terrors) > 0 {
@@ -160,8 +159,8 @@ func ChangeSignature(ctx context.Context, snapshot *cache.Snapshot, pkg *cache.P
 	}
 
 	info := findParam(pgf, rng)
-	if info == nil || info.field == nil {
-		return nil, fmt.Errorf("failed to find field")
+	if info == nil || info.decl == nil {
+		return nil, fmt.Errorf("failed to find declaration")
 	}
 
 	// Step 1: create the new declaration, which is a copy of the original decl
@@ -437,9 +436,12 @@ func findParam(pgf *parsego.File, rng protocol.Range) *paramInfo {
 			info.decl = n
 		}
 	}
-	if info.decl == nil || field == nil {
+	if info.decl == nil {
 		return nil
 	}
+	if field == nil {
+		return &info
+	}
 	pi := 0
 	// Search for field and id among parameters of decl.
 	// This search may fail, even if one or both of id and field are non nil:

gopls/internal/golang/rename.go

@@ -42,10 +42,13 @@ package golang
 // - FileID-based de-duplication of edits to different URIs for the same file.
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
 	"go/ast"
+	"go/parser"
+	"go/printer"
 	"go/token"
 	"go/types"
 	"path"
@@ -64,8 +67,10 @@ import (
 	"golang.org/x/tools/gopls/internal/cache/parsego"
 	"golang.org/x/tools/gopls/internal/file"
 	"golang.org/x/tools/gopls/internal/protocol"
+	goplsastutil "golang.org/x/tools/gopls/internal/util/astutil"
 	"golang.org/x/tools/gopls/internal/util/bug"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
+	internalastutil "golang.org/x/tools/internal/astutil"
 	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/typesinternal"
@@ -126,6 +131,15 @@ func PrepareRename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle,
 	if err != nil {
 		return nil, nil, err
 	}
+
+	// Check if we're in a 'func' keyword. If so, we hijack the renaming to
+	// change the function signature.
+	if item, err := prepareRenameFuncSignature(pgf, pos); err != nil {
+		return nil, nil, err
+	} else if item != nil {
+		return item, nil, nil
+	}
+
 	targets, node, err := objectsAt(pkg.TypesInfo(), pgf.File, pos)
 	if err != nil {
 		return nil, nil, err
@@ -193,6 +207,169 @@ func prepareRenamePackageName(ctx context.Context, snapshot *cache.Snapshot, pgf
 	}, nil
 }
 
+// prepareRenameFuncSignature prepares a change signature refactoring initiated
+// through invoking a rename request at the 'func' keyword of a function
+// declaration.
+//
+// The resulting text is the signature of the function, which may be edited to
+// the new signature.
+func prepareRenameFuncSignature(pgf *parsego.File, pos token.Pos) (*PrepareItem, error) {
+	fdecl := funcKeywordDecl(pgf, pos)
+	if fdecl == nil {
+		return nil, nil
+	}
+	ftyp := nameBlankParams(fdecl.Type)
+	var buf bytes.Buffer
+	if err := printer.Fprint(&buf, token.NewFileSet(), ftyp); err != nil { // use a new fileset so that the signature is formatted on a single line
+		return nil, err
+	}
+	rng, err := pgf.PosRange(ftyp.Func, ftyp.Func+token.Pos(len("func")))
+	if err != nil {
+		return nil, err
+	}
+	text := buf.String()
+	return &PrepareItem{
+		Range: rng,
+		Text:  text,
+	}, nil
+}
+
+// nameBlankParams returns a copy of ftype with blank or unnamed params
+// assigned a unique name.
+func nameBlankParams(ftype *ast.FuncType) *ast.FuncType {
+	ftype = internalastutil.CloneNode(ftype)
+
+	// First, collect existing names.
+	scope := make(map[string]bool)
+	for name := range goplsastutil.FlatFields(ftype.Params) {
+		if name != nil {
+			scope[name.Name] = true
+		}
+	}
+	blanks := 0
+	for name, field := range goplsastutil.FlatFields(ftype.Params) {
+		if name == nil {
+			name = ast.NewIdent("_")
+			field.Names = append(field.Names, name) // ok to append
+		}
+		if name.Name == "" || name.Name == "_" {
+			for {
+				newName := fmt.Sprintf("_%d", blanks)
+				blanks++
+				if !scope[newName] {
+					name.Name = newName
+					break
+				}
+			}
+		}
+	}
+	return ftype
+}
+
+// renameFuncSignature computes and applies the effective change signature
+// operation resulting from a 'renamed' (=rewritten) signature.
+func renameFuncSignature(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp protocol.Position, newName string) (map[protocol.DocumentURI][]protocol.TextEdit, error) {
+	// Find the renamed signature.
+	pkg, pgf, err := NarrowestPackageForFile(ctx, snapshot, f.URI())
+	if err != nil {
+		return nil, err
+	}
+	pos, err := pgf.PositionPos(pp)
+	if err != nil {
+		return nil, err
+	}
+	fdecl := funcKeywordDecl(pgf, pos)
+	if fdecl == nil {
+		return nil, nil
+	}
+	ftyp := nameBlankParams(fdecl.Type)
+
+	// Parse the user's requested new signature.
+	parsed, err := parser.ParseExpr(newName)
+	if err != nil {
+		return nil, err
+	}
+	newType, _ := parsed.(*ast.FuncType)
+	if newType == nil {
+		return nil, fmt.Errorf("parsed signature is %T, not a function type", parsed)
+	}
+
+	// Check results, before we get into handling permutations of parameters.
+	if got, want := newType.Results.NumFields(), ftyp.Results.NumFields(); got != want {
+		return nil, fmt.Errorf("changing results not yet supported (got %d results, want %d)", got, want)
+	}
+	var resultTypes []string
+	for _, field := range goplsastutil.FlatFields(ftyp.Results) {
+		resultTypes = append(resultTypes, FormatNode(token.NewFileSet(), field.Type))
+	}
+	resultIndex := 0
+	for _, field := range goplsastutil.FlatFields(newType.Results) {
+		if FormatNode(token.NewFileSet(), field.Type) != resultTypes[resultIndex] {
+			return nil, fmt.Errorf("changing results not yet supported")
+		}
+		resultIndex++
+	}
+
+	type paramInfo struct {
+		idx int
+		typ string
+	}
+	oldParams := make(map[string]paramInfo)
+	for name, field := range goplsastutil.FlatFields(ftyp.Params) {
+		oldParams[name.Name] = paramInfo{
+			idx: len(oldParams),
+			typ: types.ExprString(field.Type),
+		}
+	}
+
+	var newParams []int
+	for name, field := range goplsastutil.FlatFields(newType.Params) {
+		if name == nil {
+			return nil, fmt.Errorf("need named fields")
+		}
+		info, ok := oldParams[name.Name]
+		if !ok {
+			return nil, fmt.Errorf("couldn't find name %s: adding parameters not yet supported", name)
+		}
+		if newType := types.ExprString(field.Type); newType != info.typ {
+			return nil, fmt.Errorf("changing types (%s to %s) not yet supported", info.typ, newType)
+		}
+		newParams = append(newParams, info.idx)
+	}
+
+	rng, err := pgf.PosRange(ftyp.Func, ftyp.Func)
+	if err != nil {
+		return nil, err
+	}
+	changes, err := ChangeSignature(ctx, snapshot, pkg, pgf, rng, newParams)
+	if err != nil {
+		return nil, err
+	}
+	transposed := make(map[protocol.DocumentURI][]protocol.TextEdit)
+	for _, change := range changes {
+		transposed[change.TextDocumentEdit.TextDocument.URI] = protocol.AsTextEdits(change.TextDocumentEdit.Edits)
+	}
+	return transposed, nil
+}
+
+// funcKeywordDecl returns the FuncDecl for which pos is in the 'func' keyword,
+// if any.
+func funcKeywordDecl(pgf *parsego.File, pos token.Pos) *ast.FuncDecl {
+	path, _ := astutil.PathEnclosingInterval(pgf.File, pos, pos)
+	if len(path) < 1 {
+		return nil
+	}
+	fdecl, _ := path[0].(*ast.FuncDecl)
+	if fdecl == nil {
+		return nil
+	}
+	ftyp := fdecl.Type
+	if pos < ftyp.Func || pos > ftyp.Func+token.Pos(len("func")) { // tolerate renaming immediately after 'func'
+		return nil
+	}
+	return fdecl
+}
+
 func checkRenamable(obj types.Object) error {
 	switch obj := obj.(type) {
 	case *types.Var:
@@ -219,6 +396,12 @@ func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp pro
 	ctx, done := event.Start(ctx, "golang.Rename")
 	defer done()
 
+	if edits, err := renameFuncSignature(ctx, snapshot, f, pp, newName); err != nil {
+		return nil, false, err
+	} else if edits != nil {
+		return edits, false, nil
+	}
+
 	if !isValidIdentifier(newName) {
 		return nil, false, fmt.Errorf("invalid identifier to rename: %q", newName)
 	}

gopls/internal/test/marker/doc.go

@@ -226,10 +226,11 @@ Here is the list of supported action markers:
     callHierarchy/outgoingCalls query at the src location, and checks that
     the set of call.To locations matches want.
 
-  - preparerename(src, spn, placeholder): asserts that a textDocument/prepareRename
-    request at the src location expands to the spn location, with given
-    placeholder. If placeholder is "", this is treated as a negative
-    assertion and prepareRename should return nil.
+  - preparerename(src location, placeholder string, span=location): asserts
+    that a textDocument/prepareRename request at the src location has the given
+    placeholder text. If present, the optional span argument is verified to be
+    the span of the prepareRename result. If placeholder is "", this is treated
+    as a negative assertion and prepareRename should return nil.
 
   - quickfix(location, regexp, golden): like diag, the location and
     regexp identify an expected diagnostic, which must have exactly one

gopls/internal/test/marker/marker_test.go

@@ -513,7 +513,12 @@ func namedArg[T any](mark marker, name string, dflt T) T {
 		if e, ok := v.(T); ok {
 			return e
 		} else {
-			mark.errorf("invalid value for %q: %v", name, v)
+			v, err := convert(mark, v, reflect.TypeOf(dflt))
+			if err != nil {
+				mark.errorf("invalid value for %q: could not convert %v (%T) to %T", name, v, v, dflt)
+				return dflt
+			}
+			return v.(T)
 		}
 	}
 	return dflt
@@ -579,7 +584,7 @@ var actionMarkerFuncs = map[string]func(marker){
 	"incomingcalls":    actionMarkerFunc(incomingCallsMarker),
 	"inlayhints":       actionMarkerFunc(inlayhintsMarker),
 	"outgoingcalls":    actionMarkerFunc(outgoingCallsMarker),
-	"preparerename":    actionMarkerFunc(prepareRenameMarker),
+	"preparerename":    actionMarkerFunc(prepareRenameMarker, "span"),
 	"rank":             actionMarkerFunc(rankMarker),
 	"refs":             actionMarkerFunc(refsMarker),
 	"rename":           actionMarkerFunc(renameMarker),
@@ -2474,7 +2479,7 @@ func inlayhintsMarker(mark marker, g *Golden) {
 	compareGolden(mark, got, g)
 }
 
-func prepareRenameMarker(mark marker, src, spn protocol.Location, placeholder string) {
+func prepareRenameMarker(mark marker, src protocol.Location, placeholder string) {
 	params := &protocol.PrepareRenameParams{
 		TextDocumentPositionParams: protocol.LocationTextDocumentPositionParams(src),
 	}
@@ -2488,7 +2493,15 @@ func prepareRenameMarker(mark marker, src, spn protocol.Location, placeholder st
 		}
 		return
 	}
-	want := &protocol.PrepareRenameResult{Range: spn.Range, Placeholder: placeholder}
+
+	want := &protocol.PrepareRenameResult{
+		Placeholder: placeholder,
+	}
+	if span := namedArg(mark, "span", protocol.Location{}); span != (protocol.Location{}) {
+		want.Range = span.Range
+	} else {
+		got.Range = protocol.Range{} // ignore Range
+	}
 	if diff := cmp.Diff(want, got); diff != "" {
 		mark.errorf("mismatching PrepareRename result:\n%s", diff)
 	}

gopls/internal/test/marker/testdata/rename/func.txt

@@ -0,0 +1,55 @@
+This test checks basic functionality for renaming (=changing) a function
+signature.
+
+-- go.mod --
+module example.com
+
+go 1.20
+
+-- a/a.go --
+package a
+
+//@rename(Foo, "func(i int, s string)", unchanged)
+//@rename(Foo, "func(s string, i int)", reverse)
+//@rename(Foo, "func(s string)", dropi)
+//@rename(Foo, "func(i int)", drops)
+//@rename(Foo, "func()", dropboth)
+//@renameerr(Foo, "func(i int, s string, t bool)", "not yet supported")
+//@renameerr(Foo, "func(i string)", "not yet supported")
+//@renameerr(Foo, "func(i int, s string) int", "not yet supported")
+
+func Foo(i int, s string) { //@loc(Foo, "func")
+}
+
+func _() {
+	Foo(0, "hi")
+}
+-- @dropboth/a/a.go --
+@@ -12 +12 @@
+-func Foo(i int, s string) { //@loc(Foo, "func")
++func Foo() { //@loc(Foo, "func")
+@@ -16 +16 @@
+-	Foo(0, "hi")
++	Foo()
+-- @dropi/a/a.go --
+@@ -12 +12 @@
+-func Foo(i int, s string) { //@loc(Foo, "func")
++func Foo(s string) { //@loc(Foo, "func")
+@@ -16 +16 @@
+-	Foo(0, "hi")
++	Foo("hi")
+-- @drops/a/a.go --
+@@ -12 +12 @@
+-func Foo(i int, s string) { //@loc(Foo, "func")
++func Foo(i int) { //@loc(Foo, "func")
+@@ -16 +16 @@
+-	Foo(0, "hi")
++	Foo(0)
+-- @reverse/a/a.go --
+@@ -12 +12 @@
+-func Foo(i int, s string) { //@loc(Foo, "func")
++func Foo(s string, i int) { //@loc(Foo, "func")
+@@ -16 +16 @@
+-	Foo(0, "hi")
++	Foo("hi", 0)
+-- @unchanged/a/a.go --

gopls/internal/test/marker/testdata/rename/issue43616.txt

@@ -4,15 +4,15 @@ fields.
 -- p.go --
 package issue43616
 
-type foo int //@rename("foo", "bar", fooToBar),preparerename("oo","foo","foo")
+type foo int //@rename("foo", "bar", fooToBar),preparerename("oo","foo",span="foo")
 
 var x struct{ foo } //@renameerr("foo", "baz", "rename the type directly")
 
 var _ = x.foo //@renameerr("foo", "quux", "rename the type directly")
 -- @fooToBar/p.go --
 @@ -3 +3 @@
--type foo int //@rename("foo", "bar", fooToBar),preparerename("oo","foo","foo")
-+type bar int //@rename("foo", "bar", fooToBar),preparerename("oo","foo","foo")
+-type foo int //@rename("foo", "bar", fooToBar),preparerename("oo","foo",span="foo")
++type bar int //@rename("foo", "bar", fooToBar),preparerename("oo","foo",span="foo")
 @@ -5 +5 @@
 -var x struct{ foo } //@renameerr("foo", "baz", "rename the type directly")
 +var x struct{ bar } //@renameerr("foo", "baz", "rename the type directly")