Skip to content

Commit

Permalink
Fix printing of types in reporter output (#293)
Browse files Browse the repository at this point in the history
When printing a pointer, only elide the type for unnamed pointers.
Otherwise, we can run into situations where named and unnamed pointers
format the same way in indistinguishable ways.

When printing an interview, never skip the interface type.
Whether we skip printing the type should be determined by the
parent containers, and not locally determined.
For examples, interface values within a struct, slice, or map
will always be elided since they can be inferred.
  • Loading branch information
dsnet authored Apr 25, 2022
1 parent 79433ac commit 4664e24
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 2 deletions.
2 changes: 2 additions & 0 deletions cmp/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import (
"github.com/google/go-cmp/cmp/internal/value"
)

// TODO(≥go1.18): Use any instead of interface{}.

// Equal reports whether x and y are equal by recursively applying the
// following rules in the given order to x and y and all of their sub-values:
//
Expand Down
34 changes: 34 additions & 0 deletions cmp/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ func mustFormatGolden(path string, in []struct{ Name, Data string }) {

var now = time.Date(2009, time.November, 10, 23, 00, 00, 00, time.UTC)

// TODO(≥go1.18): Define a generic function that boxes a value on the heap.
func newInt(n int) *int { return &n }

type Stringer string
Expand Down Expand Up @@ -885,6 +886,7 @@ func reporterTests() []test {
FloatsB []MyFloat
FloatsC MyFloats
}
PointerString *string
)

return []test{{
Expand Down Expand Up @@ -1351,6 +1353,38 @@ using the AllowUnexported option.`, "\n"),
"bar": true,
}`,
reason: "short multiline JSON should prefer triple-quoted string diff as it is more readable",
}, {
label: label + "/PointerToStringOrAny",
x: func() *string {
var v string = "hello"
return &v
}(),
y: func() *interface{} {
var v interface{} = "hello"
return &v
}(),
reason: "mismatched types between any and *any should print differently",
}, {
label: label + "/NamedPointer",
x: func() *string {
v := "hello"
return &v
}(),
y: func() PointerString {
v := "hello"
return &v
}(),
reason: "mismatched pointer types should print differently",
}, {
label: label + "/MapStringAny",
x: map[string]interface{}{"key": int(0)},
y: map[string]interface{}{"key": uint(0)},
reason: "mismatched underlying value within interface",
}, {
label: label + "/StructFieldAny",
x: struct{ X interface{} }{int(0)},
y: struct{ X interface{} }{uint(0)},
reason: "mismatched underlying value within interface",
}}
}

Expand Down
8 changes: 6 additions & 2 deletions cmp/report_reflect.go
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,12 @@ func (opts formatOptions) FormatValue(v reflect.Value, parentKind reflect.Kind,
}
defer ptrs.Pop()

skipType = true // Let the underlying value print the type instead
// Skip the name only if this is an unnamed pointer type.
// Otherwise taking the address of a value does not reproduce
// the named pointer type.
if v.Type().Name() == "" {
skipType = true // Let the underlying value print the type instead
}
out = opts.FormatValue(v.Elem(), t.Kind(), ptrs)
out = wrapTrunkReference(ptrRef, opts.PrintAddresses, out)
out = &textWrap{Prefix: "&", Value: out}
Expand All @@ -293,7 +298,6 @@ func (opts formatOptions) FormatValue(v reflect.Value, parentKind reflect.Kind,
}
// 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(), t.Kind(), ptrs)
default:
panic(fmt.Sprintf("%v kind not handled", v.Kind()))
Expand Down
24 changes: 24 additions & 0 deletions cmp/testdata/diffs
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,30 @@
"""
)
>>> TestDiff/Reporter/ShortJSON
<<< TestDiff/Reporter/PointerToStringOrAny
any(
- &string("hello"),
+ &any(string("hello")),
)
>>> TestDiff/Reporter/PointerToStringOrAny
<<< TestDiff/Reporter/NamedPointer
any(
- &string("hello"),
+ cmp_test.PointerString(&string("hello")),
)
>>> TestDiff/Reporter/NamedPointer
<<< TestDiff/Reporter/MapStringAny
map[string]any{
- "key": int(0),
+ "key": uint(0),
}
>>> TestDiff/Reporter/MapStringAny
<<< TestDiff/Reporter/StructFieldAny
struct{ X any }{
- X: int(0),
+ X: uint(0),
}
>>> TestDiff/Reporter/StructFieldAny
<<< TestDiff/EmbeddedStruct/ParentStructA/Inequal
teststructs.ParentStructA{
privateStruct: teststructs.privateStruct{
Expand Down

0 comments on commit 4664e24

Please sign in to comment.