go.tools/go/types: factor method set and lookup better

No functional change, just symmetric cleanup.

R=adonovan
CC=golang-dev
https://golang.org/cl/10417045

go/types/expr.go

@@ -1376,9 +1376,12 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 				x.mode = variable
 				x.typ = obj.typ
 			case *Func:
-				// TODO(gri) Temporary assert to verify corresponding lookup via method sets.
+				// TODO(gri) Temporary check to verify corresponding lookup via method sets.
 				//           Remove eventually.
-				assert(NewMethodSet(x.typ).Lookup(check.pkg, sel) == obj)
+				if m := NewMethodSet(x.typ).Lookup(check.pkg, sel); m != obj {
+					check.dump("%s: %v", e.Pos(), obj.name)
+					panic("method sets and lookup don't agree")
+				}
 
 				// TODO(gri) This code appears elsewhere, too. Factor!
 				// verify that obj is in the method set of x.typ (or &(x.typ) if x is addressable)

go/types/lookup.go

@@ -36,26 +36,18 @@ func LookupFieldOrMethod(typ Type, pkg *Package, name string) (obj Object, index
 		return // empty fields/methods are never found
 	}
 
-	// embedded represents an embedded named type
-	type embedded struct {
-		typ       *Named // nil means use the outer typ variable instead
-		index     []int  // embedded field indices, starting with index at depth 0
-		indirect  bool   // if set, there was a pointer indirection on the path to this field
-		multiples bool   // if set, typ appears multiple times at this depth
-	}
-
 	// Start with typ as single entry at lowest depth.
 	// If typ is not a named type, insert a nil type instead.
 	typ, isPtr := deref(typ)
 	t, _ := typ.(*Named)
-	current := []embedded{{t, nil, isPtr, false}}
+	current := []embeddedType{{t, nil, isPtr, false}}
 
 	// named types that we have seen already
 	seen := make(map[*Named]bool)
 
