internal/proto: robustify GetProperties for a post-reflection world

Originally, the approach taken was to re-write GetProperties in terms
of protobuf reflection to partially support the use of GetProperties
on generated messages not in the open-struct API.

However, CL/167918 shows that doing so preserves a significant amount of
technical debt in the v1 repository.

In discussing this with @dneil, we decided that we should keep GetProperties
working for open-struct messages, but to instead ensure that GetProperties
does *not* work for non-open-struct messages.

Changes made:
* Document the API more, and place warnings on all exported entry points
deterring away new users.
* Always rename unrelated Go struct fields with an XXX_ prefix since this is
a common pattern that users check for.
* Deliberately fail if the struct does not look like an open-struct API.
* Minor bug fix for groups getting assigned the wrong wire type.

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

internal/proto/properties.go

@@ -5,10 +5,13 @@
 package proto
 
 import (
+	"fmt"
 	"reflect"
 	"strconv"
 	"strings"
 	"sync"
+
+	"github.com/golang/protobuf/v2/runtime/protoimpl"
 )
 
 // Constants that identify the encoding of a value on the wire.
@@ -21,43 +24,82 @@ const (
 	WireEndGroup   = 4
 )
 
-// StructProperties represents properties for all the fields of a struct.
+// StructProperties represents protocol buffer type information for a
+// generated protobuf message in the open-struct API.
+//
+// Deprecated: Do not use.
 type StructProperties struct {
-	Prop []*Properties // properties for each field
+	// Prop are the properties for each field.
+	//
+	// Fields belonging to a oneof are stored in OneofTypes instead, with a
+	// single Properties representing the parent oneof held here.
+	//
+	// The order of Prop matches the order of fields in the Go struct.
+	// Struct fields that are not related to protobufs have a "XXX_" prefix
+	// in the Properties.Name and must be ignored by the user.
+	Prop []*Properties
 
 	// OneofTypes contains information about the oneof fields in this message.
-	// It is keyed by the original name of a field.
+	// It is keyed by the protobuf field name.
 	OneofTypes map[string]*OneofProperties
 }
 
-// Properties represents the protocol-specific behavior of a single struct field.
+// Properties represents the type information for a protobuf message field.
+//
+// Deprecated: Do not use.
 type Properties struct {
-	Name     string // name of the field, for error messages
-	OrigName string // original name before protocol compiler (always set)
-	JSONName string // name to use for JSON; determined by protoc
-	Wire     string
+	// Name is a placeholder name with little meaningful semantic value.
+	// Fields with an "XXX_" prefix must be ignored.
+	Name string
+	// OrigName is the protobuf field name or oneof name.
+	OrigName string
+	// JSONName is the JSON name for the protobuf field.
+	JSONName string
+	// Enum is a placeholder name for enums.
+	// For historical reasons, this is neither the Go name for the enum,
+	// nor the protobuf name for the enum.
+	Enum string // Deprecated: Do not use.
+	// Wire is a string representation of the wire type.
+	Wire string
+	// WireType is the protobuf wire type for the field.
 	WireType int
-	Tag      int
+	// Tag is the protobuf field number.
+	Tag int
+	// Required reports whether this is a required field.
 	Required bool
+	// Optional reports whether this is a optional field.
 	Optional bool
+	// Repeated reports whether this is a repeated field.
 	Repeated bool
-	Packed   bool   // relevant for repeated primitives only
-	Enum     string // set for enum types only
-	Proto3   bool   // whether this is known to be a proto3 field
-	Oneof    bool   // whether this is a oneof field
+	// Packed reports whether this is a packed repeated field of scalars.
+	Packed bool
+	// Proto3 reports whether this field operates under the proto3 syntax.
+	Proto3 bool
+	// Oneof reports whether this field belongs within a oneof.
+	Oneof bool
 
-	Default    string // default value
-	HasDefault bool   // whether an explicit default was provided
+	// Default is the default value in string form.
+	Default string
+	// HasDefault reports whether the field has a default value.
+	HasDefault bool
 
-	MapKeyProp *Properties // set for map types only
-	MapValProp *Properties // set for map types only
+	// MapKeyProp is the properties for the key field for a map field.
+	MapKeyProp *Properties
+	// MapValProp is the properties for the value field for a map field.
+	MapValProp *Properties
 }
 
-// OneofProperties represents information about a specific field in a oneof.
+// OneofProperties represents the type information for a protobuf oneof.
+//
+// Deprecated: Do not use.
 type OneofProperties struct {
-	Type  reflect.Type // pointer to generated struct type for this oneof field
-	Field int          // struct field number of the containing oneof in the message
-	Prop  *Properties
+	// Type is a pointer to the generated wrapper type for the field value.
+	// This is nil for messages that are not in the open-struct API.
+	Type reflect.Type
+	// Field is the index into StructProperties.Prop for the containing oneof.
+	Field int
+	// Prop is the properties for the field.
+	Prop *Properties
 }
 
 // String formats the properties in the protobuf struct field tag style.
