Avoid leaking implementation details of the exporter

cmp/compare.go

@@ -386,6 +386,7 @@ func sanitizeValue(v reflect.Value, t reflect.Type) reflect.Value {
 }
 
 func (s *state) compareStruct(t reflect.Type, vx, vy reflect.Value) {
+	var addr bool
 	var vax, vay reflect.Value // Addressable versions of vx and vy
 
 	var mayForce, mayForceInit bool
@@ -407,6 +408,7 @@ func (s *state) compareStruct(t reflect.Type, vx, vy reflect.Value) {
 				// For retrieveUnexportedField to work, the parent struct must
 				// be addressable. Create a new copy of the values if
 				// necessary to make them addressable.
+				addr = vx.CanAddr() || vy.CanAddr()
 				vax = makeAddressable(vx)
 				vay = makeAddressable(vy)
 			}
@@ -417,6 +419,7 @@ func (s *state) compareStruct(t reflect.Type, vx, vy reflect.Value) {
 				mayForceInit = true
 			}
 			step.mayForce = mayForce
+			step.paddr = addr
 			step.pvx = vax
 			step.pvy = vay
 			step.field = t.Field(i)

cmp/compare_test.go

@@ -624,6 +624,35 @@ func comparerTests() []test {
 		y:         struct{ a int }{},
 		wantPanic: strconv.Quote(reflect.TypeOf(namedWithUnexported{}).PkgPath()) + ".(struct { a int })",
 		reason:    "panic on unnamed struct type with unexported field",
+	}, {
+		label: label,
+		x:     struct{ s fmt.Stringer }{new(bytes.Buffer)},
+		y:     struct{ s fmt.Stringer }{new(bytes.Buffer)},
+		opts: []cmp.Option{
+			cmp.AllowUnexported(struct{ s fmt.Stringer }{}),
+			cmp.FilterPath(func(p cmp.Path) bool {
+				if _, ok := p.Last().(cmp.StructField); !ok {
+					return false
+				}
+
+				t := p.Index(-1).Type()
+				vx, vy := p.Index(-1).Values()
+				pvx, pvy := p.Index(-2).Values()
+				switch {
+				case vx.Type() != t:
+					panic(fmt.Sprintf("inconsistent type: %v != %v", vx.Type(), t))
+				case vy.Type() != t:
+					panic(fmt.Sprintf("inconsistent type: %v != %v", vy.Type(), t))
+				case vx.CanAddr() != pvx.CanAddr():
+					panic(fmt.Sprintf("inconsistent addressability: %v != %v", vx.CanAddr(), pvx.CanAddr()))
+				case vy.CanAddr() != pvy.CanAddr():
+					panic(fmt.Sprintf("inconsistent addressability: %v != %v", vy.CanAddr(), pvy.CanAddr()))
+				}
+				return true
+			}, cmp.Ignore()),
+		},
+		wantEqual: true,
+		reason:    "verify that exporter does not leak implementation details",
 	}}
 }
 

cmp/export_panic.go

@@ -10,6 +10,6 @@ import "reflect"
 
 const supportExporters = false
 
-func retrieveUnexportedField(reflect.Value, reflect.StructField) reflect.Value {
+func retrieveUnexportedField(reflect.Value, reflect.StructField, bool) reflect.Value {
 	panic("no support for forcibly accessing unexported fields")
 }

cmp/export_unsafe.go

@@ -17,9 +17,16 @@ const supportExporters = true
 // a struct such that the value has read-write permissions.
 //
 // The parent struct, v, must be addressable, while f must be a StructField
-// describing the field to retrieve.
-func retrieveUnexportedField(v reflect.Value, f reflect.StructField) reflect.Value {
-	// See https://github.com/google/go-cmp/issues/167 for discussion of the
-	// following expression.
-	return reflect.NewAt(f.Type, unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr()))+f.Offset)).Elem()
+// describing the field to retrieve. If addr is false,
+// then the returned value will be shallowed copied to be non-addressable.
+func retrieveUnexportedField(v reflect.Value, f reflect.StructField, addr bool) reflect.Value {
+	ve := reflect.NewAt(f.Type, unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr()))+f.Offset)).Elem()
+	if !addr {
+		// A field is addressable if and only if the struct is addressable.
+		// If the original parent value was not addressable, shallow copy the
+		// value to make it non-addressable to avoid leaking an implementation
+		// detail of how forcibly exporting a field works.
+		ve = reflect.ValueOf(ve.Interface()).Convert(f.Type)
+	}
+	return ve
 }

cmp/path.go

@@ -177,7 +177,8 @@ type structField struct {
 	// pvx, pvy, and field are only valid if unexported is true.
 	unexported bool
 	mayForce   bool                // Forcibly allow visibility
-	pvx, pvy   reflect.Value       // Parent values
+	paddr      bool                // Was parent addressable?
+	pvx, pvy   reflect.Value       // Parent values (always addressible)
 	field      reflect.StructField // Field information
 }
 
@@ -189,8 +190,8 @@ func (sf StructField) Values() (vx, vy reflect.Value) {
 
 	// Forcibly obtain read-write access to an unexported struct field.
 	if sf.mayForce {
-		vx = retrieveUnexportedField(sf.pvx, sf.field)
-		vy = retrieveUnexportedField(sf.pvy, sf.field)
+		vx = retrieveUnexportedField(sf.pvx, sf.field, sf.paddr)
+		vy = retrieveUnexportedField(sf.pvy, sf.field, sf.paddr)
 		return vx, vy // CanInterface reports true
 	}
 	return sf.vx, sf.vy // CanInterface reports false

cmp/report_reflect.go

@@ -135,7 +135,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
 			}
 			sf := t.Field(i)
 			if supportExporters && !isExported(sf.Name) {
-				vv = retrieveUnexportedField(v, sf)
+				vv = retrieveUnexportedField(v, sf, true)
 			}
 			s := opts.WithTypeMode(autoType).FormatValue(vv, false, m)
 			list = append(list, textRecord{Key: sf.Name, Value: s})