From 4b7ff8c86b058626db80cb1138dc0bbda2caf538 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 28 Sep 2016 14:25:54 -0400 Subject: [PATCH 1/4] Allow reflectwalk to walk down chains of pointers reflectwalk currently panics is a dereferenced value turns out to be a pointer. The patch allows reflectwalk to continue traversing pointers and interfaces, calling a PointerWalker at each step if necessary. It will up to the reflectwalk consumer to determin the level of pointers found during the walk. --- reflectwalk.go | 53 ++++++++++++++++++++++++++++++--------------- reflectwalk_test.go | 22 ++++++++++++++++--- 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/reflectwalk.go b/reflectwalk.go index ecce023..a10d71a 100644 --- a/reflectwalk.go +++ b/reflectwalk.go @@ -81,35 +81,52 @@ func walk(v reflect.Value, w interface{}) (err error) { // almost any part is changed). I will try to explain here. // // First, we check if the value is an interface, if so, we really need - // to check the interface's VALUE to see whether it is a pointer (pointers - // to interfaces are not allowed). + // to check the interface's VALUE to see whether it is a pointer. // - // Check whether the value is then an interface. If so, then set pointer + // Check whether the value is then a pointer. If so, then set pointer // to true to notify the user. // + // If we still have a pointer or an interface after the indirections, then + // we unwrap another level + // // At this time, we also set "v" to be the dereferenced value. This is // because once we've unwrapped the pointer we want to use that value. pointer := false pointerV := v - if pointerV.Kind() == reflect.Interface { - pointerV = pointerV.Elem() - } - if pointerV.Kind() == reflect.Ptr { - pointer = true - v = reflect.Indirect(pointerV) - } - if pw, ok := w.(PointerWalker); ok { - if err = pw.PointerEnter(pointer); err != nil { - return - } - defer func() { - if err != nil { + for { + if pointerV.Kind() == reflect.Interface { + pointerV = pointerV.Elem() + } + if pointerV.Kind() == reflect.Ptr { + pointer = true + v = reflect.Indirect(pointerV) + } + if pw, ok := w.(PointerWalker); ok { + if err = pw.PointerEnter(pointer); err != nil { return } - err = pw.PointerExit(pointer) - }() + defer func() { + if err != nil { + return + } + + err = pw.PointerExit(pointer) + }() + } + + if pointer { + pointerV = v + } + pointer = false + + // If we still have a pointer or interface we have to indirect another level. + switch pointerV.Kind() { + case reflect.Ptr, reflect.Interface: + continue + } + break } // We preserve the original value here because if it is an interface diff --git a/reflectwalk_test.go b/reflectwalk_test.go index e52546d..7fc3626 100644 --- a/reflectwalk_test.go +++ b/reflectwalk_test.go @@ -400,17 +400,33 @@ func TestWalk_SliceWithPtr(t *testing.T) { } } +type testErr struct{} + +func (t *testErr) Error() string { + return "test error" +} + func TestWalk_Struct(t *testing.T) { w := new(TestStructWalker) + // This makes sure we can also walk over pointer-to-pointers, and the ever + // so rare pointer-to-interface type S struct { Foo string - Bar string + Bar *string + Baz **string + Err *error } + bar := "ptr" + baz := &bar + e := error(&testErr{}) + data := &S{ Foo: "foo", - Bar: "bar", + Bar: &bar, + Baz: &baz, + Err: &e, } err := Walk(data, w) @@ -418,7 +434,7 @@ func TestWalk_Struct(t *testing.T) { t.Fatalf("err: %s", err) } - expected := []string{"Foo", "Bar"} + expected := []string{"Foo", "Bar", "Baz", "Err"} if !reflect.DeepEqual(w.Fields, expected) { t.Fatalf("bad: %#v", w.Fields) } From 3808f79f89e484af0bd08a5199cd3be018006701 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 28 Sep 2016 17:31:24 -0400 Subject: [PATCH 2/4] Add test for PointerEnter PointerExit count Also add a test for raw **string walk too --- reflectwalk_test.go | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/reflectwalk_test.go b/reflectwalk_test.go index 7fc3626..2e8c85b 100644 --- a/reflectwalk_test.go +++ b/reflectwalk_test.go @@ -24,7 +24,8 @@ func (t *TestEnterExitWalker) Exit(l Location) error { } type TestPointerWalker struct { - Ps []bool + Ps []bool + exits int } func (t *TestPointerWalker) PointerEnter(v bool) error { @@ -33,6 +34,7 @@ func (t *TestPointerWalker) PointerEnter(v bool) error { } func (t *TestPointerWalker) PointerExit(v bool) error { + t.exits++ return nil } @@ -327,10 +329,15 @@ func TestWalk_Pointer(t *testing.T) { type S struct { Foo string + Bar *string + Baz **string } + s := "" + sp := &s + data := &S{ - Foo: "foo", + Baz: &sp, } err := Walk(data, w) @@ -338,10 +345,40 @@ func TestWalk_Pointer(t *testing.T) { t.Fatalf("err: %s", err) } - expected := []bool{true, false} + // The values hsould be seen in this order, setting the pointer bools: + // S true + // Foo false + // Bar true + // Baz true + // *Baz true + expected := []bool{true, false, true, true, true} + if !reflect.DeepEqual(w.Ps, expected) { + t.Fatalf("bad: %#v", w.Ps) + } + if w.exits != len(w.Ps) { + t.Fatalf("number of Enter (%d) and Exit (%d) calls don't match", len(w.Ps), w.exits) + } +} + +func TestWalk_PointerPointer(t *testing.T) { + w := new(TestPointerWalker) + + s := "" + sp := &s + pp := &sp + + err := Walk(pp, w) + if err != nil { + t.Fatalf("err: %s", err) + } + + expected := []bool{true, true} if !reflect.DeepEqual(w.Ps, expected) { t.Fatalf("bad: %#v", w.Ps) } + if w.exits != len(w.Ps) { + t.Fatalf("number of Enter (%d) and Exit (%d) calls don't match", len(w.Ps), w.exits) + } } func TestWalk_Slice(t *testing.T) { From 20c52e6f1feee4d6012112c5e90c8ccd58d4b860 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 29 Sep 2016 12:34:31 -0400 Subject: [PATCH 3/4] Close over the correct value in PointerExit The deferred closure for PointerExit closed over the pointer variable, which leads to the incorrect value being used in the PointerExit call. Copy the value into the function args. --- reflectwalk.go | 4 ++-- reflectwalk_test.go | 28 +++++++++++++++++----------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/reflectwalk.go b/reflectwalk.go index a10d71a..3a6da97 100644 --- a/reflectwalk.go +++ b/reflectwalk.go @@ -107,13 +107,13 @@ func walk(v reflect.Value, w interface{}) (err error) { return } - defer func() { + defer func(pointer bool) { if err != nil { return } err = pw.PointerExit(pointer) - }() + }(pointer) } if pointer { diff --git a/reflectwalk_test.go b/reflectwalk_test.go index 2e8c85b..bd5b181 100644 --- a/reflectwalk_test.go +++ b/reflectwalk_test.go @@ -1,6 +1,7 @@ package reflectwalk import ( + "fmt" "reflect" "testing" ) @@ -24,17 +25,22 @@ func (t *TestEnterExitWalker) Exit(l Location) error { } type TestPointerWalker struct { - Ps []bool - exits int + enters []bool + depth int + exits int } func (t *TestPointerWalker) PointerEnter(v bool) error { - t.Ps = append(t.Ps, v) + t.enters = append(t.enters, v) + t.depth++ return nil } func (t *TestPointerWalker) PointerExit(v bool) error { t.exits++ + if t.enters[t.depth-1] != v { + return fmt.Errorf("bad pointer exit %t at depth %d", v, t.depth) + } return nil } @@ -352,11 +358,11 @@ func TestWalk_Pointer(t *testing.T) { // Baz true // *Baz true expected := []bool{true, false, true, true, true} - if !reflect.DeepEqual(w.Ps, expected) { - t.Fatalf("bad: %#v", w.Ps) + if !reflect.DeepEqual(w.enters, expected) { + t.Fatalf("bad: %#v", w.enters) } - if w.exits != len(w.Ps) { - t.Fatalf("number of Enter (%d) and Exit (%d) calls don't match", len(w.Ps), w.exits) + if w.exits != len(w.enters) { + t.Fatalf("number of enters (%d) and exits (%d) don't match", len(w.enters), w.exits) } } @@ -373,11 +379,11 @@ func TestWalk_PointerPointer(t *testing.T) { } expected := []bool{true, true} - if !reflect.DeepEqual(w.Ps, expected) { - t.Fatalf("bad: %#v", w.Ps) + if !reflect.DeepEqual(w.enters, expected) { + t.Fatalf("bad: %#v", w.enters) } - if w.exits != len(w.Ps) { - t.Fatalf("number of Enter (%d) and Exit (%d) calls don't match", len(w.Ps), w.exits) + if w.exits != len(w.enters) { + t.Fatalf("number of enters (%d) and exits (%d) don't match", len(w.enters), w.exits) } } From 69e22e60fbdb21749aee47334b8da806cc6ffdbe Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 29 Sep 2016 14:07:15 -0400 Subject: [PATCH 4/4] fix PointerWalker tests Fix PointerWalker tests so that they operate on a proper stack, and check each value at exit --- reflectwalk_test.go | 53 +++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/reflectwalk_test.go b/reflectwalk_test.go index bd5b181..32765c7 100644 --- a/reflectwalk_test.go +++ b/reflectwalk_test.go @@ -25,22 +25,27 @@ func (t *TestEnterExitWalker) Exit(l Location) error { } type TestPointerWalker struct { - enters []bool - depth int - exits int + pointers []bool + count int + enters int + exits int } func (t *TestPointerWalker) PointerEnter(v bool) error { - t.enters = append(t.enters, v) - t.depth++ + t.pointers = append(t.pointers, v) + t.enters++ + if v { + t.count++ + } return nil } func (t *TestPointerWalker) PointerExit(v bool) error { t.exits++ - if t.enters[t.depth-1] != v { - return fmt.Errorf("bad pointer exit %t at depth %d", v, t.depth) + if t.pointers[len(t.pointers)-1] != v { + return fmt.Errorf("bad pointer exit '%t' at exit %d", v, t.exits) } + t.pointers = t.pointers[:len(t.pointers)-1] return nil } @@ -351,18 +356,16 @@ func TestWalk_Pointer(t *testing.T) { t.Fatalf("err: %s", err) } - // The values hsould be seen in this order, setting the pointer bools: - // S true - // Foo false - // Bar true - // Baz true - // *Baz true - expected := []bool{true, false, true, true, true} - if !reflect.DeepEqual(w.enters, expected) { - t.Fatalf("bad: %#v", w.enters) + if w.enters != 5 { + t.Fatal("expected 4 values, saw", w.enters) + } + + if w.count != 4 { + t.Fatal("exptec 3 pointers, saw", w.count) } - if w.exits != len(w.enters) { - t.Fatalf("number of enters (%d) and exits (%d) don't match", len(w.enters), w.exits) + + if w.exits != w.enters { + t.Fatalf("number of enters (%d) and exits (%d) don't match", w.enters, w.exits) } } @@ -378,12 +381,16 @@ func TestWalk_PointerPointer(t *testing.T) { t.Fatalf("err: %s", err) } - expected := []bool{true, true} - if !reflect.DeepEqual(w.enters, expected) { - t.Fatalf("bad: %#v", w.enters) + if w.enters != 2 { + t.Fatal("expected 2 values, saw", w.enters) } - if w.exits != len(w.enters) { - t.Fatalf("number of enters (%d) and exits (%d) don't match", len(w.enters), w.exits) + + if w.count != 2 { + t.Fatal("expected 2 pointers, saw", w.count) + } + + if w.exits != w.enters { + t.Fatalf("number of enters (%d) and exits (%d) don't match", w.enters, w.exits) } }