Skip to content

reflect: implement NumMethod and Implements #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions compiler/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,34 @@ func (c *compilerContext) getTypeCode(typ types.Type) llvm.Value {
structGlobal := c.makeStructTypeFields(typ)
references = llvm.ConstBitCast(structGlobal, global.Type())
}
if !references.IsNil() {
methodSet := types.NewMethodSet(typ)
numMethods := 0
_, isInterface := typ.Underlying().(*types.Interface)
for i := 0; i < methodSet.Len(); i++ {
// NumMethod (unintentionally) includes unexported methods for
// interface types but excludes unexported methods for other types.
// This makes sure we're bug-compatible with the Go reflect package.
// For a discussion, see:
// https://github.com/golang/go/issues/22075 (original bug)
// https://github.com/golang/go/issues/42123 (revert of bugfix)
if methodSet.At(i).Obj().Exported() || isInterface {
numMethods++
}
}
if !references.IsNil() || numMethods != 0 || isInterface {
// Set the 'references' field of the runtime.typecodeID struct.
globalValue := llvm.ConstNull(global.Type().ElementType())
globalValue = llvm.ConstInsertValue(globalValue, references, []uint32{0})
if !references.IsNil() {
globalValue = llvm.ConstInsertValue(globalValue, references, []uint32{0})
}
if length != 0 {
lengthValue := llvm.ConstInt(c.uintptrType, uint64(length), false)
globalValue = llvm.ConstInsertValue(globalValue, lengthValue, []uint32{1})
}
if numMethods != 0 {
numMethodsValue := llvm.ConstInt(c.uintptrType, uint64(numMethods), false)
globalValue = llvm.ConstInsertValue(globalValue, numMethodsValue, []uint32{2})
}
global.SetInitializer(globalValue)
global.SetLinkage(llvm.PrivateLinkage)
}
Expand Down Expand Up @@ -187,7 +207,7 @@ func getTypeCodeName(t types.Type) string {
case *types.Interface:
methods := make([]string, t.NumMethods())
for i := 0; i < t.NumMethods(); i++ {
methods[i] = getTypeCodeName(t.Method(i).Type())
methods[i] = t.Method(i).Name() + ":" + getTypeCodeName(t.Method(i).Type())
}
return "interface:" + "{" + strings.Join(methods, ",") + "}"
case *types.Map:
Expand Down
15 changes: 14 additions & 1 deletion src/reflect/sidetables.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,22 @@ import (
"unsafe"
)

// This stores the number of methods (for Type.NumMethod()) for each named basic
// type. It is indexed by the named type number.
//go:extern reflect.namedBasicNumMethodSidetable
var namedBasicNumMethodSidetable byte

// This stores a varint for each named type. Named types are identified by their
// name instead of by their type. The named types stored in this struct are
// non-basic types: pointer, struct, and channel.
// non-basic types: pointer, struct, channel, and interface.
//go:extern reflect.namedNonBasicTypesSidetable
var namedNonBasicTypesSidetable uintptr

// This stores the number of methods (for Type.NumMethods()) for each named
// non-basic type. It is indexed by the named type number.
//go:extern reflect.namedNonBasicNumMethodSidetable
var namedNonBasicNumMethodSidetable byte

//go:extern reflect.structTypesSidetable
var structTypesSidetable byte

Expand All @@ -19,6 +29,9 @@ var structNamesSidetable byte
//go:extern reflect.arrayTypesSidetable
var arrayTypesSidetable byte

//go:extern reflect.interfaceTypesSidetable
var interfaceTypesSidetable byte

// readStringSidetable reads a string from the given table (like
// structNamesSidetable) and returns this string. No heap allocation is
// necessary because it makes the string point directly to the raw bytes of the
Expand Down
75 changes: 69 additions & 6 deletions src/reflect/type.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,20 @@ func (t Type) Kind() Kind {
}
}

// isBasic returns true if (and only if) this is a basic type.
func (t Type) isBasic() bool {
return t%2 == 0
}

// isNamed returns true if (and only if) this is a named type.
func (t Type) isNamed() bool {
if t.isBasic() {
return t>>6 != 0
} else {
return (t>>4)%2 != 0
}
}

// Elem returns the element type for channel, slice and array types, the
// pointed-to value for pointer types, and the key type for map types.
func (t Type) Elem() Type {
Expand All @@ -170,7 +184,7 @@ func (t Type) Elem() Type {
func (t Type) stripPrefix() Type {
// Look at the 'n' bit in the type code (see the top of this file) to see
// whether this is a named type.
if (t>>4)%2 != 0 {
if t.isNamed() {
// This is a named type. The data is stored in a sidetable.
namedTypeNum := t >> 5
n := *(*uintptr)(unsafe.Pointer(uintptr(unsafe.Pointer(&namedNonBasicTypesSidetable)) + uintptr(namedTypeNum)*unsafe.Sizeof(uintptr(0))))
Expand All @@ -188,7 +202,11 @@ func (t Type) Field(i int) StructField {
}
structIdentifier := t.stripPrefix()

numField, p := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&structTypesSidetable)) + uintptr(structIdentifier)))
// Skip past the NumMethod field.
_, p := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&structTypesSidetable)) + uintptr(structIdentifier)))

