gopls: rationalize "deref" helpers

adonovan · adonovan · commit 054c06df410b · 2024-02-22T18:17:25.000Z
This change (part of the types.Alias audit) defines two helper functions for dealing with pointer types: 1. internal/typeparams.MustDeref (formerly go/ssa.mustDeref) is the type equivalent of a LOAD instruction. 2. internal/typesinternal.Unpointer strips off a Pointer constructor (possibly wrapped in an Alias). The golang.Deref function, which recursively strips off Pointer and Named constructors, is a meaningless operation. It has been replaced in all cases by Unpointer + Unalias. There are far too many functions called 'deref' with subtle variations in their behavior. I plan to standardize x/tools on few common idioms. (There is more to do.) Updates golang/go#65294 Change-Id: I502bab95e8d954715784b7e35ec801f4be4bc959 Reviewed-on: https://go-review.googlesource.com/c/tools/+/565476 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> Reviewed-by: Tim King <taking@google.com>
diff --git a/go/ssa/builder.go b/go/ssa/builder.go
@@ -328,7 +328,7 @@ func (b *builder) builtin(fn *Function, obj *types.Builtin, args []ast.Expr, typ
 		}
 
 	case "new":
-		return emitNew(fn, mustDeref(typ), pos, "new")
+		return emitNew(fn, typeparams.MustDeref(typ), pos, "new")
 
 	case "len", "cap":
 		// Special case: len or cap of an array or *array is
