proto: deprecate {Unm,M}arshalMessageSet{JSON} (#741)

Despite the naming, these are "internal-only" functions that are intended to
only be called from generated code for MessageSets.
Furthermore, MessageSets are themselves a deprecated feature from proto1 days,
such that descriptor.proto even warns against their use since the initial
open-source release of protocol buffers in 2008. Within Google,
there are no direct usages of these functions, and all existing
usages of MessageSets go through the new table-driven implementation.

In addition to deprecating the {Unm,M}arshalMessageSet{JSON} top-level functions,
also modify the generator to stop emitting MarshalJSON and UnmarshalJSON methods
for messages sets. The UnmarshalJSON method is not implemented and the
MarshalJSON method does not seem to be called anywhere in Google (verified by
making the method panic and doing a global test). The jsonpb package continues
to work with MessageSets.

I should note that when the table-driven implementation was open sourced
in late 2017 (see 8cc9e46429bfb16289d40d30b2ee3f4923b47345), it accidentally
removed generation of the Marshal and Unmarshal method. However, no one seemed
to have noticed that those methods were no longer generated.

Change-Id: I5fa3ddc452ff1906a4d7c31c6e0a2902702784ca
Cherry-Pick: github.com/golang/protobuf@951a149f90371fb8858c6c979d03bb2583611052
Original-Author: Joe Tsai <joetsai@digital-static.net>
Reviewed-on: https://go-review.googlesource.com/c/151435
Reviewed-by: Damien Neil <dneil@google.com>

proto/deprecated.go

@@ -31,8 +31,33 @@
 
 package proto
 
+import "errors"
+
 // Deprecated: do not use.
 type Stats struct{ Emalloc, Dmalloc, Encode, Decode, Chit, Cmiss, Size uint64 }
 
 // Deprecated: do not use.
 func GetStats() Stats { return Stats{} }
+
+// Deprecated: do not use.
+func MarshalMessageSet(interface{}) ([]byte, error) {
+	return nil, errors.New("proto: not implemented")
+}
+
+// Deprecated: do not use.
+func UnmarshalMessageSet([]byte, interface{}) error {
+	return errors.New("proto: not implemented")
+}
+
+// Deprecated: do not use.
+func MarshalMessageSetJSON(interface{}) ([]byte, error) {
+	return nil, errors.New("proto: not implemented")
+}
+
+// Deprecated: do not use.
+func UnmarshalMessageSetJSON([]byte, interface{}) error {
+	return errors.New("proto: not implemented")
+}
+
+// Deprecated: do not use.
+func RegisterMessageSetType(Message, int32, string) {}

proto/message_set.go

@@ -36,13 +36,7 @@ package proto
  */
 
 import (
-	"bytes"
-	"encoding/json"
 	"errors"
-	"fmt"
-	"reflect"
-	"sort"
-	"sync"
 )
 
 // errNoMessageTypeID occurs when a protocol buffer does not have a message type ID.
@@ -145,46 +139,9 @@ func skipVarint(buf []byte) []byte {
 	return buf[i+1:]
 }
 
-// MarshalMessageSet encodes the extension map represented by m in the message set wire format.
-// It is called by generated Marshal methods on protocol buffer messages with the message_set_wire_format option.
-func MarshalMessageSet(exts interface{}) ([]byte, error) {
-	return marshalMessageSet(exts, false)
-}
-
-// marshaMessageSet implements above function, with the opt to turn on / off deterministic during Marshal.
-func marshalMessageSet(exts interface{}, deterministic bool) ([]byte, error) {
-	switch exts := exts.(type) {
-	case *XXX_InternalExtensions:
-		var u marshalInfo
-		siz := u.sizeMessageSet(exts)
-		b := make([]byte, 0, siz)
-		return u.appendMessageSet(b, exts, deterministic)
-
-	case map[int32]Extension:
-		// This is an old-style extension map.
-		// Wrap it in a new-style XXX_InternalExtensions.
-		ie := XXX_InternalExtensions{
-			p: &struct {
-				mu           sync.Mutex
-				extensionMap map[int32]Extension
-			}{
-				extensionMap: exts,
-			},
-		}
-
-		var u marshalInfo
-		siz := u.sizeMessageSet(&ie)
-		b := make([]byte, 0, siz)
-		return u.appendMessageSet(b, &ie, deterministic)
-
-	default:
-		return nil, errors.New("proto: not an extension map")
-	}
-}
-
-// UnmarshalMessageSet decodes the extension map encoded in buf in the message set wire format.
+// unmarshalMessageSet decodes the extension map encoded in buf in the message set wire format.
 // It is called by Unmarshal methods on protocol buffer messages with the message_set_wire_format option.
-func UnmarshalMessageSet(buf []byte, exts interface{}) error {
+func unmarshalMessageSet(buf []byte, exts interface{}) error {
 	var m map[int32]Extension
 	switch exts := exts.(type) {
 	case *XXX_InternalExtensions:
@@ -222,93 +179,3 @@ func UnmarshalMessageSet(buf []byte, exts interface{}) error {
 	}
 	return nil
 }
-
-// MarshalMessageSetJSON encodes the extension map represented by m in JSON format.
-// It is called by generated MarshalJSON methods on protocol buffer messages with the message_set_wire_format option.
-func MarshalMessageSetJSON(exts interface{}) ([]byte, error) {
-	var m map[int32]Extension
-	switch exts := exts.(type) {
-	case *XXX_InternalExtensions:
-		var mu sync.Locker
-		m, mu = exts.extensionsRead()
-		if m != nil {
-			// Keep the extensions map locked until we're done marshaling to prevent
-			// races between marshaling and unmarshaling the lazily-{en,de}coded
-			// values.
-			mu.Lock()
-			defer mu.Unlock()
-		}
-	case map[int32]Extension:
-		m = exts
-	default:
-		return nil, errors.New("proto: not an extension map")
-	}
-	var b bytes.Buffer
-	b.WriteByte('{')
-
-	// Process the map in key order for deterministic output.
-	ids := make([]int32, 0, len(m))
-	for id := range m {
-		ids = append(ids, id)
-	}
-	sort.Sort(int32Slice(ids)) // int32Slice defined in text.go
-
-	for i, id := range ids {
-		ext := m[id]
-		msd, ok := messageSetMap[id]
-		if !ok {
-			// Unknown type; we can't render it, so skip it.
-			continue
-		}
-
-		if i > 0 && b.Len() > 1 {
-			b.WriteByte(',')
-		}
-
-		fmt.Fprintf(&b, `"[%s]":`, msd.name)
-
-		x := ext.value
-		if x == nil {
-			x = reflect.New(msd.t.Elem()).Interface()
-			if err := Unmarshal(ext.enc, x.(Message)); err != nil {
-				return nil, err
-			}
-		}
-		d, err := json.Marshal(x)
-		if err != nil {
-			return nil, err
-		}
-		b.Write(d)
-	}
-	b.WriteByte('}')
-	return b.Bytes(), nil
-}
-
-// UnmarshalMessageSetJSON decodes the extension map encoded in buf in JSON format.
-// It is called by generated UnmarshalJSON methods on protocol buffer messages with the message_set_wire_format option.
-func UnmarshalMessageSetJSON(buf []byte, exts interface{}) error {
-	// Common-case fast path.
-	if len(buf) == 0 || bytes.Equal(buf, []byte("{}")) {
-		return nil
-	}
-
-	// This is fairly tricky, and it's not clear that it is needed.
-	return errors.New("TODO: UnmarshalMessageSetJSON not yet implemented")
-}
-
-// A global registry of types that can be used in a MessageSet.
-
-var messageSetMap = make(map[int32]messageSetDesc)
-
-type messageSetDesc struct {
-	t    reflect.Type // pointer to struct
-	name string
-}
-
-// RegisterMessageSetType is called from the generated code.
-func RegisterMessageSetType(m Message, fieldNum int32, name string) {
-	messageSetMap[fieldNum] = messageSetDesc{
-		t:    reflect.TypeOf(m),
-		name: name,
-	}
-}

proto/message_set_test.go

@@ -29,49 +29,60 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-package proto
+package proto_test
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
+
+	"github.com/golang/protobuf/proto"
+	. "github.com/golang/protobuf/proto/test_proto"
 )
 
 func TestUnmarshalMessageSetWithDuplicate(t *testing.T) {
-	// Check that a repeated message set entry will be concatenated.
-	in := &messageSet{
-		Item: []*_MessageSet_Item{
-			{TypeId: Int32(12345), Message: []byte("hoo")},
-			{TypeId: Int32(12345), Message: []byte("hah")},
-		},
-	}
-	b, err := Marshal(in)
-	if err != nil {
-		t.Fatalf("Marshal: %v", err)
-	}
-	t.Logf("Marshaled bytes: %q", b)
+	/*
+		Message{
+			Tag{1, StartGroup},
+			Message{
+				Tag{2, Varint}, Uvarint(12345),
+				Tag{3, Bytes}, Bytes("hoo"),
+			},
+			Tag{1, EndGroup},
+			Tag{1, StartGroup},
+			Message{
+				Tag{2, Varint}, Uvarint(12345),
+				Tag{3, Bytes}, Bytes("hah"),
+			},
+			Tag{1, EndGroup},
+		}
+	*/
+	var in []byte
+	fmt.Sscanf("0b10b9601a03686f6f0c0b10b9601a036861680c", "%x", &in)
 
-	var extensions XXX_InternalExtensions
-	if err := UnmarshalMessageSet(b, &extensions); err != nil {
-		t.Fatalf("UnmarshalMessageSet: %v", err)
-	}
-	ext, ok := extensions.p.extensionMap[12345]
-	if !ok {
-		t.Fatalf("Didn't retrieve extension 12345; map is %v", extensions.p.extensionMap)
-	}
-	// Skip wire type/field number and length varints.
-	got := skipVarint(skipVarint(ext.enc))
-	if want := []byte("hoohah"); !bytes.Equal(got, want) {
-		t.Errorf("Combined extension is %q, want %q", got, want)
-	}
-}
+	/*
+		Message{
+			Tag{1, StartGroup},
+			Message{
+				Tag{2, Varint}, Uvarint(12345),
+				Tag{3, Bytes}, Bytes("hoohah"),
+			},
+			Tag{1, EndGroup},
+		}
+	*/
+	var want []byte
+	fmt.Sscanf("0b10b9601a06686f6f6861680c", "%x", &want)
 
-func TestMarshalMessageSetJSON_UnknownType(t *testing.T) {
-	extMap := map[int32]Extension{12345: Extension{}}
-	got, err := MarshalMessageSetJSON(extMap)
+	var m MyMessageSet
+	if err := proto.Unmarshal(in, &m); err != nil {
+		t.Fatalf("unexpected Unmarshal error: %v", err)
+	}
+	got, err := proto.Marshal(&m)
 	if err != nil {
-		t.Fatalf("MarshalMessageSetJSON: %v", err)
+		t.Fatalf("unexpected Marshal error: %v", err)
 	}
-	if want := []byte("{}"); !bytes.Equal(got, want) {
-		t.Errorf("MarshalMessageSetJSON(%v) = %q, want %q", extMap, got, want)
+
+	if !bytes.Equal(got, want) {
+		t.Errorf("output mismatch:\ngot  %x\nwant %x", got, want)
 	}
 }

proto/table_unmarshal.go

@@ -136,7 +136,7 @@ func (u *unmarshalInfo) unmarshal(m pointer, b []byte) error {
 		u.computeUnmarshalInfo()
 	}
 	if u.isMessageSet {
-		return UnmarshalMessageSet(b, m.offset(u.extensions).toExtensions())
+		return unmarshalMessageSet(b, m.offset(u.extensions).toExtensions())
 	}
 	var reqMask uint64 // bitmask of required fields we've seen.
 	var errLater error

protoc-gen-go/generator/generator.go

@@ -2409,17 +2409,6 @@ func (g *Generator) generateCommonMethods(mc *msgCtx) {
 
 	// Extension support methods
 	if len(mc.message.ExtensionRange) > 0 {
-		// message_set_wire_format only makes sense when extensions are defined.
-		if opts := mc.message.Options; opts != nil && opts.GetMessageSetWireFormat() {
-			g.P()
-			g.P("func (m *", mc.goName, ") MarshalJSON() ([]byte, error) {")
-			g.P("return ", g.Pkg["proto"], ".MarshalMessageSetJSON(&m.XXX_InternalExtensions)")
-			g.P("}")
-			g.P("func (m *", mc.goName, ") UnmarshalJSON(buf []byte) error {")
-			g.P("return ", g.Pkg["proto"], ".UnmarshalMessageSetJSON(buf, &m.XXX_InternalExtensions)")
-			g.P("}")
-		}
-
 		g.P()
 		g.P("var extRange_", mc.goName, " = []", g.Pkg["proto"], ".ExtensionRange{")
 		for _, r := range mc.message.ExtensionRange {
@@ -2825,10 +2814,8 @@ func (g *Generator) generateExtension(ext *ExtensionDescriptor) {
 	// In addition, the situation for when to apply this special case is implemented
 	// differently in other languages:
 	// https://github.com/google/protobuf/blob/aff10976/src/google/protobuf/text_format.cc#L1560
-	mset := false
 	if extDesc.GetOptions().GetMessageSetWireFormat() && typeName[len(typeName)-1] == "message_set_extension" {
 		typeName = typeName[:len(typeName)-1]
-		mset = true
 	}
 
 	// For text formatting, the package must be exactly what the .proto file declares,
@@ -2850,10 +2837,6 @@ func (g *Generator) generateExtension(ext *ExtensionDescriptor) {
 	g.P()
 
 	g.addInitf("%s.RegisterExtension(%s)", g.Pkg["proto"], ext.DescName())
-	if mset {
-		// Generate a bit more code to register with message_set.go.
-		g.addInitf("%s.RegisterMessageSetType((%s)(nil), %d, %q)", g.Pkg["proto"], fieldType, *field.Number, extName)
-	}
 
 	g.file.addExport(ext, constOrVarSymbol{ccTypeName, "var", ""})
 }