This CL migrates messages, enums, and primitive types all onto the same blanket
implementation of the `ProxiedInMapValue` trait. This gets us to the point
where messages and enums no longer need to generate any significant amount of
extra code just in case they might be used as a map value.
There are a few big pieces to this:
- I generalized the message-specific FFI endpoints in `rust/cpp_kernel/map.cc`
to be able to additionally handle enums and primitive types as values. This
mostly consisted of replacing `MessageLite*` parameters with a new `MapValue`
tagged union.
- On the Rust side, I added a new blanket implementation of
`ProxiedInMapValue` in rust/cpp.rs. It relies on its value type to implement
a new `CppMapTypeConversions` trait so that it can convert to and from the
`MapValue` tagged union used for FFI.
- In the Rust generated code, I deleted the generated `ProxiedInMapValue`
implementations for messages and enums and replaced them with
implementations of the `CppMapTypeConversions` trait.
PiperOrigin-RevId: 687355817
We generate these constants to enable map operations, but this is no longer
necessary now that we can get the relevant size and alignment information for
each message through its vtable.
PiperOrigin-RevId: 680712939
We have been relying on a per-message generated `placement_new` function for
implementing map insertion, but this CL simplifies things by removing that.
Instead, we do a reflective swap if possible, or else fall back on a copy.
This will probably make insertions a bit slower, but I think it may be worth it
because it should make it much simpler to have a blanket implementation for
ProxedInMapValue that works for all map types.
It looks like it should be possible to make this faster in the future by
implementing a bitwise move that will work for any message.
PiperOrigin-RevId: 676495920
`UntypedMapIterator::next_unchecked` currently expects to be passed a pointer
to a C++ function that it can use to dereference an iterator. However, this is
awkward because it's not natural for this C++ function to have the same
signature for every map type. Maps with a message as value need a
`MapNodeSizeInfo`, but other map types do not. We are working around this by
sometimes passing an ignored placeholder constant, but this is messy.
This CL replaces the `extern "C"` functions with closures. This way, we can
capture the `MapNodeSizeInfo` in the closure in cases where we need it, but
otherwise we no longer need to pass around placeholder values.
(Note: `MapNodeSizeInfo` is going away soon, but this is still relevant because
it will likely need to be replaced by a message default instance pointer.)
PiperOrigin-RevId: 676438640
Putting it into BUILD files unintentionally forces it on all our downstream users. Instead, we just want to enable this during testing and let them choose for themselves in their builds.
Note, that this expands the scope of -Werror to our entire repo for CI, so a bunch of fixes and opt-outs had to be applied to get this change passing.
Closed#14714
PiperOrigin-RevId: 666903224
This change adds delete, clear, serialize, parse, copy_from, and merge_from
operations to the runtime. Since these operations can all be implemented easily
on the `MessageLite` interface, we can use a common implementation in the
runtime instead of generating per-message thunks for all of these.
I suspect this will also make it possible to remove some of our generated trait
implementations and replace them with blanket implementations, but I will leave
that for a future change.
PiperOrigin-RevId: 665910927
With the C++ kernel for Rust, we currently need to generate quite a few C++
thunks for operations on map fields. For each message we generate, we generate
these thunks for all possible map types that could have that message as a
value. These operations are for things such as insertion, removal, clearing,
iterating, etc.
The reason we do this is that templated types don't play well with FFI, so we
effectively need separate FFI endpoints for every possible combination of key
and value types used (or even potentially used) as a map field.
This CL fixes the problem by replacing the generated thunks with functions in
the runtime that can operate on `proto2::MessageLite*` without needing to care
about the specific message type.
The way it works is that we implement the operations using either
`UntypedMapBase` (the base class of all map types, which knows nothing about
the key and value types) or `KeyMapBase`, which knows the key type but not the
value type. I roughly followed the example of the table-driven parser, which
has a similar problem of needing to operate generically on maps without having
access to the concrete types.
I removed 54 thunks per message (that's 6 key types times 9 operations per
key), but had to add two new thunks per message:
- The `size_info` thunk looks up the `MapNodeSizeInfoT`, which is stored in a
small constant table. The important thing here is an offset indicating where
to look for the value in each map entry. This offset can be different for
every pair of key and value types, but we can safely assume that the result
does not depend on the signedness of the key. As a result we only need to
store four entries per message: one each for i32, i64, bool, and string.
- The `placement_new` thunk move-constructs a message in place. We need this
to be able to efficiently implement map insertion.
There are two big things that this CL does not address yet but which I plan to
follow up on:
- Enums still generate many map-related C++ thunks that could be replaced with
a common implementation. This should actually be much easier to handle than
messages, because every enum has the same representation as an i32.
- We still generate six `ProxiedInMapValue` implementations for every message,
but it should be possible to replace these with a blanket implementation that
works for all message types.
PiperOrigin-RevId: 657681421
By changing the return type to `auto`, we can handle `std::string` and other
types in a single definition without needing a separate overload.
PiperOrigin-RevId: 653272253
Our ASAN test runs have not had the heap checker enabled, so this has allowed a
few memory leaks to slip in. This CL fixes all of them so that we can turn on
the heap checker.
The first one takes place whenever we add an entry into a string-valued map
using the C++ kernel. The problem is that `InnerProtoString::into_raw()` gives
up ownership of the raw `std::string` pointer it holds, but then we never
delete that pointer. This CL fixes the problem by deleting the pointer in C++
right after we perform the map insertion. To simplify things, I created a
`MakeCleanup()` helper function that we always call in our map insertion
thunks, but it's a no-op in the cases where we don't need to free anything.
There were a couple similar memory leaks related to repeated field accessors in
the C++ kernel, and those were simple to fix just by adding the necessary
`delete` call.
Finally, there were two benign memory leaks in the upb kernel involving global
variables used for empty repeated fields and maps. It turned out that we did
not need to use `Box` at all here, so removing that simplified things and fixed
the leaks.
PiperOrigin-RevId: 652947042
* The public Repeated::{push, set} and Map::insert methods now accept any value that implements IntoProxied<T>, allowing us to move owned values instead of copying them.
* This change also updates the FFI layer for strings/bytes in the repeated and maps thunks to accept a std::string* that can be moved rather than a PtrAndLen type that needs to be copied.
* Tests are updated to no longer .as_view() when setting a message / string on a repeated / map field. The IntoProxied trait makes calling .as_view() obsolete.
PiperOrigin-RevId: 650580788
This will mean that calling DebugString on a MessageLite* which is actually a full Message will get the debug info instead of the minimal output.
PiperOrigin-RevId: 649103508
Besides unnecessary inconsistency on our C symbols, double underscores anywhere in the name are reserved for stdlib use. In practice its unlikely these symbols would ever hit a collision problem (maybe the prior name 'utf8_debug_string' with no prefix as having some risk), but safer to just standardize on this and have no concerns going forward.
PiperOrigin-RevId: 648709299
This change adds a cfg attribute 'cpp_lite' to the C++ kernel of Protobuf Rust. If C++ lite is selected on the command line, the cfg attribute 'cpp_lite' is set. The root cause of the test failure was that the Debug implementation for full C++ protos uses text proto which is not available in C++ lite. The fix uses the 'cpp_lite' cfg attribute to select a different Debug implementation that doesn't rely on text proto
PiperOrigin-RevId: 640552701
This change implements a custom Debug for messages, views and muts in the C++ kernel. Debug defers to proto2::Utf8Format.
It implements this only for the C++ kernel. We will need to pull in additional dependencies beyond minitables to implement it for UPB as well. This will be done at a later point.
PiperOrigin-RevId: 613191236
It now uses the same prefix as other thunks needed for the proxied type,
so the RawMapThunk helper can be used for enums.
Calling it a "iter next" thunk is misleading.
It does not increment the iterator as "next" implies,
it only gets the current key/value the iterator points to.
PiperOrigin-RevId: 609527442
We recently updated the codebase to comply with the Bazel layering check, which
essentially requires any C++ header inclusion to be matched with a build
dependency on a target providing that header.
As part of that, I removed a handful of dependencies from the `//:protobuf`
target, since these dependencies were not set up in a way that respected the
layering check. However, I realized that this may cause a number of breakages,
especially since we did not provide the correct public targets until very
recently.
This change effectively adds back in the missing dependencies, so that projects
which do not yet adhere to the layering check can continue to depend on them
indirectly. This way, we still adhere to the layering check and make it
possible for projects that depend on us to do so, but in most cases we won't
immediately break anyone.
PiperOrigin-RevId: 607021111
We now support fields with bytes as map values e.g. map<i32, bytes>. The implementation for the C++ runtime was straightforward. The majority of the changes in this CL are about the UPB runtime. In UPB, when we insert Rust bytes/string into the map we need to first copy the bytes onto the maps arena. To support this I have rewritten the macro that implements the ProxiedInMapValue types. I refactored the functionality to convert between UPB and Rust types into the 'UpbTypeConversions' trait. This trait has a function 'to_message_value_if_required' which does the copying for bytes and strings.
PiperOrigin-RevId: 599118416
- ProxiedInMapValue is defined in maps.rs, and no longer in the runtime files {upb, cpp}.rs.
- ProxiedInMapValue's methods accept and return Proxied types.
- InnerMapMut no longer has any generic type parameters.
- Through this refactoring the Map type is no longer a ZST. Creating a new map is now as simple as `Map::new()`.
PiperOrigin-RevId: 597765165
This check enforces that each C++ build target has the correct dependencies for
all headers that it includes. We have many targets that were not correct with
respect to this check, so I fixed them up.
I also cleaned up the C++ targets related to the well-known types. I created a
cc_proto_library() target for each one and removed the :wkt_cc_protos target,
since this was necessary to satisfy the layering check. I deleted the
//src/google/protobuf:protobuf_nowkt target and deprecated :protobuf_nowkt,
because the distinction between the :protobuf and :protobuf_nowkt targets was
not really correct. Neither one exposed the headers for the well-known types in
a way that was valid with respect to the layering check, and the idea of
bundling all the well-known types together is not idiomatic in Bazel anyway.
This is a breaking change, because the //:protobuf target no longer bundles the
well-known types. From now on they should be accessed through the new
//:*_cc_proto aliases in our top-level package.
I renamed the :port_def target to :port, which simplifies things a bit by
matching our internal name.
The original motivation for this change was that to move utf8_range onto our CI
infrastructure, we needed to make its dependency rules_fuzzing compatible with
Bazel 6. The rules_fuzzing project builds with the layering check, and I found
that the process of upgrading it to Bazel 6 made it take a dependency on
protobuf, which caused it to break due to layering violations. I was able to
work around this, but it would still be nice to comply with the layering check
so that we don't have to worry about this kind of thing in the future.
PiperOrigin-RevId: 595516736
To satisfy the layering check, we need to depend on :gtest for the headers, in
addition to :gtest_main which provides the main() function.
There are a bunch of formatting changes as a side effect of this, but they
should be harmless.
PiperOrigin-RevId: 594318263
This change implements maps with keys and values of type string e.g. Map<ProtoStr, i32> and Map<ProtoStr, ProtoStr>.
Implementing the Map type for ProtoStr has been different from scalar types because ProtoStr is an unsized type i.e. its size is not known at compile time. The existing Map implementation assumed sized types in many places. To make unsized types fit into the existing code architecture I have added an associated type 'Value' to the MapWith*KeyOps traits. The associated type needs to be sized and is the type returned by the Map::get(self, key) method e.g. for aProtoStr, the `type Value = &ProtoStr`.
PiperOrigin-RevId: 588783751
This CL implements Maps for scalar types for the C++ runtime. It's orthogonal to cl/580453646. This CL is constrained by having to force template instantiation of proto2::Map<K, V>. Put differently, a Rust protobuf::Map<K, V> implementation needs to call 'extern "C"' functions with both key and value type in the function name (e.g. __pb_rust_Map_i32_f64_get()). We use macros to generate a Map implementation for every (K,V)-pair. An alternative would have been to use vtables.
Luckily a key in a protobuf map can only be integer types, bool and string. So the number of key types is bounded by the specification, while the number of value types is not i.e. any protobuf message can be a value in a map. Given these constraints we introduce one 'MapKeyOps' trait per key type e.g. MapKeyBOOLOps or MapKeyI32Ops. These traits need to be implemented for every value type e.g. 'impl MapKeyBOOLOps for i32' will implement 'Map::<bool, i32>'. In particular the MapKeyOps traits can also be implemented for generated messages without violating the orphan rule.
This CL also contains significant changes to the UPB runtime so that both upb.rs and cpp.rs export a similar interface to simplify the implementation in map.rs and the generated code.
This CL does not yet implement the Proxied trait.
PiperOrigin-RevId: 582951914
This change moves almost everything in the `upb/` directory up one level, so
that for example `upb/upb/generated_code_support.h` becomes just
`upb/generated_code_support.h`. The only exceptions I made to this were that I
left `upb/cmake` and `upb/BUILD` where they are, mostly because that avoids
conflict with other files and the current locations seem reasonable for now.
The `python/` directory is a little bit of a challenge because we had to merge
the existing directory there with `upb/python/`. I made `upb/python/BUILD` into
the BUILD file for the merged directory, and it effectively loads the contents
of the other BUILD file via `python/build_targets.bzl`, but I plan to clean
this up soon.
PiperOrigin-RevId: 568651768
This adds `#![deny(unsafe_op_in_unsafe_fn)]` which removes the
implicit `unsafe` block that `unsafe fn` does.
It also adds many more `SAFETY` docs, corrects some incomplete
ones, and catches a null pointer returned by `upb_Arena_New`.
PiperOrigin-RevId: 549067106
Adam Cozzette
Protobuf Team Bot
Mike Kruskal
Yamil Morales
Jakob Buchgraber
Marcel Hlopko
Derek Benson
Alyssa Haroldsen
Kevin King
Joshua Haberman
Hong Shin