From b1e4bb3b8d7c165fe8113ed8b52165eff7f2e96a Mon Sep 17 00:00:00 2001 From: David Freese Date: Fri, 26 Feb 2021 17:34:01 -0800 Subject: [PATCH 1/9] Fix clippy lints with 1.50.0 1.50.0 introduced a new clippy lint that has one false positive that this disables and one that was actually valid. Given a refactor in that file, prost_attrs was made into a private function. Note that #436, or similar, is also needed to get CI green again. --- prost-derive/src/field/mod.rs | 10 +++++----- src/encoding.rs | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/prost-derive/src/field/mod.rs b/prost-derive/src/field/mod.rs index 9e2e70c4c..09fef830e 100644 --- a/prost-derive/src/field/mod.rs +++ b/prost-derive/src/field/mod.rs @@ -32,7 +32,7 @@ impl Field { /// If the meta items are invalid, an error will be returned. /// If the field should be ignored, `None` is returned. pub fn new(attrs: Vec, inferred_tag: Option) -> Result, Error> { - let attrs = prost_attrs(attrs)?; + let attrs = prost_attrs(attrs); // TODO: check for ignore attribute. @@ -58,7 +58,7 @@ impl Field { /// If the meta items are invalid, an error will be returned. /// If the field should be ignored, `None` is returned. pub fn new_oneof(attrs: Vec) -> Result, Error> { - let attrs = prost_attrs(attrs)?; + let attrs = prost_attrs(attrs); // TODO: check for ignore attribute. @@ -224,8 +224,8 @@ impl fmt::Display for Label { } /// Get the items belonging to the 'prost' list attribute, e.g. `#[prost(foo, bar="baz")]`. -pub(super) fn prost_attrs(attrs: Vec) -> Result, Error> { - Ok(attrs +fn prost_attrs(attrs: Vec) -> Vec { + attrs .iter() .flat_map(Attribute::parse_meta) .flat_map(|meta| match meta { @@ -244,7 +244,7 @@ pub(super) fn prost_attrs(attrs: Vec) -> Result, Error> { NestedMeta::Lit(lit) => bail!("invalid prost attribute: {:?}", lit), } }) - .collect()) + .collect() } pub fn set_option(option: &mut Option, value: T, message: &str) -> Result<(), Error> diff --git a/src/encoding.rs b/src/encoding.rs index c599f662d..7b04c78a0 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -259,6 +259,7 @@ impl DecodeContext { #[cfg(feature = "no-recursion-limit")] #[inline] + #[allow(clippy::unnecessary_wraps)] // needed in other features pub(crate) fn limit_reached(&self) -> Result<(), DecodeError> { Ok(()) } From 83f1b4cd27e2ca946cfb83f9a0e7b683fffc8222 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Wed, 10 Feb 2021 21:15:25 -0500 Subject: [PATCH 2/9] Remove the downloading of big test data that's no longer available See https://github.com/protocolbuffers/protobuf/issues/8283 --- protobuf/benches/dataset.rs | 6 ------ protobuf/build.rs | 5 +---- protobuf/src/lib.rs | 21 --------------------- 3 files changed, 1 insertion(+), 31 deletions(-) diff --git a/protobuf/benches/dataset.rs b/protobuf/benches/dataset.rs index 4572b8c55..847008555 100644 --- a/protobuf/benches/dataset.rs +++ b/protobuf/benches/dataset.rs @@ -84,9 +84,6 @@ dataset!(google_message1_proto2, proto2::GoogleMessage1); dataset!(google_message1_proto3, proto3::GoogleMessage1); dataset!(google_message2, proto2::GoogleMessage2); dataset!(google_message3_1, GoogleMessage3); -dataset!(google_message3_2, GoogleMessage3); -dataset!(google_message3_3, GoogleMessage3); -dataset!(google_message3_4, GoogleMessage3); dataset!(google_message3_5, GoogleMessage3); dataset!(google_message4, GoogleMessage4); @@ -95,9 +92,6 @@ criterion_group!( google_message1_proto2, google_message1_proto3, google_message2, - google_message3_2, - google_message3_3, - google_message3_4, ); criterion_group! { diff --git a/protobuf/build.rs b/protobuf/build.rs index 1f025e0f8..b0c5694d2 100644 --- a/protobuf/build.rs +++ b/protobuf/build.rs @@ -270,8 +270,5 @@ fn install_datasets(src_dir: &Path, prefix_dir: &Path) -> Result<()> { .with_context(|| format!("failed to move {}", dataset.display()))?; } - download_tarball( - "https://storage.googleapis.com/protobuf_opensource_benchmark_data/datasets.tar.gz", - share_dir, - ) + Ok(()) } diff --git a/protobuf/src/lib.rs b/protobuf/src/lib.rs index db083e6ce..819708e6b 100644 --- a/protobuf/src/lib.rs +++ b/protobuf/src/lib.rs @@ -34,27 +34,6 @@ pub mod benchmarks { )) } - pub fn google_message3_2() -> &'static Path { - Path::new(concat!( - env!("PROTOBUF"), - "/share/dataset.google_message3_2.pb" - )) - } - - pub fn google_message3_3() -> &'static Path { - Path::new(concat!( - env!("PROTOBUF"), - "/share/dataset.google_message3_3.pb" - )) - } - - pub fn google_message3_4() -> &'static Path { - Path::new(concat!( - env!("PROTOBUF"), - "/share/dataset.google_message3_4.pb" - )) - } - pub fn google_message3_5() -> &'static Path { Path::new(concat!( env!("PROTOBUF"), From f67f43def3d6930b11887912c6f425ffa26575d1 Mon Sep 17 00:00:00 2001 From: pluth <39027073+pluth@users.noreply.github.com> Date: Fri, 19 Mar 2021 16:23:39 +0100 Subject: [PATCH 3/9] Enable zero-copy support for bytes::Bytes fields I just noticed this while poking through the code. Fields of type `Bytes` can now be slices of a greater `Bytes` that is passed to Message::decode The referenced issue https://github.com/tokio-rs/bytes/issues/374 was implemented as Buf::copy_to_bytes and `Bytes` features the zero-copy implementation https://docs.rs/bytes/1.0.1/src/bytes/bytes.rs.html#526-560 --- src/encoding.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/encoding.rs b/src/encoding.rs index 7b04c78a0..252358685 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -991,12 +991,7 @@ pub mod bytes { // > last value it sees. // // [1]: https://developers.google.com/protocol-buffers/docs/encoding#optional - - // NOTE: The use of BufExt::take() currently prevents zero-copy decoding - // for bytes fields backed by Bytes when docoding from Bytes. This could - // be addressed in the future by specialization. - // See also: https://github.com/tokio-rs/bytes/issues/374 - value.replace_with(buf.take(len)); + value.replace_with(buf.copy_to_bytes(len)); Ok(()) } From eb3dcdb58e4f047acdf9c6b01f5e8163fe46ac04 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" <193874+carols10cents@users.noreply.github.com> Date: Sun, 21 Mar 2021 18:15:13 -0400 Subject: [PATCH 4/9] Documentation improvements (#446) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is mostly to fix #69 by documenting generated enum conversion functions in the README. I also wanted to make sure the generated code example was up to date, and it wasn't, so there's a commit for that. And while setting up a project to generate the code from the tutorial example, I hit a small hiccup in prost-build's documentation, so there's a fix for that :) Thank you! I'm happy to make any changes ❤️ --- README.md | 111 +++++++++++++++++++++++++++++++++++------ prost-build/src/lib.rs | 6 +-- 2 files changed, 100 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index d5de8a649..049200332 100644 --- a/README.md +++ b/README.md @@ -107,10 +107,91 @@ Scalar value types are converted as follows: #### Enumerations All `.proto` enumeration types convert to the Rust `i32` type. Additionally, -each enumeration type gets a corresponding Rust `enum` type, with helper methods -to convert `i32` values to the enum type. The `enum` type isn't used directly as -a field, because the Protobuf spec mandates that enumerations values are 'open', -and decoding unrecognized enumeration values must be possible. +each enumeration type gets a corresponding Rust `enum` type. For example, this +`proto` enum: + +```proto +enum PhoneType { + MOBILE = 0; + HOME = 1; + WORK = 2; +} +``` + +gets this corresponding Rust enum [1]: + +```rust +pub enum PhoneType { + Mobile = 0, + Home = 1, + Work = 2, +} +``` + +You can convert a `PhoneType` value to an `i32` by doing: + +```rust +PhoneType::Mobile as i32 +``` + +The `#[derive(::prost::Enumeration)]` annotation added to the generated +`PhoneType` adds these associated functions to the type: + +```rust +impl PhoneType { + pub fn is_valid(value: i32) -> bool { ... } + pub fn from_i32(value: i32) -> Option { ... } +} +``` + +so you can convert an `i32` to its corresponding `PhoneType` value by doing, +for example: + +```rust +let phone_type = 2i32; + +match PhoneType::from_i32(phone_type) { + Some(PhoneType::Mobile) => ..., + Some(PhoneType::Home) => ..., + Some(PhoneType::Work) => ..., + None => ..., +} +``` + +Additionally, wherever a `proto` enum is used as a field in a `Message`, the +message will have 'accessor' methods to get/set the value of the field as the +Rust enum type. For instance, this proto `PhoneNumber` message that has a field +named `type` of type `PhoneType`: + +```proto +message PhoneNumber { + string number = 1; + PhoneType type = 2; +} +``` + +will become the following Rust type [1] with methods `type` and `set_type`: + +```rust +pub struct PhoneNumber { + pub number: String, + pub r#type: i32, // the `r#` is needed because `type` is a Rust keyword +} + +impl PhoneNumber { + pub fn r#type(&self) -> PhoneType { ... } + pub fn set_type(&mut self, value: PhoneType) { ... } +} +``` + +Note that the getter methods will return the Rust enum's default value if the +field has an invalid `i32` value. + +The `enum` type isn't used directly as a field, because the Protobuf spec +mandates that enumerations values are 'open', and decoding unrecognized +enumeration values must be possible. + +[1] Annotations have been elided for clarity. See below for a full example. #### Field Modifiers @@ -215,27 +296,29 @@ message AddressBook { and the generated Rust code (`tutorial.rs`): ```rust -#[derive(Clone, Debug, PartialEq, Message)] +#[derive(Clone, PartialEq, ::prost::Message)] pub struct Person { #[prost(string, tag="1")] - pub name: String, + pub name: ::prost::alloc::string::String, /// Unique ID number for this person. #[prost(int32, tag="2")] pub id: i32, #[prost(string, tag="3")] - pub email: String, + pub email: ::prost::alloc::string::String, #[prost(message, repeated, tag="4")] - pub phones: Vec, + pub phones: ::prost::alloc::vec::Vec, } +/// Nested message and enum types in `Person`. pub mod person { - #[derive(Clone, Debug, PartialEq, Message)] + #[derive(Clone, PartialEq, ::prost::Message)] pub struct PhoneNumber { #[prost(string, tag="1")] - pub number: String, + pub number: ::prost::alloc::string::String, #[prost(enumeration="PhoneType", tag="2")] - pub type_: i32, + pub r#type: i32, } - #[derive(Clone, Copy, Debug, PartialEq, Eq, Enumeration)] + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)] + #[repr(i32)] pub enum PhoneType { Mobile = 0, Home = 1, @@ -243,10 +326,10 @@ pub mod person { } } /// Our address book file is just one of these. -#[derive(Clone, Debug, PartialEq, Message)] +#[derive(Clone, PartialEq, ::prost::Message)] pub struct AddressBook { #[prost(message, repeated, tag="1")] - pub people: Vec, + pub people: ::prost::alloc::vec::Vec, } ``` diff --git a/prost-build/src/lib.rs b/prost-build/src/lib.rs index 6353b7d0d..041e350e2 100644 --- a/prost-build/src/lib.rs +++ b/prost-build/src/lib.rs @@ -51,10 +51,10 @@ //! `build.rs` build-script: //! //! ```rust,no_run -//! # use std::io::Result; +//! use std::io::Result; //! fn main() -> Result<()> { -//! prost_build::compile_protos(&["src/items.proto"], &["src/"])?; -//! Ok(()) +//! prost_build::compile_protos(&["src/items.proto"], &["src/"])?; +//! Ok(()) //! } //! ``` //! From ed864c6f27c7c70ac93a8e92861921d4ad11f624 Mon Sep 17 00:00:00 2001 From: Jan Fornoff Date: Sun, 21 Mar 2021 23:33:30 +0100 Subject: [PATCH 5/9] Fix typo in README (#434) * Fix typo in README * Another typo --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 049200332..c99219d31 100644 --- a/README.md +++ b/README.md @@ -352,7 +352,7 @@ prost = { version = "0.6", default-features = false, features = ["prost-derive"] prost-types = { version = "0.6", default-features = false } ``` -Additionally, configure `prost-buid` to output `BTreeMap`s instead of `HashMap`s +Additionally, configure `prost-build` to output `BTreeMap`s instead of `HashMap`s for all Protobuf `map` fields in your `build.rs`: ```rust @@ -432,7 +432,7 @@ pub enum Gender { There are two complications with trying to serialize Protobuf messages with Serde: - - Protobuf fields require a numbered tag, and curently there appears to be no + - Protobuf fields require a numbered tag, and currently there appears to be no mechanism suitable for this in `serde`. - The mapping of Protobuf type to Rust type is not 1-to-1. As a result, trait-based approaches to dispatching don't work very well. Example: six From 480c744189556524d2d1ed234fa30f5084ec4bd9 Mon Sep 17 00:00:00 2001 From: Sascha Wise Date: Mon, 22 Mar 2021 11:49:29 +1300 Subject: [PATCH 6/9] make DecodeError::new public (#421) With the release of prost 0.7 `DecodeError::new` is marked as `pub(crate)`. This makes custom implementations of `Message` difficult to create. Since `DecodeError` is already used in a public trait (`Message`) it seems reasonable to be able to construct it externally. --- src/error.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index 73bf197ca..fc098299c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -30,8 +30,9 @@ impl DecodeError { /// Creates a new `DecodeError` with a 'best effort' root cause description. /// /// Meant to be used only by `Message` implementations. + #[doc(hidden)] #[cold] - pub(crate) fn new(description: impl Into>) -> DecodeError { + pub fn new(description: impl Into>) -> DecodeError { DecodeError { inner: Box::new(Inner { description: description.into(), From 7a38484e81803a8ddd7b7c6a33c7951136cb6257 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sun, 21 Mar 2021 17:41:27 -0700 Subject: [PATCH 7/9] build(deps): update to itertools 0.10 (#447) Co-authored-by: Dan Burkert --- prost-build/Cargo.toml | 2 +- prost-derive/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/prost-build/Cargo.toml b/prost-build/Cargo.toml index 139a61240..536e316f6 100644 --- a/prost-build/Cargo.toml +++ b/prost-build/Cargo.toml @@ -12,7 +12,7 @@ edition = "2018" [dependencies] bytes = { version = "1", default-features = false } heck = "0.3" -itertools = "0.9" +itertools = "0.10" log = "0.4" multimap = { version = "0.8", default-features = false } petgraph = { version = "0.5", default-features = false } diff --git a/prost-derive/Cargo.toml b/prost-derive/Cargo.toml index 2460bd972..0c0b747ec 100644 --- a/prost-derive/Cargo.toml +++ b/prost-derive/Cargo.toml @@ -14,7 +14,7 @@ proc_macro = true [dependencies] anyhow = "1" -itertools = "0.9" +itertools = "0.10" proc-macro2 = "1" quote = "1" syn = { version = "1", features = [ "extra-traits" ] } From 32ad5a2e1036a9c9425f0a84f032ba9f343b133e Mon Sep 17 00:00:00 2001 From: David Freese Date: Sun, 21 Mar 2021 17:41:51 -0700 Subject: [PATCH 8/9] Update from deprecated criterion::Benchmark to BenchmarkGroup (#444) This gets rid of the warnings generated by the benchmark code. This does not completely replecate the old behavior, which created three benchmarks, all with their own throughput, but given the same name. I don't think that same thing is possible anymore with the non-deprecated APIs. --- benches/varint.rs | 84 +++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/benches/varint.rs b/benches/varint.rs index b2bc8ca37..34951f5ea 100644 --- a/benches/varint.rs +++ b/benches/varint.rs @@ -1,13 +1,14 @@ use std::mem; use bytes::Buf; -use criterion::{Benchmark, Criterion, Throughput}; +use criterion::{Criterion, Throughput}; use prost::encoding::{decode_varint, encode_varint, encoded_len_varint}; use rand::{rngs::StdRng, seq::SliceRandom, SeedableRng}; fn benchmark_varint(criterion: &mut Criterion, name: &str, mut values: Vec) { // Shuffle the values in a stable order. values.shuffle(&mut StdRng::seed_from_u64(0)); + let name = format!("varint/{}", name); let encoded_len = values .iter() @@ -16,53 +17,58 @@ fn benchmark_varint(criterion: &mut Criterion, name: &str, mut values: Vec) .sum::() as u64; let decoded_len = (values.len() * mem::size_of::()) as u64; - let encode_values = values.clone(); - let encode = Benchmark::new("encode", move |b| { - let mut buf = Vec::::with_capacity(encode_values.len() * 10); - b.iter(|| { - buf.clear(); - for &value in &encode_values { - encode_varint(value, &mut buf); + criterion + .benchmark_group(&name) + .bench_function("encode", { + let encode_values = values.clone(); + move |b| { + let mut buf = Vec::::with_capacity(encode_values.len() * 10); + b.iter(|| { + buf.clear(); + for &value in &encode_values { + encode_varint(value, &mut buf); + } + criterion::black_box(&buf); + }) } - criterion::black_box(&buf); }) - }) - .throughput(Throughput::Bytes(encoded_len)); + .throughput(Throughput::Bytes(encoded_len)); - let decode_values = values.clone(); - let decode = Benchmark::new("decode", move |b| { - let mut buf = Vec::with_capacity(decode_values.len() * 10); - for &value in &decode_values { - encode_varint(value, &mut buf); - } + criterion + .benchmark_group(&name) + .bench_function("decode", { + let decode_values = values.clone(); - b.iter(|| { - let mut buf = &mut buf.as_slice(); - while buf.has_remaining() { - let result = decode_varint(&mut buf); - debug_assert!(result.is_ok()); - criterion::black_box(&result); - } - }) - }) - .throughput(Throughput::Bytes(decoded_len)); + move |b| { + let mut buf = Vec::with_capacity(decode_values.len() * 10); + for &value in &decode_values { + encode_varint(value, &mut buf); + } - let encoded_len = Benchmark::new("encoded_len", move |b| { - b.iter(|| { - let mut sum = 0; - for &value in &values { - sum += encoded_len_varint(value); + b.iter(|| { + let mut buf = &mut buf.as_slice(); + while buf.has_remaining() { + let result = decode_varint(&mut buf); + debug_assert!(result.is_ok()); + criterion::black_box(&result); + } + }) } - criterion::black_box(sum); }) - }) - .throughput(Throughput::Bytes(decoded_len)); + .throughput(Throughput::Bytes(decoded_len)); - let name = format!("varint/{}", name); criterion - .bench(&name, encode) - .bench(&name, decode) - .bench(&name, encoded_len); + .benchmark_group(&name) + .bench_function("encoded_len", move |b| { + b.iter(|| { + let mut sum = 0; + for &value in &values { + sum += encoded_len_varint(value); + } + criterion::black_box(sum); + }) + }) + .throughput(Throughput::Bytes(decoded_len)); } fn main() { From 28e4522f8ae738d4574cce104da6fd88d9ce0c14 Mon Sep 17 00:00:00 2001 From: Steven Fackler Date: Mon, 3 May 2021 21:32:25 -0400 Subject: [PATCH 9/9] Fix messages with optionals and oneofs (#455) --- prost-build/src/code_generator.rs | 12 ++++++------ tests/src/lib.rs | 5 ++++- tests/src/proto3_presence.proto | 4 ++++ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/prost-build/src/code_generator.rs b/prost-build/src/code_generator.rs index af50c28c4..156f006dc 100644 --- a/prost-build/src/code_generator.rs +++ b/prost-build/src/code_generator.rs @@ -237,12 +237,12 @@ impl<'a> CodeGenerator<'a> { for (idx, oneof) in message.oneof_decl.into_iter().enumerate() { let idx = idx as i32; - self.append_oneof( - &fq_message_name, - oneof, - idx, - oneof_fields.remove(&idx).unwrap(), - ); + // optional fields create a synthetic oneof that we want to skip + let fields = match oneof_fields.remove(&idx) { + Some(fields) => fields, + None => continue, + }; + self.append_oneof(&fq_message_name, oneof, idx, fields); } self.pop_mod(); diff --git a/tests/src/lib.rs b/tests/src/lib.rs index b20bc49bc..f56d73e7a 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -597,7 +597,10 @@ mod tests { #[test] fn test_proto3_presence() { - let msg = proto3::presence::A { b: Some(42) }; + let msg = proto3::presence::A { + b: Some(42), + foo: Some(proto3::presence::a::Foo::C(13)), + }; check_message(&msg); } diff --git a/tests/src/proto3_presence.proto b/tests/src/proto3_presence.proto index d47decce3..1e12c9bcb 100644 --- a/tests/src/proto3_presence.proto +++ b/tests/src/proto3_presence.proto @@ -4,4 +4,8 @@ package proto3.presence; message A { optional int32 b = 1; + + oneof foo { + int32 c = 2; + } }