internal/impl: use XXX_unrecognized exclusively for unknown fields

The protobuf data model makes no distinction between unknown fields
that are within the extension field ranges or not. Now that we eagerly
unmarshal extensions, there is even less need for storing unknown
fields in the extension map. Instead, use the XXX_unrecognized field
exclusively for this purpose.

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

internal/impl/legacy_extension.go

@@ -183,12 +183,7 @@ func (p legacyExtensionTypes) Remove(t pref.ExtensionType) {
 	if x.Value != nil {
 		panic("value for extension descriptor still populated")
 	}
-	x.Desc = nil
-	if len(x.Raw) == 0 {
-		p.x.Clear(t.Number())
-	} else {
-		p.x.Set(t.Number(), x)
-	}
+	p.x.Clear(t.Number())
 }
 
 func (p legacyExtensionTypes) ByNumber(n pref.FieldNumber) pref.ExtensionType {
@@ -269,9 +264,6 @@ type ExtensionFieldV1 struct {
 	// The Value may only be populated if Desc is also populated.
 	Value interface{} // TODO: switch to protoreflect.Value
 
-	// Raw is the raw encoded bytes for the extension field.
-	// It is possible for Raw to be populated irrespective of whether the
-	// other fields are populated.
 	Raw []byte // TODO: remove; let this be handled by XXX_unrecognized
 }
 

internal/impl/legacy_test.go

@@ -43,6 +43,7 @@ func TestLegacyUnknown(t *testing.T) {
 	rawOf := func(toks ...pack.Token) pref.RawFields {
 		return pref.RawFields(pack.Message(toks).Marshal())
 	}
+	raw1 := rawOf(pack.Tag{1, pack.BytesType}, pack.Bytes("1"))                      // 0a0131
 	raw1a := rawOf(pack.Tag{1, pack.VarintType}, pack.Svarint(-4321))                // 08c143
 	raw1b := rawOf(pack.Tag{1, pack.Fixed32Type}, pack.Uint32(0xdeadbeef))           // 0defbeadde
 	raw1c := rawOf(pack.Tag{1, pack.Fixed64Type}, pack.Float64(math.Pi))             // 09182d4454fb210940
@@ -51,17 +52,6 @@ func TestLegacyUnknown(t *testing.T) {
 	raw3a := rawOf(pack.Tag{3, pack.StartGroupType}, pack.Tag{3, pack.EndGroupType}) // 1b1c
 	raw3b := rawOf(pack.Tag{3, pack.BytesType}, pack.Bytes("\xde\xad\xbe\xef"))      // 1a04deadbeef
 
-	raw1 := rawOf(pack.Tag{1, pack.BytesType}, pack.Bytes("1"))    // 0a0131
-	raw3 := rawOf(pack.Tag{3, pack.BytesType}, pack.Bytes("3"))    // 1a0133
-	raw10 := rawOf(pack.Tag{10, pack.BytesType}, pack.Bytes("10")) // 52023130 - extension
-	raw15 := rawOf(pack.Tag{15, pack.BytesType}, pack.Bytes("15")) // 7a023135 - extension
-	raw26 := rawOf(pack.Tag{26, pack.BytesType}, pack.Bytes("26")) // d201023236
-	raw32 := rawOf(pack.Tag{32, pack.BytesType}, pack.Bytes("32")) // 8202023332
-	raw45 := rawOf(pack.Tag{45, pack.BytesType}, pack.Bytes("45")) // ea02023435 - extension
-	raw46 := rawOf(pack.Tag{45, pack.BytesType}, pack.Bytes("46")) // ea02023436 - extension
-	raw47 := rawOf(pack.Tag{45, pack.BytesType}, pack.Bytes("47")) // ea02023437 - extension
-	raw99 := rawOf(pack.Tag{99, pack.BytesType}, pack.Bytes("99")) // 9a06023939
-
 	joinRaw := func(bs ...pref.RawFields) (out []byte) {
 		for _, b := range bs {
 			out = append(out, b...)
@@ -177,93 +167,6 @@ func TestLegacyUnknown(t *testing.T) {
 	if got, want := m.XXX_unrecognized, joinRaw(raw1); !bytes.Equal(got, want) {
 		t.Errorf("data mismatch:\ngot:  %x\nwant: %x", got, want)
 	}
-
-	fs.Set(45, raw45)
-	fs.Set(10, raw10) // extension
-	fs.Set(32, raw32)
-	fs.Set(1, nil) // deletion
-	fs.Set(26, raw26)
-	fs.Set(47, raw47) // extension
-	fs.Set(46, raw46) // extension
-	if got, want := fs.Len(), 6; got != want {
-		t.Errorf("Len() = %d, want %d", got, want)
-	}
-	if got, want := m.XXX_unrecognized, joinRaw(raw32, raw26); !bytes.Equal(got, want) {
-		t.Errorf("data mismatch:\ngot:  %x\nwant: %x", got, want)
-	}
-
-	// Verify iteration order.
-	i = 0
-	want = []struct {
-		num pref.FieldNumber
-		raw pref.RawFields
-	}{
-		{32, raw32},
-		{26, raw26},
-		{10, raw10}, // extension
-		{45, raw45}, // extension
-		{46, raw46}, // extension
-		{47, raw47}, // extension
-	}
-	fs.Range(func(num pref.FieldNumber, raw pref.RawFields) bool {
-		if i < len(want) {
-			if num != want[i].num || !bytes.Equal(raw, want[i].raw) {
-				t.Errorf("Range(%d) = (%d, %x), want (%d, %x)", i, num, raw, want[i].num, want[i].raw)
-			}
-		} else {
-			t.Errorf("unexpected Range iteration: %d", i)
-		}
-		i++
-		return true
-	})
-
-	// Perform partial deletion while iterating.
-	i = 0
-	fs.Range(func(num pref.FieldNumber, raw pref.RawFields) bool {
-		if i%2 == 0 {
-			fs.Set(num, nil)
-		}
-		i++
-		return true
-	})
-
-	if got, want := fs.Len(), 3; got != want {
-		t.Errorf("Len() = %d, want %d", got, want)
-	}
-	if got, want := m.XXX_unrecognized, joinRaw(raw26); !bytes.Equal(got, want) {
-		t.Errorf("data mismatch:\ngot:  %x\nwant: %x", got, want)
-	}
-
-	fs.Set(15, raw15) // extension
-	fs.Set(3, raw3)
-	fs.Set(99, raw99)
-	if got, want := fs.Len(), 6; got != want {
-		t.Errorf("Len() = %d, want %d", got, want)
-	}
-	if got, want := m.XXX_unrecognized, joinRaw(raw26, raw3, raw99); !bytes.Equal(got, want) {
-		t.Errorf("data mismatch:\ngot:  %x\nwant: %x", got, want)
-	}
-
-	// Perform partial iteration.
-	i = 0
-	want = []struct {
-		num pref.FieldNumber
-		raw pref.RawFields
-	}{
-		{26, raw26},
-		{3, raw3},
-	}
-	fs.Range(func(num pref.FieldNumber, raw pref.RawFields) bool {
-		if i < len(want) {
-			if num != want[i].num || !bytes.Equal(raw, want[i].raw) {
-				t.Errorf("Range(%d) = (%d, %x), want (%d, %x)", i, num, raw, want[i].num, want[i].raw)
-			}
-		} else {
-			t.Errorf("unexpected Range iteration: %d", i)
-		}
-		i++
-		return i < 2
-	})
 }
 
 func mustMakeExtensionType(x *ptype.StandaloneExtension, v interface{}) pref.ExtensionType {

internal/impl/legacy_unknown.go

@@ -7,7 +7,6 @@ package impl
 import (
 	"container/list"
 	"reflect"
-	"sort"
 
 	"github.com/golang/protobuf/v2/internal/encoding/wire"
 	pref "github.com/golang/protobuf/v2/reflect/protoreflect"
@@ -21,98 +20,13 @@ func makeLegacyUnknownFieldsFunc(t reflect.Type) func(p *messageDataType) pref.U
 		return nil
 	}
 	fieldOffset := offsetOf(fu)
-	unkFunc := func(p *messageDataType) pref.UnknownFields {
+	return func(p *messageDataType) pref.UnknownFields {
 		if p.p.IsNil() {
 			return emptyUnknownFields{}
 		}
 		rv := p.p.Apply(fieldOffset).AsValueOf(bytesType)
 		return (*legacyUnknownBytes)(rv.Interface().(*[]byte))
 	}
-	extFunc := makeLegacyExtensionMapFunc(t)
-	if extFunc != nil {
-		return func(p *messageDataType) pref.UnknownFields {
-			if p.p.IsNil() {
-				return emptyUnknownFields{}
-			}
-			return &legacyUnknownBytesAndExtensionMap{
-				unkFunc(p), extFunc(p), p.mi.PBType.ExtensionRanges(),
-			}
-		}
-	}
-	return unkFunc
-}
-
-// legacyUnknownBytesAndExtensionMap is a wrapper around both XXX_unrecognized
-// and also the extension field map.
-type legacyUnknownBytesAndExtensionMap struct {
-	u pref.UnknownFields
-	x *legacyExtensionMap
-	r pref.FieldRanges
-}
-
-func (fs *legacyUnknownBytesAndExtensionMap) Len() int {
-	n := fs.u.Len()
-	fs.x.Range(func(_ pref.FieldNumber, x ExtensionFieldV1) bool {
-		if len(x.Raw) > 0 {
-			n++
-		}
-		return true
-	})
-	return n
-}
-
-func (fs *legacyUnknownBytesAndExtensionMap) Get(num pref.FieldNumber) (raw pref.RawFields) {
-	if fs.r.Has(num) {
-		return fs.x.Get(num).Raw
-	}
-	return fs.u.Get(num)
-}
-
-func (fs *legacyUnknownBytesAndExtensionMap) Set(num pref.FieldNumber, raw pref.RawFields) {
-	if fs.r.Has(num) {
-		x := fs.x.Get(num)
-		x.Raw = raw
-		fs.x.Set(num, x)
-		return
-	}
-	fs.u.Set(num, raw)
-}
-
-func (fs *legacyUnknownBytesAndExtensionMap) Range(f func(pref.FieldNumber, pref.RawFields) bool) {
-	// Range over unknown fields not in the extension range.
-	// Create a closure around f to capture whether iteration terminated early.
-	var stop bool
-	fs.u.Range(func(n pref.FieldNumber, b pref.RawFields) bool {
-		stop = stop || !f(n, b)
-		return !stop
-	})
-	if stop {
-		return
-	}
-
-	// Range over unknown fields in the extension range in ascending order
-	// to ensure protoreflect.UnknownFields.Range remains deterministic.
-	type entry struct {
-		num pref.FieldNumber
-		raw pref.RawFields
-	}
-	var xs []entry
-	fs.x.Range(func(n pref.FieldNumber, x ExtensionFieldV1) bool {
-		if len(x.Raw) > 0 {
-			xs = append(xs, entry{n, x.Raw})
-		}
-		return true
-	})
-	sort.Slice(xs, func(i, j int) bool { return xs[i].num < xs[j].num })
-	for _, x := range xs {
-		if !f(x.num, x.raw) {
-			return
-		}
-	}
-}
-
-func (fs *legacyUnknownBytesAndExtensionMap) IsSupported() bool {
-	return true
 }
 
 // legacyUnknownBytes is a wrapper around XXX_unrecognized that implements