proto: never return nil []byte from Marshal when successful

It is a sufficiently common pattern to do something like the following:
	m.Proto2BytesField, ... = proto.Marshal(m2)

where the user is relying on Marshal to never return a nil byte slice,
otherwise it subtly changes the semantics of how the generated API
handles whether a proto2 "optional bytes" fields is populated or not.

Change-Id: Ie7508dbcdcc5f3295885609a91907c6eb4f04c1e
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/228838
Reviewed-by: Damien Neil <dneil@google.com>

proto/encode.go

@@ -80,6 +80,9 @@ func Marshal(m Message) ([]byte, error) {
 	}
 
 	out, err := MarshalOptions{}.marshal(nil, m.ProtoReflect())
+	if len(out.Buf) == 0 && err == nil {
+		out.Buf = emptyBytesForMessage(m)
+	}
 	return out.Buf, err
 }
 
@@ -91,9 +94,26 @@ func (o MarshalOptions) Marshal(m Message) ([]byte, error) {
 	}
 
 	out, err := o.marshal(nil, m.ProtoReflect())
+	if len(out.Buf) == 0 && err == nil {
+		out.Buf = emptyBytesForMessage(m)
+	}
 	return out.Buf, err
 }
 
+// emptyBytesForMessage returns a nil buffer if and only if m is invalid,
+// otherwise it returns a non-nil empty buffer.
+//
+// This is to assist the edge-case where user-code does the following:
+//	m1.OptionalBytes, _ = proto.Marshal(m2)
+// where they expect the proto2 "optional_bytes" field to be populated
+// if any only if m2 is a valid message.
+func emptyBytesForMessage(m Message) []byte {
+	if m == nil || !m.ProtoReflect().IsValid() {
+		return nil
+	}
+	return emptyBuf[:]
+}
+
 // MarshalAppend appends the wire-format encoding of m to b,
 // returning the result.
 func (o MarshalOptions) MarshalAppend(b []byte, m Message) ([]byte, error) {

proto/encode_test.go

@@ -247,3 +247,28 @@ func TestEncodeLarge(t *testing.T) {
 		t.Errorf("after round-trip marshal, got len(m.OptionalBytes) = %v, want %v", got, want)
 	}
 }
+
+// TestEncodeEmpty tests for boundary conditions when producing an empty output.
+// These tests are not necessarily a statement of proper behavior,
+// but exist to detect accidental changes in behavior.
+func TestEncodeEmpty(t *testing.T) {
+	for _, m := range []proto.Message{nil, (*testpb.TestAllTypes)(nil), &testpb.TestAllTypes{}} {
+		isValid := m != nil && m.ProtoReflect().IsValid()
+
+		b, err := proto.Marshal(m)
+		if err != nil {
+			t.Errorf("proto.Marshal() = %v", err)
+		}
+		if isNil := b == nil; isNil == isValid {
+			t.Errorf("proto.Marshal() == nil: %v, want %v", isNil, !isValid)
+		}
+
+		b, err = proto.MarshalOptions{}.Marshal(m)
+		if err != nil {
+			t.Errorf("proto.MarshalOptions{}.Marshal() = %v", err)
+		}
+		if isNil := b == nil; isNil == isValid {
+			t.Errorf("proto.MarshalOptions{}.Marshal() = %v, want %v", isNil, !isValid)
+		}
+	}
+}