// Read the NumField field.
numField, p := readVarint(p)
if uint(i) >= uint(numField) {
panic("reflect: field index out of range")
}
Expand Down Expand Up @@ -287,7 +305,10 @@ func (t Type) NumField() int {
panic(&TypeError{"NumField"})
}
structIdentifier := t.stripPrefix()
n, _ := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&structTypesSidetable)) + uintptr(structIdentifier)))
// Skip past the NumMethod field.
_, p := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&structTypesSidetable)) + uintptr(structIdentifier)))
// Read the NumField field.
n, _ := readVarint(p)
return int(n)
}

Expand Down Expand Up @@ -405,10 +426,23 @@ func (t Type) AssignableTo(u Type) bool {
}

func (t Type) Implements(u Type) bool {
if t.Kind() != Interface {
if u.Kind() != Interface {
panic("reflect: non-interface type passed to Type.Implements")
}
return u.AssignableTo(t)
if u.NumMethod() == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is subtly incorrect, as an interface can include unexported methods. I am not sure though whether there is an easy way to solve this right now. If you wish to leave this for further work, consider adding a comment about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant: golang/go#22075

Copy link
Member Author

@aykevl aykevl Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this might be a bold thing to do but what if we reverted to the old (incorrect) behavior? It would make the implementation of Implements a lot easier, at least as long as we don't need to fully support it. The limited implementation here should be enough for packages like encoding/json.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By which you mean, the same behavior as Go currently has? I would agree with that, lets implement the same bugs 🤡

Copy link
Member Author

@aykevl aykevl Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the same buggy behavior (to be bug-compatible 😉). Reading the bug report the final decision on whether the docs or the implementation should be fixed does not seem to be made yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this is slightly less trivial than I expected: I discovered that anonymous interface types (like var x interface{ F() }) do not have the correct NumMethod.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the status with this one, @aykevl ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to look into this further, haven't done that yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder that we still need to resolve this. 😸

// Empty interface (not even unexported methods, see NumNethod
// implementation comments), therefore implements any type.
return true
}
// Type u has (possibly unexported) methods.
if t.Kind() == Interface && t.NumMethod() == 0 {
// Type t has no methods (not even unexported methods since it is an
// interface), while u has. Therefore, t cannot implement u.
return false
}
// Now we'd have to compare individual methods.
// This has not yet been implemented.
panic("unimplemented: (reflect.Type).Implements()")
}

// Comparable returns whether values of this type can be compared to each other.
Expand Down Expand Up @@ -454,7 +488,36 @@ func (t Type) ConvertibleTo(u Type) bool {
}

func (t Type) NumMethod() int {
panic("unimplemented: (reflect.Type).NumMethod()")
if t.isBasic() {
if !t.isNamed() {
// Not a named type, so can't have methods.
return 0
}
// Named type methods are stored in a sidetable.
namedTypeNum := t >> 6
return int(*(*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(&namedBasicNumMethodSidetable)) + uintptr(namedTypeNum) - 1)))
} else {
if !t.isNamed() {
// Most non-named types cannot have methods. However, structs can
// because they can embed named values.
switch t.Kind() {
case Struct:
structIdentifier := t.stripPrefix()
numMethod, _ := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&structTypesSidetable)) + uintptr(structIdentifier)))
return int(numMethod)
case Interface:
interfaceIdentifier := t.stripPrefix()
numMethod, _ := readVarint(unsafe.Pointer(uintptr(unsafe.Pointer(&interfaceTypesSidetable)) + uintptr(interfaceIdentifier)))
return int(numMethod)
default:
// This type cannot have methods.
return 0
}
}
// Named non-basic type, so read the number of methods from a sidetable.
namedTypeNum := t >> 5
return int(*(*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(&namedNonBasicNumMethodSidetable)) + uintptr(namedTypeNum))))
}
}