-	// search current depth if there's work to do
+	// search current depth
 	for len(current) > 0 {
-		var next []embedded // embedded types found at current depth
+		var next []embeddedType // embedded types found at current depth
 
 		// look for (pkg, name) in all types at this depth
 		for _, e := range current {
@@ -119,8 +111,8 @@ func LookupFieldOrMethod(typ Type, pkg *Package, name string) (obj Object, index
 						// Ignore embedded basic types - only user-defined
 						// named types can have methods or struct fields.
 						typ, isPtr := deref(f.typ)
-						if t, _ := typ.Deref().(*Named); t != nil {
-							next = append(next, embedded{t, concat(e.index, i), e.indirect || isPtr, e.multiples})
+						if t, _ := typ.(*Named); t != nil {
+							next = append(next, embeddedType{t, concat(e.index, i), e.indirect || isPtr, e.multiples})
 						}
 					}
 				}
@@ -144,24 +136,7 @@ func LookupFieldOrMethod(typ Type, pkg *Package, name string) (obj Object, index
 			return // found a match
 		}
 
-		// Consolidate next: collect multiple entries with the same
-		// type into a single entry marked as containing multiples.
-		n := len(next)
-		if n > 1 {
-			n := 0                       // number of entries w/ unique type
-			prev := make(map[*Named]int) // index at which type was previously seen
-			for _, e := range next {
-				if i, found := prev[e.typ]; found {
-					next[i].multiples = true
-					// ignore this entry
-				} else {
-					prev[e.typ] = n
-					next[n] = e
-					n++
-				}
-			}
-		}
-		current = next[:n]
+		current = consolidateMultiples(next)
 	}
 
 	index = nil
@@ -169,6 +144,37 @@ func LookupFieldOrMethod(typ Type, pkg *Package, name string) (obj Object, index
 	return // not found
 }
 
+// embeddedType represents an embedded named type
+type embeddedType struct {
+	typ       *Named // nil means use the outer typ variable instead
+	index     []int  // embedded field indices, starting with index at depth 0
+	indirect  bool   // if set, there was a pointer indirection on the path to this field
+	multiples bool   // if set, typ appears multiple times at this depth
+}
+
+// consolidateMultiples collects multiple list entries with the same type
+// into a single entry marked as containing multiples. The result is the
+// consolidated list.
+func consolidateMultiples(list []embeddedType) []embeddedType {
+	if len(list) <= 1 {
+		return list // at most one entry - nothing to do
+	}
+
+	n := 0                       // number of entries w/ unique type
+	prev := make(map[*Named]int) // index at which type was previously seen
+	for _, e := range list {
+		if i, found := prev[e.typ]; found {
+			list[i].multiples = true
+			// ignore this entry
+		} else {
+			prev[e.typ] = n
+			list[n] = e
+			n++
+		}
+	}
+	return list[:n]
+}
+
 // MissingMethod returns (nil, false) if typ implements T, otherwise
 // it returns the first missing method required by T and whether it
 // is missing or simply has the wrong type.

go/types/methodset.go

@@ -58,92 +58,49 @@ func (s *MethodSet) Lookup(pkg *Package, name string) *Func {
 // NewMethodSet computes the method set for the given type.
 // BUG(gri): The pointer-ness of the receiver type is still ignored.
 func NewMethodSet(typ Type) *MethodSet {
-	isPtr := false
-	if p, ok := typ.Underlying().(*Pointer); ok {
-		typ = p.base
-		isPtr = true
-	}
-
-	// TODO(gri) consult isPtr for precise method set computation
-	_ = isPtr
-
 	// method set up to the current depth
 	// TODO(gri) allocate lazily, method sets are often empty
 	base := make(methodSet)
 
+	// Start with typ as single entry at lowest depth.
+	// If typ is not a named type, insert a nil type instead.
+	typ, isPtr := deref(typ)
+	t, _ := typ.(*Named)
+	current := []embeddedType{{t, nil, isPtr, false}}
+
 	// named types that we have seen already
 	seen := make(map[*Named]bool)
 
-	// We treat the top-most level separately because it's simpler
-	// (no incoming multiples) and because it's the common case.
+	// collect methods at current depth
+	for len(current) > 0 {
+		var next []embeddedType // embedded types found at current depth
 
-	if t, _ := typ.(*Named); t != nil {
-		seen[t] = true
-		base.add(t.methods, false)
-		typ = t.underlying
-	}
-
-	// embedded named types at the current and next lower depth
-	type embedded struct {
-		typ       *Named
-		multiples bool
-	}
-	var current, next []embedded
-
-	switch t := typ.(type) {
-	case *Struct:
-		for _, f := range t.fields {
-			// Fields and methods must be distinct at the most shallow depth.
-			// If they are not, the type checker reported an error before, so
-			// we are ignoring potential conflicts here.
-			if f.anonymous {
-				// Ignore embedded basic types - only user-defined
-				// named types can have methods or struct fields.
-				if t, _ := f.typ.Deref().(*Named); t != nil {
-					next = append(next, embedded{t, false})
-				}
-			}
-		}
-
-	case *Interface:
-		base.add(t.methods, false)
-	}
-
-	// collect methods at next lower depth
-	for len(next) > 0 {
-		// Consolidate next: collect multiple entries with the same
-		// type into a single entry marked as containing multiples.
-		n := 0                       // number of entries w/ unique type
-		prev := make(map[*Named]int) // index at which type was previously seen
-		for _, e := range next {
-			if i, found := prev[e.typ]; found {
-				next[i].multiples = true
-				// ignore this entry
-			} else {
-				prev[e.typ] = n
-				next[n] = e
-				n++
-			}
-		}
-		// next[:n] is the list of embedded entries to process
-
-		// The underlying arrays of current and next are different, thus
-		// swapping is safe and they never share the same underlying array.
-		current, next = next[:n], current[:0] // don't waste underlying array
-
-		// field and method sets at this depth
+		// field and method sets for current depth
 		fset := make(fieldSet)
 		mset := make(methodSet)
 
 		for _, e := range current {
-			if seen[e.typ] {
-				continue
+			// The very first time only, e.typ may be nil.
+			// In this case, we don't have a named type and
+			// we simply continue with the underlying type.
+			if e.typ != nil {
+				if seen[e.typ] {
+					// We have seen this type before, at a more shallow depth
+					// (note that multiples of this type at the current depth
+					// were eliminated before). The type at that depth shadows
+					// this same type at the current depth, so we can ignore
+					// this one.
+					continue
+				}
+				seen[e.typ] = true
+
+				mset.add(e.typ.methods, e.multiples)
+
+				// continue with underlying type
+				typ = e.typ.underlying
 			}
-			seen[e.typ] = true
 
-			mset.add(e.typ.methods, e.multiples)
-
-			switch t := e.typ.underlying.(type) {
+			switch t := typ.(type) {
 			case *Struct:
 				for _, f := range t.fields {
 					fset.add(f, e.multiples)
@@ -155,8 +112,9 @@ func NewMethodSet(typ Type) *MethodSet {
 					if f.anonymous {
 						// Ignore embedded basic types - only user-defined
 						// named types can have methods or have struct fields.
-						if t, _ := f.typ.Deref().(*Named); t != nil {
-							next = append(next, embedded{t, e.multiples})
+						typ, isPtr := deref(f.typ)
+						if t, _ := typ.(*Named); t != nil {
+							next = append(next, embeddedType{t, nil, e.indirect || isPtr, e.multiples})
 						}
 					}
 				}
@@ -188,6 +146,8 @@ func NewMethodSet(typ Type) *MethodSet {
 				}
 			}
 		}
+
+		current = consolidateMultiples(next)
 	}
 
 	// collect methods

go/types/testdata/decls2a.src

@@ -32,7 +32,12 @@ type T1c struct {
 
 func (T1c) Pointer /* ERROR "field and method" */ () int { return 0 }
 
-var _ = T1c{}.Pointer
+// Disabled for now: LookupFieldOrMethod will find Pointer even though
+// it's double-declared (it would cost extra in the common case to verify
+// this). But the MethodSet computation will not find it due to the name
+// collision caused by the double-declaration, leading to an internal
+// inconsistency while we are verifying one computation against the other.
+// var _ = T1c{}.Pointer
 
 // T2's method declared before the type.
 func (*T2) f /* ERROR "field and method" */ () {}