From d349fa25dfe2a86c01620f8b049c5e78e46759f3 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Wed, 30 Aug 2017 14:17:24 -0700 Subject: [PATCH] cmd/compile: fix and improve struct field reflect information The previous logic was overly complicated, generated suboptimally encoded struct type descriptors, and mishandled embeddings of predeclared universal types. Fixes #21122. Fixes #21353. Fixes #21696. Fixes #21702. Updates #21357. Change-Id: If34761fa6dbe4af2af59dee501e7f30845320376 Reviewed-on: https://go-review.googlesource.com/60410 Run-TryBot: Matthew Dempsky TryBot-Result: Gobot Gobot Reviewed-by: David Crawshaw --- src/cmd/compile/internal/gc/reflect.go | 50 +++++---------- src/reflect/all_test.go | 88 ++++++++++++++++++++++++++ src/reflect/type.go | 21 ++---- src/runtime/type.go | 6 +- 4 files changed, 111 insertions(+), 54 deletions(-) diff --git a/src/cmd/compile/internal/gc/reflect.go b/src/cmd/compile/internal/gc/reflect.go index c436c55c6a..c5730dbcb8 100644 --- a/src/cmd/compile/internal/gc/reflect.go +++ b/src/cmd/compile/internal/gc/reflect.go @@ -568,30 +568,12 @@ func dgopkgpathOff(s *obj.LSym, ot int, pkg *types.Pkg) int { return dsymptrOff(s, ot, pkg.Pathsym, 0) } -// isExportedField reports whether a struct field is exported. -// It also returns the package to use for PkgPath for an unexported field. -func isExportedField(ft *types.Field) (bool, *types.Pkg) { - if ft.Sym != nil && ft.Embedded == 0 { - return exportname(ft.Sym.Name), ft.Sym.Pkg - } - if ft.Type.Sym != nil && - (ft.Type.Sym.Pkg == builtinpkg || !exportname(ft.Type.Sym.Name)) { - return false, ft.Type.Sym.Pkg - } - return true, nil -} - // dnameField dumps a reflect.name for a struct field. func dnameField(lsym *obj.LSym, ot int, spkg *types.Pkg, ft *types.Field) int { - var name string - if ft.Sym != nil { - name = ft.Sym.Name + if !exportname(ft.Sym.Name) && ft.Sym.Pkg != spkg { + Fatalf("package mismatch for %v", ft.Sym) } - isExported, fpkg := isExportedField(ft) - if isExported || fpkg == spkg { - fpkg = nil - } - nsym := dname(name, ft.Note, fpkg, isExported) + nsym := dname(ft.Sym.Name, ft.Note, nil, exportname(ft.Sym.Name)) return dsymptr(lsym, ot, nsym, 0) } @@ -1356,21 +1338,21 @@ ok: n++ } - ot = dcommontype(lsym, ot, t) - pkg := localpkg - if t.Sym != nil { - pkg = t.Sym.Pkg - } else { - // Unnamed type. Grab the package from the first field, if any. - for _, f := range t.Fields().Slice() { - if f.Embedded != 0 { - continue - } - pkg = f.Sym.Pkg + // All non-exported struct field names within a struct + // type must originate from a single package. By + // identifying and recording that package within the + // struct type descriptor, we can omit that + // information from the field descriptors. + var spkg *types.Pkg + for _, f := range t.Fields().Slice() { + if !exportname(f.Sym.Name) { + spkg = f.Sym.Pkg break } } - ot = dgopkgpath(lsym, ot, pkg) + + ot = dcommontype(lsym, ot, t) + ot = dgopkgpath(lsym, ot, spkg) ot = dsymptr(lsym, ot, lsym, ot+3*Widthptr+uncommonSize(t)) ot = duintptr(lsym, ot, uint64(n)) ot = duintptr(lsym, ot, uint64(n)) @@ -1380,7 +1362,7 @@ ok: for _, f := range t.Fields().Slice() { // ../../../../runtime/type.go:/structField - ot = dnameField(lsym, ot, pkg, f) + ot = dnameField(lsym, ot, spkg, f) ot = dsymptr(lsym, ot, dtypesym(f.Type).Linksym(), 0) offsetAnon := uint64(f.Offset) << 1 if offsetAnon>>1 != uint64(f.Offset) { diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go index 962c326c03..7c83364a45 100644 --- a/src/reflect/all_test.go +++ b/src/reflect/all_test.go @@ -321,6 +321,89 @@ func TestSetValue(t *testing.T) { } } +func TestCanSetField(t *testing.T) { + type embed struct{ x, X int } + type Embed struct{ x, X int } + type S1 struct { + embed + x, X int + } + type S2 struct { + *embed + x, X int + } + type S3 struct { + Embed + x, X int + } + type S4 struct { + *Embed + x, X int + } + + type testCase struct { + index []int + canSet bool + } + tests := []struct { + val Value + cases []testCase + }{{ + val: ValueOf(&S1{}), + cases: []testCase{ + {[]int{0}, false}, + {[]int{0, 0}, false}, + {[]int{0, 1}, true}, + {[]int{1}, false}, + {[]int{2}, true}, + }, + }, { + val: ValueOf(&S2{embed: &embed{}}), + cases: []testCase{ + {[]int{0}, false}, + {[]int{0, 0}, false}, + {[]int{0, 1}, true}, + {[]int{1}, false}, + {[]int{2}, true}, + }, + }, { + val: ValueOf(&S3{}), + cases: []testCase{ + {[]int{0}, true}, + {[]int{0, 0}, false}, + {[]int{0, 1}, true}, + {[]int{1}, false}, + {[]int{2}, true}, + }, + }, { + val: ValueOf(&S4{Embed: &Embed{}}), + cases: []testCase{ + {[]int{0}, true}, + {[]int{0, 0}, false}, + {[]int{0, 1}, true}, + {[]int{1}, false}, + {[]int{2}, true}, + }, + }} + + for _, tt := range tests { + t.Run(tt.val.Type().Name(), func(t *testing.T) { + for _, tc := range tt.cases { + f := tt.val + for _, i := range tc.index { + if f.Kind() == Ptr { + f = f.Elem() + } + f = f.Field(i) + } + if got := f.CanSet(); got != tc.canSet { + t.Errorf("CanSet() = %v, want %v", got, tc.canSet) + } + } + }) + } +} + var _i = 7 var valueToStringTests = []pair{ @@ -2357,10 +2440,13 @@ func TestImportPath(t *testing.T) { } func TestFieldPkgPath(t *testing.T) { + type x int typ := TypeOf(struct { Exported string unexported string OtherPkgFields + int // issue 21702 + *x // issue 21122 }{}) type pkgpathTest struct { @@ -2387,6 +2473,8 @@ func TestFieldPkgPath(t *testing.T) { {[]int{2}, "", true}, // OtherPkgFields {[]int{2, 0}, "", false}, // OtherExported {[]int{2, 1}, "reflect", false}, // otherUnexported + {[]int{3}, "reflect_test", true}, // int + {[]int{4}, "reflect_test", true}, // *x }) type localOtherPkgFields OtherPkgFields diff --git a/src/reflect/type.go b/src/reflect/type.go index 9f02219c8e..0ecc2b3bca 100644 --- a/src/reflect/type.go +++ b/src/reflect/type.go @@ -1216,10 +1216,7 @@ func (t *structType) Field(i int) (f StructField) { f.Name = p.name.name() f.Anonymous = p.anon() if !p.name.isExported() { - f.PkgPath = p.name.pkgPath() - if f.PkgPath == "" { - f.PkgPath = t.pkgPath.name() - } + f.PkgPath = t.pkgPath.name() } if tag := p.name.tag(); tag != "" { f.Tag = StructTag(tag) @@ -1677,6 +1674,9 @@ func haveIdenticalUnderlyingType(T, V *rtype, cmpTags bool) bool { if len(t.fields) != len(v.fields) { return false } + if t.pkgPath.name() != v.pkgPath.name() { + return false + } for i := range t.fields { tf := &t.fields[i] vf := &v.fields[i] @@ -1692,19 +1692,6 @@ func haveIdenticalUnderlyingType(T, V *rtype, cmpTags bool) bool { if tf.offsetAnon != vf.offsetAnon { return false } - if !tf.name.isExported() { - tp := tf.name.pkgPath() - if tp == "" { - tp = t.pkgPath.name() - } - vp := vf.name.pkgPath() - if vp == "" { - vp = v.pkgPath.name() - } - if tp != vp { - return false - } - } } return true } diff --git a/src/runtime/type.go b/src/runtime/type.go index bf54d54eb4..b3df3353ce 100644 --- a/src/runtime/type.go +++ b/src/runtime/type.go @@ -655,15 +655,15 @@ func typesEqual(t, v *_type, seen map[_typePair]struct{}) bool { if len(st.fields) != len(sv.fields) { return false } + if st.pkgPath.name() != sv.pkgPath.name() { + return false + } for i := range st.fields { tf := &st.fields[i] vf := &sv.fields[i] if tf.name.name() != vf.name.name() { return false } - if tf.name.pkgPath() != vf.name.pkgPath() { - return false - } if !typesEqual(tf.typ, vf.typ, seen) { return false }