internal/impl: avoid preserving presence on proto3 bytes field

When setting a proto3 bytes field to an empty, non-nil bytes slice,
just store a nil slice in the underderlying storage.
This is done to avoid presenting the illusion to the user that
presence is preserved for proto3.

Updates golang/protobuf#896

Change-Id: I1b97bedd547d336863c65d9418d8f07edf69ccd6
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/185577
Reviewed-by: Herbie Ong <herbie@google.com>

internal/impl/message_field.go

@@ -204,11 +204,15 @@ func fieldInfoForList(fd pref.FieldDescriptor, fs reflect.StructField, x exporte
 	}
 }
 
-var emptyBytes = reflect.ValueOf([]byte{})
+var (
+	nilBytes   = reflect.ValueOf([]byte(nil))
+	emptyBytes = reflect.ValueOf([]byte{})
+)
 
 func fieldInfoForScalar(fd pref.FieldDescriptor, fs reflect.StructField, x exporter) fieldInfo {
 	ft := fs.Type
 	nullable := fd.Syntax() == pref.Proto2
+	isBytes := ft.Kind() == reflect.Slice && ft.Elem().Kind() == reflect.Uint8
 	if nullable {
 		if ft.Kind() != reflect.Ptr && ft.Kind() != reflect.Slice {
 			panic(fmt.Sprintf("invalid type: got %v, want pointer", ft))
@@ -274,8 +278,12 @@ func fieldInfoForScalar(fd pref.FieldDescriptor, fs reflect.StructField, x expor
 				rv = rv.Elem()
 			}
 			rv.Set(conv.GoValueOf(v))
-			if nullable && rv.Kind() == reflect.Slice && rv.IsNil() {
-				rv.Set(emptyBytes)
+			if isBytes && rv.Len() == 0 {
+				if nullable {
+					rv.Set(emptyBytes) // preserve presence in proto2
+				} else {
+					rv.Set(nilBytes) // do not preserve presence in proto3
+				}
 			}
 		},
 	}

internal/impl/message_test.go

@@ -270,6 +270,11 @@ func TestScalarProto2(t *testing.T) {
 		}).ProtoReflect()},
 		clearFields{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22},
 		equalMessage{new(ScalarProto2).ProtoReflect()},
+
+		// Setting a bytes field nil empty bytes should preserve presence.
+		setFields{10: V([]byte(nil)), 11: V([]byte(nil)), 21: V([]byte(nil)), 22: V([]byte(nil))},
+		getFields{10: V([]byte{}), 11: V([]byte(nil)), 21: V([]byte{}), 22: V([]byte(nil))},
+		hasFields{10: true, 11: true, 21: true, 22: true},
 	})
 
 	// Test read-only operations on nil message.
@@ -391,6 +396,11 @@ func TestScalarProto3(t *testing.T) {
 		hasFields{6: false, 7: false},
 		setFields{6: V(float32(math.Copysign(0, -1))), 7: V(float64(math.Copysign(0, -1)))},
 		hasFields{6: true, 7: true},
+
+		// Setting a bytes field to non-nil empty bytes should not preserve presence.
+		setFields{10: V([]byte{}), 11: V([]byte{}), 21: V([]byte{}), 22: V([]byte{})},
+		getFields{10: V([]byte(nil)), 11: V([]byte(nil)), 21: V([]byte(nil)), 22: V([]byte(nil))},
+		hasFields{10: false, 11: false, 21: false, 22: false},
 	})
 
 	// Test read-only operations on nil message.
@@ -1200,7 +1210,6 @@ var cmpOpts = cmp.Options{
 		}
 	}),
 	cmpopts.EquateNaNs(),
-	cmpopts.EquateEmpty(),
 }
 
 func testMessage(t *testing.T, p path, m pref.Message, tt messageOps) {