This cleanup was done in June 2020 by Dan Scales,
but never upstreamed to the open source Go Protobuf repository.
Change-Id: I175d9588b5bb536fa276133f8fe6073278e6d3b0
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/605895
Reviewed-by: Christian Höppner <hoeppi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Before this commit, message-level Go editions features were not parsed
correctly.
Change-Id: I94ead5428fadae5fd70ed991fa405ce9388e9660
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/603015
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
go_features.proto apparently lived in package google.protobuf,
but was later moved to package pb, without updating generate-protos.
This change fixes the oversight and re-generates go_features_gen.go.
Change-Id: I6fd68fdb7b146f32dd610c0bad6ecf5ec940ae78
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/602055
Reviewed-by: Lasse Folger <lassefolger@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The IsLazy() method is intentionally not added to the
protoreflect.FieldDescriptor interface because users should not need it.
The Go Protobuf runtime will use type assertion to dynamically call the method.
There is no change in behavior, this is preparation only.
Change-Id: I4500c17f5edd1bdc40447eb89bff115b4e8eab06
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/596539
Reviewed-by: Chressie Himpel <chressie@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Change-Id: I5207e3e1d51e55147a7ec8374548aeb6a5312172
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/595337
Reviewed-by: Lasse Folger <lassefolger@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Resolvesgolang/protobuf#1615
The protoc compiler disallows setting the message encoding feature of
map fields to delimited since maps, at least for now (as of edition
2023) should always use normal length-prefixed encoding.
But the field (and a message value field inside the map entry) could
inherit such a feature value if it were set as a file-wide default. At
the point where the code changes the kind from message to group, based
on the field's resolved features, the message type hasn't yet been
resolved. So this change adds a check after the FieldDescriptor's
message type is resolved, to change the kind back from group to
message if the field is a map field or a field in a map entry message.
Change-Id: I785269a4ecd80d1a17866c08b2afc0b01440e0e3
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/588976
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cassondra Foesch <cfoesch@gmail.com>
Reviewed-by: Mike Kruskal <mkruskal@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
This updates the repo to use the latest artifacts from the v27.0
final release of protoc.
Change-Id: I4216038b6f40430c3f9209c0bdd387de0b82e23f
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/588875
Reviewed-by: Florian Zenker <floriank@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When a message (within an extension) is lazily decoded, its size cache is
initialized to 0 (the zero value for an int32). This doesn’t mean the size cache
reads 0, but rather that it was not initialized.
This fixes TestExtensionGetRace being flaky since CL 580015.
related to golang/protobuf#1609
Change-Id: Ia305badadd300679975f230005c3e33c94050e4a
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/586396
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Extensions will be kept in wire format over proto.Size and proto.Marshal.
This change is a significant performance optimization for jobs that read and
write Protobuf messages of the same type, but do not need to process extensions.
This change is based on work by Patrik Nyblom.
Note that the proto.Size semantics for lazy messages might be surprising;
see https://protobuf.dev/reference/go/size/ for details.
We have been running this change for about two weeks in Google,
all known breakages have already been addressed with CL 579995.
related to golang/protobuf#1609
Change-Id: I16be78d15304d775bb30e76356a1a61d61300b43
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/580015
Reviewed-by: Lasse Folger <lassefolger@google.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This is a result of the discussion in [1]. Before editions, a group defined a multiple things:
* a type
* a field
* an encoding scheme
With editions this has changed and groups no longer exist and the different parts have to be defined individually. Most importantly, the field and the type also had the same name (usually and CamelCase name). To keep compatibility with proto2 groups, [2] introduced a concept of group-like fields and adjusted the Text/JSON parsers to accept the type name instead of the field name for such fields. This means you can convert from proto2 groups to editions without changing the semantics.
Furthermore, to avoid suprises with group-like fields (e.g. when a user by coincident specified a field that is group-like) protobuf decided that group-like fields should always accept the type and the field name for group like fields. This also allows us to eventually emit the field name rather than the type name for group like fields in the future.
This change implements this decision in Go.
[1] https://github.com/protocolbuffers/protobuf/issues/16239
[2] https://go.dev/cl/575916
Change-Id: I701c4cd228d2e0867b2a87771b6c6331459c4910
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/582755
Reviewed-by: Lasse Folger <lassefolger@google.com>
Reviewed-by: Mike Kruskal <mkruskal@google.com>
Commit-Queue: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
This change required some changes to the editions default handling code
because the descriptor.proto changed upstream [2]. The defaults are no
longer one feature set but are split into overridable and
not-overridable features which have to be merged.
I had to do bootstraping in 4 phases but the results should be correct:
1. generate everything depending on descriptor.proto
2. generate new defaults binary proto
3. adjust all code that works with defaults (*/edition.go files)
4. generate everything else
The was required because 1. is a prerequisite for 3. while 2. and 3. are
a prerequisite for 4. (2. and 3. can probably be done in parallel).
The new release also introduced new conformance tests. The go
implementation is not yet conformant and the tests will be fixed in a
follow up change because they require changes to the protojson and
protoext encoders.
[1] e5502c746e
Change-Id: Iddf248f6582a0402ab31256f6e64755d870ed82c
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/582635
Auto-Submit: Lasse Folger <lassefolger@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicolas Hillegeer <aktau@google.com>
Change-Id: I2db557669ada6e031140a09b3a92bd901220f8f3
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/580975
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
This used to not be necessary, but a subsequent change will change behavior
based on MarshalOptions.Deterministic, so it is important that we do not
accidentally drop MarshalOptions anywhere.
related to golang/protobuf#1609
Change-Id: I6b53352707d93939642a627eb41c930f8ac3157f
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/579995
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
There currently is no risk of producing invalid wire format,
but that will change with subsequent changes regarding lazy decoding.
We have been running this change in production for about a month,
without ever triggering the check (until lazy decoding is involved).
related to golang/protobuf#1609
Change-Id: I3c5c956aee2fa81f99dea03ed2a977a1547081fc
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/579595
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When I implemented this initially, I thought the parent of an extension is the
extendee. This is incorrect. The parent is the scope in which the extension is
defined. This CL changes the code to use the correct parent. This also allows
us to reduce some complexity in the implementation because we don't need to
wait until the extendee is resolved before we can resolve the features.
Change-Id: I6d7012f7502ef95457ab96f3e8abc4ab763d5bcb
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/579275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Lasse Folger <lassefolger@google.com>
These need to be converted back to the appropriate label/type enums to produce valid descriptor protos under editions.
Change-Id: Ife04c4c556ffb06d1bc477725ff49058928a75ca
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575916
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
protoc guarantees that the edition defaults will be ordered, but not contiguous. Gaps represent no changes to the defaults and should be handled.
Change-Id: I01fde5ff89b2b206b066c5a415083f6526a4ed91
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575876
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Stapelberg <stapelberg@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
This brings go into conformance with other implementations. Group-like message fields with delimited encoding will continue to use the type name for text-format, but everything else will use the field name.
Change-Id: Ib6d07f19ccfa853ce0370392c89fd24fb7148793
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575896
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Commit-Queue: Michael Stapelberg <stapelberg@google.com>
messageInfo() looks like this:
func (ms *messageState) messageInfo() *MessageInfo {
mi := ms.LoadMessageInfo()
if mi == nil {
panic("invalid nil message info; this suggests memory corruption due to a race or shallow copy on the message struct")
}
return mi
}
func (ms *messageState) LoadMessageInfo() *MessageInfo {
return (*MessageInfo)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&ms.atomicMessageInfo))))
}
Which is an atomic load and a predictable branch. On x86, this 64-bit
load is just a MOV. On other platforms, like ARM64, there's actual
atomics involved (LDAR).
Meaning, it's cheap, but not free. Eliminate redundant copies of this
(Common Subexpression Elimination).
The newly added benchmarks improve by (geomean) 2.5%:
$ benchstat pre post | head -10
goarch: amd64
cpu: AMD Ryzen Threadripper PRO 3995WX 64-Cores
│ pre │ post │
│ sec/op │ sec/op vs base │
Extension/Has/None-12 106.4n ± 2% 104.0n ± 2% -2.21% (p=0.020 n=10)
Extension/Has/Set-12 116.4n ± 1% 114.4n ± 2% -1.76% (p=0.017 n=10)
Extension/Get/None-12 184.2n ± 1% 181.0n ± 1% -1.68% (p=0.003 n=10)
Extension/Get/Set-12 144.5n ± 3% 140.7n ± 2% -2.63% (p=0.041 n=10)
Extension/Set-12 227.2n ± 2% 218.6n ± 2% -3.81% (p=0.000 n=10)
geomean 149.6n 145.9n -2.42%
I didn't test on ARM64, but the difference should be larger due to the
reduced atomics.
Change-Id: I8eebeb6f753425b743368a7f5c7be4d48537e5c3
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575036
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Commit-Queue: Nicolas Hillegeer <aktau@google.com>
Auto-Submit: Nicolas Hillegeer <aktau@google.com>
Extensions are unmarshaled lazily if protolegacy is true. The current
implementation of proto.HasExtension forces this unmarshaling to happen.
Change that.
Lazy message extensions are unmarshaled on first access, see
(*ExtensionField).Value. This leads to an (expensive) unmarshal
operation even if the user only wanted to know whether the extension is
present.
Granted, in most cases a HasExtension returning true will be followed by
a GetExtension. Due to memoization (see (*ExtensionField).lazyInit), the
cost will just shift from HasExtension to GetExtension. But, this CL
allows building cheaper functionality that only needs to know about
extension existence.
Why can this validation be removed?
- All tests pass.
- This check was added in CL 229558. The author (Joe Tsai) noted:
> Technically this shouldn't be needed, but I couldn't adequately reason
> whether a nil message value would ever be set through non-reflection
> means.
Like the author, I believe it's not needed:
- `proto.SetExtension` does not allow setting invalid messages (see
proto/extension.go).
- Likewise, (*extensionMap).Set panics when attempting to set an
invalid value.
- Unmarshaling does not produce submessages for which `IsValid` is
false.
The added test fails without the fix:
$ go test -tags=protolegacy -test.v -test.run=TestHasExtensionNoAlloc proto/extension_test.go
=== RUN TestHasExtensionNoAlloc
=== RUN TestHasExtensionNoAlloc/Nil
=== RUN TestHasExtensionNoAlloc/Eager
=== RUN TestHasExtensionNoAlloc/Lazy
extension_test.go:156: proto.HasExtension should not allocate, but allocated 3.00B per run
--- FAIL: TestHasExtensionNoAlloc (0.00s)
--- PASS: TestHasExtensionNoAlloc/Nil (0.00s)
--- PASS: TestHasExtensionNoAlloc/Eager (0.00s)
--- FAIL: TestHasExtensionNoAlloc/Lazy (0.00s)
FAIL
FAIL command-line-arguments 0.018s
The tests are disabled in race mode because the race instrumentation for
closures et al. always allocates. The protolegacy tests were previously
only run in race mode. I added a non-race variant in
integration_test.go.
Change-Id: Idbc67c1cf0aea8833a2735ca7bfc8d2466ceaf44
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/575035
Reviewed-by: Nicolas Hillegeer <aktau@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Lasse Folger <lassefolger@google.com>
Auto-Submit: Nicolas Hillegeer <aktau@google.com>
I have choosen to implement this via interace assertion so that
other implementers of protoreflect.FileDescriptor can implement
this as well.
Change-Id: Ib907895044e89bdba1009cc09129d3dd1224561f
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/573055
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change removes the remaining usages of Syntax() from Go Protobuf
and uses edition features instead.
It also adds a new function to the EnumDescriptor interface checking if
the enum is using a Closed semantics.
All of these changes were tested on the Google corpus.
Change-Id: I7a8110f6f3b6ed24bf7ece500b4942371302c56c
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/572695
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This change removes most usages of Syntax() from the repository and uses
edition features for instead. The appropriate edition feature defaults are
loaded for proto2/proto3 when the initialization of the descriptors
start.
All of these changes were tested on the Google corpus.
Change-Id: Ieca076a2b38ca8e50e084cd32e725b7b3dcb4171
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/572435
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
In internal/encoding/json, report an error when encountering a }
when we are expecting an object field value. For example, the input
`{"":}` now correctly results in an error at the closing } token.
In encoding/protojson, check for an unexpected EOF token in
skipJSONValue. This is redundant with the check in internal/encoding/json,
but adds a bit more defense against any other similar bugs that
might exist.
Fixes CVE-2024-24786
Change-Id: I03d52512acb5091c8549e31ca74541d57e56c99d
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/569356
TryBot-Bypass: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Commit-Queue: Damien Neil <dneil@google.com>
Before this change there was a data race if you initialized the
descriptor of a message in parallel to the descriptor of any extension of
that message because the extendees feature set is read during the
initialization of the extension.
Change-Id: Id896b9fbf848209fce7dae8a7f40e2d61a3b2825
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/568695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
In scope of this change, I had to fix the `IsPacked()` implementation
for field descriptors because it was not returning the correct values
for edition protos.
Change-Id: Ic1ba9d0b3552ddf16360a80336c14632f2ce6f16
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/566039
Auto-Submit: Lasse Folger <lassefolger@google.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Before this change the implementation would ignore editions options set on
extension declarations.
This change also add much more coverage for valid messages.
Change-Id: I0e7029b1430a39859cdd4a93667ac20c7e0ff20e
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/565396
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Auto-Submit: Lasse Folger <lassefolger@google.com>
