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>
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>
In the upcoming 3.12.x release of protoc, the proto3 language will be
amended to support true presence for scalars. This CL adds support
to both the generator and runtime to support these semantics.
Newly added public API:
protogen.Plugin.SupportedFeatures
protoreflect.FieldDescriptor.HasPresence
protoreflect.FieldDescriptor.HasOptionalKeyword
protoreflect.OneofDescriptor.IsSynthetic
Change-Id: I7c86bf66d0ae56642109beb5f2132184593747ad
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/230698
Reviewed-by: Damien Neil <dneil@google.com>
Changes made:
* Ensure protoreflect.ExtensionType.IsValidInterface never panics,
especially if given a nil interface value.
* Have protoreflect.ExtensionType.IsValid{Interface,Value} only
perform type-checks. It does not do value checks (i.e., whether the
value itself is valid). Value validity is left to when an actual
protoreflect.Message.Set operation is performed.
* Add special-casing on proto.SetExtension to treat an invalid
message or list as functionally equivalent to Clear. This is to
be more consistent with the legacy SetExtension implementation
which never panicked when given such values.
* Add special-casing on proto.HasExtension to treat a mismatched
extension descriptor as simply not being present in the message.
This is also to be more consistent with the legacy HasExtension
implementation which did the same thing.
Change-Id: Idf0419abf27b9f85d9b92bd2ff8088e25b7990cc
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/229558
Reviewed-by: Damien Neil <dneil@google.com>
Two changes:
* Add RangeExtensions as a more suitable replacement for legacy
proto.ClearExtensions and proto.ExtensionDescs functions.
* Make HasExtension and GetExtension treat nil message interface
as an empty message to more consistently match legacy behavior.
Change-Id: I8eb1887a33d0737f2f80a2b80358cc296087ba3b
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/229157
Reviewed-by: Damien Neil <dneil@google.com>
Remove a stray bit of punctuation that crept into one of the license
headers and got copied around everywhere.
Change-Id: Iebe4e882650ab6dab28f132b5e324e2ab0b99a73
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/220339
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
The proto package tests often test several variations of messages with a
similar shape. For example, most tests are performed with a proto2
message with a regular field, a proto2 message with an extension field,
and a proto3 message.
Add a protobuild package which can initialize all these variations from
a single template. For example, these three messages:
&testpb.TestAllTypes{OptionalInt32: proto.Int32(1)}
&test3pb.TestAllTypes{OptionalInt32: 1}
m := &testpb.TestAllExtensions{}
proto.SetExtension(m, &testpb.E_OptionalInt32, 1)
can all be constructed from the template:
protobuild.Message{"optional_int32": 1}
This reduces redundancy in tests and will make it more practical to
test alternative code generators.
Change-Id: I3245a4bf74ee1bce957bc772fed513d427720677
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/217457
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
After taking the lock on a lazy extension's state, check to see if it
was initialized while we were waiting for the lock.
Change-Id: I1cbd52e9d655eec6c9142c97689ae36f219a28f2
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/216898
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Avoid dots and dashes in the directory to avoid issues on
build systems that cannot support them well.
Change-Id: I7ea5e6ce0b16c7158c7e53bcf5c3c1a334fe4718
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/214342
Reviewed-by: Damien Neil <dneil@google.com>
The protolegacy package is a minimal version of the v1 proto package.
This allows us to use this stub version as the dependency for
internal/testprotos/legacy packages and avoid a dependency
on the real v1 proto package.
The implementation of most v1 functionality will panic if called.
This way, we know if we the v2 code depends on one of those
unimplemented functions.
Updates golang/protobuf#962
Change-Id: I20b4091706fd456e4b01ae0931cce30a872639b0
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/199297
Reviewed-by: Damien Neil <dneil@google.com>
Descriptor methods generally return a Descriptor with no Go type
information. ExtensionType's Descriptor is an exception, returning an
ExtensionTypeDescriptor containing both the proto descriptor and a
reference back to the ExtensionType. The pure descriptor is accessed
by xt.Descriptor().Descriptor().
Rename ExtensionType's Descriptor method to TypeDescriptor to make it
clear that it behaves a bit differently.
Change 1/2: Add the TypeDescriptor method and deprecate Descriptor.
Change-Id: I1806095044d35a474d60f94d2a28bdf528f12238
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/192139
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Change protoiface.ExtensionDescV1 to implement protoreflect.ExtensionType.
ExtensionDescV1's Name field conflicts with the Descriptor Name method,
so change the protoreflect.{Message,Enum,Extension}Type types to no
longer implement the corresponding Descriptor interface. This also leads
to a clearer distinction between the two types.
Introduce a protoreflect.ExtensionTypeDescriptor type which bridges
between ExtensionType and ExtensionDescriptor.
Add extension accessor functions to the proto package:
proto.{Has,Clear,Get,Set}Extension. These functions take a
protoreflect.ExtensionType parameter, which allows writing the
same function call using either the old or new API:
proto.GetExtension(message, somepb.E_ExtensionFoo)
Fixesgolang/protobuf#908
Change-Id: Ibc65d12a46666297849114fd3aefbc4a597d9f08
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/189199
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
