runtime/protoiface: API adjustments

The following adjustments were made:
* The pragma.NoUnkeyedLiterals is moved to be the first field.
This is done to keep the options struct smaller. Even if the last
field is zero-length, Go GC implementation details forces the struct
to be padded at the end.
* Methods are documented as always treating AllowPartial as true.
* Added a support flag for UnmarshalOptions.DiscardUnknown.

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

internal/impl/codec_message.go

@@ -98,7 +98,7 @@ func (mi *MessageInfo) makeMethods(t reflect.Type, si structInfo) {
 
 	mi.needsInitCheck = needsInitCheck(mi.PBType)
 	mi.methods = piface.Methods{
-		Flags:         piface.MethodFlagDeterministicMarshal,
+		Flags:         piface.SupportMarshalDeterministic | piface.SupportUnmarshalDiscardUnknown,
 		MarshalAppend: mi.marshalAppend,
 		Unmarshal:     mi.unmarshal,
 		Size:          mi.size,

internal/impl/encode.go

@@ -47,14 +47,14 @@ func (o marshalOptions) Deterministic() bool { return o&marshalDeterministic !=
 func (o marshalOptions) UseCachedSize() bool { return o&marshalUseCachedSize != 0 }
 
 // size is protoreflect.Methods.Size.
-func (mi *MessageInfo) size(m pref.Message) (size int) {
+func (mi *MessageInfo) size(m pref.Message, opts piface.MarshalOptions) (size int) {
 	var p pointer
 	if ms, ok := m.(*messageState); ok {
 		p = ms.pointer()
 	} else {
 		p = m.(*messageReflectWrapper).pointer()
 	}
-	return mi.sizePointer(p, 0)
+	return mi.sizePointer(p, newMarshalOptions(opts))
 }
 
 func (mi *MessageInfo) sizePointer(p pointer, opts marshalOptions) (size int) {

proto/decode.go

@@ -18,6 +18,8 @@ import (
 // Example usage:
 //   err := UnmarshalOptions{DiscardUnknown: true}.Unmarshal(b, m)
 type UnmarshalOptions struct {
+	pragma.NoUnkeyedLiterals
+
 	// AllowPartial accepts input for messages that will result in missing
 	// required fields. If AllowPartial is false (the default), Unmarshal will
 	// return an error if there are any missing required fields.
@@ -31,8 +33,6 @@ type UnmarshalOptions struct {
 	Resolver interface {
 		protoregistry.ExtensionTypeResolver
 	}
-
-	pragma.NoUnkeyedLiterals
 }
 
 var _ = protoiface.UnmarshalOptions(UnmarshalOptions{})
@@ -60,7 +60,8 @@ func (o UnmarshalOptions) Unmarshal(b []byte, m Message) error {
 }
 
 func (o UnmarshalOptions) unmarshalMessage(b []byte, m protoreflect.Message) error {
-	if methods := protoMethods(m); methods != nil && methods.Unmarshal != nil {
+	if methods := protoMethods(m); methods != nil && methods.Unmarshal != nil &&
+		!(o.DiscardUnknown && methods.Flags&protoiface.SupportUnmarshalDiscardUnknown == 0) {
 		return methods.Unmarshal(b, m, protoiface.UnmarshalOptions(o))
 	}
 	return o.unmarshalMessageSlow(b, m)

proto/encode.go

@@ -19,6 +19,8 @@ import (
 // Example usage:
 //   b, err := MarshalOptions{Deterministic: true}.Marshal(m)
 type MarshalOptions struct {
+	pragma.NoUnkeyedLiterals
+
 	// AllowPartial allows messages that have missing required fields to marshal
 	// without returning an error. If AllowPartial is false (the default),
 	// Marshal will return an error if there are any missing required fields.
@@ -31,6 +33,8 @@ type MarshalOptions struct {
 	// the same message will return the same bytes, and that different
 	// processes of the same binary (which may be executing on different
 	// machines) will serialize equal messages to the same bytes.
+	// It has no effect on the resulting size of the encoded message compared
+	// to a non-deterministic marshal.
 	//
 	// Note that the deterministic serialization is NOT canonical across
 	// languages. It is not guaranteed to remain stable over time. It is
@@ -64,8 +68,6 @@ type MarshalOptions struct {
 	// There is absolutely no guarantee that Size followed by Marshal with
 	// UseCachedSize set will perform equivalently to Marshal alone.
 	UseCachedSize bool
-
-	pragma.NoUnkeyedLiterals
 }
 
 var _ = protoiface.MarshalOptions(MarshalOptions{})
@@ -83,15 +85,11 @@ func (o MarshalOptions) Marshal(m Message) ([]byte, error) {
 // MarshalAppend appends the wire-format encoding of m to b,
 // returning the result.
 func (o MarshalOptions) MarshalAppend(b []byte, m Message) ([]byte, error) {
-	// Set AllowPartial in recursive calls to marshal to avoid duplicating
-	// effort with the single initialization check below.
-	allowPartial := o.AllowPartial
-	o.AllowPartial = true
 	out, err := o.marshalMessage(b, m.ProtoReflect())
 	if err != nil {
 		return nil, err
 	}
-	if allowPartial {
+	if o.AllowPartial {
 		return out, nil
 	}
 	return out, IsInitialized(m)
@@ -99,16 +97,14 @@ func (o MarshalOptions) MarshalAppend(b []byte, m Message) ([]byte, error) {
 
 func (o MarshalOptions) marshalMessage(b []byte, m protoreflect.Message) ([]byte, error) {
 	if methods := protoMethods(m); methods != nil && methods.MarshalAppend != nil &&
-		!(o.Deterministic && methods.Flags&protoiface.MethodFlagDeterministicMarshal == 0) {
-		if methods.Size != nil {
-			sz := methods.Size(m)
-			if cap(b) < len(b)+sz {
-				x := make([]byte, len(b), len(b)+sz)
-				copy(x, b)
-				b = x
-			}
-			o.UseCachedSize = true
+		!(o.Deterministic && methods.Flags&protoiface.SupportMarshalDeterministic == 0) {
+		sz := methods.Size(m, protoiface.MarshalOptions(o))
+		if cap(b) < len(b)+sz {
+			x := make([]byte, len(b), len(b)+sz)
+			copy(x, b)
+			b = x
 		}
+		o.UseCachedSize = true
 		return methods.MarshalAppend(b, m, protoiface.MarshalOptions(o))
 	}
 	return o.marshalMessageSlow(b, m)

proto/size.go

@@ -7,6 +7,7 @@ package proto
 import (
 	"google.golang.org/protobuf/internal/encoding/wire"
 	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/runtime/protoiface"
 )
 
 // Size returns the size in bytes of the wire-format encoding of m.
@@ -21,7 +22,7 @@ func (o MarshalOptions) Size(m Message) int {
 
 func sizeMessage(m protoreflect.Message) (size int) {
 	if methods := protoMethods(m); methods != nil && methods.Size != nil {
-		return methods.Size(m)
+		return methods.Size(m, protoiface.MarshalOptions{})
 	}
 	return sizeMessageSlow(m)
 }

runtime/protoiface/methods.go

@@ -17,60 +17,65 @@ import (
 
 // Methoder is an optional interface implemented by protoreflect.Message to
 // provide fast-path implementations of various operations.
+// The returned Methods struct must not be mutated.
 type Methoder interface {
 	ProtoMethods() *Methods // may return nil
 }
 
 // Methods is a set of optional fast-path implementations of various operations.
 type Methods struct {
+	pragma.NoUnkeyedLiterals
+
 	// Flags indicate support for optional features.
-	Flags MethodFlag
+	Flags SupportFlags
+
+	// Size returns the size in bytes of the wire-format encoding of m.
+	// MarshalAppend must be provided if a custom Size is provided.
+	Size func(m protoreflect.Message, opts MarshalOptions) int
 
 	// MarshalAppend appends the wire-format encoding of m to b, returning the result.
-	// It does not perform required field checks.
+	// Size must be provided if a custom MarshalAppend is provided.
+	// It must not perform required field checks.
 	MarshalAppend func(b []byte, m protoreflect.Message, opts MarshalOptions) ([]byte, error)
 
-	// Size returns the size in bytes of the wire-format encoding of m.
-	Size func(m protoreflect.Message) int
-
-	// Unmarshal parses the wire-format message in b and places the result in m.
-	// It does not reset m or perform required field checks.
+	// Unmarshal parses the wire-format message in b and merges the result in m.
+	// It must not reset m or perform required field checks.
 	Unmarshal func(b []byte, m protoreflect.Message, opts UnmarshalOptions) error
 
 	// IsInitialized returns an error if any required fields in m are not set.
 	IsInitialized func(m protoreflect.Message) error
-
-	pragma.NoUnkeyedLiterals
 }
 
-// MethodFlag indicates support for optional fast-path features.
-type MethodFlag int64
+type SupportFlags uint64
 
 const (
-	// MethodFlagDeterministicMarshal indicates support for deterministic marshaling.
-	MethodFlagDeterministicMarshal MethodFlag = 1 << iota
+	// SupportMarshalDeterministic reports whether MarshalOptions.Deterministic is supported.
+	SupportMarshalDeterministic SupportFlags = 1 << iota
+
+	// SupportUnmarshalDiscardUnknown reports whether UnmarshalOptions.DiscardUnknown is supported.
+	SupportUnmarshalDiscardUnknown
 )
 
 // MarshalOptions configure the marshaler.
 //
 // This type is identical to the one in package proto.
 type MarshalOptions struct {
-	AllowPartial  bool
+	pragma.NoUnkeyedLiterals
+
+	AllowPartial  bool // must be treated as true by method implementations
 	Deterministic bool
 	UseCachedSize bool
-
-	pragma.NoUnkeyedLiterals
 }
 
 // UnmarshalOptions configures the unmarshaler.
 //
 // This type is identical to the one in package proto.
 type UnmarshalOptions struct {
-	AllowPartial   bool
+	pragma.NoUnkeyedLiterals
+
+	AllowPartial   bool // must be treated as true by method implementations
 	DiscardUnknown bool
 	Resolver       interface {
 		protoregistry.ExtensionTypeResolver
 	}
-
-	pragma.NoUnkeyedLiterals
 }