@@ -128,11 +170,12 @@ func (p *Properties) Parse(tag string) {
 		case s == "fixed64":
 			p.Wire = s
 			p.WireType = WireFixed64
-		case s == "bytes" || s == "group":
-			// NOTE: Historically, this used WireBytes even for groups,
-			// when it should have been WireStartGroup.
+		case s == "bytes":
 			p.Wire = s
 			p.WireType = WireBytes
+		case s == "group":
+			p.Wire = s
+			p.WireType = WireStartGroup
 		case s == "packed":
 			p.Packed = true
 		case s == "proto3":
@@ -150,6 +193,8 @@ func (p *Properties) Parse(tag string) {
 }
 
 // Init populates the properties from a protocol buffer struct tag.
+//
+// Deprecated: Do not use.
 func (p *Properties) Init(typ reflect.Type, name, tag string, f *reflect.StructField) {
 	p.init(typ, name, tag, f)
 }
@@ -172,8 +217,11 @@ func (p *Properties) init(typ reflect.Type, name, tag string, f *reflect.StructF
 
 var propertiesCache sync.Map // map[reflect.Type]*StructProperties
 
-// GetProperties returns the list of properties for the type represented by t.
-// t must represent a generated struct type of a protocol message.
+// GetProperties returns the list of properties for the type represented by t,
+// which must be a generated protocol buffer message in the open-struct API,
+// where protobuf message fields are represented by exported Go struct fields.
+//
+// Deprecated: Use v2 protobuf reflection instead.
 func GetProperties(t reflect.Type) *StructProperties {
 	if p, ok := propertiesCache.Load(t); ok {
 		return p.(*StructProperties)
@@ -182,26 +230,33 @@ func GetProperties(t reflect.Type) *StructProperties {
 	return p.(*StructProperties)
 }
 
-func (sp *StructProperties) Len() int           { return len(sp.Prop) }
-func (sp *StructProperties) Less(i, j int) bool { return false }
-func (sp *StructProperties) Swap(i, j int)      { return }
-
 func newProperties(t reflect.Type) *StructProperties {
 	if t.Kind() != reflect.Struct {
-		panic("proto: type must have kind struct")
+		panic(fmt.Sprintf("%v is not a generated message in the open-struct API", t))
 	}
 
+	var foundField bool
 	prop := new(StructProperties)
 
 	// Construct a list of properties for each field in the struct.
 	for i := 0; i < t.NumField(); i++ {
 		p := new(Properties)
 		f := t.Field(i)
-		p.init(f.Type, f.Name, f.Tag.Get("protobuf"), &f)
+		tagField := f.Tag.Get("protobuf")
+		p.init(f.Type, f.Name, tagField, &f)
+		foundField = foundField || p.Tag > 0
+
+		tagOneof := f.Tag.Get("protobuf_oneof")
+		if tagOneof != "" {
+			p.OrigName = tagOneof
+		}
 
-		if name := f.Tag.Get("protobuf_oneof"); name != "" {
-			p.OrigName = name
+		// Rename unrelated struct fields with the "XXX_" prefix since so much
+		// user code simply checks for this to exclude special fields.
+		if tagField == "" && tagOneof == "" && !strings.HasPrefix(p.Name, "XXX_") {
+			p.Name = "XXX_invalid_" + p.Name
 		}
+
 		prop.Prop = append(prop.Prop, p)
 	}
 
@@ -223,18 +278,38 @@ func newProperties(t reflect.Type) *StructProperties {
 			f := p.Type.Elem().Field(0)
 			p.Prop.Name = f.Name
 			p.Prop.Parse(f.Tag.Get("protobuf"))
+			foundField = foundField || p.Prop.Tag > 0
 
 			// Determine the struct field that contains this oneof.
 			// Each wrapper is assignable to exactly one parent field.
-			for i := 0; i < t.NumField(); i++ {
+			var foundOneof bool
+			for i := 0; i < t.NumField() && !foundOneof; i++ {
 				if p.Type.AssignableTo(t.Field(i).Type) {
 					p.Field = i
-					break
+					foundOneof = true
 				}
 			}
+			if !foundOneof {
+				panic(fmt.Sprintf("%v is not a generated message in the open-struct API", t))
+			}
 			prop.OneofTypes[p.Prop.OrigName] = p
 		}
 	}
 
+	// If we found no fields, it is possible that the struct uses a
+	// generated API that is not an open-struct layout.
+	if !foundField {
+		// Check with protobuf reflection to make sure this isn't
+		// an empty protobuf message.
+		mt := protoimpl.X.MessageOf(reflect.New(t).Interface()).Type()
+		if mt.Fields().Len() > 0 {
+			panic(fmt.Sprintf("%v is not a generated message in the open-struct API", t))
+		}
+	}
+
 	return prop
 }
+
+func (sp *StructProperties) Len() int           { return len(sp.Prop) }
+func (sp *StructProperties) Less(i, j int) bool { return false }
+func (sp *StructProperties) Swap(i, j int)      { return }

proto/properties.go

@@ -134,11 +134,12 @@ func (p *Properties) Parse(tag string) {
 		case s == "fixed64":
 			p.Wire = s
 			p.WireType = WireFixed64
-		case s == "bytes" || s == "group":
-			// NOTE: Historically, this used WireBytes even for groups,
-			// when it should have been WireStartGroup.
+		case s == "bytes":
 			p.Wire = s
 			p.WireType = WireBytes
+		case s == "group":
+			p.Wire = s
+			p.WireType = WireStartGroup
 		case s == "packed":
 			p.Packed = true
 		case s == "proto3":