From 44914b370698a5a9ce868549d62d79473faebacc Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Fri, 12 Jun 2020 15:29:35 -0700 Subject: [PATCH] Disambiguate reporter output (#216) The reporter tries to aggresively elide data that is not interesting to the user. However, doing so many result in an output that does not visually indicate the difference between semantically different objects. This CL modifies the reporter to try increasingly verbose presets until two different objects are formatted differently. This CL includes a custom implementation of reflect.Type.String that can print the type with fully qualified names to disambiguate types that happen to have the same base package name. Fixes #194 --- cmp/compare_test.go | 135 +++++++++++++++++++---- cmp/internal/teststructs/foo1/foo.go | 10 ++ cmp/internal/teststructs/foo2/foo.go | 10 ++ cmp/internal/value/name.go | 157 +++++++++++++++++++++++++++ cmp/internal/value/name_test.go | 144 ++++++++++++++++++++++++ cmp/report_compare.go | 74 ++++++++++++- cmp/report_reflect.go | 15 ++- cmp/report_slices.go | 4 +- cmp/report_text.go | 5 +- cmp/testdata/diffs | 87 +++++++++++++-- 10 files changed, 597 insertions(+), 44 deletions(-) create mode 100644 cmp/internal/teststructs/foo1/foo.go create mode 100644 cmp/internal/teststructs/foo2/foo.go create mode 100644 cmp/internal/value/name.go create mode 100644 cmp/internal/value/name_test.go diff --git a/cmp/compare_test.go b/cmp/compare_test.go index 11979d0..be8a2f6 100644 --- a/cmp/compare_test.go +++ b/cmp/compare_test.go @@ -29,6 +29,8 @@ import ( pb "github.com/google/go-cmp/cmp/internal/testprotos" ts "github.com/google/go-cmp/cmp/internal/teststructs" + foo1 "github.com/google/go-cmp/cmp/internal/teststructs/foo1" + foo2 "github.com/google/go-cmp/cmp/internal/teststructs/foo2" ) func init() { @@ -101,7 +103,12 @@ func mustFormatGolden(path string, in []struct{ Name, Data string }) { var now = time.Date(2009, time.November, 10, 23, 00, 00, 00, time.UTC) -func intPtr(n int) *int { return &n } +func newInt(n int) *int { return &n } + +type Stringer string + +func newStringer(s string) fmt.Stringer { return (*Stringer)(&s) } +func (s Stringer) String() string { return string(s) } type test struct { label string // Test name @@ -149,6 +156,7 @@ func TestDiff(t *testing.T) { }() // TODO: Require every test case to provide a reason. + // TODO: Forbid any test cases with the same name. if tt.wantPanic == "" { if gotPanic != "" { t.Fatalf("unexpected panic message: %s\nreason: %v", gotPanic, tt.reason) @@ -295,26 +303,26 @@ func comparerTests() []test { wantPanic: "cannot handle unexported field", }, { label: label, - x: &struct{ A *int }{intPtr(4)}, - y: &struct{ A *int }{intPtr(4)}, + x: &struct{ A *int }{newInt(4)}, + y: &struct{ A *int }{newInt(4)}, wantEqual: true, }, { label: label, - x: &struct{ A *int }{intPtr(4)}, - y: &struct{ A *int }{intPtr(5)}, + x: &struct{ A *int }{newInt(4)}, + y: &struct{ A *int }{newInt(5)}, wantEqual: false, }, { label: label, - x: &struct{ A *int }{intPtr(4)}, - y: &struct{ A *int }{intPtr(5)}, + x: &struct{ A *int }{newInt(4)}, + y: &struct{ A *int }{newInt(5)}, opts: []cmp.Option{ cmp.Comparer(func(x, y int) bool { return true }), }, wantEqual: true, }, { label: label, - x: &struct{ A *int }{intPtr(4)}, - y: &struct{ A *int }{intPtr(5)}, + x: &struct{ A *int }{newInt(4)}, + y: &struct{ A *int }{newInt(5)}, opts: []cmp.Option{ cmp.Comparer(func(x, y *int) bool { return x != nil && y != nil }), }, @@ -555,15 +563,6 @@ func comparerTests() []test { new(int): "world", }, wantEqual: false, - }, { - label: label, - x: intPtr(0), - y: intPtr(0), - opts: []cmp.Option{ - cmp.Comparer(func(x, y *int) bool { return x == y }), - }, - // TODO: This diff output is unhelpful and should show the address. - wantEqual: false, }, { label: label, x: [2][]int{ @@ -822,6 +821,100 @@ func reporterTests() []test { ) return []test{{ + label: label + "/AmbiguousType", + x: foo1.Bar{}, + y: foo2.Bar{}, + wantEqual: false, + reason: "reporter should display the qualified type name to disambiguate between the two values", + }, { + label: label + "/AmbiguousPointer", + x: newInt(0), + y: newInt(0), + opts: []cmp.Option{ + cmp.Comparer(func(x, y *int) bool { return x == y }), + }, + wantEqual: false, + reason: "reporter should display the address to disambiguate between the two values", + }, { + label: label + "/AmbiguousPointerStruct", + x: struct{ I *int }{newInt(0)}, + y: struct{ I *int }{newInt(0)}, + opts: []cmp.Option{ + cmp.Comparer(func(x, y *int) bool { return x == y }), + }, + wantEqual: false, + reason: "reporter should display the address to disambiguate between the two struct fields", + }, { + label: label + "/AmbiguousPointerSlice", + x: []*int{newInt(0)}, + y: []*int{newInt(0)}, + opts: []cmp.Option{ + cmp.Comparer(func(x, y *int) bool { return x == y }), + }, + wantEqual: false, + reason: "reporter should display the address to disambiguate between the two slice elements", + }, { + label: label + "/AmbiguousPointerMap", + x: map[string]*int{"zero": newInt(0)}, + y: map[string]*int{"zero": newInt(0)}, + opts: []cmp.Option{ + cmp.Comparer(func(x, y *int) bool { return x == y }), + }, + wantEqual: false, + reason: "reporter should display the address to disambiguate between the two map values", + }, { + label: label + "/AmbiguousStringer", + x: Stringer("hello"), + y: newStringer("hello"), + wantEqual: false, + reason: "reporter should avoid calling String to disambiguate between the two values", + }, { + label: label + "/AmbiguousStringerStruct", + x: struct{ S fmt.Stringer }{Stringer("hello")}, + y: struct{ S fmt.Stringer }{newStringer("hello")}, + wantEqual: false, + reason: "reporter should avoid calling String to disambiguate between the two struct fields", + }, { + label: label + "/AmbiguousStringerSlice", + x: []fmt.Stringer{Stringer("hello")}, + y: []fmt.Stringer{newStringer("hello")}, + wantEqual: false, + reason: "reporter should avoid calling String to disambiguate between the two slice elements", + }, { + label: label + "/AmbiguousStringerMap", + x: map[string]fmt.Stringer{"zero": Stringer("hello")}, + y: map[string]fmt.Stringer{"zero": newStringer("hello")}, + wantEqual: false, + reason: "reporter should avoid calling String to disambiguate between the two map values", + }, { + label: label + "/AmbiguousSliceHeader", + x: make([]int, 0, 5), + y: make([]int, 0, 1000), + opts: []cmp.Option{ + cmp.Comparer(func(x, y []int) bool { return cap(x) == cap(y) }), + }, + wantEqual: false, + reason: "reporter should display the slice header to disambiguate between the two slice values", + }, { + label: label + "/AmbiguousStringerMapKey", + x: map[interface{}]string{ + nil: "nil", + Stringer("hello"): "goodbye", + foo1.Bar{"fizz"}: "buzz", + }, + y: map[interface{}]string{ + newStringer("hello"): "goodbye", + foo2.Bar{"fizz"}: "buzz", + }, + wantEqual: false, + reason: "reporter should avoid calling String to disambiguate between the two map keys", + }, { + label: label + "/NonAmbiguousStringerMapKey", + x: map[interface{}]string{Stringer("hello"): "goodbye"}, + y: map[interface{}]string{newStringer("fizz"): "buzz"}, + wantEqual: false, + reason: "reporter should call String as there is no ambiguity between the two map keys", + }, { label: "/InvalidUTF8", x: MyString("\xed\xa0\x80"), wantEqual: false, @@ -2146,7 +2239,7 @@ func project1Tests() []test { Target: "corporation", Immutable: &ts.GoatImmutable{ ID: "southbay", - State: (*pb.Goat_States)(intPtr(5)), + State: (*pb.Goat_States)(newInt(5)), Started: now, }, }, @@ -2174,7 +2267,7 @@ func project1Tests() []test { Immutable: &ts.EagleImmutable{ ID: "eagleID", Birthday: now, - MissingCall: (*pb.Eagle_MissingCalls)(intPtr(55)), + MissingCall: (*pb.Eagle_MissingCalls)(newInt(55)), }, } } @@ -2219,7 +2312,7 @@ func project1Tests() []test { x: func() ts.Eagle { eg := createEagle() eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.ID = "southbay2" - eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.State = (*pb.Goat_States)(intPtr(6)) + eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.State = (*pb.Goat_States)(newInt(6)) eg.Slaps[0].Immutable.MildSlap = false return eg }(), diff --git a/cmp/internal/teststructs/foo1/foo.go b/cmp/internal/teststructs/foo1/foo.go new file mode 100644 index 0000000..c769dfb --- /dev/null +++ b/cmp/internal/teststructs/foo1/foo.go @@ -0,0 +1,10 @@ +// Copyright 2020, The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE.md file. + +// Package foo is deliberately named differently than the parent directory. +// It contain declarations that have ambiguity in their short names, +// relative to a different package also called foo. +package foo + +type Bar struct{ S string } diff --git a/cmp/internal/teststructs/foo2/foo.go b/cmp/internal/teststructs/foo2/foo.go new file mode 100644 index 0000000..c769dfb --- /dev/null +++ b/cmp/internal/teststructs/foo2/foo.go @@ -0,0 +1,10 @@ +// Copyright 2020, The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE.md file. + +// Package foo is deliberately named differently than the parent directory. +// It contain declarations that have ambiguity in their short names, +// relative to a different package also called foo. +package foo + +type Bar struct{ S string } diff --git a/cmp/internal/value/name.go b/cmp/internal/value/name.go new file mode 100644 index 0000000..8228e7d --- /dev/null +++ b/cmp/internal/value/name.go @@ -0,0 +1,157 @@ +// Copyright 2020, The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE.md file. + +package value + +import ( + "reflect" + "strconv" +) + +// TypeString is nearly identical to reflect.Type.String, +// but has an additional option to specify that full type names be used. +func TypeString(t reflect.Type, qualified bool) string { + return string(appendTypeName(nil, t, qualified, false)) +} + +func appendTypeName(b []byte, t reflect.Type, qualified, elideFunc bool) []byte { + // BUG: Go reflection provides no way to disambiguate two named types + // of the same name and within the same package, + // but declared within the namespace of different functions. + + // Named type. + if t.Name() != "" { + if qualified && t.PkgPath() != "" { + b = append(b, '"') + b = append(b, t.PkgPath()...) + b = append(b, '"') + b = append(b, '.') + b = append(b, t.Name()...) + } else { + b = append(b, t.String()...) + } + return b + } + + // Unnamed type. + switch k := t.Kind(); k { + case reflect.Bool, reflect.String, reflect.UnsafePointer, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, + reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128: + b = append(b, k.String()...) + case reflect.Chan: + if t.ChanDir() == reflect.RecvDir { + b = append(b, "<-"...) + } + b = append(b, "chan"...) + if t.ChanDir() == reflect.SendDir { + b = append(b, "<-"...) + } + b = append(b, ' ') + b = appendTypeName(b, t.Elem(), qualified, false) + case reflect.Func: + if !elideFunc { + b = append(b, "func"...) + } + b = append(b, '(') + for i := 0; i < t.NumIn(); i++ { + if i > 0 { + b = append(b, ", "...) + } + if i == t.NumIn()-1 && t.IsVariadic() { + b = append(b, "..."...) + b = appendTypeName(b, t.In(i).Elem(), qualified, false) + } else { + b = appendTypeName(b, t.In(i), qualified, false) + } + } + b = append(b, ')') + switch t.NumOut() { + case 0: + // Do nothing + case 1: + b = append(b, ' ') + b = appendTypeName(b, t.Out(0), qualified, false) + default: + b = append(b, " ("...) + for i := 0; i < t.NumOut(); i++ { + if i > 0 { + b = append(b, ", "...) + } + b = appendTypeName(b, t.Out(i), qualified, false) + } + b = append(b, ')') + } + case reflect.Struct: + b = append(b, "struct{ "...) + for i := 0; i < t.NumField(); i++ { + if i > 0 { + b = append(b, "; "...) + } + sf := t.Field(i) + if !sf.Anonymous { + if qualified && sf.PkgPath != "" { + b = append(b, '"') + b = append(b, sf.PkgPath...) + b = append(b, '"') + b = append(b, '.') + } + b = append(b, sf.Name...) + b = append(b, ' ') + } + b = appendTypeName(b, sf.Type, qualified, false) + if sf.Tag != "" { + b = append(b, ' ') + b = strconv.AppendQuote(b, string(sf.Tag)) + } + } + if b[len(b)-1] == ' ' { + b = b[:len(b)-1] + } else { + b = append(b, ' ') + } + b = append(b, '}') + case reflect.Slice, reflect.Array: + b = append(b, '[') + if k == reflect.Array { + b = strconv.AppendUint(b, uint64(t.Len()), 10) + } + b = append(b, ']') + b = appendTypeName(b, t.Elem(), qualified, false) + case reflect.Map: + b = append(b, "map["...) + b = appendTypeName(b, t.Key(), qualified, false) + b = append(b, ']') + b = appendTypeName(b, t.Elem(), qualified, false) + case reflect.Ptr: + b = append(b, '*') + b = appendTypeName(b, t.Elem(), qualified, false) + case reflect.Interface: + b = append(b, "interface{ "...) + for i := 0; i < t.NumMethod(); i++ { + if i > 0 { + b = append(b, "; "...) + } + m := t.Method(i) + if qualified && m.PkgPath != "" { + b = append(b, '"') + b = append(b, m.PkgPath...) + b = append(b, '"') + b = append(b, '.') + } + b = append(b, m.Name...) + b = appendTypeName(b, m.Type, qualified, true) + } + if b[len(b)-1] == ' ' { + b = b[:len(b)-1] + } else { + b = append(b, ' ') + } + b = append(b, '}') + default: + panic("invalid kind: " + k.String()) + } + return b +} diff --git a/cmp/internal/value/name_test.go b/cmp/internal/value/name_test.go new file mode 100644 index 0000000..ddb31d4 --- /dev/null +++ b/cmp/internal/value/name_test.go @@ -0,0 +1,144 @@ +// Copyright 2020, The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE.md file. + +package value + +import ( + "reflect" + "strings" + "testing" +) + +type Named struct{} + +var pkgPath = reflect.TypeOf(Named{}).PkgPath() + +func TestTypeString(t *testing.T) { + tests := []struct { + in interface{} + want string + }{{ + in: bool(false), + want: "bool", + }, { + in: int(0), + want: "int", + }, { + in: float64(0), + want: "float64", + }, { + in: string(""), + want: "string", + }, { + in: Named{}, + want: "$PackagePath.Named", + }, { + in: (chan Named)(nil), + want: "chan $PackagePath.Named", + }, { + in: (<-chan Named)(nil), + want: "<-chan $PackagePath.Named", + }, { + in: (chan<- Named)(nil), + want: "chan<- $PackagePath.Named", + }, { + in: (func())(nil), + want: "func()", + }, { + in: (func(Named))(nil), + want: "func($PackagePath.Named)", + }, { + in: (func() Named)(nil), + want: "func() $PackagePath.Named", + }, { + in: (func(int, Named) (int, error))(nil), + want: "func(int, $PackagePath.Named) (int, error)", + }, { + in: (func(...Named))(nil), + want: "func(...$PackagePath.Named)", + }, { + in: struct{}{}, + want: "struct{}", + }, { + in: struct{ Named }{}, + want: "struct{ $PackagePath.Named }", + }, { + in: struct { + Named `tag` + }{}, + want: "struct{ $PackagePath.Named \"tag\" }", + }, { + in: struct{ Named Named }{}, + want: "struct{ Named $PackagePath.Named }", + }, { + in: struct { + Named Named `tag` + }{}, + want: "struct{ Named $PackagePath.Named \"tag\" }", + }, { + in: struct { + Int int + Named Named + }{}, + want: "struct{ Int int; Named $PackagePath.Named }", + }, { + in: struct { + _ int + x Named + }{}, + want: "struct{ $FieldPrefix._ int; $FieldPrefix.x $PackagePath.Named }", + }, { + in: []Named(nil), + want: "[]$PackagePath.Named", + }, { + in: []*Named(nil), + want: "[]*$PackagePath.Named", + }, { + in: [10]Named{}, + want: "[10]$PackagePath.Named", + }, { + in: [10]*Named{}, + want: "[10]*$PackagePath.Named", + }, { + in: map[string]string(nil), + want: "map[string]string", + }, { + in: map[Named]Named(nil), + want: "map[$PackagePath.Named]$PackagePath.Named", + }, { + in: (*Named)(nil), + want: "*$PackagePath.Named", + }, { + in: (*interface{})(nil), + want: "*interface{}", + }, { + in: (*interface{ Read([]byte) (int, error) })(nil), + want: "*interface{ Read([]uint8) (int, error) }", + }, { + in: (*interface { + F1() + F2(Named) + F3() Named + F4(int, Named) (int, error) + F5(...Named) + })(nil), + want: "*interface{ F1(); F2($PackagePath.Named); F3() $PackagePath.Named; F4(int, $PackagePath.Named) (int, error); F5(...$PackagePath.Named) }", + }} + + for _, tt := range tests { + typ := reflect.TypeOf(tt.in) + wantShort := tt.want + wantShort = strings.Replace(wantShort, "$PackagePath", "value", -1) + wantShort = strings.Replace(wantShort, "$FieldPrefix.", "", -1) + if gotShort := TypeString(typ, false); gotShort != wantShort { + t.Errorf("TypeString(%v, false) mismatch:\ngot: %v\nwant: %v", typ, gotShort, wantShort) + } + wantQualified := tt.want + wantQualified = strings.Replace(wantQualified, "$PackagePath", `"`+pkgPath+`"`, -1) + wantQualified = strings.Replace(wantQualified, "$FieldPrefix", `"`+pkgPath+`"`, -1) + if gotQualified := TypeString(typ, true); gotQualified != wantQualified { + t.Errorf("TypeString(%v, true) mismatch:\ngot: %v\nwant: %v", typ, gotQualified, wantQualified) + } + } +} diff --git a/cmp/report_compare.go b/cmp/report_compare.go index 2ac3cc6..be03a25 100644 --- a/cmp/report_compare.go +++ b/cmp/report_compare.go @@ -11,10 +11,6 @@ import ( "github.com/google/go-cmp/cmp/internal/value" ) -// TODO: Enforce unique outputs? -// * Avoid Stringer methods if it results in same output? -// * Print pointer address if outputs still equal? - // numContextRecords is the number of surrounding equal records to print. const numContextRecords = 2 @@ -83,6 +79,22 @@ func (opts formatOptions) verbosity() uint { } } +const maxVerbosityPreset = 3 + +// verbosityPreset modifies the verbosity settings given an index +// between 0 and maxVerbosityPreset, inclusive. +func verbosityPreset(opts formatOptions, i int) formatOptions { + opts.VerbosityLevel = int(opts.verbosity()) + 2*i + if i > 0 { + opts.AvoidStringer = true + } + if i >= maxVerbosityPreset { + opts.PrintAddresses = true + opts.QualifiedNames = true + } + return opts +} + // FormatDiff converts a valueNode tree into a textNode tree, where the later // is a textual representation of the differences detected in the former. func (opts formatOptions) FormatDiff(v *valueNode) textNode { @@ -125,6 +137,11 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode { var list textList outx := opts.WithTypeMode(elideType).FormatValue(v.ValueX, withinSlice, visitedPointers{}) outy := opts.WithTypeMode(elideType).FormatValue(v.ValueY, withinSlice, visitedPointers{}) + for i := 0; i <= maxVerbosityPreset && outx != nil && outy != nil && outx.Equal(outy); i++ { + opts2 := verbosityPreset(opts, i).WithTypeMode(elideType) + outx = opts2.FormatValue(v.ValueX, withinSlice, visitedPointers{}) + outy = opts2.FormatValue(v.ValueY, withinSlice, visitedPointers{}) + } if outx != nil { list = append(list, textRecord{Diff: '-', Value: outx}) } @@ -178,7 +195,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te case reflect.Map: name = "entry" opts = opts.WithTypeMode(elideType) - formatKey = formatMapKey + formatKey = func(v reflect.Value) string { return formatMapKey(v, false) } } maxLen := -1 @@ -241,6 +258,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te // Handle differencing. var numDiffs int var list textList + var keys []reflect.Value // invariant: len(list) == len(keys) groups := coalesceAdjacentRecords(name, recs) maxGroup := diffStats{Name: name} for i, ds := range groups { @@ -274,14 +292,19 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te for _, r := range recs[:numLo] { out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value) list = append(list, textRecord{Key: formatKey(r.Key), Value: out}) + keys = append(keys, r.Key) } if numEqual > numLo+numHi { ds.NumIdentical -= numLo + numHi list.AppendEllipsis(ds) + for len(keys) < len(list) { + keys = append(keys, reflect.Value{}) + } } for _, r := range recs[numEqual-numHi : numEqual] { out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value) list = append(list, textRecord{Key: formatKey(r.Key), Value: out}) + keys = append(keys, r.Key) } recs = recs[numEqual:] continue @@ -293,18 +316,27 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te case opts.CanFormatDiffSlice(r.Value): out := opts.FormatDiffSlice(r.Value) list = append(list, textRecord{Key: formatKey(r.Key), Value: out}) + keys = append(keys, r.Key) case r.Value.NumChildren == r.Value.MaxDepth: outx := opts.WithDiffMode(diffRemoved).FormatDiff(r.Value) outy := opts.WithDiffMode(diffInserted).FormatDiff(r.Value) + for i := 0; i <= maxVerbosityPreset && outx != nil && outy != nil && outx.Equal(outy); i++ { + opts2 := verbosityPreset(opts, i) + outx = opts2.WithDiffMode(diffRemoved).FormatDiff(r.Value) + outy = opts2.WithDiffMode(diffInserted).FormatDiff(r.Value) + } if outx != nil { list = append(list, textRecord{Diff: diffRemoved, Key: formatKey(r.Key), Value: outx}) + keys = append(keys, r.Key) } if outy != nil { list = append(list, textRecord{Diff: diffInserted, Key: formatKey(r.Key), Value: outy}) + keys = append(keys, r.Key) } default: out := opts.FormatDiff(r.Value) list = append(list, textRecord{Key: formatKey(r.Key), Value: out}) + keys = append(keys, r.Key) } } recs = recs[ds.NumDiff():] @@ -314,7 +346,39 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te assert(len(recs) == 0) } else { list.AppendEllipsis(maxGroup) + for len(keys) < len(list) { + keys = append(keys, reflect.Value{}) + } + } + assert(len(list) == len(keys)) + + // For maps, the default formatting logic uses fmt.Stringer which may + // produce ambiguous output. Avoid calling String to disambiguate. + if k == reflect.Map { + var ambiguous bool + seenKeys := map[string]reflect.Value{} + for i, currKey := range keys { + if currKey.IsValid() { + strKey := list[i].Key + prevKey, seen := seenKeys[strKey] + if seen && prevKey.CanInterface() && currKey.CanInterface() { + ambiguous = prevKey.Interface() != currKey.Interface() + if ambiguous { + break + } + } + seenKeys[strKey] = currKey + } + } + if ambiguous { + for i, k := range keys { + if k.IsValid() { + list[i].Key = formatMapKey(k, true) + } + } + } } + return textWrap{"{", list, "}"} } diff --git a/cmp/report_reflect.go b/cmp/report_reflect.go index ecc71f0..8b4325d 100644 --- a/cmp/report_reflect.go +++ b/cmp/report_reflect.go @@ -30,6 +30,10 @@ type formatValueOptions struct { // slice elements, and maps. PrintAddresses bool + // QualifiedNames controls whether FormatType uses the fully qualified name + // (including the full package path as opposed to just the package name). + QualifiedNames bool + // VerbosityLevel controls the amount of output to produce. // A higher value produces more output. A value of zero or lower produces // no output (represented using an ellipsis). @@ -62,7 +66,7 @@ func (opts formatOptions) FormatType(t reflect.Type, s textNode) textNode { } // Determine the type label, applying special handling for unnamed types. - typeName := t.String() + typeName := value.TypeString(t, opts.QualifiedNames) if t.Name() == "" { // According to Go grammar, certain type literals contain symbols that // do not strongly bind to the next lexicographical token (e.g., *T). @@ -70,8 +74,6 @@ func (opts formatOptions) FormatType(t reflect.Type, s textNode) textNode { case reflect.Chan, reflect.Func, reflect.Ptr: typeName = "(" + typeName + ")" } - typeName = strings.Replace(typeName, "struct {", "struct{", -1) - typeName = strings.Replace(typeName, "interface {", "interface{", -1) } // Avoid wrap the value in parenthesis if unnecessary. @@ -236,7 +238,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit list.AppendEllipsis(diffStats{}) break } - sk := formatMapKey(k) + sk := formatMapKey(k, false) sv := opts.WithTypeMode(elideType).FormatValue(v.MapIndex(k), false, m) list = append(list, textRecord{Key: sk, Value: sv}) } @@ -272,10 +274,13 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit // formatMapKey formats v as if it were a map key. // The result is guaranteed to be a single line. -func formatMapKey(v reflect.Value) string { +func formatMapKey(v reflect.Value, disambiguate bool) string { var opts formatOptions + opts.DiffMode = diffIdentical opts.TypeMode = elideType opts.PrintShallowPointer = true + opts.AvoidStringer = disambiguate + opts.QualifiedNames = disambiguate s := opts.FormatValue(v, false, visitedPointers{}).String() return strings.TrimSpace(s) } diff --git a/cmp/report_slices.go b/cmp/report_slices.go index cfd1b60..49fc5ec 100644 --- a/cmp/report_slices.go +++ b/cmp/report_slices.go @@ -26,8 +26,8 @@ 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.IsNil() || v.ValueY.IsNil()): - return false // Both of values have to be non-nil + 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: diff --git a/cmp/report_text.go b/cmp/report_text.go index 17a376e..b8ec9d2 100644 --- a/cmp/report_text.go +++ b/cmp/report_text.go @@ -10,6 +10,7 @@ import ( "math/rand" "strings" "time" + "unicode/utf8" "github.com/google/go-cmp/cmp/internal/flags" ) @@ -239,14 +240,14 @@ func (s textList) formatExpandedTo(b []byte, d diffMode, n indentMode) []byte { _, isLine := r.Value.(textLine) return r.Key == "" || !isLine }, - func(r textRecord) int { return len(r.Key) }, + func(r textRecord) int { return utf8.RuneCountInString(r.Key) }, ) alignValueLens := s.alignLens( func(r textRecord) bool { _, isLine := r.Value.(textLine) return !isLine || r.Value.Equal(textEllipsis) || r.Comment == nil }, - func(r textRecord) int { return len(r.Value.(textLine)) }, + func(r textRecord) int { return utf8.RuneCount(r.Value.(textLine)) }, ) // Format lists of simple lists in a batched form. diff --git a/cmp/testdata/diffs b/cmp/testdata/diffs index d960785..c161591 100644 --- a/cmp/testdata/diffs +++ b/cmp/testdata/diffs @@ -160,12 +160,6 @@ } >>> TestDiff/Comparer#43 <<< TestDiff/Comparer#44 - (*int)( -- &0, -+ &0, - ) ->>> TestDiff/Comparer#44 -<<< TestDiff/Comparer#45 [2][]int{ {..., 1, 2, 3, ...}, { @@ -175,8 +169,8 @@ ... // 3 ignored elements }, } ->>> TestDiff/Comparer#45 -<<< TestDiff/Comparer#46 +>>> TestDiff/Comparer#44 +<<< TestDiff/Comparer#45 [2]map[string]int{ {"KEEP3": 3, "keep1": 1, "keep2": 2, ...}, { @@ -185,7 +179,7 @@ + "keep2": 2, }, } ->>> TestDiff/Comparer#46 +>>> TestDiff/Comparer#45 <<< TestDiff/Transformer uint8(Inverse(λ, uint16(Inverse(λ, uint32(Inverse(λ, uint64( - 0, @@ -258,6 +252,81 @@ })), } >>> TestDiff/Transformer#05 +<<< TestDiff/Reporter/AmbiguousType + interface{}( +- "github.com/google/go-cmp/cmp/internal/teststructs/foo1".Bar{}, ++ "github.com/google/go-cmp/cmp/internal/teststructs/foo2".Bar{}, + ) +>>> TestDiff/Reporter/AmbiguousType +<<< TestDiff/Reporter/AmbiguousPointer + (*int)( +- &⟪0xdeadf00f⟫0, ++ &⟪0xdeadf00f⟫0, + ) +>>> TestDiff/Reporter/AmbiguousPointer +<<< TestDiff/Reporter/AmbiguousPointerStruct + struct{ I *int }{ +- I: &⟪0xdeadf00f⟫0, ++ I: &⟪0xdeadf00f⟫0, + } +>>> TestDiff/Reporter/AmbiguousPointerStruct +<<< TestDiff/Reporter/AmbiguousPointerSlice + []*int{ +- &⟪0xdeadf00f⟫0, ++ &⟪0xdeadf00f⟫0, + } +>>> TestDiff/Reporter/AmbiguousPointerSlice +<<< TestDiff/Reporter/AmbiguousPointerMap + map[string]*int{ +- "zero": &⟪0xdeadf00f⟫0, ++ "zero": &⟪0xdeadf00f⟫0, + } +>>> TestDiff/Reporter/AmbiguousPointerMap +<<< TestDiff/Reporter/AmbiguousStringer + interface{}( +- cmp_test.Stringer("hello"), ++ &cmp_test.Stringer("hello"), + ) +>>> TestDiff/Reporter/AmbiguousStringer +<<< TestDiff/Reporter/AmbiguousStringerStruct + struct{ S fmt.Stringer }{ +- S: cmp_test.Stringer("hello"), ++ S: &cmp_test.Stringer("hello"), + } +>>> TestDiff/Reporter/AmbiguousStringerStruct +<<< TestDiff/Reporter/AmbiguousStringerSlice + []fmt.Stringer{ +- cmp_test.Stringer("hello"), ++ &cmp_test.Stringer("hello"), + } +>>> TestDiff/Reporter/AmbiguousStringerSlice +<<< TestDiff/Reporter/AmbiguousStringerMap + map[string]fmt.Stringer{ +- "zero": cmp_test.Stringer("hello"), ++ "zero": &cmp_test.Stringer("hello"), + } +>>> TestDiff/Reporter/AmbiguousStringerMap +<<< TestDiff/Reporter/AmbiguousSliceHeader + []int( +- ⟪ptr:0xdeadf00f, len:0, cap:5⟫{}, ++ ⟪ptr:0xdeadf00f, len:0, cap:1000⟫{}, + ) +>>> TestDiff/Reporter/AmbiguousSliceHeader +<<< TestDiff/Reporter/AmbiguousStringerMapKey + map[interface{}]string{ +- nil: "nil", ++ &⟪0xdeadf00f⟫"github.com/google/go-cmp/cmp_test".Stringer("hello"): "goodbye", +- "github.com/google/go-cmp/cmp_test".Stringer("hello"): "goodbye", +- "github.com/google/go-cmp/cmp/internal/teststructs/foo1".Bar{S: "fizz"}: "buzz", ++ "github.com/google/go-cmp/cmp/internal/teststructs/foo2".Bar{S: "fizz"}: "buzz", + } +>>> TestDiff/Reporter/AmbiguousStringerMapKey +<<< TestDiff/Reporter/NonAmbiguousStringerMapKey + map[interface{}]string{ ++ s"fizz": "buzz", +- s"hello": "goodbye", + } +>>> TestDiff/Reporter/NonAmbiguousStringerMapKey <<< TestDiff//InvalidUTF8 interface{}( - cmp_test.MyString("\xed\xa0\x80"),