Skip to content

Commit

Permalink
refactor/satisfy: fix visiting functions in the unsafe package
Browse files Browse the repository at this point in the history
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 <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
  • Loading branch information
findleyr committed Oct 17, 2022
1 parent 9b5e55b commit b93a56f
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 10 deletions.
9 changes: 5 additions & 4 deletions gopls/internal/lsp/cmd/test/rename.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"testing"

"golang.org/x/tools/gopls/internal/lsp/tests/compare"
"golang.org/x/tools/gopls/internal/span"
)

Expand All @@ -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)
}
30 changes: 30 additions & 0 deletions gopls/internal/regtest/misc/rename_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = `
Expand Down
22 changes: 18 additions & 4 deletions refactor/satisfy/find.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down
16 changes: 14 additions & 2 deletions refactor/satisfy/find_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package satisfy_test
import (
"fmt"
"go/ast"
"go/importer"
"go/parser"
"go/token"
"go/types"
Expand All @@ -27,6 +28,8 @@ func TestGenericCoreOperations(t *testing.T) {

const src = `package foo
import "unsafe"
type I interface { f() }
type impl struct{}
Expand All @@ -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) {}
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand All @@ -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
}
Expand Down

0 comments on commit b93a56f

Please sign in to comment.