internal/impl: treat a nil oneof wrapper as if it were unset

The old implementation had the behavior where a nil wrapper value:
	m := new(foopb.Message)
	m.OneofField = (*foopb.Message_OneofUint32)(nil)
was functionally equivalent to it being directly set to nil:
	m := new(foopb.Message)
	m.OneofField = nil
preserve this semantic in both the table-drive implementation
and the reflection implementation.

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

internal/impl/codec_field.go

@@ -30,7 +30,7 @@ func makeOneofFieldCoder(si structInfo, fd pref.FieldDescriptor) pointerCoderFun
 			return pointer{}, false
 		}
 		v = v.Elem() // interface -> *struct
-		if v.Elem().Type() != ot {
+		if v.IsNil() || v.Elem().Type() != ot {
 			return pointer{}, false
 		}
 		return pointerOfValue(v).Apply(zeroOffset), true

internal/impl/message_reflect_field.go

@@ -56,7 +56,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export
 				return false
 			}
 			rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
-			if rv.IsNil() || rv.Elem().Type().Elem() != ot {
+			if rv.IsNil() || rv.Elem().Type().Elem() != ot || rv.Elem().IsNil() {
 				return false
 			}
 			return true
@@ -64,6 +64,8 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export
 		clear: func(p pointer) {
 			rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
 			if rv.IsNil() || rv.Elem().Type().Elem() != ot {
+				// NOTE: We intentionally don't check for rv.Elem().IsNil()
+				// so that (*OneofWrapperType)(nil) gets cleared to nil.
 				return
 			}
 			rv.Set(reflect.Zero(rv.Type()))
@@ -73,7 +75,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export
 				return conv.Zero()
 			}
 			rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
-			if rv.IsNil() || rv.Elem().Type().Elem() != ot {
+			if rv.IsNil() || rv.Elem().Type().Elem() != ot || rv.Elem().IsNil() {
 				return conv.Zero()
 			}
 			rv = rv.Elem().Elem().Field(0)
@@ -81,7 +83,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export
 		},
 		set: func(p pointer, v pref.Value) {
 			rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
-			if rv.IsNil() || rv.Elem().Type().Elem() != ot {
+			if rv.IsNil() || rv.Elem().Type().Elem() != ot || rv.Elem().IsNil() {
 				rv.Set(reflect.New(ot))
 			}
 			rv = rv.Elem().Elem().Field(0)
@@ -92,7 +94,7 @@ func fieldInfoForOneof(fd pref.FieldDescriptor, fs reflect.StructField, x export
 				panic("invalid Mutable on field with non-composite type")
 			}
 			rv := p.Apply(fieldOffset).AsValueOf(fs.Type).Elem()
-			if rv.IsNil() || rv.Elem().Type().Elem() != ot {
+			if rv.IsNil() || rv.Elem().Type().Elem() != ot || rv.Elem().IsNil() {
 				rv.Set(reflect.New(ot))
 			}
 			rv = rv.Elem().Elem().Field(0)

internal/testprotos/test/test.proto

@@ -124,6 +124,11 @@ message TestAllTypes {
     double        oneof_double         = 118;
     NestedEnum    oneof_enum           = 119;
   }
+
+  // A oneof with exactly one field.
+  oneof oneof_optional {
+    uint32 oneof_optional_uint32 = 120;
+  }
 }
 
 message TestDeprecatedMessage {

proto/decode_test.go

@@ -122,6 +122,19 @@ func TestDecodeZeroLengthBytes(t *testing.T) {
 	}
 }
 
+func TestDecodeOneofNilWrapper(t *testing.T) {
+	wire := pack.Message{
+		pack.Tag{111, pack.VarintType}, pack.Varint(1111),
+	}.Marshal()
+	m := &testpb.TestAllTypes{OneofField: (*testpb.TestAllTypes_OneofUint32)(nil)}
+	if err := proto.Unmarshal(wire, m); err != nil {
+		t.Fatal(err)
+	}
+	if got := m.GetOneofUint32(); got != 1111 {
+		t.Errorf("GetOneofUint32() = %v, want %v", got, 1111)
+	}
+}
+
 var testProtos = []testProto{
 	{
 		desc: "basic scalar types",

proto/encode_test.go

@@ -13,6 +13,7 @@ import (
 	"google.golang.org/protobuf/internal/flags"
 	"google.golang.org/protobuf/proto"
 
+	testpb "google.golang.org/protobuf/internal/testprotos/test"
 	test3pb "google.golang.org/protobuf/internal/testprotos/test3"
 )
 
@@ -130,7 +131,7 @@ func TestEncodeRequiredFieldChecks(t *testing.T) {
 	}
 }
 
-func TestMarshalAppend(t *testing.T) {
+func TestEncodeAppend(t *testing.T) {
 	want := []byte("prefix")
 	got := append([]byte(nil), want...)
 	got, err := proto.MarshalOptions{}.MarshalAppend(got, &test3pb.TestAllTypes{
@@ -143,3 +144,14 @@ func TestMarshalAppend(t *testing.T) {
 		t.Fatalf("MarshalAppend modified prefix: got %v, want prefix %v", got, want)
 	}
 }
+
+func TestEncodeOneofNilWrapper(t *testing.T) {
+	m := &testpb.TestAllTypes{OneofField: (*testpb.TestAllTypes_OneofUint32)(nil)}
+	b, err := proto.Marshal(m)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(b) > 0 {
+		t.Errorf("Marshal return non-empty, want empty")
+	}
+}

proto/reset_test.go

@@ -21,6 +21,7 @@ func TestReset(t *testing.T) {
 		MapStringString:        map[string]string{},
 		OptionalForeignMessage: &testpb.ForeignMessage{},
 		OneofField:             (*testpb.TestAllTypes_OneofUint32)(nil),
+		OneofOptional:          (*testpb.TestAllTypes_OneofOptionalUint32)(nil),
 	}
 	m.ProtoReflect().SetUnknown([]byte{})
 
@@ -47,6 +48,9 @@ func TestReset(t *testing.T) {
 	if m.OneofField != nil {
 		t.Errorf("m.OneofField = %p, want nil", m.OneofField)
 	}
+	if m.OneofOptional != nil {
+		t.Errorf("m.OneofOptional = %p, want nil", m.OneofOptional)
+	}
 
 	if got := m.ProtoReflect().GetUnknown(); got != nil {
 		t.Errorf("m.ProtoReflect().GetUnknown() = %d, want nil", got)