From 77ae86f624cb174e21763cffcbbf070eb06cb016 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 17 Jun 2020 17:25:47 -0700 Subject: Improve reporting of values with cycles (#217) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the reporter could handle formatting values with cycles in that it did not crash with a stack overflow. However, the output was not particularly understandable as it did not surface to the user why a particular value was truncated, and if it was truncated due to a cyclic reference, what was the referent. This change annotates the reporter tree with pointer information so that a later pass can inject reference information if it is needed to produce more understandable output. Consider the following example: map[string]*cmp_test.CycleAlpha{ "Foo": &⟪ref#0⟫{ Name: "Foo", Bravos: map[string]*cmp_test.CycleBravo{ "FooBravo": &{ - ID: 101, + ID: 0, Name: "FooBravo", Mods: 100, Alphas: {"Foo": &⟪ref#0⟫(...)}, }, }, }, } This graph contains a cycle. To ensure that a graph can be formatted, the cycle is truncated as indicated with: &⟪ref#0⟫(...). The referent was identified earlier with: &⟪ref#0⟫{...}. --- cmp/report_reflect.go | 156 +++++++++++++++++++++++++------------------------- 1 file changed, 78 insertions(+), 78 deletions(-) (limited to 'cmp/report_reflect.go') diff --git a/cmp/report_reflect.go b/cmp/report_reflect.go index 8b4325d..2d722ea 100644 --- a/cmp/report_reflect.go +++ b/cmp/report_reflect.go @@ -12,7 +12,6 @@ import ( "unicode" "unicode/utf8" - "github.com/google/go-cmp/cmp/internal/flags" "github.com/google/go-cmp/cmp/internal/value" ) @@ -21,11 +20,6 @@ type formatValueOptions struct { // methods like error.Error or fmt.Stringer.String. AvoidStringer bool - // PrintShallowPointer controls whether to print the next pointer. - // Useful when printing map keys, where pointer comparison is performed - // on the pointer address rather than the pointed-at value. - PrintShallowPointer bool - // PrintAddresses controls whether to print the address of all pointers, // slice elements, and maps. PrintAddresses bool @@ -75,26 +69,56 @@ func (opts formatOptions) FormatType(t reflect.Type, s textNode) textNode { typeName = "(" + typeName + ")" } } + return &textWrap{Prefix: typeName, Value: wrapParens(s)} +} + +// wrapParens wraps s with a set of parenthesis, but avoids it if the +// wrapped node itself is already surrounded by a pair of parenthesis or braces. +// It handles unwrapping one level of pointer-reference nodes. +func wrapParens(s textNode) textNode { + var refNode *textWrap + if s2, ok := s.(*textWrap); ok { + // Unwrap a single pointer reference node. + switch s2.Metadata.(type) { + case leafReference, trunkReference, trunkReferences: + refNode = s2 + if s3, ok := refNode.Value.(*textWrap); ok { + s2 = s3 + } + } - // Avoid wrap the value in parenthesis if unnecessary. - if s, ok := s.(textWrap); ok { - hasParens := strings.HasPrefix(s.Prefix, "(") && strings.HasSuffix(s.Suffix, ")") - hasBraces := strings.HasPrefix(s.Prefix, "{") && strings.HasSuffix(s.Suffix, "}") + // Already has delimiters that make parenthesis unnecessary. + hasParens := strings.HasPrefix(s2.Prefix, "(") && strings.HasSuffix(s2.Suffix, ")") + hasBraces := strings.HasPrefix(s2.Prefix, "{") && strings.HasSuffix(s2.Suffix, "}") if hasParens || hasBraces { - return textWrap{typeName, s, ""} + return s } } - return textWrap{typeName + "(", s, ")"} + if refNode != nil { + refNode.Value = &textWrap{Prefix: "(", Value: refNode.Value, Suffix: ")"} + return s + } + return &textWrap{Prefix: "(", Value: s, Suffix: ")"} } // FormatValue prints the reflect.Value, taking extra care to avoid descending -// into pointers already in m. As pointers are visited, m is also updated. -func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visitedPointers) (out textNode) { +// into pointers already in ptrs. As pointers are visited, ptrs is also updated. +func (opts formatOptions) FormatValue(v reflect.Value, parentKind reflect.Kind, ptrs *pointerReferences) (out textNode) { if !v.IsValid() { return nil } t := v.Type() + // Check slice element for cycles. + if parentKind == reflect.Slice { + ptrRef, visited := ptrs.Push(v.Addr()) + if visited { + return makeLeafReference(ptrRef, false) + } + defer ptrs.Pop() + defer func() { out = wrapTrunkReference(ptrRef, false, out) }() + } + // Check whether there is an Error or String method to call. if !opts.AvoidStringer && v.CanInterface() { // Avoid calling Error or String methods on nil receivers since many @@ -128,7 +152,6 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit } }() - var ptr string switch t.Kind() { case reflect.Bool: return textLine(fmt.Sprint(v.Bool())) @@ -137,7 +160,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit case reflect.Uint, reflect.Uint16, reflect.Uint32, reflect.Uint64: return textLine(fmt.Sprint(v.Uint())) case reflect.Uint8: - if withinSlice { + if parentKind == reflect.Slice || parentKind == reflect.Array { return textLine(formatHex(v.Uint())) } return textLine(fmt.Sprint(v.Uint())) @@ -157,7 +180,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit } return textLine(formatString(v.String())) case reflect.UnsafePointer, reflect.Chan, reflect.Func: - return textLine(formatPointer(v)) + return textLine(formatPointer(value.PointerOf(v), true)) case reflect.Struct: var list textList v := makeAddressable(v) // needed for retrieveUnexportedField @@ -179,17 +202,14 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit if supportExporters && !isExported(sf.Name) { vv = retrieveUnexportedField(v, sf, true) } - s := opts.WithTypeMode(autoType).FormatValue(vv, false, m) + s := opts.WithTypeMode(autoType).FormatValue(vv, t.Kind(), ptrs) list = append(list, textRecord{Key: sf.Name, Value: s}) } - return textWrap{"{", list, "}"} + return &textWrap{Prefix: "{", Value: list, Suffix: "}"} case reflect.Slice: if v.IsNil() { return textNil } - if opts.PrintAddresses { - ptr = fmt.Sprintf("⟪ptr:0x%x, len:%d, cap:%d⟫", pointerValue(v), v.Len(), v.Cap()) - } fallthrough case reflect.Array: maxLen := v.Len() @@ -203,29 +223,27 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit list.AppendEllipsis(diffStats{}) break } - vi := v.Index(i) - if vi.CanAddr() { // Check for cyclic elements - p := vi.Addr() - if m.Visit(p) { - var out textNode - out = textLine(formatPointer(p)) - out = opts.WithTypeMode(emitType).FormatType(p.Type(), out) - out = textWrap{"*", out, ""} - list = append(list, textRecord{Value: out}) - continue - } - } - s := opts.WithTypeMode(elideType).FormatValue(vi, true, m) + s := opts.WithTypeMode(elideType).FormatValue(v.Index(i), t.Kind(), ptrs) list = append(list, textRecord{Value: s}) } - return textWrap{ptr + "{", list, "}"} + + out = &textWrap{Prefix: "{", Value: list, Suffix: "}"} + if t.Kind() == reflect.Slice && opts.PrintAddresses { + header := fmt.Sprintf("ptr:%v, len:%d, cap:%d", formatPointer(value.PointerOf(v), false), v.Len(), v.Cap()) + out = &textWrap{Prefix: pointerDelimPrefix + header + pointerDelimSuffix, Value: out} + } + return out case reflect.Map: if v.IsNil() { return textNil } - if m.Visit(v) { - return textLine(formatPointer(v)) + + // Check pointer for cycles. + ptrRef, visited := ptrs.Push(v) + if visited { + return makeLeafReference(ptrRef, opts.PrintAddresses) } + defer ptrs.Pop() maxLen := v.Len() if opts.LimitVerbosity { @@ -238,27 +256,32 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit list.AppendEllipsis(diffStats{}) break } - sk := formatMapKey(k, false) - sv := opts.WithTypeMode(elideType).FormatValue(v.MapIndex(k), false, m) + sk := formatMapKey(k, false, ptrs) + sv := opts.WithTypeMode(elideType).FormatValue(v.MapIndex(k), t.Kind(), ptrs) list = append(list, textRecord{Key: sk, Value: sv}) } - if opts.PrintAddresses { - ptr = formatPointer(v) - } - return textWrap{ptr + "{", list, "}"} + + out = &textWrap{Prefix: "{", Value: list, Suffix: "}"} + out = wrapTrunkReference(ptrRef, opts.PrintAddresses, out) + return out case reflect.Ptr: if v.IsNil() { return textNil } - if m.Visit(v) { - return textLine(formatPointer(v)) - } - if opts.PrintAddresses || opts.PrintShallowPointer { - ptr = formatPointer(v) - opts.PrintShallowPointer = false + + // Check pointer for cycles. + ptrRef, visited := ptrs.Push(v) + if visited { + out = makeLeafReference(ptrRef, opts.PrintAddresses) + return &textWrap{Prefix: "&", Value: out} } + defer ptrs.Pop() + skipType = true // Let the underlying value print the type instead - return textWrap{"&" + ptr, opts.FormatValue(v.Elem(), false, m), ""} + out = opts.FormatValue(v.Elem(), t.Kind(), ptrs) + out = wrapTrunkReference(ptrRef, opts.PrintAddresses, out) + out = &textWrap{Prefix: "&", Value: out} + return out case reflect.Interface: if v.IsNil() { return textNil @@ -266,7 +289,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit // Interfaces accept different concrete types, // so configure the underlying value to explicitly print the type. skipType = true // Print the concrete type instead - return opts.WithTypeMode(emitType).FormatValue(v.Elem(), false, m) + return opts.WithTypeMode(emitType).FormatValue(v.Elem(), t.Kind(), ptrs) default: panic(fmt.Sprintf("%v kind not handled", v.Kind())) } @@ -274,14 +297,14 @@ 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, disambiguate bool) string { +func formatMapKey(v reflect.Value, disambiguate bool, ptrs *pointerReferences) string { var opts formatOptions opts.DiffMode = diffIdentical opts.TypeMode = elideType - opts.PrintShallowPointer = true + opts.PrintAddresses = disambiguate opts.AvoidStringer = disambiguate opts.QualifiedNames = disambiguate - s := opts.FormatValue(v, false, visitedPointers{}).String() + s := opts.FormatValue(v, reflect.Map, ptrs).String() return strings.TrimSpace(s) } @@ -328,26 +351,3 @@ func formatHex(u uint64) string { } return fmt.Sprintf(f, u) } - -// formatPointer prints the address of the pointer. -func formatPointer(v reflect.Value) string { - return fmt.Sprintf("⟪0x%x⟫", pointerValue(v)) -} -func pointerValue(v reflect.Value) uintptr { - p := v.Pointer() - if flags.Deterministic { - p = 0xdeadf00f // Only used for stable testing purposes - } - return p -} - -type visitedPointers map[value.Pointer]struct{} - -// Visit inserts pointer v into the visited map and reports whether it had -// already been visited before. -func (m visitedPointers) Visit(v reflect.Value) bool { - p := value.PointerOf(v) - _, visited := m[p] - m[p] = struct{}{} - return visited -} -- cgit v1.2.3