Skip to content
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

Fix "Panic: reflect.Value.Interface: cannot return value obtained fro… #5

Closed
wants to merge 3 commits 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
78 changes: 77 additions & 1 deletion sheriff.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"reflect"
"strings"
"unsafe"

"github.com/hashicorp/go-version"
)
Expand Down Expand Up @@ -144,7 +145,7 @@ func Marshal(options *Options, data interface{}) (interface{}, error) {
//
// There is support for types implementing the Marshaller interface, arbitrary structs, slices, maps and base types.
func marshalValue(options *Options, v reflect.Value) (interface{}, error) {
val := v.Interface()
val := interfaceOf(v)

if marshaller, ok := val.(Marshaller); ok {
return marshaller.Marshal(options)
Expand Down Expand Up @@ -218,3 +219,78 @@ func listContains(a []string, b []string) bool {
}
return false
}

// interfaceOf returns v.Interface() even if v.CanInterface() == false.
// This enables us to call fmt.Printf on a value even if it's derived
// from inside an unexported field.
// See https://code.google.com/p/go/issues/detail?id=8965
// for a possible future alternative to this hack.
// from https://github.com/juju/testing/blob/master/checkers/deepequal.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

This enables us to call fmt.Printf on a value even if it's derived

We don't use it for fmt.Printf or am I missing something? I assume this is a verbatim copy of the comment in deepequal. I think we should adapt that comment to match our case.
Also this sentence is wrong as it's in Go already:

for a possible future alternative to this hack.

func interfaceOf(v reflect.Value) interface{} {
if !v.IsValid() {
return nil
}
return bypassCanInterface(v).Interface()
}

type flag uintptr

var flagRO flag

// constants copied from reflect/value.go
const (
// The value of flagRO up to and including Go 1.3.
flagRO1p3 = 1 << 0

// The value of flagRO from Go 1.4.
flagRO1p4 = 1 << 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsure if we should not just use this value because we don't test sheriff in older versions than 1.6 anyway.

)

var flagValOffset = func() uintptr {
field, ok := reflect.TypeOf(reflect.Value{}).FieldByName("flag")
if !ok {
panic("reflect.Value has no flag field")
}
return field.Offset
}()

func flagField(v *reflect.Value) *flag {
return (*flag)(unsafe.Pointer(uintptr(unsafe.Pointer(v)) + flagValOffset))
}

// bypassCanInterface returns a version of v that
// bypasses the CanInterface check.
func bypassCanInterface(v reflect.Value) reflect.Value {
if !v.IsValid() || v.CanInterface() {
return v
}
*flagField(&v) &^= flagRO
return v
}

// Sanity checks against future reflect package changes
// to the type or semantics of the Value.flag field.
func init() {
field, ok := reflect.TypeOf(reflect.Value{}).FieldByName("flag")
if !ok {
panic("reflect.Value has no flag field")
}
if field.Type.Kind() != reflect.TypeOf(flag(0)).Kind() {
panic("reflect.Value flag field has changed kind")
}
var t struct {
a int
A int
}
vA := reflect.ValueOf(t).FieldByName("A")
va := reflect.ValueOf(t).FieldByName("a")
flagA := *flagField(&vA)
flaga := *flagField(&va)

// Infer flagRO from the difference between the flags
// for the (otherwise identical) fields in t.
flagRO = flagA ^ flaga
if flagRO != flagRO1p3 && flagRO != flagRO1p4 {
panic("reflect.Value read-only flag has changed semantics")
}
}
29 changes: 29 additions & 0 deletions sheriff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,3 +464,32 @@ func TestMarshal_EmbeddedFieldEmpty(t *testing.T) {

assert.Equal(t, string(expected), string(actual))
}

type TestInlineStruct struct {
tableName struct{} `json:"-"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

go vet says this is an error because json tag in unexported field name. The test must actually test this specific case, right?

./sheriff_test.go:469: struct field tableName has json tag but is not exported
(https://travis-ci.org/liip/sheriff/jobs/297684025)

Seems like we need to switch to specific flags only.. I can handle this later on.


Field string `json:"field"`
Field2 *string `json:"field2"`
}

func TestMarshal_InlineStruct(t *testing.T) {
v := TestInlineStruct{
Field: "World",
Field2: nil,
}
o := &Options{}

actualMap, err := Marshal(o, v)
assert.NoError(t, err)

actual, err := json.Marshal(actualMap)
assert.NoError(t, err)

expected, err := json.Marshal(map[string]interface{}{
"field": "World",
"field2": nil,
})
assert.NoError(t, err)

assert.Equal(t, string(expected), string(actual))
}