Skip to content

Commit

Permalink
Improve reporting of values with cycles
Browse files Browse the repository at this point in the history 
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⟫{...}.
  • Loading branch information
@dsnet
dsnet committed Jun 18, 2020
1 parent c49bfce commit ae31fba
Show file tree
Hide file tree
Showing 9 changed files with 490 additions and 200 deletions.
10 changes: 10 additions & 0 deletions cmp/internal/value/pointer_purego.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,13 @@ func PointerOf(v reflect.Value) Pointer {
// assumes that the GC implementation does not use a moving collector.
return Pointer{v.Pointer(), v.Type()}
}

// IsNil reports whether the pointer is nil.
func (p Pointer) IsNil() bool {
return p.p == 0
}

// Uintptr returns the pointer as a uintptr.
func (p Pointer) Uintptr() uintptr {
return p.p
}
10 changes: 10 additions & 0 deletions cmp/internal/value/pointer_unsafe.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,13 @@ func PointerOf(v reflect.Value) Pointer {
// which is necessary if the GC ever uses a moving collector.
return Pointer{unsafe.Pointer(v.Pointer()), v.Type()}
}

// IsNil reports whether the pointer is nil.
func (p Pointer) IsNil() bool {
return p.p == nil
}

// Uintptr returns the pointer as a uintptr.
func (p Pointer) Uintptr() uintptr {
return uintptr(p.p)
}
5 changes: 4 additions & 1 deletion cmp/report.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ func (r *defaultReporter) String() string {
if r.root.NumDiff == 0 {
return ""
}
return formatOptions{}.FormatDiff(r.root).String()
ptrs := new(pointerReferences)
text := formatOptions{}.FormatDiff(r.root, ptrs)
resolveReferences(text)
return text.String()
}

func assert(ok bool) {
Expand Down
85 changes: 53 additions & 32 deletions cmp/report_compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func verbosityPreset(opts formatOptions, i int) formatOptions {

// 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 {
func (opts formatOptions) FormatDiff(v *valueNode, ptrs *pointerReferences) (out textNode) {
if opts.DiffMode == diffIdentical {
opts = opts.WithVerbosity(1)
} else {
Expand All @@ -110,9 +110,9 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
return opts.FormatDiffSlice(v)
}

var withinSlice bool
if v.parent != nil && (v.parent.Type.Kind() == reflect.Slice || v.parent.Type.Kind() == reflect.Array) {
withinSlice = true
var parentKind reflect.Kind
if v.parent != nil && v.parent.TransformerName == "" {
parentKind = v.parent.Type.Kind()
}

// For leaf nodes, format the value based on the reflect.Values alone.
Expand All @@ -121,8 +121,8 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
case diffUnknown, diffIdentical:
// Format Equal.
if v.NumDiff == 0 {
outx := opts.FormatValue(v.ValueX, withinSlice, visitedPointers{})
outy := opts.FormatValue(v.ValueY, withinSlice, visitedPointers{})
outx := opts.FormatValue(v.ValueX, parentKind, ptrs)
outy := opts.FormatValue(v.ValueY, parentKind, ptrs)
if v.NumIgnored > 0 && v.NumSame == 0 {
return textEllipsis
} else if outx.Len() < outy.Len() {
Expand All @@ -135,12 +135,12 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
// Format unequal.
assert(opts.DiffMode == diffUnknown)
var list textList
outx := opts.WithTypeMode(elideType).FormatValue(v.ValueX, withinSlice, visitedPointers{})
outy := opts.WithTypeMode(elideType).FormatValue(v.ValueY, withinSlice, visitedPointers{})
outx := opts.WithTypeMode(elideType).FormatValue(v.ValueX, parentKind, ptrs)
outy := opts.WithTypeMode(elideType).FormatValue(v.ValueY, parentKind, ptrs)
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{})
outx = opts2.FormatValue(v.ValueX, parentKind, ptrs)
outy = opts2.FormatValue(v.ValueY, parentKind, ptrs)
}
if outx != nil {
list = append(list, textRecord{Diff: '-', Value: outx})
Expand All @@ -150,36 +150,57 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
}
return opts.WithTypeMode(emitType).FormatType(v.Type, list)
case diffRemoved:
return opts.FormatValue(v.ValueX, withinSlice, visitedPointers{})
return opts.FormatValue(v.ValueX, parentKind, ptrs)
case diffInserted:
return opts.FormatValue(v.ValueY, withinSlice, visitedPointers{})
return opts.FormatValue(v.ValueY, parentKind, ptrs)
default:
panic("invalid diff mode")
}
}

// TODO: Print cycle reference for pointers, maps, and elements of a slice.
// Register slice element to support cycle detection.
if parentKind == reflect.Slice {
ptrRefs := ptrs.PushPair(v.ValueX, v.ValueY, opts.DiffMode, true)
defer ptrs.Pop()
defer func() { out = wrapTrunkReferences(ptrRefs, out) }()
}

// Descend into the child value node.
if v.TransformerName != "" {
out := opts.WithTypeMode(emitType).FormatDiff(v.Value)
out = textWrap{"Inverse(" + v.TransformerName + ", ", out, ")"}
out := opts.WithTypeMode(emitType).FormatDiff(v.Value, ptrs)
out = &textWrap{Prefix: "Inverse(" + v.TransformerName + ", ", Value: out, Suffix: ")"}
return opts.FormatType(v.Type, out)
} else {
switch k := v.Type.Kind(); k {
case reflect.Struct, reflect.Array, reflect.Slice, reflect.Map:
return opts.FormatType(v.Type, opts.formatDiffList(v.Records, k))
case reflect.Struct, reflect.Array, reflect.Slice:
out = opts.formatDiffList(v.Records, k, ptrs)
out = opts.FormatType(v.Type, out)
case reflect.Map:
// Register map to support cycle detection.
ptrRefs := ptrs.PushPair(v.ValueX, v.ValueY, opts.DiffMode, false)
defer ptrs.Pop()

out = opts.formatDiffList(v.Records, k, ptrs)
out = wrapTrunkReferences(ptrRefs, out)
out = opts.FormatType(v.Type, out)
case reflect.Ptr:
return textWrap{"&", opts.FormatDiff(v.Value), ""}
// Register pointer to support cycle detection.
ptrRefs := ptrs.PushPair(v.ValueX, v.ValueY, opts.DiffMode, false)
defer ptrs.Pop()

out = opts.FormatDiff(v.Value, ptrs)
out = wrapTrunkReferences(ptrRefs, out)
out = &textWrap{Prefix: "&", Value: out}
case reflect.Interface:
return opts.WithTypeMode(emitType).FormatDiff(v.Value)
out = opts.WithTypeMode(emitType).FormatDiff(v.Value, ptrs)
default:
panic(fmt.Sprintf("%v cannot have children", k))
}
return out
}
}

func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) textNode {
func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind, ptrs *pointerReferences) textNode {
// Derive record name based on the data structure kind.
var name string
var formatKey func(reflect.Value) string
Expand All @@ -195,7 +216,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
case reflect.Map:
name = "entry"
opts = opts.WithTypeMode(elideType)
formatKey = func(v reflect.Value) string { return formatMapKey(v, false) }
formatKey = func(v reflect.Value) string { return formatMapKey(v, false, ptrs) }
}