func (t Type) Name() string {
Expand Down
4 changes: 4 additions & 0 deletions src/reflect/value.go
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,10 @@ func maskAndShift(value, offset, size uintptr) uintptr {
return (uintptr(value) >> (offset * 8)) & mask
}

func (v Value) NumMethod() int {
return v.Type().NumMethod()
}

func (v Value) MapKeys() []Value {
panic("unimplemented: (reflect.Value).MapKeys()")
}
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ type typecodeID struct {

// The array length, for array types.
length uintptr

// Number of methods on this type. Usually 0 for non-named types.
numMethods uintptr
}

// structField is used by the compiler to pass information to the interface
Expand Down
27 changes: 26 additions & 1 deletion testdata/reflect.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,18 @@ type (
next *linkedList `description:"chain"`
foo int
}
myitf interface {
Foo()
bar()
baz()
}
)

// To test Type.NumMethod.
func (n myint) foo() {}
func (n myint) bar() {}
func (n myint) Baz() {}

func main() {
println("matching types")
println(reflect.TypeOf(int(3)) == reflect.TypeOf(int(5)))
Expand Down Expand Up @@ -111,6 +121,16 @@ func main() {
&linkedList{
foo: 42,
},
// interfaces
struct {
a interface{}
b interface {
Foo()
bar()
}
c myitf
d *myitf
}{},
} {
showValue(reflect.ValueOf(v), "")
}
Expand Down Expand Up @@ -284,6 +304,9 @@ func showValue(rv reflect.Value, indent string) {
if !rt.Comparable() {
print(" comparable=false")
}
if rt.NumMethod() != 0 {
print(" methods=", rt.NumMethod())
}
println()
switch rt.Kind() {
case reflect.Bool:
Expand Down Expand Up @@ -339,7 +362,9 @@ func showValue(rv reflect.Value, indent string) {
for i := 0; i < rv.NumField(); i++ {
field := rt.Field(i)
println(indent+" field:", i, field.Name)
println(indent+" tag:", field.Tag)
if field.Tag != "" {
println(indent+" tag:", field.Tag)
}
println(indent+" embedded:", field.Anonymous)
showValue(rv.Field(i), indent+" ")
}
Expand Down
41 changes: 26 additions & 15 deletions testdata/reflect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ reflect type: complex64
complex: (+1.200000e+000+3.000000e-001i)
reflect type: complex128
complex: (+1.300000e+000+4.000000e-001i)
reflect type: int
reflect type: int methods=1
int: 32
reflect type: string
string: foo 3
Expand All @@ -77,7 +77,7 @@ reflect type: ptr
reflect type: ptr
pointer: true interface
nil: false
reflect type: interface settable=true
reflect type: interface settable=true methods=1
interface
nil: true
reflect type: ptr
Expand Down Expand Up @@ -230,28 +230,24 @@ reflect type: map comparable=false
nil: false
reflect type: struct
struct: 0
reflect type: struct
reflect type: struct methods=1
struct: 1
field: 0 error
tag:
embedded: true
reflect type: interface
reflect type: interface methods=1
interface
nil: true
reflect type: struct
struct: 3
field: 0 a
tag:
embedded: false
reflect type: uint8
uint: 42
field: 1 b
tag:
embedded: false
reflect type: int16
int: 321
field: 2 c
tag:
embedded: false
reflect type: int8
int: 123
Expand All @@ -263,27 +259,22 @@ reflect type: struct comparable=false
reflect type: int
int: 5
field: 1 some
tag:
embedded: false
reflect type: struct
struct: 2
field: 0 X
tag:
embedded: false
reflect type: int16
int: -5
field: 1 Y
tag:
embedded: false
reflect type: int16
int: 3
field: 2 zero
tag:
embedded: false
reflect type: struct
struct: 0
field: 3 buf
tag:
embedded: false
reflect type: slice comparable=false
slice: uint8 2 2
Expand All @@ -296,7 +287,6 @@ reflect type: struct comparable=false
reflect type: uint8
uint: 111
field: 4 Buf
tag:
embedded: false
reflect type: slice comparable=false
slice: uint8 1 1
Expand All @@ -317,10 +307,31 @@ reflect type: ptr
pointer: false struct
nil: true
field: 1 foo
tag:
embedded: false
reflect type: int
int: 42
reflect type: struct
struct: 4
field: 0 a
embedded: false
reflect type: interface
interface
nil: true
field: 1 b
embedded: false
reflect type: interface methods=2
interface
nil: true
field: 2 c
embedded: false
reflect type: interface methods=3
interface
nil: true
field: 3 d
embedded: false
reflect type: ptr
pointer: false interface
nil: true

sizes:
int8 1 8
Expand Down
Loading