From 68a01433ff50824e49b58f363ce076f009fed9a7 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Thu, 28 Sep 2017 15:20:45 -0700 Subject: [PATCH] Use raw literal for string formatting When formatting strings, avoid using strconv.Quote if the string looks like it has already been escaped. Instead, use a raw string literal by wrapping the string with '`' characters if possible. For now, we still use strconv.Quote if the input string contains newlines to maintain the property that Format outputs a single line. Also, prefix strings obtained by the Stringer.String method with a 's'. This allows users to more easily distinguish when an output really is of type string or if the String method was used. --- cmp/compare_test.go | 44 ++++++++++++++++++++----------- cmp/internal/value/format.go | 38 ++++++++++++++++---------- cmp/internal/value/format_test.go | 2 +- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/cmp/compare_test.go b/cmp/compare_test.go index c26ac3f..734e46a 100644 --- a/cmp/compare_test.go +++ b/cmp/compare_test.go @@ -206,7 +206,7 @@ func comparerTests() []test { label: label, x: &struct{ R *bytes.Buffer }{new(bytes.Buffer)}, y: &struct{ R *bytes.Buffer }{}, - wantDiff: "root.R:\n\t-: \"\"\n\t+: \n", + wantDiff: "root.R:\n\t-: s\"\"\n\t+: \n", }, { label: label, x: &struct{ R *bytes.Buffer }{new(bytes.Buffer)}, @@ -262,8 +262,8 @@ func comparerTests() []test { })}, wantDiff: ` {[]*regexp.Regexp}[1]: - -: "a*b*c*" - +: "a*b*d*"`, + -: s"a*b*c*" + +: s"a*b*d*"`, }, { label: label, x: func() ***int { @@ -312,8 +312,8 @@ func comparerTests() []test { opts: []cmp.Option{cmp.Comparer(func(x, y fmt.Stringer) bool { return x.String() == y.String() })}, wantDiff: ` root: - -: "hello" - +: "hello2"`, + -: s"hello" + +: s"hello2"`, }, { label: label, x: md5.Sum([]byte{'a'}), @@ -405,6 +405,20 @@ root: {[]int}: -: []int{0, 0, 0, 0, 0, 0, 0, 0, 0, 0} +: []int{0, 0, 0, 0, 0, 0, 0, 0, 0, 0}`, + }, { + // Ensure reasonable Stringer formatting of map keys. + label: label, + x: map[*pb.Stringer]*pb.Stringer{{"hello"}: {"world"}}, + y: map[*pb.Stringer]*pb.Stringer(nil), + wantDiff: ` +{map[*testprotos.Stringer]*testprotos.Stringer}: + -: map[*testprotos.Stringer]*testprotos.Stringer{s"hello": s"world"} + +: map[*testprotos.Stringer]*testprotos.Stringer(nil)`, + }, { + // Ensure Stringer avoids double-quote escaping if possible. + label: label, + x: []*pb.Stringer{{`multi\nline\nline\nline`}}, + wantDiff: ":\n\t-: []*testprotos.Stringer{s`multi\\nline\\nline\\nline`}\n\t+: ", }} } @@ -1535,7 +1549,7 @@ func project1Tests() []test { Args: &pb.MetaData{Stringer: pb.Stringer{"metadata2"}}, }}}, opts: []cmp.Option{cmp.Comparer(pb.Equal)}, - wantDiff: "{teststructs.Eagle}.Slaps[4].Args:\n\t-: \"metadata\"\n\t+: \"metadata2\"\n", + wantDiff: "{teststructs.Eagle}.Slaps[4].Args:\n\t-: s\"metadata\"\n\t+: s\"metadata2\"\n", }, { label: label, x: createEagle(), @@ -1657,11 +1671,11 @@ func project2Tests() []test { opts: []cmp.Option{cmp.Comparer(pb.Equal), equalDish}, wantDiff: ` {teststructs.GermBatch}.DirtyGerms[18][0->?]: - -: "germ2" + -: s"germ2" +: {teststructs.GermBatch}.DirtyGerms[18][?->2]: -: - +: "germ2"`, + +: s"germ2"`, }, { label: label, x: createBatch(), @@ -1690,9 +1704,9 @@ func project2Tests() []test { wantDiff: ` {teststructs.GermBatch}.DirtyGerms[17]: -: - +: []*testprotos.Germ{"germ1"} + +: []*testprotos.Germ{s"germ1"} {teststructs.GermBatch}.DirtyGerms[18][2->?]: - -: "germ4" + -: s"germ4" +: {teststructs.GermBatch}.DishMap[1]: -: (*teststructs.Dish)(nil) @@ -1777,14 +1791,14 @@ func project3Tests() []test { -: teststructs.DiscordState(554) +: teststructs.DiscordState(500) λ({teststructs.Dirt}.Proto): - -: "blah" - +: "proto" + -: s"blah" + +: s"proto" {teststructs.Dirt}.wizard["albus"]: - -: "dumbledore" + -: s"dumbledore" +: {teststructs.Dirt}.wizard["harry"]: - -: "potter" - +: "otter"`, + -: s"potter" + +: s"otter"`, }} } diff --git a/cmp/internal/value/format.go b/cmp/internal/value/format.go index 9b27152..74d7f0c 100644 --- a/cmp/internal/value/format.go +++ b/cmp/internal/value/format.go @@ -8,9 +8,9 @@ package value import ( "fmt" "reflect" + "strconv" "strings" "unicode" - "unicode/utf8" ) var stringerIface = reflect.TypeOf((*fmt.Stringer)(nil)).Elem() @@ -43,7 +43,10 @@ func formatAny(v reflect.Value, conf formatConfig, visited map[uintptr]bool) str if (v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface) && v.IsNil() { return "" } - return fmt.Sprintf("%q", v.Interface().(fmt.Stringer).String()) + + const stringerPrefix = "s" // Indicates that the String method was used + s := v.Interface().(fmt.Stringer).String() + return stringerPrefix + formatString(s) } switch v.Kind() { @@ -62,7 +65,7 @@ func formatAny(v reflect.Value, conf formatConfig, visited map[uintptr]bool) str case reflect.Complex64, reflect.Complex128: return formatPrimitive(v.Type(), v.Complex(), conf) case reflect.String: - return formatPrimitive(v.Type(), fmt.Sprintf("%q", v), conf) + return formatPrimitive(v.Type(), formatString(v.String()), conf) case reflect.UnsafePointer, reflect.Chan, reflect.Func: return formatPointer(v, conf) case reflect.Ptr: @@ -123,11 +126,13 @@ func formatAny(v reflect.Value, conf formatConfig, visited map[uintptr]bool) str visited = insertPointer(visited, v.Pointer()) var ss []string - subConf := conf - subConf.printType = v.Type().Elem().Kind() == reflect.Interface + keyConf, valConf := conf, conf + keyConf.printType = v.Type().Key().Kind() == reflect.Interface + keyConf.followPointers = false + valConf.printType = v.Type().Elem().Kind() == reflect.Interface for _, k := range SortKeys(v.MapKeys()) { - sk := formatAny(k, formatConfig{realPointers: conf.realPointers}, visited) - sv := formatAny(v.MapIndex(k), subConf, visited) + sk := formatAny(k, keyConf, visited) + sv := formatAny(v.MapIndex(k), valConf, visited) ss = append(ss, fmt.Sprintf("%s: %s", sk, sv)) } s := fmt.Sprintf("{%s}", strings.Join(ss, ", ")) @@ -145,7 +150,7 @@ func formatAny(v reflect.Value, conf formatConfig, visited map[uintptr]bool) str continue // Elide zero value fields } name := v.Type().Field(i).Name - subConf.useStringer = conf.useStringer && isExported(name) + subConf.useStringer = conf.useStringer s := formatAny(vv, subConf, visited) ss = append(ss, fmt.Sprintf("%s: %s", name, s)) } @@ -159,6 +164,17 @@ func formatAny(v reflect.Value, conf formatConfig, visited map[uintptr]bool) str } } +func formatString(s string) string { + // Avoid performing quote-escaping if the string is already escaped. + hasEscapes := strings.ContainsAny(s, `\`) + allPrintable := strings.IndexFunc(s, unicode.IsPrint) >= 0 + rawAllowed := !strings.ContainsAny(s, "`\n") + if hasEscapes && allPrintable && rawAllowed { + return "`" + s + "`" + } + return strconv.Quote(s) +} + func formatPrimitive(t reflect.Type, v interface{}, conf formatConfig) string { if conf.printType && t.PkgPath() != "" { return fmt.Sprintf("%v(%v)", t, v) @@ -247,9 +263,3 @@ func isZero(v reflect.Value) bool { } return false } - -// isExported reports whether the identifier is exported. -func isExported(id string) bool { - r, _ := utf8.DecodeRuneInString(id) - return unicode.IsUpper(r) -} diff --git a/cmp/internal/value/format_test.go b/cmp/internal/value/format_test.go index b56380f..36c4f23 100644 --- a/cmp/internal/value/format_test.go +++ b/cmp/internal/value/format_test.go @@ -47,7 +47,7 @@ func TestFormat(t *testing.T) { want: "map[value.key]string{{a: 5, b: \"key\", c: (chan bool)(0x00)}: \"hello\"}", }, { in: map[io.Reader]string{new(bytes.Reader): "hello"}, - want: "map[io.Reader]string{0x00: \"hello\"}", + want: "map[io.Reader]string{(*bytes.Reader)(0x00): \"hello\"}", }, { in: func() interface{} { var a = []interface{}{nil}