From dc6435e426906757e7c944f74758028d3d2edc3c Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 3 Mar 2021 09:41:45 -0800 Subject: [PATCH] De-virtualize interfaces for specialized diffing (#254) Specialized diffing strings and slices should occur for interface types where both values have the same concrete type. This is especially relevant for protocmp.Transform, which transforms every proto.Message as a map[string]interface{}. --- cmp/compare_test.go | 10 ++++++++++ cmp/report_slices.go | 25 +++++++++++++++++++++---- cmp/testdata/diffs | 22 ++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/cmp/compare_test.go b/cmp/compare_test.go index 84f7353..172e3d4 100644 --- a/cmp/compare_test.go +++ b/cmp/compare_test.go @@ -1287,6 +1287,16 @@ using the AllowUnexported option.`, "\n"), return &b }(): 0}, reason: "printing map keys should have some verbosity limit imposed", + }, { + label: label + "/LargeStringInInterface", + x: struct{ X interface{} }{"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam sit amet pretium ligula, at gravida quam. Integer iaculis, velit at sagittis ultricies, lacus metus scelerisque turpis, ornare feugiat nulla nisl ac erat. Maecenas elementum ultricies libero, sed efficitur lacus molestie non. Nulla ac pretium dolor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Pellentesque mi lorem, consectetur id porttitor id, sollicitudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis."}, + y: struct{ X interface{} }{"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam sit amet pretium ligula, at gravida quam. Integer iaculis, velit at sagittis ultricies, lacus metus scelerisque turpis, ornare feugiat nulla nisl ac erat. Maecenas elementum ultricies libero, sed efficitur lacus molestie non. Nulla ac pretium dolor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Pellentesque mi lorem, consectetur id porttitor id, sollicitudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis,"}, + reason: "strings within an interface should benefit from specialized diffing", + }, { + label: label + "/LargeBytesInInterface", + x: struct{ X interface{} }{[]byte("Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam sit amet pretium ligula, at gravida quam. Integer iaculis, velit at sagittis ultricies, lacus metus scelerisque turpis, ornare feugiat nulla nisl ac erat. Maecenas elementum ultricies libero, sed efficitur lacus molestie non. Nulla ac pretium dolor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Pellentesque mi lorem, consectetur id porttitor id, sollicitudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis.")}, + y: struct{ X interface{} }{[]byte("Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam sit amet pretium ligula, at gravida quam. Integer iaculis, velit at sagittis ultricies, lacus metus scelerisque turpis, ornare feugiat nulla nisl ac erat. Maecenas elementum ultricies libero, sed efficitur lacus molestie non. Nulla ac pretium dolor. Pellentesque habitant morbi tristique senectus et netus et malesuada fames ac turpis egestas. Orci varius natoque penatibus et magnis dis parturient montes, nascetur ridiculus mus. Pellentesque mi lorem, consectetur id porttitor id, sollicitudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis,")}, + reason: "bytes slice within an interface should benefit from specialized diffing", }} } diff --git a/cmp/report_slices.go b/cmp/report_slices.go index da04caf..168f92f 100644 --- a/cmp/report_slices.go +++ b/cmp/report_slices.go @@ -26,8 +26,6 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool { return false // No differences detected case !v.ValueX.IsValid() || !v.ValueY.IsValid(): return false // Both values must be valid - case v.Type.Kind() == reflect.Slice && (v.ValueX.Len() == 0 || v.ValueY.Len() == 0): - return false // Both slice values have to be non-empty case v.NumIgnored > 0: return false // Some ignore option was used case v.NumTransformed > 0: @@ -45,7 +43,16 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool { return false } - switch t := v.Type; t.Kind() { + // Check whether this is an interface with the same concrete types. + t := v.Type + vx, vy := v.ValueX, v.ValueY + if t.Kind() == reflect.Interface && !vx.IsNil() && !vy.IsNil() && vx.Elem().Type() == vy.Elem().Type() { + vx, vy = vx.Elem(), vy.Elem() + t = vx.Type() + } + + // Check whether we provide specialized diffing for this type. + switch t.Kind() { case reflect.String: case reflect.Array, reflect.Slice: // Only slices of primitive types have specialized handling. @@ -57,6 +64,11 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool { return false } + // Both slice values have to be non-empty. + if t.Kind() == reflect.Slice && (vx.Len() == 0 || vy.Len() == 0) { + return false + } + // If a sufficient number of elements already differ, // use specialized formatting even if length requirement is not met. if v.NumDiff > v.NumSame { @@ -68,7 +80,7 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool { // Use specialized string diffing for longer slices or strings. const minLength = 64 - return v.ValueX.Len() >= minLength && v.ValueY.Len() >= minLength + return vx.Len() >= minLength && vy.Len() >= minLength } // FormatDiffSlice prints a diff for the slices (or strings) represented by v. @@ -77,6 +89,11 @@ func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool { func (opts formatOptions) FormatDiffSlice(v *valueNode) textNode { assert(opts.DiffMode == diffUnknown) t, vx, vy := v.Type, v.ValueX, v.ValueY + if t.Kind() == reflect.Interface { + vx, vy = vx.Elem(), vy.Elem() + t = vx.Type() + opts = opts.WithTypeMode(emitType) + } // Auto-detect the type of the data. var isLinedText, isText, isBinary bool diff --git a/cmp/testdata/diffs b/cmp/testdata/diffs index 81d5769..e2ffdd2 100644 --- a/cmp/testdata/diffs +++ b/cmp/testdata/diffs @@ -1014,6 +1014,28 @@ + &⟪0xdeadf00f⟫⟪ptr:0xdeadf00f, len:1048576, cap:1048576⟫{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ...}: 0, } >>> TestDiff/Reporter/LargeMapKey +<<< TestDiff/Reporter/LargeStringInInterface + struct{ X interface{} }{ + X: strings.Join({ + ... // 485 identical bytes + "s mus. Pellentesque mi lorem, consectetur id porttitor id, solli", + "citudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis", +- ".", ++ ",", + }, ""), + } +>>> TestDiff/Reporter/LargeStringInInterface +<<< TestDiff/Reporter/LargeBytesInInterface + struct{ X interface{} }{ + X: bytes.Join({ + ... // 485 identical bytes + "s mus. Pellentesque mi lorem, consectetur id porttitor id, solli", + "citudin sit amet enim. Duis eu dolor magna. Nunc ut augue turpis", +- ".", ++ ",", + }, ""), + } +>>> TestDiff/Reporter/LargeBytesInInterface <<< TestDiff/EmbeddedStruct/ParentStructA/Inequal teststructs.ParentStructA{ privateStruct: teststructs.privateStruct{