go.tools/go/types: Use []*Field instead of *Scope for struct fields

This partially reverts a previous change, using a []*Field is a better
representation for struct fields than a *Scope, after all; however
*Fields remain Objects.

Fixes golang/go#5670.

R=adonovan, axwalk
CC=golang-dev
https://golang.org/cl/10207043

go/types/api.go

@@ -112,7 +112,7 @@ type Context struct {
 	// of the given struct fields, in bytes. Otherwise DefaultOffsetsof
 	// is called. Offsetsof must implement the offset guarantees
 	// required by the spec.
-	Offsetsof func(fields *Scope) []int64
+	Offsetsof func(fields []*Field) []int64
 
 	// If Sizeof != nil, it is called to determine the size of the
 	// given type. Otherwise, DefaultSizeof is called. Sizeof must

go/types/errors.go

@@ -258,20 +258,17 @@ func writeType(buf *bytes.Buffer, typ Type) {
 
 	case *Struct:
 		buf.WriteString("struct{")
-		if t.fields != nil {
+		for i, f := range t.fields {
-			for i, obj := range t.fields.entries {
+			if i > 0 {
-				if i > 0 {
+				buf.WriteString("; ")
-					buf.WriteString("; ")
+			}
-				}
+			if !f.anonymous {
-				f := obj.(*Field)
+				buf.WriteString(f.name)
-				if !f.anonymous {
+				buf.WriteByte(' ')
-					buf.WriteString(f.name)
+			}
-					buf.WriteByte(' ')
+			writeType(buf, f.typ)
-				}
+			if tag := t.Tag(i); tag != "" {
-				writeType(buf, f.typ)
+				fmt.Fprintf(buf, " %q", tag)
-				if tag := t.Tag(i); tag != "" {
-					fmt.Fprintf(buf, " %q", tag)
-				}
 			}
 		}
 		buf.WriteByte('}')

go/types/expr.go

@@ -182,7 +182,7 @@ func (check *checker) tag(t *ast.BasicLit) string {
 	return ""
 }
 
-func (check *checker) collectFields(scope *Scope, list *ast.FieldList, cycleOk bool) (tags []string) {
+func (check *checker) collectFields(scope *Scope, list *ast.FieldList, cycleOk bool) (fields []*Field, tags []string) {
 	if list == nil {
 		return
 	}
@@ -190,9 +190,8 @@ func (check *checker) collectFields(scope *Scope, list *ast.FieldList, cycleOk b
 	var typ Type   // current field typ
 	var tag string // current field tag
 	add := func(field *ast.Field, ident *ast.Ident, name string, anonymous bool, pos token.Pos) {
-		// TODO(gri): rethink this - at the moment we allocate only a prefix
 		if tag != "" && tags == nil {
-			tags = make([]string, scope.NumEntries())
+			tags = make([]string, len(fields))
 		}
 		if tags != nil {
 			tags = append(tags, tag)
@@ -200,6 +199,7 @@ func (check *checker) collectFields(scope *Scope, list *ast.FieldList, cycleOk b
 
 		fld := NewField(pos, check.pkg, name, typ, anonymous)
 		check.declare(scope, ident, fld)
+		fields = append(fields, fld)
 	}
 
 	for _, f := range list.List {
@@ -1159,7 +1159,7 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 			fields := utyp.fields
 			if _, ok := e.Elts[0].(*ast.KeyValueExpr); ok {
 				// all elements must have keys
-				visited := make([]bool, fields.NumEntries())
+				visited := make([]bool, len(fields))
 				for _, e := range e.Elts {
 					kv, _ := e.(*ast.KeyValueExpr)
 					if kv == nil {
@@ -1171,12 +1171,12 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 						check.errorf(kv.Pos(), "invalid field name %s in struct literal", kv.Key)
 						continue
 					}
-					i := utyp.fields.Index(check.pkg, key.Name)
+					i := utyp.index(check.pkg, key.Name)
 					if i < 0 {
 						check.errorf(kv.Pos(), "unknown field %s in struct literal", key.Name)
 						continue
 					}
-					fld := fields.At(i).(*Field)
+					fld := fields[i]
 					check.callIdent(key, fld)
 					// 0 <= i < len(fields)
 					if visited[i] {
@@ -1201,12 +1201,12 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 						continue
 					}
 					check.expr(x, e, nil, iota)
-					if i >= fields.NumEntries() {
+					if i >= len(fields) {
 						check.errorf(x.pos(), "too many values in struct literal")
 						break // cannot continue
 					}
 					// i < len(fields)
-					etyp := fields.At(i).Type()
+					etyp := fields[i].typ
 					if !check.assignment(x, etyp) {
 						if x.mode != invalid {
 							check.errorf(x.pos(), "cannot use %s as %s value in struct literal", x, etyp)
@@ -1214,7 +1214,7 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 						continue
 					}
 				}
-				if len(e.Elts) < fields.NumEntries() {
+				if len(e.Elts) < len(fields) {
 					check.errorf(e.Rbrace, "too few values in struct literal")
 					// ok to continue
 				}
@@ -1703,9 +1703,9 @@ func (check *checker) rawExpr(x *operand, e ast.Expr, hint Type, iota int, cycle
 
 	case *ast.StructType:
 		scope := NewScope(check.topScope)
-		tags := check.collectFields(scope, e.Fields, cycleOk)
+		fields, tags := check.collectFields(scope, e.Fields, cycleOk)
 		x.mode = typexpr
-		x.typ = &Struct{fields: scope, tags: tags}
+		x.typ = &Struct{fields: fields, tags: tags}
 
 	case *ast.FuncType:
 		scope := NewScope(check.topScope)

go/types/gcimporter.go

@@ -485,29 +485,27 @@ func (p *gcParser) parseField() (*Field, string) {
 // FieldList  = Field { ";" Field } .
 //
 func (p *gcParser) parseStructType() Type {
-	var fields *Scope // lazily initialized
+	var fields []*Field
 	var tags []string
 
 	p.expectKeyword("struct")
 	p.expect('{')
+	scope := NewScope(nil)
 	for i := 0; p.tok != '}'; i++ {
 		if i > 0 {
 			p.expect(';')
 		}
 		fld, tag := p.parseField()
-		// TODO(gri) same code in collectFields (expr.go) - factor?
 		if tag != "" && tags == nil {
 			tags = make([]string, i)
 		}
 		if tags != nil {
 			tags = append(tags, tag)
 		}
-		if fields == nil {
+		if alt := scope.Insert(fld); alt != nil {
-			fields = NewScope(nil)
-		}
-		if alt := fields.Insert(fld); alt != nil {
 			p.errorf("multiple fields named %s.%s", alt.Pkg().name, alt.Name())
 		}
+		fields = append(fields, fld)
 	}
 	p.expect('}')
 

go/types/operand.go

@@ -276,11 +276,7 @@ func lookupFieldBreadthFirst(list []embeddedType, pkg *Package, name string) (re
 			switch t := typ.underlying.(type) {
 			case *Struct:
 				// look for a matching field and collect embedded types
-				if t.fields == nil {
+				for i, f := range t.fields {
-					break
-				}
-				for i, obj := range t.fields.entries {
-					f := obj.(*Field)
 					if f.isMatch(pkg, name) {
 						assert(f.typ != nil)
 						if !potentialMatch(e.multiples, variable, f) {
@@ -375,12 +371,8 @@ func lookupField(typ Type, pkg *Package, name string) lookupResult {
 
 	switch t := typ.(type) {
 	case *Struct:
-		if t.fields == nil {
-			break
-		}
 		var next []embeddedType
-		for i, obj := range t.fields.entries {
+		for i, f := range t.fields {
-			f := obj.(*Field)
 			if f.isMatch(pkg, name) {
 				return lookupResult{variable, f, []int{i}}
 			}

go/types/predicates.go

@@ -74,11 +74,9 @@ func isComparable(typ Type) bool {
 		// assumes types are equal for pointers and channels
 		return true
 	case *Struct:
-		if t.fields != nil {
+		for _, f := range t.fields {
-			for _, f := range t.fields.entries {
+			if !isComparable(f.typ) {
-				if !isComparable(f.Type()) {
+				return false
-					return false
-				}
 			}
 		}
 		return true
@@ -131,16 +129,13 @@ func IsIdentical(x, y Type) bool {
 		// name. Lower-case field names from different packages are always different.
 		if y, ok := y.(*Struct); ok {
 			if x.NumFields() == y.NumFields() {
-				if x.fields != nil {
+				for i, f := range x.fields {
-					for i, obj := range x.fields.entries {
+					g := y.fields[i]
-						f := obj.(*Field)
+					if f.anonymous != g.anonymous ||
-						g := y.fields.At(i).(*Field)
+						x.Tag(i) != y.Tag(i) ||
-						if f.anonymous != g.anonymous ||
+						!f.isMatch(g.pkg, g.name) ||
-							x.Tag(i) != y.Tag(i) ||
+						!IsIdentical(f.typ, g.typ) {
-							!f.isMatch(g.pkg, g.name) ||
+						return false
-							!IsIdentical(f.typ, g.typ) {
-							return false
-						}
 					}
 				}
 				return true

go/types/resolver.go

@@ -483,10 +483,9 @@ func (check *checker) declareType(obj *TypeName, typ ast.Expr, cycleOk bool) {
 			if t.fields == nil {
 				break
 			}
-			for _, f := range t.fields.entries {
+			for _, f := range t.fields {
-				name := f.Name()
+				if m := scope.Lookup(nil, f.name); m != nil {
-				if m := scope.Lookup(nil, name); m != nil {
+					check.errorf(m.Pos(), "type %s has both field and method named %s", obj.name, f.name)
-					check.errorf(m.Pos(), "type %s has both field and method named %s", obj.name, name)
 					// ok to continue
 				}
 			}

go/types/scope.go

@@ -52,64 +52,54 @@ func (s *Scope) At(i int) Object {
 	return s.entries[i]
 }
 
-// Index returns the index of the scope entry with the given package
+// Lookup returns the object in scope s with the given package
-// (path) and name if such an entry exists in s; otherwise the result
+// and name if such an object exists; otherwise the result is nil.
-// is negative. A nil scope acts like an empty scope, and parent scopes
+// A nil scope acts like an empty scope, and parent scopes are ignored.
-// are ignored.
 //
 // If pkg != nil, both pkg.Path() and name are used to identify an
 // entry, per the Go rules for identifier equality. If pkg == nil,
 // only the name is used and the package path is ignored.
-func (s *Scope) Index(pkg *Package, name string) int {
+func (s *Scope) Lookup(pkg *Package, name string) Object {
 	if s == nil {
-		return -1 // empty scope
+		return nil // empty scope
 	}
 
 	// fast path: only the name must match
 	if pkg == nil {
-		for i, obj := range s.entries {
+		for _, obj := range s.entries {
 			if obj.Name() == name {
-				return i
+				return obj
 			}
 		}
-		return -1
+		return nil
 	}
 
 	// slow path: both pkg path and name must match
 	// TODO(gri) if packages were canonicalized, we could just compare the packages
-	for i, obj := range s.entries {
+	for _, obj := range s.entries {
 		// spec:
 		// "Two identifiers are different if they are spelled differently,
 		// or if they appear in different packages and are not exported.
 		// Otherwise, they are the same."
 		if obj.Name() == name && (ast.IsExported(name) || obj.Pkg().path == pkg.path) {
-			return i
+			return obj
 		}
 	}
 
 	// not found
-	return -1
+	return nil
 
 	// TODO(gri) Optimize Lookup by also maintaining a map representation
 	//           for larger scopes.
 }
 
-// Lookup returns the scope entry At(i) for i = Index(pkg, name), if i >= 0.
-// Otherwise it returns nil.
-func (s *Scope) Lookup(pkg *Package, name string) Object {
-	if i := s.Index(pkg, name); i >= 0 {
-		return s.At(i)
-	}
-	return nil
-}
-
 // LookupParent follows the parent chain of scopes starting with s until it finds
-// a scope where Lookup(nil, name) returns a non-nil entry, and then returns that
+// a scope where Lookup(nil, name) returns a non-nil object, and then returns that
-// entry. If no such scope exists, the result is nil.
+// object. If no such scope exists, the result is nil.
 func (s *Scope) LookupParent(name string) Object {
 	for s != nil {
-		if i := s.Index(nil, name); i >= 0 {
+		if obj := s.Lookup(nil, name); obj != nil {
-			return s.At(i)
+			return obj
 		}
 		s = s.parent
 	}

go/types/sizes.go

@@ -50,7 +50,7 @@ func (ctxt *Context) offsetof(typ Type, index []int) int64 {
 			return -1
 		}
 		o += ctxt.offsetsof(s)[i]
-		typ = s.fields.At(i).Type()
+		typ = s.fields[i].typ
 	}
 	return o
 }
@@ -84,12 +84,9 @@ func DefaultAlignof(typ Type) int64 {
 		// is the largest of the values unsafe.Alignof(x.f) for each
 		// field f of x, but at least 1."
 		max := int64(1)
-		if t.fields != nil {
+		for _, f := range t.fields {
-			for _, obj := range t.fields.entries {
+			if a := DefaultAlignof(f.typ); a > max {
-				f := obj.(*Field)
+				max = a
-				if a := DefaultAlignof(f.typ); a > max {
-					max = a
-				}
 			}
 		}
 		return max
@@ -113,15 +110,10 @@ func align(x, a int64) int64 {
 
 // DefaultOffsetsof implements the default field offset computation
 // for unsafe.Offsetof. It is used if Context.Offsetsof == nil.
-func DefaultOffsetsof(fields *Scope) []int64 {
+func DefaultOffsetsof(fields []*Field) []int64 {
-	n := fields.NumEntries()
+	offsets := make([]int64, len(fields))
-	if n == 0 {
-		return nil
-	}
-	offsets := make([]int64, n)
 	var o int64
-	for i, obj := range fields.entries {
+	for i, f := range fields {
-		f := obj.(*Field)
 		a := DefaultAlignof(f.typ)
 		o = align(o, a)
 		offsets[i] = o
@@ -162,7 +154,7 @@ func DefaultSizeof(typ Type) int64 {
 			offsets = DefaultOffsetsof(t.fields)
 			t.offsets = offsets
 		}
-		return offsets[n-1] + DefaultSizeof(t.fields.At(n-1).Type())
+		return offsets[n-1] + DefaultSizeof(t.fields[n-1].typ)
 	case *Signature:
 		return DefaultPtrSize * 2
 	}

go/types/testdata/builtins.src

@@ -410,6 +410,14 @@ func _Sizeof() {
 
 	var y2 S2
 	assert(unsafe.Sizeof(y2) == 8)
+
+	// test case for issue 5670
+	type T struct {
+		a int32
+		_ int32
+		c int32
+	}
+	assert(unsafe.Sizeof(T{}) == 12)
 }
 
 // self-testing only

go/types/types.go

@@ -132,17 +132,26 @@ func (s *Slice) Elem() Type { return s.elt }
 
 // A Struct represents a struct type.
 type Struct struct {
-	fields  *Scope
+	fields  []*Field
 	tags    []string // field tags; nil if there are no tags
 	offsets []int64  // field offsets in bytes, lazily computed
 }
 
-func NewStruct(fields *Scope, tags []string) *Struct {
+// NewStruct returns a new struct with the given fields and corresponding field tags.
+// If a field with index i has a tag, tags[i] must be that tag, but len(tags) may be
+// only as long as required to hold the tag with the largest index i. Consequently,
+// if no field has a tag, tags may be nil.
+func NewStruct(fields []*Field, tags []string) *Struct {
 	return &Struct{fields: fields, tags: tags}
 }
 
-func (s *Struct) NumFields() int     { return s.fields.NumEntries() }
+// NumFields returns the number of fields in the struct (including blank and anonymous fields).
-func (s *Struct) Field(i int) *Field { return s.fields.At(i).(*Field) }
+func (s *Struct) NumFields() int { return len(s.fields) }
+
+// Field returns the i'th field for 0 <= i < NumFields().
+func (s *Struct) Field(i int) *Field { return s.fields[i] }
+
+// Tag returns the i'th field tag for 0 <= i < NumFields().
 func (s *Struct) Tag(i int) string {
 	if i < len(s.tags) {
 		return s.tags[i]
@@ -150,6 +159,21 @@ func (s *Struct) Tag(i int) string {
 	return ""
 }
 
+// Index returns the index for the field in s with matching package and name.
+// TODO(gri) should this be exported?
+func (s *Struct) index(pkg *Package, name string) int {
+	for i, f := range s.fields {
+		// spec:
+		// "Two identifiers are different if they are spelled differently,
+		// or if they appear in different packages and are not exported.
+		// Otherwise, they are the same."
+		if f.name == name && (ast.IsExported(name) || f.pkg.path == pkg.path) {
+			return i
+		}
+	}
+	return -1
+}
+
 // A Pointer represents a pointer type.
 type Pointer struct {
 	base Type
@@ -264,6 +288,7 @@ func (b *Builtin) Name() string {
 
 // An Interface represents an interface type.
 type Interface struct {
+	// TODO(gri) Change back to a sorted slice of methods.
 	methods *Scope // may be nil
 }
 
@@ -315,7 +340,8 @@ func (c *Chan) Elem() Type { return c.elt }
 type Named struct {
 	obj        *TypeName // corresponding declared object
 	underlying Type      // nil if not fully declared yet; never a *Named
-	methods    *Scope    // directly associated methods (not the method set of this type); may be nil
+	// TODO(gri): change back to a sorted slice of methods
+	methods *Scope // directly associated methods (not the method set of this type); may be nil
 }
 
 // NewNamed returns a new named type for the given type name, underlying type, and associated methods.