proto: distinguish between invalid and empty messages in Equal

The v1 proto.Equal function treats (*Message)(nil) and new(Message) as being different, while v2 proto.Equal treated them as equal since a typed nil pointer is functionally an empty message since the protobuf data model has no concept of presence as a first-class property of messages. Unfortunately, a significant amount of code depends on this distinction that it would be difficult to migrate users from v1 to v2 unless we preserved similar semantics in the v2 proto.Equal. Also, double down on these semantics for protocmp.Transform. Fixes #965 Change-Id: I21e78ba6251401a0ac0ccf495188093973cd7f3f Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/213238 Reviewed-by: Damien Neil <[email protected]>
golang · Jan 6, 2020 · 96a4473 · 96a4473
1 parent 14ac241
commit 96a4473
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 16 deletions.
diff --git a/proto/decode_test.go b/proto/decode_test.go
@@ -40,7 +40,7 @@ func TestDecode(t *testing.T) {
 				for i := range wire {
 					wire[i] = 0
 				}
-				if !proto.Equal(got, want) {
+				if !proto.Equal(got, want) && got.ProtoReflect().IsValid() && want.ProtoReflect().IsValid() {
 					t.Errorf("Unmarshal returned unexpected result; got:\n%v\nwant:\n%v", marshalText(got), marshalText(want))
 				}
 			})

diff --git a/proto/encode_test.go b/proto/encode_test.go
@@ -45,7 +45,7 @@ func TestEncode(t *testing.T) {
 					t.Errorf("Unmarshal error: %v\nMessage:\n%v", err, marshalText(want))
 					return
 				}
-				if !proto.Equal(got, want) {
+				if !proto.Equal(got, want) && got.ProtoReflect().IsValid() && want.ProtoReflect().IsValid() {
 					t.Errorf("Unmarshal returned unexpected result; got:\n%v\nwant:\n%v", marshalText(got), marshalText(want))
 				}
 			})
@@ -81,7 +81,7 @@ func TestEncodeDeterministic(t *testing.T) {
 					t.Errorf("Unmarshal error: %v\nMessage:\n%v", err, marshalText(want))
 					return
 				}
-				if !proto.Equal(got, want) {
+				if !proto.Equal(got, want) && got.ProtoReflect().IsValid() && want.ProtoReflect().IsValid() {
 					t.Errorf("Unmarshal returned unexpected result; got:\n%v\nwant:\n%v", marshalText(got), marshalText(want))
 				}
 			})

diff --git a/proto/equal.go b/proto/equal.go
@@ -19,7 +19,8 @@ import (
 //
 // Two messages are equal if they belong to the same message descriptor,
 // have the same set of populated known and extension field values,
-// and the same set of unknown fields values.
+// and the same set of unknown fields values. If either of the top-level
+// messages are invalid, then Equal reports true only if both are invalid.
 //
 // Scalar values are compared with the equivalent of the == operator in Go,
 // except bytes values which are compared using bytes.Equal and
@@ -32,7 +33,12 @@ func Equal(x, y Message) bool {
 	if x == nil || y == nil {
 		return x == nil && y == nil
 	}
-	return equalMessage(x.ProtoReflect(), y.ProtoReflect())
+	mx := x.ProtoReflect()
+	my := y.ProtoReflect()
+	if !mx.IsValid() || !my.IsValid() {
+		return !mx.IsValid() && !my.IsValid()
+	}
+	return equalMessage(mx, my)
 }
 
 // equalMessage compares two messages.

diff --git a/proto/equal_test.go b/proto/equal_test.go
@@ -30,6 +30,14 @@ func TestEqual(t *testing.T) {
 			eq: false,
 		}, {
 			x:  (*testpb.TestAllTypes)(nil),
+			y:  (*testpb.TestAllTypes)(nil),
+			eq: true,
+		}, {
+			x:  new(testpb.TestAllTypes),
+			y:  (*testpb.TestAllTypes)(nil),
+			eq: false,
+		}, {
+			x:  new(testpb.TestAllTypes),
 			y:  new(testpb.TestAllTypes),
 			eq: true,
 		},

diff --git a/testing/protocmp/xform.go b/testing/protocmp/xform.go
@@ -153,18 +153,12 @@ func Transform(...option) cmp.Option {
 		}
 
 		return false
-	}, cmp.Transformer("protocmp.Transform", func(m interface{}) Message {
-		if m == nil {
+	}, cmp.Transformer("protocmp.Transform", func(v interface{}) Message {
+		m := protoimpl.X.MessageOf(v)
+		if m == nil || !m.IsValid() {
 			return nil
 		}
-
-		// TODO: Should typed nil messages result in a nil Message?
-		// For now, do so as it is easier to remove this check than to add it.
-		if v := reflect.ValueOf(m); v.Kind() == reflect.Ptr && v.IsNil() {
-			return nil
-		}
-
-		return transformMessage(protoimpl.X.MessageOf(m))
+		return transformMessage(m)
 	}))
 }
 

diff --git a/testing/protocmp/xform_test.go b/testing/protocmp/xform_test.go
@@ -22,7 +22,6 @@ func init() {
 }
 
 func TestTransform(t *testing.T) {
-	t.Skip()
 	tests := []struct {
 		in         proto.Message
 		want       Message