maxLen := -1
Expand Down Expand Up @@ -242,14 +263,14 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
}
continue
}
if out := opts.FormatDiff(r.Value); out != nil {
if out := opts.FormatDiff(r.Value, ptrs); out != nil {
list = append(list, textRecord{Key: formatKey(r.Key), Value: out})
}
}
if deferredEllipsis {
list.AppendEllipsis(diffStats{})
}
return textWrap{"{", list, "}"}
return &textWrap{Prefix: "{", Value: list, Suffix: "}"}
case diffUnknown:
default:
panic("invalid diff mode")
Expand Down Expand Up @@ -290,7 +311,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te

// Format the equal values.
for _, r := range recs[:numLo] {
out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value)
out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value, ptrs)
list = append(list, textRecord{Key: formatKey(r.Key), Value: out})
keys = append(keys, r.Key)
}
Expand All @@ -302,7 +323,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
}
}
for _, r := range recs[numEqual-numHi : numEqual] {
out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value)
out := opts.WithDiffMode(diffIdentical).FormatDiff(r.Value, ptrs)
list = append(list, textRecord{Key: formatKey(r.Key), Value: out})
keys = append(keys, r.Key)
}
Expand All @@ -318,12 +339,12 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
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)
outx := opts.WithDiffMode(diffRemoved).FormatDiff(r.Value, ptrs)
outy := opts.WithDiffMode(diffInserted).FormatDiff(r.Value, ptrs)
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)
outx = opts2.WithDiffMode(diffRemoved).FormatDiff(r.Value, ptrs)
outy = opts2.WithDiffMode(diffInserted).FormatDiff(r.Value, ptrs)
}
if outx != nil {
list = append(list, textRecord{Diff: diffRemoved, Key: formatKey(r.Key), Value: outx})
Expand All @@ -334,7 +355,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
keys = append(keys, r.Key)
}
default:
out := opts.FormatDiff(r.Value)
out := opts.FormatDiff(r.Value, ptrs)
list = append(list, textRecord{Key: formatKey(r.Key), Value: out})
keys = append(keys, r.Key)
}
Expand Down Expand Up @@ -373,13 +394,13 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
if ambiguous {
for i, k := range keys {
if k.IsValid() {
list[i].Key = formatMapKey(k, true)
list[i].Key = formatMapKey(k, true, ptrs)
}
}
}
}

return textWrap{"{", list, "}"}
return &textWrap{Prefix: "{", Value: list, Suffix: "}"}
}

// coalesceAdjacentRecords coalesces the list of records into groups of
Expand Down

0 comments on commit ae31fba

Please sign in to comment.