From 62b5bb0f498b48587942ed0d20a528d90261e329 Mon Sep 17 00:00:00 2001 From: Eyal Posener Date: Sat, 17 Mar 2018 11:42:29 +0200 Subject: [PATCH] Add DetectCycles to detect cycles in compared structs The current comparison fails on stack overflow when given a cyclic struct (such as a cyclic linked list for example). This fix adds a new option: DecetctCycles, which finds cycles by adding information to the path stack, when iterating over pointers. When a cycle is checked (every time a pointer kind is compared) the path stack is iterated to find cycles. The comparison of cycles is by they length. Fixes: #74 --- cmp/compare.go | 73 +++++++++++++++++++++++++++++++++-- cmp/compare_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++++ cmp/options.go | 14 +++++++ cmp/path.go | 3 ++ 4 files changed, 180 insertions(+), 3 deletions(-) diff --git a/cmp/compare.go b/cmp/compare.go index 1260603..76471b8 100644 --- a/cmp/compare.go +++ b/cmp/compare.go @@ -119,6 +119,10 @@ type state struct { // These fields, once set by processOption, will not change. exporters map[reflect.Type]bool // Set of structs with unexported field visibility opts Options // List of all fundamental and filter options + + // detectCycles is set by the DetectCycles option, and it indicates to + // enable the cycles check when a pointer is compared. + detectCycles bool } func newState(opts []Option) *state { @@ -156,6 +160,8 @@ func (s *state) processOption(opt Option) { panic("difference reporter already registered") } s.reporter = opt + case detectCycles: + s.detectCycles = true default: panic(fmt.Sprintf("unknown option %T", opt)) } @@ -180,8 +186,6 @@ func (s *state) statelessCompare(vx, vy reflect.Value) diff.Result { } func (s *state) compareAny(vx, vy reflect.Value) { - // TODO: Support cyclic data structures. - // Rule 0: Differing types are never equal. if !vx.IsValid() || !vy.IsValid() { s.report(vx.IsValid() == vy.IsValid(), vx, vy) @@ -239,8 +243,21 @@ func (s *state) compareAny(vx, vy reflect.Value) { s.report(vx.IsNil() && vy.IsNil(), vx, vy) return } - s.curPath.push(&indirect{pathStep{t.Elem()}}) + s.curPath.push(&indirect{ + pathStep: pathStep{t.Elem()}, + xAddr: vx.Elem().UnsafeAddr(), + yAddr: vy.Elem().UnsafeAddr(), + }) defer s.curPath.pop() + + // If detectCycles is enabled, look in the path stack and search for + // pointer cycle. If one is find, compare the cycles. + if s.detectCycles { + if cyclesEqual, ok := compareCycles(s.curPath); ok { + s.report(cyclesEqual, vx, vy) + return + } + } s.compareAny(vx.Elem(), vy.Elem()) return case reflect.Interface: @@ -552,3 +569,53 @@ func makeAddressable(v reflect.Value) reflect.Value { vc.Set(v) return vc } + +// compareCycles calculates the cyclic pointer chain length by going +// over the path stack. +// It calculates both a cyclic chain of x values and for y values, and +// compare them. +// It returns: +// equals if detected cycles are equal. +// ok as true if cycles were detected. +func compareCycles(p Path) (equals bool, ok bool) { + if len(p) == 0 { + return false, false + } + + // Check the current path step for the address values. + // If it is not a cycleStep, there is no point to check the + // chains lengths + curStep, ok := p[len(p)-1].(*indirect) + if !ok || (curStep.xAddr == 0 && curStep.yAddr == 0) { + return false, false + } + + // Find the next occurrence in the chain path of either xAddr or yAddr + // of the curStep. + var xLen, yLen, length int + for i := len(p) - 2; i > 0; i-- { + cs, ok := p[i].(*indirect) + if !ok || (curStep.xAddr == 0 && curStep.yAddr == 0) { + continue + } + + // since this step is a pointer step, increase the chain length + length += 1 + + // Check for the same address. We want to return the smallest cycle, + // so we update only if the current length value is 0 + if xLen == 0 && cs.xAddr == curStep.xAddr { + xLen = length + } + if yLen == 0 && cs.yAddr == curStep.yAddr { + yLen = length + } + + // If we found lengths for both x and y, we can return, there is no + // need to go over the whole stack. + if xLen != 0 && yLen != 0 { + break + } + } + return xLen == yLen, yLen != 0 && xLen != 0 +} diff --git a/cmp/compare_test.go b/cmp/compare_test.go index 84c645b..21e4024 100644 --- a/cmp/compare_test.go +++ b/cmp/compare_test.go @@ -28,6 +28,8 @@ import ( var now = time.Now() +var reAddress = regexp.MustCompile(`\(0x[0-9a-f]+\)`) + func intPtr(n int) *int { return &n } type test struct { @@ -44,6 +46,7 @@ func TestDiff(t *testing.T) { tests = append(tests, transformerTests()...) tests = append(tests, embeddedTests()...) tests = append(tests, methodTests()...) + tests = append(tests, detectCyclesTest()...) tests = append(tests, project1Tests()...) tests = append(tests, project2Tests()...) tests = append(tests, project3Tests()...) @@ -63,12 +66,21 @@ func TestDiff(t *testing.T) { } } }() + + // Add to all test the DetectCycles options: see if it breaks any test, and see that + // it works for the detectCycleTest + tt.opts = append(tt.opts, cmp.DetectCycles()) + gotDiff = cmp.Diff(tt.x, tt.y, tt.opts...) }() if tt.wantPanic == "" { if gotPanic != "" { t.Fatalf("unexpected panic message: %s", gotPanic) } + + // change all addresses in the diff to be 0x00, so they could be expected + gotDiff = reAddress.ReplaceAllString(gotDiff, "(0x00)") + if got, want := strings.TrimSpace(gotDiff), strings.TrimSpace(tt.wantDiff); got != want { t.Fatalf("difference message:\ngot:\n%s\n\nwant:\n%s", got, want) } @@ -1966,6 +1978,87 @@ func project4Tests() []test { }} } +func detectCyclesTest() []test { + const label = "DetectCycles/" + + type node struct { + Value string + Next *node + } + + var a = node{Value: "a"} + a.Next = &a + + var anotherA = node{Value: "a"} + anotherA.Next = &anotherA + + var b = node{Value: "b"} + b.Next = &b + + // a cyclic link list in length 2 + var len21, len22 node + len21.Next = &len22 + len22.Next = &len21 + + // a cyclic link list in length 3 + var len31, len32, len33 node + len31.Next = &len32 + len32.Next = &len33 + len33.Next = &len31 + + var insideA1, insideA2, insideA3 node + insideA1.Next = &insideA2 + insideA2.Next = &insideA3 + insideA3.Next = &insideA1 + insideA2.Value = "a" + + var insideB1, insideB2, insideB3 node + insideB1.Next = &insideB2 + insideB2.Next = &insideB3 + insideB3.Next = &insideB1 + insideB2.Value = "b" + + return []test{{ + label: label + "simple cycle/different", + x: a, + y: b, + wantDiff: ` +{cmp_test.node}.Value: + -: "a" + +: "b" +{cmp_test.node}.Next.Value: + -: "a" + +: "b" +`, + }, { + label: label + "simple cycle/equal", + x: a, + y: anotherA, + }, { + label: label + "simple cycle/equal identity", + x: a, + y: a, + }, { + label: label + "different size cycles", + x: len21, + y: len31, + wantDiff: ` +*{cmp_test.node}.Next.Next.Next.Next: + -: &cmp_test.node{Next: &cmp_test.node{Next: (*cmp_test.node)(0x00)}} + +: &cmp_test.node{Next: &cmp_test.node{Next: &cmp_test.node{Next: (*cmp_test.node)(0x00)}}} +`, + }, { + label: label + "value inside an equal cycle is different", + x: insideA1, + y: insideB1, + wantDiff: ` +{cmp_test.node}.Next.Value: + -: "a" + +: "b" +`, + }} +} + // TODO: Delete this hack when we drop Go1.6 support. func tRunParallel(t *testing.T, name string, f func(t *testing.T)) { type runner interface { diff --git a/cmp/options.go b/cmp/options.go index 91d4b06..e957428 100644 --- a/cmp/options.go +++ b/cmp/options.go @@ -451,3 +451,17 @@ func getFuncName(p uintptr) string { } return name } + +// DetectCycles is an option that prevents infinite searching in cyclic data structure. +// It iterates the whole path stack every time a pointer is tested to check for +// cycles thus it reduces the runtime performance. +// It finds cycles by adding information to the path stack any time a pointer is tested. +func DetectCycles() Option { return detectCycles{} } + +type detectCycles struct{} + +func (c detectCycles) filter(s *state, vx, vy reflect.Value, t reflect.Type) applicableOption { + return nil +} + +func (c detectCycles) String() string { return "DetectCycles()" } diff --git a/cmp/path.go b/cmp/path.go index c08a3cf..c859acc 100644 --- a/cmp/path.go +++ b/cmp/path.go @@ -213,6 +213,9 @@ type ( } indirect struct { pathStep + // xAddr and yAddr are set if the kind of the current step is Ptr. + // They contain the addresses pointing by vx an vy. + xAddr, yAddr uintptr } transform struct { pathStep