From b93a56f289b07a90bd39c92c8d470f9c21d2623d Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 14 Oct 2022 12:21:42 -0400 Subject: [PATCH] refactor/satisfy: fix visiting functions in the unsafe package Unlike other calls of qualified functions, the type of certain functions in the unsafe package (namely Slice, Add) is not constant, and cannot be determined by the types.Func being called. Update Finder.expr to pre-emptively handle calls to functions in the unsafe package, similarly to how it handles other builtins. Fixes golang/go#56227 Change-Id: I7af51c1ecacbdf35e39c8e7b8273ffe6f953427f Reviewed-on: https://go-review.googlesource.com/c/tools/+/443096 Reviewed-by: Alan Donovan Run-TryBot: Robert Findley TryBot-Result: Gopher Robot --- gopls/internal/lsp/cmd/test/rename.go | 9 ++++--- gopls/internal/regtest/misc/rename_test.go | 30 ++++++++++++++++++++++ refactor/satisfy/find.go | 22 +++++++++++++--- refactor/satisfy/find_test.go | 16 ++++++++++-- 4 files changed, 67 insertions(+), 10 deletions(-) diff --git a/gopls/internal/lsp/cmd/test/rename.go b/gopls/internal/lsp/cmd/test/rename.go index 0750d70281d..a9eb31e3877 100644 --- a/gopls/internal/lsp/cmd/test/rename.go +++ b/gopls/internal/lsp/cmd/test/rename.go @@ -8,6 +8,7 @@ import ( "fmt" "testing" + "golang.org/x/tools/gopls/internal/lsp/tests/compare" "golang.org/x/tools/gopls/internal/span" ) @@ -17,13 +18,13 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { loc := fmt.Sprintf("%v", spn) got, err := r.NormalizeGoplsCmd(t, "rename", loc, newText) got += err - expect := string(r.data.Golden(t, goldenTag, filename, func() ([]byte, error) { + want := string(r.data.Golden(t, goldenTag, filename, func() ([]byte, error) { return []byte(got), nil })) - if expect != got { - t.Errorf("rename failed with %v %v\nexpected:\n%s\ngot:\n%s", loc, newText, expect, got) + if diff := compare.Text(want, got); diff != "" { + t.Errorf("rename failed with %v %v (-want +got):\n%s", loc, newText, diff) } // now check we can build a valid unified diff unified, _ := r.NormalizeGoplsCmd(t, "rename", "-d", loc, newText) - checkUnified(t, filename, expect, unified) + checkUnified(t, filename, want, unified) } diff --git a/gopls/internal/regtest/misc/rename_test.go b/gopls/internal/regtest/misc/rename_test.go index 1f6ee1c2b85..aa0a47ad7b8 100644 --- a/gopls/internal/regtest/misc/rename_test.go +++ b/gopls/internal/regtest/misc/rename_test.go @@ -52,6 +52,36 @@ func main() { }) } +// Test case for golang/go#56227 +func TestRenameWithUnsafeSlice(t *testing.T) { + testenv.NeedsGo1Point(t, 17) // unsafe.Slice was added in Go 1.17 + const files = ` +-- go.mod -- +module mod.com + +go 1.18 +-- p.go -- +package p + +import "unsafe" + +type T struct{} + +func (T) M() {} + +func _() { + x := [3]int{1, 2, 3} + ptr := unsafe.Pointer(&x) + _ = unsafe.Slice((*int)(ptr), 3) +} +` + + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("p.go") + env.Rename("p.go", env.RegexpSearch("p.go", "M"), "N") // must not panic + }) +} + func TestPrepareRenameWithNoPackageDeclaration(t *testing.T) { testenv.NeedsGo1Point(t, 15) const files = ` diff --git a/refactor/satisfy/find.go b/refactor/satisfy/find.go index aacb56bce8b..6b4d5284aec 100644 --- a/refactor/satisfy/find.go +++ b/refactor/satisfy/find.go @@ -198,7 +198,8 @@ func (f *Finder) call(sig *types.Signature, args []ast.Expr) { } } -func (f *Finder) builtin(obj *types.Builtin, sig *types.Signature, args []ast.Expr, T types.Type) types.Type { +// builtin visits the arguments of a builtin type with signature sig. +func (f *Finder) builtin(obj *types.Builtin, sig *types.Signature, args []ast.Expr) { switch obj.Name() { case "make", "new": // skip the type operand @@ -228,8 +229,6 @@ func (f *Finder) builtin(obj *types.Builtin, sig *types.Signature, args []ast.Ex // ordinary call f.call(sig, args) } - - return T } func (f *Finder) extract(tuple types.Type, i int) types.Type { @@ -439,12 +438,27 @@ func (f *Finder) expr(e ast.Expr) types.Type { f.assign(tvFun.Type, arg0) } else { // function call + + // unsafe call. Treat calls to functions in unsafe like ordinary calls, + // except that their signature cannot be determined by their func obj. + // Without this special handling, f.expr(e.Fun) would fail below. + if s, ok := unparen(e.Fun).(*ast.SelectorExpr); ok { + if obj, ok := f.info.Uses[s.Sel].(*types.Builtin); ok && obj.Pkg().Path() == "unsafe" { + sig := f.info.Types[e.Fun].Type.(*types.Signature) + f.call(sig, e.Args) + return tv.Type + } + } + + // builtin call if id, ok := unparen(e.Fun).(*ast.Ident); ok { if obj, ok := f.info.Uses[id].(*types.Builtin); ok { sig := f.info.Types[id].Type.(*types.Signature) - return f.builtin(obj, sig, e.Args, tv.Type) + f.builtin(obj, sig, e.Args) + return tv.Type } } + // ordinary call f.call(coreType(f.expr(e.Fun)).(*types.Signature), e.Args) } diff --git a/refactor/satisfy/find_test.go b/refactor/satisfy/find_test.go index 234bce905d3..35a1e87caf4 100644 --- a/refactor/satisfy/find_test.go +++ b/refactor/satisfy/find_test.go @@ -7,6 +7,7 @@ package satisfy_test import ( "fmt" "go/ast" + "go/importer" "go/parser" "go/token" "go/types" @@ -27,6 +28,8 @@ func TestGenericCoreOperations(t *testing.T) { const src = `package foo +import "unsafe" + type I interface { f() } type impl struct{} @@ -53,6 +56,7 @@ type R struct{impl} type S struct{impl} type T struct{impl} type U struct{impl} +type V struct{impl} type Generic[T any] struct{impl} func (Generic[T]) g(T) {} @@ -153,8 +157,13 @@ func _[T any]() { type Gen2[T any] struct{} func (f Gen2[T]) g(string) { global = f } // GI[string] <- Gen2[T] -var global GI[string] +var global GI[string] +func _() { + var x [3]V + // golang/go#56227: the finder should visit calls in the unsafe package. + _ = unsafe.Slice(&x[0], func() int { var _ I = x[0]; return 3 }()) // I <- V +} ` got := constraints(t, src) want := []string{ @@ -184,6 +193,7 @@ var global GI[string] "p.I <- p.S", "p.I <- p.T", "p.I <- p.U", + "p.I <- p.V", } if !reflect.DeepEqual(got, want) { t.Fatalf("found unexpected constraints: got %s, want %s", got, want) @@ -209,7 +219,9 @@ func constraints(t *testing.T, src string) []string { Selections: make(map[*ast.SelectorExpr]*types.Selection), } typeparams.InitInstanceInfo(info) - conf := types.Config{} + conf := types.Config{ + Importer: importer.Default(), + } if _, err := conf.Check("p", fset, files, info); err != nil { t.Fatal(err) // type error }