From 5efab5e9c05a793570485077d786a86ff7f0a5b8 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 11 Jun 2013 19:54:47 -0700 Subject: [PATCH] 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 | 2 +- go/types/errors.go | 25 ++++++++++----------- go/types/expr.go | 22 +++++++++---------- go/types/gcimporter.go | 10 ++++----- go/types/operand.go | 12 ++-------- go/types/predicates.go | 25 +++++++++------------ go/types/resolver.go | 7 +++--- go/types/scope.go | 40 +++++++++++++--------------------- go/types/sizes.go | 24 +++++++------------- go/types/testdata/builtins.src | 8 +++++++ go/types/types.go | 36 +++++++++++++++++++++++++----- 11 files changed, 104 insertions(+), 107 deletions(-) diff --git a/go/types/api.go b/go/types/api.go index 357c541c715..2e19d5ef797 100644 --- a/go/types/api.go +++ b/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 diff --git a/go/types/errors.go b/go/types/errors.go index 105f0254028..8e01716ec8a 100644 --- a/go/types/errors.go +++ b/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, obj := range t.fields.entries { - if i > 0 { - buf.WriteString("; ") - } - f := obj.(*Field) - if !f.anonymous { - buf.WriteString(f.name) - buf.WriteByte(' ') - } - writeType(buf, f.typ) - if tag := t.Tag(i); tag != "" { - fmt.Fprintf(buf, " %q", tag) - } + for i, f := range t.fields { + if i > 0 { + buf.WriteString("; ") + } + if !f.anonymous { + buf.WriteString(f.name) + buf.WriteByte(' ') + } + writeType(buf, f.typ) + if tag := t.Tag(i); tag != "" { + fmt.Fprintf(buf, " %q", tag) } } buf.WriteByte('}') diff --git a/go/types/expr.go b/go/types/expr.go index 38bbc4bb15f..f18f2ddce12 100644 --- a/go/types/expr.go +++ b/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) diff --git a/go/types/gcimporter.go b/go/types/gcimporter.go index 05d9746acf6..5384e2c3daf 100644 --- a/go/types/gcimporter.go +++ b/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 { - fields = NewScope(nil) - } - if alt := fields.Insert(fld); alt != nil { + if alt := scope.Insert(fld); alt != nil { p.errorf("multiple fields named %s.%s", alt.Pkg().name, alt.Name()) } + fields = append(fields, fld) } p.expect('}') diff --git a/go/types/operand.go b/go/types/operand.go index e30043aebaf..486e84b203e 100644 --- a/go/types/operand.go +++ b/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 { - break - } - for i, obj := range t.fields.entries { - f := obj.(*Field) + for i, f := range t.fields { 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 { - f := obj.(*Field) + for i, f := range t.fields { if f.isMatch(pkg, name) { return lookupResult{variable, f, []int{i}} } diff --git a/go/types/predicates.go b/go/types/predicates.go index c8c5224fa94..882d6cb514c 100644 --- a/go/types/predicates.go +++ b/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.entries { - if !isComparable(f.Type()) { - return false - } + for _, f := range t.fields { + if !isComparable(f.typ) { + 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, obj := range x.fields.entries { - f := obj.(*Field) - g := y.fields.At(i).(*Field) - if f.anonymous != g.anonymous || - x.Tag(i) != y.Tag(i) || - !f.isMatch(g.pkg, g.name) || - !IsIdentical(f.typ, g.typ) { - return false - } + for i, f := range x.fields { + g := y.fields[i] + if f.anonymous != g.anonymous || + x.Tag(i) != y.Tag(i) || + !f.isMatch(g.pkg, g.name) || + !IsIdentical(f.typ, g.typ) { + return false } } return true diff --git a/go/types/resolver.go b/go/types/resolver.go index a780707dc22..7fe69f1d912 100644 --- a/go/types/resolver.go +++ b/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 { - name := f.Name() - if m := scope.Lookup(nil, name); m != nil { - check.errorf(m.Pos(), "type %s has both field and method named %s", obj.name, name) + for _, f := range t.fields { + if m := scope.Lookup(nil, f.name); m != nil { + check.errorf(m.Pos(), "type %s has both field and method named %s", obj.name, f.name) // ok to continue } } diff --git a/go/types/scope.go b/go/types/scope.go index a150593e9ab..5271b84ac9a 100644 --- a/go/types/scope.go +++ b/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 -// (path) and name if such an entry exists in s; otherwise the result -// is negative. A nil scope acts like an empty scope, and parent scopes -// are ignored. +// Lookup returns the object in scope s with the given package +// and name if such an object exists; otherwise the result is nil. +// A nil scope acts like an empty scope, and parent scopes 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 -// entry. If no such scope exists, the result is nil. +// a scope where Lookup(nil, name) returns a non-nil object, and then returns that +// 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 { - return s.At(i) + if obj := s.Lookup(nil, name); obj != nil { + return obj } s = s.parent } diff --git a/go/types/sizes.go b/go/types/sizes.go index ea327591968..9154e12542d 100644 --- a/go/types/sizes.go +++ b/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 _, obj := range t.fields.entries { - f := obj.(*Field) - if a := DefaultAlignof(f.typ); a > max { - max = a - } + for _, f := range t.fields { + 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 { - n := fields.NumEntries() - if n == 0 { - return nil - } - offsets := make([]int64, n) +func DefaultOffsetsof(fields []*Field) []int64 { + offsets := make([]int64, len(fields)) var o int64 - for i, obj := range fields.entries { - f := obj.(*Field) + for i, f := range fields { 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 } diff --git a/go/types/testdata/builtins.src b/go/types/testdata/builtins.src index 43f3e506f02..6df085dfa38 100644 --- a/go/types/testdata/builtins.src +++ b/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 diff --git a/go/types/types.go b/go/types/types.go index ba1d5025b55..bfd954d8bf8 100644 --- a/go/types/types.go +++ b/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() } -func (s *Struct) Field(i int) *Field { return s.fields.At(i).(*Field) } +// NumFields returns the number of fields in the struct (including blank and anonymous fields). +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.