go.tools/go/types: LookupFieldOrMethod checks method set

LookupFieldOrMethod now also decides whether a found
method is actually in the method set. Simplifies call
sites. Added corresponding API tests.

TODO (separate CL): Decide what the correct value for
the indirect result should be (as required for code
generation). For now, the result value for indirect
is unchanged from before if a field/method is found.

Fixes golang/go#8584.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://golang.org/cl/132260043

cmd/vet/types.go

@@ -326,7 +326,8 @@ func (f *File) isErrorMethodCall(call *ast.CallExpr) bool {
 // that workaround is no longer necessary.
 // TODO: This could be better once issue 6259 is fixed.
 func (f *File) hasMethod(typ types.Type, name string) bool {
-	obj, _, _ := types.LookupFieldOrMethod(typ, f.pkg.typesPkg, name)
+	// assume we have an addressable variable of type typ
+	obj, _, _ := types.LookupFieldOrMethod(typ, true, f.pkg.typesPkg, name)
 	_, ok := obj.(*types.Func)
 	return ok
 }

go/ssa/interp/external.go

@@ -140,7 +140,7 @@ func wrapError(err error) value {
 
 func ext۰sync۰Pool۰Get(fr *frame, args []value) value {
 	Pool := fr.i.prog.ImportedPackage("sync").Type("Pool").Object()
-	_, newIndex, _ := types.LookupFieldOrMethod(Pool.Type(), Pool.Pkg(), "New")
+	_, newIndex, _ := types.LookupFieldOrMethod(Pool.Type(), false, Pool.Pkg(), "New")
 
 	if New := (*args[0].(*value)).(structure)[newIndex[0]]; New != nil {
 		return call(fr.i, fr, 0, New, nil)

go/types/api_test.go

@@ -740,12 +740,12 @@ func main() {
 		"new(A).f": {"method (*main.A) f(int)", "->[0 0]"},
 		"A{}.g":    {"method (main.A) g()", ".[1 0]"},
 		"new(A).g": {"method (*main.A) g()", "->[1 0]"},
-		"new(A).h": {"method (*main.A) h()", "->[1 1]"},
+		"new(A).h": {"method (*main.A) h()", "->[1 1]"}, // TODO(gri) should this report .[1 1] ?
 		"B{}.f":    {"method (main.B) f(int)", ".[0]"},
 		"new(B).f": {"method (*main.B) f(int)", "->[0]"},
 		"C{}.g":    {"method (main.C) g()", ".[0]"},
 		"new(C).g": {"method (*main.C) g()", "->[0]"},
-		"new(C).h": {"method (*main.C) h()", "->[1]"},
+		"new(C).h": {"method (*main.C) h()", "->[1]"}, // TODO(gri) should this report .[1] ?
 
 		"A.f":    {"method expr (main.A) f(main.A, int)", "->[0 0]"},
 		"(*A).f": {"method expr (*main.A) f(*main.A, int)", "->[0 0]"},
@@ -827,3 +827,77 @@ var _ = a.C2
 	makePkg("a", libSrc)
 	makePkg("main", mainSrc) // don't crash when type-checking this package
 }
+
+func TestLookupFieldOrMethod(t *testing.T) {
+	// Test cases assume a lookup of the form a.f or x.f, where a stands for an
+	// addressable value, and x for a non-addressable value (even though a variable
+	// for ease of test case writing).
+	var tests = []struct {
+		src      string
+		found    bool
+		index    []int
+		indirect bool
+	}{
+		// field lookups
+		{"var x T; type T struct{}", false, nil, false},
+		{"var x T; type T struct{ f int }", true, []int{0}, false},
+		{"var x T; type T struct{ a, b, f, c int }", true, []int{2}, false},
+
+		// method lookups
+		{"var a T; type T struct{}; func (T) f() {}", true, []int{0}, false},
+		{"var a *T; type T struct{}; func (T) f() {}", true, []int{0}, true},
+		{"var a T; type T struct{}; func (*T) f() {}", true, []int{0}, false},
+		{"var a *T; type T struct{}; func (*T) f() {}", true, []int{0}, true}, // TODO(gri) should this report indirect = false?
+
+		// collisions
+		{"type ( E1 struct{ f int }; E2 struct{ f int }; x struct{ E1; *E2 })", false, []int{1, 0}, false},
+		{"type ( E1 struct{ f int }; E2 struct{}; x struct{ E1; *E2 }); func (E2) f() {}", false, []int{1, 0}, false},
+
+		// outside methodset
+		// (*T).f method exists, but value of type T is not addressable
+		{"var x T; type T struct{}; func (*T) f() {}", false, nil, true},
+	}
+
+	for _, test := range tests {
+		pkg, err := pkgFor("test", "package p;"+test.src, nil)
+		if err != nil {
+			t.Errorf("%s: incorrect test case: %s", test.src, err)
+			continue
+		}
+
+		obj := pkg.Scope().Lookup("a")
+		if obj == nil {
+			if obj = pkg.Scope().Lookup("x"); obj == nil {
+				t.Errorf("%s: incorrect test case - no object a or x", test.src)
+				continue
+			}
+		}
+
+		f, index, indirect := LookupFieldOrMethod(obj.Type(), obj.Name() == "a", pkg, "f")
+		if (f != nil) != test.found {
+			if f == nil {
+				t.Errorf("%s: got no object; want one", test.src)
+			} else {
+				t.Errorf("%s: got object = %v; want none", test.src, f)
+			}
+		}
+		if !sameSlice(index, test.index) {
+			t.Errorf("%s: got index = %v; want %v", test.src, index, test.index)
+		}
+		if indirect != test.indirect {
+			t.Errorf("%s: got indirect = %v; want %v", test.src, indirect, test.indirect)
+		}
+	}
+}
+
+func sameSlice(a, b []int) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, x := range a {
+		if x != b[i] {
+			return false
+		}
+	}
+	return true
+}

go/types/builtins.go

@@ -489,7 +489,7 @@ func (check *Checker) builtin(x *operand, call *ast.CallExpr, id builtinId) (_ b
 		}
 		base := derefStructPtr(x.typ)
 		sel := selx.Sel.Name
-		obj, index, indirect := LookupFieldOrMethod(base, check.pkg, sel)
+		obj, index, indirect := LookupFieldOrMethod(base, false, check.pkg, sel)
 		switch obj.(type) {
 		case nil:
 			check.invalidArg(x.pos(), "%s has no single field %s", base, sel)

go/types/call.go

@@ -302,12 +302,15 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) {
 		goto Error
 	}
 
-	obj, index, indirect = LookupFieldOrMethod(x.typ, check.pkg, sel)
+	obj, index, indirect = LookupFieldOrMethod(x.typ, x.mode == variable, check.pkg, sel)
 	if obj == nil {
-		if index != nil {
+		switch {
+		case index != nil:
 			// TODO(gri) should provide actual type where the conflict happens
 			check.invalidOp(e.Pos(), "ambiguous selector %s", sel)
-		} else {
+		case indirect:
+			check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x.typ)
+		default:
 			check.invalidOp(e.Pos(), "%s has no field or method %s", x, sel)
 		}
 		goto Error
@@ -321,12 +324,6 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) {
 			goto Error
 		}
 
-		// verify that m is in the method set of x.typ
-		if !indirect && ptrRecv(m) {
-			check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x.typ)
-			goto Error
-		}
-
 		check.recordSelection(e, MethodExpr, x.typ, m, index, indirect)
 
 		// the receiver type becomes the type of the first function
@@ -358,18 +355,8 @@ func (check *Checker) selector(x *operand, e *ast.SelectorExpr) {
 			x.typ = obj.typ
 
 		case *Func:
-			// TODO(gri) This code appears elsewhere, too. Factor!
+			// TODO(gri) If we needed to take into account the receiver's
-			// verify that obj is in the method set of x.typ (or &(x.typ) if x is addressable)
+			// addressability, should we report the type &(x.typ) instead?
-			//
-			// spec: "A method call x.m() is valid if the method set of (the type of) x
-			//        contains m and the argument list can be assigned to the parameter
-			//        list of m. If x is addressable and &x's method set contains m, x.m()
-			//        is shorthand for (&x).m()".
-			if !indirect && x.mode != variable && ptrRecv(obj) {
-				check.invalidOp(e.Pos(), "%s is not in method set of %s", sel, x)
-				goto Error
-			}
-
 			check.recordSelection(e, MethodVal, x.typ, obj, index, indirect)
 
 			if debug {

go/types/lookup.go

@@ -6,15 +6,11 @@
 
 package types
 
-// TODO(gri) The named type consolidation and seen maps below must be
-//           indexed by unique keys for a given type. Verify that named
-//           types always have only one representation (even when imported
-//           indirectly via different packages.)
-
 // LookupFieldOrMethod looks up a field or method with given package and name
 // in T and returns the corresponding *Var or *Func, an index sequence, and a
 // bool indicating if there were any pointer indirections on the path to the
-// field or method.
+// field or method. If addressable is set, T is the type of an addressable
+// variable (only matters for method lookups).
 //
 // The last index entry is the field or method index in the (possibly embedded)
 // type where the entry was found, either:
@@ -23,14 +19,21 @@ package types
 //	2) the list of all methods (method set) of an interface type; or
 //	3) the list of fields of a struct type.
 //
-// The earlier index entries are the indices of the embedded fields traversed
+// The earlier index entries are the indices of the anonymous struct fields
-// to get to the found entry, starting at depth 0.
+// traversed to get to the found entry, starting at depth 0.
 //
 // If no entry is found, a nil object is returned. In this case, the returned
-// index sequence points to an ambiguous entry if it exists, or it is nil.
+// index and indirect values have the following meaning:
 //
-func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index []int, indirect bool) {
+//	- If index != nil, the index sequence points to an ambiguous entry
-	obj, index, indirect = lookupFieldOrMethod(T, pkg, name)
+//	(the same name appeared more than once at the same embedding level).
+//
+//	- If indirect is set, a method with a pointer receiver type was found
+//      but there was no pointer on the path from the actual receiver type to
+//	the method's formal receiver base type, nor was the receiver addressable.
+//
+func LookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (obj Object, index []int, indirect bool) {
+	obj, index, indirect = lookupFieldOrMethod(T, addressable, pkg, name)
 	if obj != nil {
 		return
 	}
@@ -50,16 +53,13 @@ func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 	// of P (i.e., *S). We cannot do this always because we might find
 	// methods that don't exist for P but for S (e.g., m). Thus, if the
 	// result is a method we need to discard it.
-	//
+	// (See also issue 8590).
-	// TODO(gri) WTF? There isn't a more direct way? Perhaps we should
-	//           outlaw named types to pointer types - they are almost
-	//           never what one wants, anyway.
 	if t, _ := T.(*Named); t != nil {
 		u := t.underlying
 		if _, ok := u.(*Pointer); ok {
 			// typ is a named type with an underlying type of the form *T,
 			// start the search with the underlying type *T
-			if obj2, index2, indirect2 := lookupFieldOrMethod(u, pkg, name); obj2 != nil {
+			if obj2, index2, indirect2 := lookupFieldOrMethod(u, false, pkg, name); obj2 != nil {
 				// only if the result is a field can we keep it
 				if _, ok := obj2.(*Var); ok {
 					return obj2, index2, indirect2
@@ -71,7 +71,12 @@ func LookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 	return
 }
 
-func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index []int, indirect bool) {
+// TODO(gri) The named type consolidation and seen maps below must be
+//           indexed by unique keys for a given type. Verify that named
+//           types always have only one representation (even when imported
+//           indirectly via different packages.)
+
+func lookupFieldOrMethod(T Type, addressable bool, pkg *Package, name string) (obj Object, index []int, indirect bool) {
 	// WARNING: The code in this function is extremely subtle - do not modify casually!
 	//          This function and NewMethodSet should be kept in sync.
 
@@ -129,8 +134,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 					assert(m.typ != nil)
 					index = concat(e.index, i)
 					if obj != nil || e.multiples {
-						obj = nil // collision
+						return nil, index, false // collision
-						return
 					}
 					obj = m
 					indirect = e.indirect
@@ -149,8 +153,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 						assert(f.typ != nil)
 						index = concat(e.index, i)
 						if obj != nil || e.multiples {
-							obj = nil // collision
+							return nil, index, false // collision
-							return
 						}
 						obj = f
 						indirect = e.indirect
@@ -182,8 +185,7 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 					assert(m.typ != nil)
 					index = concat(e.index, i)
 					if obj != nil || e.multiples {
-						obj = nil // collision
+						return nil, index, false // collision
-						return
 					}
 					obj = m
 					indirect = e.indirect
@@ -192,15 +194,21 @@ func lookupFieldOrMethod(T Type, pkg *Package, name string) (obj Object, index [
 		}
 
 		if obj != nil {
-			return // found a match
+			// found a potential match
+			// spec: "A method call x.m() is valid if the method set of (the type of) x
+			//        contains m and the argument list can be assigned to the parameter
+			//        list of m. If x is addressable and &x's method set contains m, x.m()
+			//        is shorthand for (&x).m()".
+			if f, _ := obj.(*Func); f != nil && ptrRecv(f) && !indirect && !addressable {
+				return nil, nil, true // pointer/addressable receiver required
+			}
+			return
 		}
 
 		current = consolidateMultiples(next)
 	}
 
-	index = nil
+	return nil, nil, false // not found
-	indirect = false
-	return // not found
 }
 
 // embeddedType represents an embedded named type
@@ -270,22 +278,14 @@ func MissingMethod(V Type, T *Interface, static bool) (method *Func, wrongType b
 
 	// A concrete type implements T if it implements all methods of T.
 	for _, m := range T.allMethods {
-		obj, _, indirect := lookupFieldOrMethod(V, m.pkg, m.name)
+		obj, _, _ := lookupFieldOrMethod(V, false, m.pkg, m.name)
-		if obj == nil {
-			return m, false
-		}
 
 		f, _ := obj.(*Func)
 		if f == nil {
 			return m, false
 		}
 
-		// verify that f is in the method set of typ
+		if !Identical(f.typ, m.typ) {
-		if !indirect && ptrRecv(f) {
-			return m, false
-		}
-
-		if !Identical(obj.Type(), m.typ) {
 			return m, true
 		}
 	}

go/types/methodset.go

@@ -242,6 +242,10 @@ func (s methodSet) add(list []*Func, index []int, indirect bool, multiples bool)
 		key := f.Id()
 		// if f is not in the set, add it
 		if !multiples {
+			// TODO(gri) A found method may not be added because it's not in the method set
+			// (!indirect && ptrRecv(f)). A 2nd method on the same level may be in the method
+			// set and may not collide with the first one, thus leading to a false positive.
+			// Is that possible? Investigate.
 			if _, found := s[key]; !found && (indirect || !ptrRecv(f)) {
 				s[key] = &Selection{MethodVal, nil, f, concat(index, i), indirect}
 				continue

refactor/eg/match.go

@@ -166,7 +166,7 @@ func (tr *Transformer) matchSelectorExpr(x, y *ast.SelectorExpr) bool {
 	if xobj, ok := tr.wildcardObj(x.X); ok {
 		field := x.Sel.Name
 		yt := tr.info.TypeOf(y.X)
-		o, _, _ := types.LookupFieldOrMethod(yt, tr.currentPkg, field)
+		o, _, _ := types.LookupFieldOrMethod(yt, true, tr.currentPkg, field)
 		if o != nil {
 			tr.env[xobj.Name()] = y.X // record binding
 			return true