diff --git a/cmp/compare_test.go b/cmp/compare_test.go index 11979d0..28cd5cf 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,93 @@ 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{Stringer("hello"): "goodbye"}, + y: map[interface{}]string{newStringer("hello"): "goodbye"}, + 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 +2232,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 +2260,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 +2305,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..989bdc0 --- /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{} diff --git a/cmp/internal/teststructs/foo2/foo.go b/cmp/internal/teststructs/foo2/foo.go new file mode 100644 index 0000000..989bdc0 --- /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{} 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..9bf4c84 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}) } @@ -165,20 +182,29 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode { func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) textNode { // Derive record name based on the data structure kind. var name string - var formatKey func(reflect.Value) string + var formatKey func(reflect.Value) fmt.Stringer + type mapKeyStringer struct { + stringer + key reflect.Value + } switch k { case reflect.Struct: name = "field" opts = opts.WithTypeMode(autoType) - formatKey = func(v reflect.Value) string { return v.String() } + formatKey = func(v reflect.Value) fmt.Stringer { return stringer(v.String()) } case reflect.Slice, reflect.Array: name = "element" opts = opts.WithTypeMode(elideType) - formatKey = func(reflect.Value) string { return "" } + formatKey = func(reflect.Value) fmt.Stringer { return nil } case reflect.Map: name = "entry" opts = opts.WithTypeMode(elideType) - formatKey = formatMapKey + formatKey = func(v reflect.Value) fmt.Stringer { + return &mapKeyStringer{ + // By default, format keys by using the String method. + stringer: stringer(formatMapKey(v, false)), key: v, + } + } } maxLen := -1 @@ -296,6 +322,11 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te 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}) } @@ -315,6 +346,31 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te } else { list.AppendEllipsis(maxGroup) } + + // 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[stringer]reflect.Value{} + for _, r := range list { + if k, ok := r.Key.(*mapKeyStringer); ok { + v, ok := seenKeys[k.stringer] + ambiguous = ok && v.Interface() != k.key.Interface() + if ambiguous { + break + } + seenKeys[k.stringer] = k.key + } + } + if ambiguous { + for _, r := range list { + if k, ok := r.Key.(*mapKeyStringer); ok { + k.stringer = stringer(formatMapKey(k.key, true)) // avoid String method + } + } + } + } + return textWrap{"{", list, "}"} } diff --git a/cmp/report_reflect.go b/cmp/report_reflect.go index ecc71f0..b19c60e 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. @@ -178,7 +180,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit vv = retrieveUnexportedField(v, sf, true) } s := opts.WithTypeMode(autoType).FormatValue(vv, false, m) - list = append(list, textRecord{Key: sf.Name, Value: s}) + list = append(list, textRecord{Key: stringer(sf.Name), Value: s}) } return textWrap{"{", list, "}"} case reflect.Slice: @@ -236,7 +238,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit list.AppendEllipsis(diffStats{}) break } - sk := formatMapKey(k) + sk := stringer(formatMapKey(k, false)) sv := opts.WithTypeMode(elideType).FormatValue(v.MapIndex(k), false, m) list = append(list, textRecord{Key: sk, Value: sv}) } @@ -272,9 +274,10 @@ 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, avoidStringer bool) string { var opts formatOptions opts.TypeMode = elideType + opts.AvoidStringer = avoidStringer opts.PrintShallowPointer = true 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..8e6920f 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: @@ -234,7 +234,7 @@ func (opts formatOptions) FormatDiffSlice(v *valueNode) textNode { ss = append(ss, formatHex(v.Index(i).Uint())) } s := strings.Join(ss, ", ") - comment := commentString(fmt.Sprintf("%c|%v|", d, formatASCII(v.String()))) + comment := stringer(fmt.Sprintf("%c|%v|", d, formatASCII(v.String()))) return textRecord{Diff: d, Value: textLine(s), Comment: comment} }, ) diff --git a/cmp/report_text.go b/cmp/report_text.go index 17a376e..992cf29 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" ) @@ -139,7 +140,7 @@ func (s textWrap) formatExpandedTo(b []byte, d diffMode, n indentMode) []byte { type textList []textRecord type textRecord struct { Diff diffMode // e.g., 0 or '-' or '+' - Key string // e.g., "MyField" + Key fmt.Stringer // e.g., "MyField" Value textNode // textWrap | textLine ElideComma bool // avoid trailing comma Comment fmt.Stringer // e.g., "6 identical fields" @@ -165,8 +166,8 @@ func (s *textList) AppendEllipsis(ds diffStats) { func (s textList) Len() (n int) { for i, r := range s { - n += len(r.Key) - if r.Key != "" { + if r.Key != nil { + n += len(r.Key.String()) n += len(": ") } n += r.Value.Len() @@ -207,8 +208,8 @@ func (s textList) formatCompactTo(b []byte, d diffMode) ([]byte, textNode) { if r.Diff == diffInserted || r.Diff == diffRemoved { multiLine = true } - b = append(b, r.Key...) - if r.Key != "" { + if r.Key != nil { + b = append(b, r.Key.String()...) b = append(b, ": "...) } b, s[i].Value = r.Value.formatCompactTo(b, d|r.Diff) @@ -237,16 +238,16 @@ func (s textList) formatExpandedTo(b []byte, d diffMode, n indentMode) []byte { alignKeyLens := s.alignLens( func(r textRecord) bool { _, isLine := r.Value.(textLine) - return r.Key == "" || !isLine + return r.Key == nil || !isLine }, - func(r textRecord) int { return len(r.Key) }, + func(r textRecord) int { return utf8.RuneCountInString(r.Key.String()) }, ) 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. @@ -255,7 +256,7 @@ func (s textList) formatExpandedTo(b []byte, d diffMode, n indentMode) []byte { var isSimple bool for _, r := range s { _, isLine := r.Value.(textLine) - isSimple = r.Diff == 0 && r.Key == "" && isLine && r.Comment == nil + isSimple = r.Diff == 0 && r.Key == nil && isLine && r.Comment == nil if !isSimple { break } @@ -287,8 +288,8 @@ func (s textList) formatExpandedTo(b []byte, d diffMode, n indentMode) []byte { n++ for i, r := range s { b = n.appendIndent(append(b, '\n'), d|r.Diff) - if r.Key != "" { - b = append(b, r.Key+": "...) + if r.Key != nil { + b = append(b, r.Key.String()+": "...) } b = alignKeyLens[i].appendChar(b, ' ') @@ -424,6 +425,6 @@ func (s diffStats) String() string { } } -type commentString string +type stringer string -func (s commentString) String() string { return string(s) } +func (s stringer) String() string { return string(s) } diff --git a/cmp/testdata/diffs b/cmp/testdata/diffs index d960785..c2ad3cf 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,78 @@ })), } >>> 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{ ++ &⟪0xdeadf00f⟫cmp_test.Stringer("hello"): "goodbye", +- cmp_test.Stringer("hello"): "goodbye", + } +>>> 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"),