From a4538f2e1e1d3f1b98f6ac5f0496a8d7f4c920b3 Mon Sep 17 00:00:00 2001 From: Casper Meijn Date: Wed, 28 Feb 2024 20:13:35 +0100 Subject: [PATCH 1/2] fix: fq_message_name should begin with one dot (#981) When package name is empty, but type_path is filled, the fq_message_name should begin with a single dot. Two duplicate implementations are moved to a separate function. A test is added that creates a sub message without a package name. The code generator uses the message name to generate a rust module path, which is invalid. fixes: #843 --- prost-build/src/code_generator.rs | 38 ++++++++-------------- tests/src/build.rs | 4 +++ tests/src/lib.rs | 2 ++ tests/src/submessage_without_package.proto | 8 +++++ tests/src/submessage_without_package.rs | 6 ++++ 5 files changed, 34 insertions(+), 24 deletions(-) create mode 100644 tests/src/submessage_without_package.proto create mode 100644 tests/src/submessage_without_package.rs diff --git a/prost-build/src/code_generator.rs b/prost-build/src/code_generator.rs index 4f348baaf..ecd21852a 100644 --- a/prost-build/src/code_generator.rs +++ b/prost-build/src/code_generator.rs @@ -122,18 +122,7 @@ impl<'a> CodeGenerator<'a> { debug!(" message: {:?}", message.name()); let message_name = message.name().to_string(); - let fq_message_name = format!( - "{}{}{}{}.{}", - if self.package.is_empty() && self.type_path.is_empty() { - "" - } else { - "." - }, - self.package.trim_matches('.'), - if self.type_path.is_empty() { "" } else { "." }, - self.type_path.join("."), - message_name, - ); + let fq_message_name = self.fq_name(&message_name); // Skip external types. if self.extern_paths.resolve_ident(&fq_message_name).is_some() { @@ -701,18 +690,7 @@ impl<'a> CodeGenerator<'a> { let enum_name = to_upper_camel(proto_enum_name); let enum_values = &desc.value; - let fq_proto_enum_name = format!( - "{}{}{}{}.{}", - if self.package.is_empty() && self.type_path.is_empty() { - "" - } else { - "." - }, - self.package.trim_matches('.'), - if self.type_path.is_empty() { "" } else { "." }, - self.type_path.join("."), - proto_enum_name, - ); + let fq_proto_enum_name = self.fq_name(proto_enum_name); if self .extern_paths @@ -1065,6 +1043,18 @@ impl<'a> CodeGenerator<'a> { .as_ref() .map_or(false, FieldOptions::deprecated) } + + /// Returns the fully-qualified name, starting with a dot + fn fq_name(&self, message_name: &str) -> String { + format!( + "{}{}{}{}.{}", + if self.package.is_empty() { "" } else { "." }, + self.package.trim_matches('.'), + if self.type_path.is_empty() { "" } else { "." }, + self.type_path.join("."), + message_name, + ) + } } /// Returns `true` if the repeated field type can be packed. diff --git a/tests/src/build.rs b/tests/src/build.rs index aec3f6458..e2b95e6c4 100644 --- a/tests/src/build.rs +++ b/tests/src/build.rs @@ -111,6 +111,10 @@ fn main() { .compile_protos(&[src.join("option_struct.proto")], includes) .unwrap(); + config + .compile_protos(&[src.join("submessage_without_package.proto")], includes) + .unwrap(); + prost_build::Config::new() .protoc_arg("--experimental_allow_proto3_optional") .compile_protos(&[src.join("proto3_presence.proto")], includes) diff --git a/tests/src/lib.rs b/tests/src/lib.rs index 954c6b367..b95f2d77a 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -46,6 +46,8 @@ mod no_unused_results; #[cfg(feature = "std")] mod skip_debug; #[cfg(test)] +mod submessage_without_package; +#[cfg(test)] mod type_names; mod test_enum_named_option_value { diff --git a/tests/src/submessage_without_package.proto b/tests/src/submessage_without_package.proto new file mode 100644 index 000000000..c637b29bc --- /dev/null +++ b/tests/src/submessage_without_package.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +message M { + message SubMessage { + map item = 1; + } + SubMessage reply = 2; +} diff --git a/tests/src/submessage_without_package.rs b/tests/src/submessage_without_package.rs new file mode 100644 index 000000000..8f02a26e6 --- /dev/null +++ b/tests/src/submessage_without_package.rs @@ -0,0 +1,6 @@ +include!(concat!(env!("OUT_DIR"), "/_.rs")); + +#[test] +fn test_submessage_without_package() { + let _msg = M::default(); +} From 93534070eab4770ed17ef9cca3ab2a0848ca897c Mon Sep 17 00:00:00 2001 From: Kent Ross Date: Wed, 28 Feb 2024 11:40:49 -0800 Subject: [PATCH 2/2] really test decode_varint_slow (#977) * really test decode_varint_slow instead of passing it slices that have already been emptied * reborrow with methods not &* --------- Co-authored-by: Lucio Franco --- conformance/src/main.rs | 4 ++-- prost-build/src/ast.rs | 2 +- prost-build/src/lib.rs | 2 +- prost-types/src/lib.rs | 2 +- protobuf/benches/dataset.rs | 2 +- src/encoding.rs | 15 +++++++-------- tests/src/lib.rs | 18 ++++++++++-------- 7 files changed, 23 insertions(+), 22 deletions(-) diff --git a/conformance/src/main.rs b/conformance/src/main.rs index 157d859ad..42f5101b9 100644 --- a/conformance/src/main.rs +++ b/conformance/src/main.rs @@ -27,7 +27,7 @@ fn main() -> io::Result<()> { bytes.resize(len, 0); io::stdin().read_exact(&mut bytes)?; - let result = match ConformanceRequest::decode(&*bytes) { + let result = match ConformanceRequest::decode(bytes.as_slice()) { Ok(request) => handle_request(request), Err(error) => conformance_response::Result::ParseError(format!("{:?}", error)), }; @@ -93,7 +93,7 @@ fn handle_request(request: ConformanceRequest) -> conformance_response::Result { Some(conformance_request::Payload::ProtobufPayload(buf)) => buf, }; - let roundtrip = match &*request.message_type { + let roundtrip = match request.message_type.as_str() { "protobuf_test_messages.proto2.TestAllTypesProto2" => roundtrip::(&buf), "protobuf_test_messages.proto3.TestAllTypesProto3" => roundtrip::(&buf), _ => { diff --git a/prost-build/src/ast.rs b/prost-build/src/ast.rs index af352271d..9a6a0de99 100644 --- a/prost-build/src/ast.rs +++ b/prost-build/src/ast.rs @@ -190,7 +190,7 @@ where fn map_codeblock(kind: CodeBlockKind) -> CodeBlockKind { match kind { CodeBlockKind::Fenced(s) => { - if &*s == "rust" { + if s.as_ref() == "rust" { CodeBlockKind::Fenced("compile_fail".into()) } else { CodeBlockKind::Fenced(format!("text,{}", s).into()) diff --git a/prost-build/src/lib.rs b/prost-build/src/lib.rs index 9b71094b3..07a7083a8 100644 --- a/prost-build/src/lib.rs +++ b/prost-build/src/lib.rs @@ -1149,7 +1149,7 @@ impl Config { ), ) })?; - let file_descriptor_set = FileDescriptorSet::decode(&*buf).map_err(|error| { + let file_descriptor_set = FileDescriptorSet::decode(buf.as_slice()).map_err(|error| { Error::new( ErrorKind::InvalidInput, format!("invalid FileDescriptorSet: {}", error), diff --git a/prost-types/src/lib.rs b/prost-types/src/lib.rs index ddb50fb3f..81fcd9670 100644 --- a/prost-types/src/lib.rs +++ b/prost-types/src/lib.rs @@ -66,7 +66,7 @@ impl Any { ) { (Some(expected), Some(actual)) => { if expected == actual { - return Ok(M::decode(&*self.value)?); + return Ok(M::decode(self.value.as_slice())?); } } _ => (), diff --git a/protobuf/benches/dataset.rs b/protobuf/benches/dataset.rs index 847008555..ba45a29b6 100644 --- a/protobuf/benches/dataset.rs +++ b/protobuf/benches/dataset.rs @@ -16,7 +16,7 @@ fn load_dataset(dataset: &Path) -> Result> { let mut f = File::open(dataset)?; let mut buf = Vec::new(); f.read_to_end(&mut buf)?; - Ok(BenchmarkDataset::decode(&*buf)?) + Ok(BenchmarkDataset::decode(buf.as_slice())?) } fn benchmark_dataset(criterion: &mut Criterion, name: &str, dataset: &'static Path) diff --git a/src/encoding.rs b/src/encoding.rs index e4d2aa274..0c77123c5 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -1602,7 +1602,7 @@ mod test { #[test] fn varint() { - fn check(value: u64, mut encoded: &[u8]) { + fn check(value: u64, encoded: &[u8]) { // Small buffer. let mut buf = Vec::with_capacity(1); encode_varint(value, &mut buf); @@ -1615,11 +1615,11 @@ mod test { assert_eq!(encoded_len_varint(value), encoded.len()); - let roundtrip_value = - decode_varint(&mut <&[u8]>::clone(&encoded)).expect("decoding failed"); + let roundtrip_value = decode_varint(&mut encoded.clone()).expect("decoding failed"); assert_eq!(value, roundtrip_value); - let roundtrip_value = decode_varint_slow(&mut encoded).expect("slow decoding failed"); + let roundtrip_value = + decode_varint_slow(&mut encoded.clone()).expect("slow decoding failed"); assert_eq!(value, roundtrip_value); } @@ -1680,11 +1680,10 @@ mod test { #[test] fn varint_overflow() { - let mut u64_max_plus_one: &[u8] = - &[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02]; + let u64_max_plus_one: &[u8] = &[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x02]; - decode_varint(&mut u64_max_plus_one).expect_err("decoding u64::MAX + 1 succeeded"); - decode_varint_slow(&mut u64_max_plus_one) + decode_varint(&mut u64_max_plus_one.clone()).expect_err("decoding u64::MAX + 1 succeeded"); + decode_varint_slow(&mut u64_max_plus_one.clone()) .expect_err("slow decoding u64::MAX + 1 succeeded"); } diff --git a/tests/src/lib.rs b/tests/src/lib.rs index b95f2d77a..7d3d94867 100644 --- a/tests/src/lib.rs +++ b/tests/src/lib.rs @@ -199,6 +199,7 @@ where if let Err(error) = all_types.encode(&mut buf1) { return RoundtripResult::Error(error.into()); } + let buf1 = buf1; if encoded_len != buf1.len() { return RoundtripResult::Error(anyhow!( "expected encoded len ({}) did not match actual encoded len ({})", @@ -207,7 +208,7 @@ where )); } - let roundtrip = match M::decode(&*buf1) { + let roundtrip = match M::decode(buf1.as_slice()) { Ok(roundtrip) => roundtrip, Err(error) => return RoundtripResult::Error(anyhow::Error::new(error)), }; @@ -216,6 +217,7 @@ where if let Err(error) = roundtrip.encode(&mut buf2) { return RoundtripResult::Error(error.into()); } + let buf2 = buf2; let buf3 = roundtrip.encode_to_vec(); /* @@ -249,7 +251,7 @@ where msg.encode(&mut buf).unwrap(); assert_eq!(expected_len, buf.len()); - let mut buf = &*buf; + let mut buf = buf.as_slice(); let roundtrip = M::decode(&mut buf).unwrap(); assert!( @@ -430,7 +432,7 @@ mod tests { let mut buf = Vec::new(); a.encode(&mut buf).unwrap(); - A::decode(&*buf).map(|_| ()) + A::decode(buf.as_slice()).map(|_| ()) } assert!(build_and_roundtrip(100).is_ok()); @@ -453,7 +455,7 @@ mod tests { let mut buf = Vec::new(); a.encode(&mut buf).unwrap(); - A::decode(&*buf).map(|_| ()) + A::decode(buf.as_slice()).map(|_| ()) } assert!(build_and_roundtrip(99).is_ok()); @@ -476,7 +478,7 @@ mod tests { let mut buf = Vec::new(); a.encode(&mut buf).unwrap(); - NestedGroup2::decode(&*buf).map(|_| ()) + NestedGroup2::decode(buf.as_slice()).map(|_| ()) } assert!(build_and_roundtrip(50).is_ok()); @@ -497,7 +499,7 @@ mod tests { let mut buf = Vec::new(); c.encode(&mut buf).unwrap(); - C::decode(&*buf).map(|_| ()) + C::decode(buf.as_slice()).map(|_| ()) } assert!(build_and_roundtrip(100).is_ok()); @@ -518,7 +520,7 @@ mod tests { let mut buf = Vec::new(); d.encode(&mut buf).unwrap(); - D::decode(&*buf).map(|_| ()) + D::decode(buf.as_slice()).map(|_| ()) } assert!(build_and_roundtrip(50).is_ok()); @@ -681,7 +683,7 @@ mod tests { let mut bytes = Vec::new(); msg2.encode(&mut bytes).unwrap(); - assert_eq!(&*bytes, msg2_bytes); + assert_eq!(bytes.as_slice(), msg2_bytes); assert_eq!(groups::Test2::decode(msg2_bytes), Ok(msg2)); }