@@ -419,7 +419,7 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue {
 		wantAddr := true
 		v := b.receiver(fn, e.X, wantAddr, escaping, sel)
 		index := sel.index[len(sel.index)-1]
-		fld := fieldOf(mustDeref(v.Type()), index) // v is an addr.
+		fld := fieldOf(typeparams.MustDeref(v.Type()), index) // v is an addr.
 
 		// Due to the two phases of resolving AssignStmt, a panic from x.f = p()
 		// when x is nil is required to come after the side-effects of
@@ -468,7 +468,7 @@ func (b *builder) addr(fn *Function, e ast.Expr, escaping bool) lvalue {
 			v.setType(et)
 			return fn.emit(v)
 		}
-		return &lazyAddress{addr: emit, t: mustDeref(et), pos: e.Lbrack, expr: e}
+		return &lazyAddress{addr: emit, t: typeparams.MustDeref(et), pos: e.Lbrack, expr: e}
 
 	case *ast.StarExpr:
 		return &address{addr: b.expr(fn, e.X), pos: e.Star, expr: e}
diff --git a/go/ssa/create.go b/go/ssa/create.go
@@ -259,6 +259,7 @@ func (prog *Program) CreatePackage(pkg *types.Package, files []*ast.File, info *
 			obj := scope.Lookup(name)
 			memberFromObject(p, obj, nil, "")
 			if obj, ok := obj.(*types.TypeName); ok {
+				// No Unalias: aliases should not duplicate methods.
 				if named, ok := obj.Type().(*types.Named); ok {
 					for i, n := 0, named.NumMethods(); i < n; i++ {
 						memberFromObject(p, named.Method(i), nil, "")
diff --git a/go/ssa/emit.go b/go/ssa/emit.go
@@ -11,6 +11,8 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
+
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // emitAlloc emits to f a new Alloc instruction allocating a variable
@@ -64,7 +66,7 @@ func emitLocalVar(f *Function, v *types.Var) *Alloc {
 // new temporary, and returns the value so defined.
 func emitLoad(f *Function, addr Value) *UnOp {
 	v := &UnOp{Op: token.MUL, X: addr}
-	v.setType(mustDeref(addr.Type()))
+	v.setType(typeparams.MustDeref(addr.Type()))
 	f.emit(v)
 	return v
 }
@@ -414,7 +416,7 @@ func emitTypeCoercion(f *Function, v Value, typ types.Type) Value {
 // emitStore emits to f an instruction to store value val at location
 // addr, applying implicit conversions as required by assignability rules.
 func emitStore(f *Function, addr, val Value, pos token.Pos) *Store {
-	typ := mustDeref(addr.Type())
+	typ := typeparams.MustDeref(addr.Type())
 	s := &Store{
 		Addr: addr,
 		Val:  emitConv(f, val, typ),
diff --git a/go/ssa/func.go b/go/ssa/func.go
@@ -14,6 +14,8 @@ import (
 	"io"
 	"os"
 	"strings"
+
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // Like ObjectOf, but panics instead of returning nil.
@@ -531,7 +533,7 @@ func WriteFunction(buf *bytes.Buffer, f *Function) {
 	if len(f.Locals) > 0 {
 		buf.WriteString("# Locals:\n")
 		for i, l := range f.Locals {
-			fmt.Fprintf(buf, "# % 3d:\t%s %s\n", i, l.Name(), relType(mustDeref(l.Type()), from))
+			fmt.Fprintf(buf, "# % 3d:\t%s %s\n", i, l.Name(), relType(typeparams.MustDeref(l.Type()), from))
 		}
 	}
 	writeSignature(buf, from, f.Name(), f.Signature)
diff --git a/go/ssa/lift.go b/go/ssa/lift.go
@@ -43,6 +43,8 @@ import (
 	"go/token"
 	"math/big"
 	"os"
+
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // If true, show diagnostic information at each step of lifting.
@@ -465,7 +467,7 @@ func liftAlloc(df domFrontier, alloc *Alloc, newPhis newPhiMap, fresh *int) bool
 				*fresh++
 
 				phi.pos = alloc.Pos()
-				phi.setType(mustDeref(alloc.Type()))
+				phi.setType(typeparams.MustDeref(alloc.Type()))
 				phi.block = v
 				if debugLifting {
 					fmt.Fprintf(os.Stderr, "\tplace %s = %s at block %s\n", phi.Name(), phi, v)
@@ -510,7 +512,7 @@ func replaceAll(x, y Value) {
 func renamed(renaming []Value, alloc *Alloc) Value {
 	v := renaming[alloc.index]
 	if v == nil {
-		v = zeroConst(mustDeref(alloc.Type()))
+		v = zeroConst(typeparams.MustDeref(alloc.Type()))
 		renaming[alloc.index] = v
 	}
 	return v
diff --git a/go/ssa/lvalue.go b/go/ssa/lvalue.go
@@ -11,6 +11,8 @@ import (
 	"go/ast"
 	"go/token"
 	"go/types"
+
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // An lvalue represents an assignable location that may appear on the
@@ -52,7 +54,7 @@ func (a *address) address(fn *Function) Value {
 }
 
 func (a *address) typ() types.Type {
-	return mustDeref(a.addr.Type())
+	return typeparams.MustDeref(a.addr.Type())
 }
 
 // An element is an lvalue represented by m[k], the location of an
diff --git a/go/ssa/print.go b/go/ssa/print.go
@@ -17,6 +17,7 @@ import (
 	"strings"
 
 	"golang.org/x/tools/go/types/typeutil"
+	"golang.org/x/tools/internal/typeparams"
 )
 
 // relName returns the name of v relative to i.
@@ -94,7 +95,7 @@ func (v *Alloc) String() string {
 		op = "new"
 	}
 	from := v.Parent().relPkg()
-	return fmt.Sprintf("%s %s (%s)", op, relType(mustDeref(v.Type()), from), v.Comment)
+	return fmt.Sprintf("%s %s (%s)", op, relType(typeparams.MustDeref(v.Type()), from), v.Comment)
 }
 
 func (v *Phi) String() string {
@@ -260,7 +261,7 @@ func (v *MakeChan) String() string {
 func (v *FieldAddr) String() string {
 	// Be robust against a bad index.
 	name := "?"
-	if fld := fieldOf(mustDeref(v.X.Type()), v.Field); fld != nil {
+	if fld := fieldOf(typeparams.MustDeref(v.X.Type()), v.Field); fld != nil {
 		name = fld.Name()
 	}
 	return fmt.Sprintf("&%s.%s [#%d]", relName(v.X, v), name, v.Field)
@@ -449,7 +450,7 @@ func WritePackage(buf *bytes.Buffer, p *Package) {
 
 		case *Global:
 			fmt.Fprintf(buf, "  var   %-*s %s\n",
-				maxname, name, relType(mustDeref(mem.Type()), from))
+				maxname, name, relType(typeparams.MustDeref(mem.Type()), from))
 		}
 	}
 
diff --git a/go/ssa/util.go b/go/ssa/util.go
@@ -115,15 +115,6 @@ func deref(typ types.Type) (types.Type, bool) {
 	return typ, false
 }
 
-// mustDeref returns the element type of a type with a pointer core type.
-// Panics on failure.
-func mustDeref(typ types.Type) types.Type {
-	if et, ok := deref(typ); ok {
-		return et
-	}
-	panic("cannot dereference type " + typ.String())
-}
-
 // recvType returns the receiver type of method obj.
 func recvType(obj *types.Func) types.Type {
 	return obj.Type().(*types.Signature).Recv().Type()
diff --git a/gopls/internal/golang/completion/completion.go b/gopls/internal/golang/completion/completion.go
@@ -37,10 +37,12 @@ import (
 	goplsastutil "golang.org/x/tools/gopls/internal/util/astutil"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/gopls/internal/util/typesutil"
+	"golang.org/x/tools/internal/aliases"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/fuzzy"
 	"golang.org/x/tools/internal/imports"
 	"golang.org/x/tools/internal/typeparams"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // A CompletionItem represents a possible completion suggested by the algorithm.
@@ -1585,7 +1587,7 @@ func (c *completer) lexical(ctx context.Context) error {
 	}
 
 	if c.inference.objType != nil {
-		if named, _ := golang.Deref(c.inference.objType).(*types.Named); named != nil {
+		if named, ok := aliases.Unalias(typesinternal.Unpointer(c.inference.objType)).(*types.Named); ok {
 			// If we expected a named type, check the type's package for
 			// completion items. This is useful when the current file hasn't
 			// imported the type's package yet.
@@ -1651,14 +1653,14 @@ func (c *completer) injectType(ctx context.Context, t types.Type) {
 		return
 	}
 
-	t = golang.Deref(t)
+	t = typesinternal.Unpointer(t)
 
 	// If we have an expected type and it is _not_ a named type, handle
 	// it specially. Non-named types like "[]int" will never be
 	// considered via a lexical search, so we need to directly inject
 	// them. Also allow generic types since lexical search does not
 	// infer instantiated versions of them.
-	if named, _ := t.(*types.Named); named == nil || named.TypeParams().Len() > 0 {
+	if named, ok := aliases.Unalias(t).(*types.Named); !ok || named.TypeParams().Len() > 0 {
 		// If our expected type is "[]int", this will add a literal
 		// candidate of "[]int{}".
 		c.literal(ctx, t, nil)
@@ -1896,7 +1898,7 @@ func enclosingCompositeLiteral(path []ast.Node, pos token.Pos, info *types.Info)
 
 			clInfo := compLitInfo{
 				cl:     n,
-				clType: golang.Deref(tv.Type).Underlying(),
+				clType: typesinternal.Unpointer(tv.Type).Underlying(),
 			}
 
 			var (
diff --git a/gopls/internal/golang/completion/literal.go b/gopls/internal/golang/completion/literal.go
@@ -14,7 +14,9 @@ import (
 	"golang.org/x/tools/gopls/internal/golang"
 	"golang.org/x/tools/gopls/internal/golang/completion/snippet"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/internal/aliases"
 	"golang.org/x/tools/internal/event"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // literal generates composite literal, function literal, and make()
@@ -49,10 +51,11 @@ func (c *completer) literal(ctx context.Context, literalType types.Type, imp *im
 	//
 	// don't offer "mySlice{}" since we have already added a candidate
 	// of "[]int{}".
-	if _, named := literalType.(*types.Named); named && expType != nil {
-		if _, named := golang.Deref(expType).(*types.Named); !named {
-			return
-		}
+	if is[*types.Named](aliases.Unalias(literalType)) &&
+		expType != nil &&
+		!is[*types.Named](aliases.Unalias(typesinternal.Unpointer(expType))) {
+
+		return
 	}
 
 	// Check if an object of type literalType would match our expected type.
@@ -589,3 +592,8 @@ func (c *completer) typeParamInScope(tp *types.TypeParam) bool {
 	_, foundObj := scope.LookupParent(obj.Name(), c.pos)
 	return obj == foundObj
 }
+
+func is[T any](x any) bool {
+	_, ok := x.(T)
+	return ok
+}
diff --git a/gopls/internal/golang/completion/postfix_snippets.go b/gopls/internal/golang/completion/postfix_snippets.go
@@ -21,8 +21,10 @@ import (
 	"golang.org/x/tools/gopls/internal/golang/completion/snippet"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
+	"golang.org/x/tools/internal/aliases"
 	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/imports"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // Postfix snippets are artificial methods that allow the user to
@@ -465,7 +467,7 @@ func (a *postfixTmplArgs) VarName(t types.Type, nonNamedDefault string) string {
 	// go/types predicates are undefined on types.Typ[types.Invalid].
 	if !types.Identical(t, types.Typ[types.Invalid]) && types.Implements(t, errorIntf) {
 		name = "err"
-	} else if _, isNamed := golang.Deref(t).(*types.Named); !isNamed {
+	} else if !is[*types.Named](aliases.Unalias(typesinternal.Unpointer(t))) {
 		name = nonNamedDefault
 	}
 
diff --git a/gopls/internal/golang/completion/util.go b/gopls/internal/golang/completion/util.go
@@ -14,6 +14,7 @@ import (
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/internal/diff"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // exprAtPos returns the index of the expression containing pos.
@@ -38,7 +39,12 @@ func eachField(T types.Type, fn func(*types.Var)) {
 
 	var visit func(T types.Type)
 	visit = func(T types.Type) {
-		if T, ok := golang.Deref(T).Underlying().(*types.Struct); ok {
+		// T may be a Struct, optionally Named, with an optional
+		// Pointer (with optional Aliases at every step!):
+		// Consider: type T *struct{ f int }; _ = T(nil).f
+		//
+		// TODO(adonovan): use typeparams.Deref in next CL.
+		if T, ok := typesinternal.Unpointer(T.Underlying()).Underlying().(*types.Struct); ok {
 			if seen.At(T) != nil {
 				return
 			}
diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go
@@ -1226,7 +1226,7 @@ func promotedFields(t types.Type, from *types.Package) []promotedField {
 	var fields []promotedField
 	var visit func(t types.Type, stack []*types.Named)
 	visit = func(t types.Type, stack []*types.Named) {
-		tStruct, ok := Deref(t).Underlying().(*types.Struct)
+		tStruct, ok := typesinternal.Unpointer(t).Underlying().(*types.Struct)
 		if !ok {
 			return
 		}
diff --git a/gopls/internal/golang/identifier.go b/gopls/internal/golang/identifier.go
@@ -8,6 +8,9 @@ import (
 	"errors"
 	"go/ast"
 	"go/types"
+
+	"golang.org/x/tools/internal/aliases"
+	"golang.org/x/tools/internal/typesinternal"
 )
 
 // ErrNoIdentFound is error returned when no identifier is found at a particular position
@@ -23,24 +26,30 @@ func inferredSignature(info *types.Info, id *ast.Ident) *types.Signature {
 	return sig
 }
 
+// searchForEnclosing returns, given the AST path to a SelectorExpr,
+// the exported named type of the innermost implicit field selection.
+//
+// For example, given "new(A).d" where this is (due to embedding) a
+// shorthand for "new(A).b.c.d", it returns the named type of c,
+// if it is exported, otherwise the type of b, or A.
 func searchForEnclosing(info *types.Info, path []ast.Node) *types.TypeName {
 	for _, n := range path {
 		switch n := n.(type) {
 		case *ast.SelectorExpr:
 			if sel, ok := info.Selections[n]; ok {
-				recv := Deref(sel.Recv())
+				recv := typesinternal.Unpointer(sel.Recv())
 
 				// Keep track of the last exported type seen.
 				var exported *types.TypeName
-				if named, ok := recv.(*types.Named); ok && named.Obj().Exported() {
+				if named, ok := aliases.Unalias(recv).(*types.Named); ok && named.Obj().Exported() {
 					exported = named.Obj()
 				}
 				// We don't want the last element, as that's the field or
 				// method itself.
 				for _, index := range sel.Index()[:len(sel.Index())-1] {
 					if r, ok := recv.Underlying().(*types.Struct); ok {
-						recv = Deref(r.Field(index).Type())
-						if named, ok := recv.(*types.Named); ok && named.Obj().Exported() {
+						recv = typesinternal.Unpointer(r.Field(index).Type())
+						if named, ok := aliases.Unalias(recv).(*types.Named); ok && named.Obj().Exported() {
 							exported = named.Obj()
 						}
 					}
diff --git a/gopls/internal/golang/rename_check.go b/gopls/internal/golang/rename_check.go
@@ -45,6 +45,8 @@ import (
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
+	"golang.org/x/tools/internal/aliases"
+	"golang.org/x/tools/internal/typesinternal"
 	"golang.org/x/tools/refactor/satisfy"
 )
 
@@ -371,7 +373,7 @@ func forEachLexicalRef(pkg *cache.Package, obj types.Object, fn func(id *ast.Ide
 				return visit(nil) // pop stack, don't descend
 			}
 			// TODO(adonovan): fix: for generics, should be T.core not T.underlying.
-			if _, ok := Deref(tv.Type).Underlying().(*types.Struct); ok {
+			if _, ok := typesinternal.Unpointer(tv.Type).Underlying().(*types.Struct); ok {
 				if n.Type != nil {
 					ast.Inspect(n.Type, visit)
 				}
@@ -501,7 +503,7 @@ func (r *renamer) checkStructField(from *types.Var) {
 	if from.Anonymous() {
 		if named, ok := from.Type().(*types.Named); ok {
 			r.check(named.Obj())
-		} else if named, ok := Deref(from.Type()).(*types.Named); ok {
+		} else if named, ok := aliases.Unalias(typesinternal.Unpointer(from.Type())).(*types.Named); ok {
 			r.check(named.Obj())
 		}
 	}
diff --git a/gopls/internal/golang/util.go b/gopls/internal/golang/util.go
diff --git a/internal/typeparams/coretype.go b/internal/typeparams/coretype.go
diff --git a/internal/typesinternal/recv.go b/internal/typesinternal/recv.go

Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,8 @@ import (`
`14`	`14`	`"io"`
`15`	`15`	`"os"`
`16`	`16`	`"strings"`
	`17`	`+`
	`18`	`+ "golang.org/x/tools/internal/typeparams"`
`17`	`19`	`)`
`18`	`20`
`19`	`21`	`// Like ObjectOf, but panics instead of returning nil.`
`@@ -531,7 +533,7 @@ func WriteFunction(buf bytes.Buffer, f Function) {`
`531`	`533`	`if len(f.Locals) > 0 {`
`532`	`534`	`buf.WriteString("# Locals:\n")`
`533`	`535`	`for i, l := range f.Locals {`
`534`		`- fmt.Fprintf(buf, "# % 3d:\t%s %s\n", i, l.Name(), relType(mustDeref(l.Type()), from))`
	`536`	`+ fmt.Fprintf(buf, "# % 3d:\t%s %s\n", i, l.Name(), relType(typeparams.MustDeref(l.Type()), from))`
`535`	`537`	`}`
`536`	`538`	`}`
`537`	`539`	`writeSignature(buf, from, f.Name(), f.Signature)`
Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,8 @@ import (`
`11`	`11`	`"go/ast"`
`12`	`12`	`"go/token"`
`13`	`13`	`"go/types"`
	`14`	`+`
	`15`	`+ "golang.org/x/tools/internal/typeparams"`
`14`	`16`	`)`
`15`	`17`
`16`	`18`	`// An lvalue represents an assignable location that may appear on the`
`@@ -52,7 +54,7 @@ func (a address) address(fn Function) Value {`
`52`	`54`	`}`
`53`	`55`
`54`	`56`	`func (a *address) typ() types.Type {`
`55`		`- return mustDeref(a.addr.Type())`
	`57`	`+ return typeparams.MustDeref(a.addr.Type())`
`56`	`58`	`}`
`57`	`59`
`58`	`60`	`// An element is an lvalue represented by m[k], the location of an`
Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@ import (`
`17`	`17`	`"strings"`
`18`	`18`
`19`	`19`	`"golang.org/x/tools/go/types/typeutil"`
	`20`	`+ "golang.org/x/tools/internal/typeparams"`
`20`	`21`	`)`
`21`	`22`
`22`	`23`	`// relName returns the name of v relative to i.`
`@@ -94,7 +95,7 @@ func (v *Alloc) String() string {`
`94`	`95`	`op = "new"`
`95`	`96`	`}`
`96`	`97`	`from := v.Parent().relPkg()`
`97`		`- return fmt.Sprintf("%s %s (%s)", op, relType(mustDeref(v.Type()), from), v.Comment)`
	`98`	`+ return fmt.Sprintf("%s %s (%s)", op, relType(typeparams.MustDeref(v.Type()), from), v.Comment)`
`98`	`99`	`}`
`99`	`100`
`100`	`101`	`func (v *Phi) String() string {`
`@@ -260,7 +261,7 @@ func (v *MakeChan) String() string {`
`260`	`261`	`func (v *FieldAddr) String() string {`
`261`	`262`	`// Be robust against a bad index.`
`262`	`263`	`name := "?"`
`263`		`- if fld := fieldOf(mustDeref(v.X.Type()), v.Field); fld != nil {`
	`264`	`+ if fld := fieldOf(typeparams.MustDeref(v.X.Type()), v.Field); fld != nil {`
`264`	`265`	`name = fld.Name()`
`265`	`266`	`}`
`266`	`267`	`return fmt.Sprintf("&%s.%s [#%d]", relName(v.X, v), name, v.Field)`
`@@ -449,7 +450,7 @@ func WritePackage(buf bytes.Buffer, p Package) {`
`449`	`450`
`450`	`451`	`case *Global:`
`451`	`452`	`fmt.Fprintf(buf, " var %-*s %s\n",`
`452`		`- maxname, name, relType(mustDeref(mem.Type()), from))`
	`453`	`+ maxname, name, relType(typeparams.MustDeref(mem.Type()), from))`
`453`	`454`	`}`
`454`	`455`	`}`
`455`	`456`