From 5a4818746461c93e078a46606d8dccb86c5b5e9d Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 14 Jun 2023 10:29:31 -0700 Subject: [PATCH] Move internal-only but kernel-agnostic paths into an `__internal` module Add some documentation along the way. PiperOrigin-RevId: 540311409 --- rust/BUILD | 2 + rust/internal.rs | 71 +++++++++++++++++++ rust/protobuf.rs | 15 ++-- rust/shared.rs | 40 +++-------- rust/test/cpp/interop/main.rs | 11 ++- .../compiler/rust/accessors/singular_bytes.cc | 2 +- .../protobuf/compiler/rust/generator.cc | 3 +- src/google/protobuf/compiler/rust/message.cc | 24 +++---- 8 files changed, 113 insertions(+), 55 deletions(-) create mode 100644 rust/internal.rs diff --git a/rust/BUILD b/rust/BUILD index cbafd02733..0e541069bf 100644 --- a/rust/BUILD +++ b/rust/BUILD @@ -49,6 +49,7 @@ rust_library( rust_library( name = "protobuf_upb", srcs = [ + "internal.rs", "proxied.rs", "shared.rs", "upb.rs", @@ -83,6 +84,7 @@ rust_library( name = "protobuf_cpp", srcs = [ "cpp.rs", + "internal.rs", "proxied.rs", "shared.rs", ], diff --git a/rust/internal.rs b/rust/internal.rs new file mode 100644 index 0000000000..ae078490f6 --- /dev/null +++ b/rust/internal.rs @@ -0,0 +1,71 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +//! Kernel-agnostic logic for the Rust Protobuf runtime that should not be +//! exposed to through the `protobuf` path but must be public for use by +//! generated code. + +use std::slice; + +/// Represents an ABI-stable version of `NonNull<[u8]>`/`string_view` (a +/// borrowed slice of bytes) for FFI use only. +/// +/// Has semantics similar to `std::string_view` in C++ and `&[u8]` in Rust, +/// but is not ABI-compatible with either. +/// +/// If `len` is 0, then `ptr` can be null or dangling. C++ considers a dangling +/// 0-len `std::string_view` to be invalid, and Rust considers a `&[u8]` with a +/// null data pointer to be invalid. +#[repr(C)] +#[derive(Copy, Clone)] +pub struct PtrAndLen { + /// Pointer to the first byte. + /// Borrows the memory. + pub ptr: *const u8, + + /// Length of the `[u8]` pointed to by `ptr`. + pub len: usize, +} + +impl PtrAndLen { + /// Unsafely dereference this slice. + /// + /// # Safety + /// - `ptr` must be valid for `len` bytes. It can be null or dangling if + /// `self.len == 0`. + pub unsafe fn as_ref<'a>(self) -> &'a [u8] { + if self.ptr.is_null() { + assert_eq!(self.len, 0, "Non-empty slice with null data pointer"); + &[] + } else { + slice::from_raw_parts(self.ptr, self.len) + } + } +} diff --git a/rust/protobuf.rs b/rust/protobuf.rs index 3569b1c6ab..d73f8ad448 100644 --- a/rust/protobuf.rs +++ b/rust/protobuf.rs @@ -30,12 +30,15 @@ //! Rust Protobuf Runtime //! -//! This file forwards to the kernel specific implementation. Rust Protobuf -//! gencode actually depends directly on kernel specific crates. The only reason -//! this crate exists is to be able to use `protobuf` as a crate name for both -//! cpp and upb kernels from user code. +//! This file forwards to `shared.rs`, which exports a kernel-specific +//! `__runtime`. Rust Protobuf gencode actually depends directly on kernel +//! specific crates. The only reason this crate exists is to be able to use +//! `protobuf` as a crate name for both cpp and upb kernels from user code. #[cfg(cpp_kernel)] -pub use protobuf_cpp::*; +use protobuf_cpp as kernel; + #[cfg(upb_kernel)] -pub use protobuf_upb::*; +use protobuf_upb as kernel; + +pub use kernel::{Mut, MutProxy, ParseError, Proxied, View, ViewProxy}; diff --git a/rust/shared.rs b/rust/shared.rs index 0ac02af232..64bedabf67 100644 --- a/rust/shared.rs +++ b/rust/shared.rs @@ -30,9 +30,12 @@ //! Kernel-agnostic logic for the Rust Protobuf Runtime. //! -//! For kernel-specific logic this crate delegates to the respective __runtime +//! For kernel-specific logic this crate delegates to the respective `__runtime` //! crate. +/// Everything in `__runtime` is allowed to change without it being considered +/// a breaking change for the protobuf library. Nothing in here should be +/// exported in `protobuf.rs`. #[cfg(cpp_kernel)] #[path = "cpp.rs"] pub mod __runtime; @@ -42,11 +45,15 @@ pub mod __runtime; mod proxied; -pub use __runtime::SerializedData; +pub use proxied::{Mut, MutProxy, Proxied, View, ViewProxy}; + +/// Everything in `__internal` is allowed to change without it being considered +/// a breaking change for the protobuf library. Nothing in here should be +/// exported in `protobuf.rs`. +#[path = "internal.rs"] +pub mod __internal; use std::fmt; -use std::slice; -use std::ptr::NonNull; /// An error that happened during deserialization. #[derive(Debug, Clone)] @@ -57,28 +64,3 @@ impl fmt::Display for ParseError { write!(f, "Couldn't deserialize given bytes into a proto") } } - -/// An ABI-stable, FFI-safe borrowed slice of bytes. -/// -/// Has semantics equivalent to `&[u8]` in Rust and `std::string_view` in C++, -/// but is not ABI-compatible with them. -/// -/// TODO: Is `ptr` allowed to be null or dangling when `len` is 0? -#[repr(C)] -#[derive(Copy, Clone)] -pub struct PtrAndLen { - /// Borrows the memory. - pub ptr: *const u8, - pub len: usize, -} - -impl PtrAndLen { - pub unsafe fn as_ref<'a>(self) -> &'a [u8] { - assert!(self.len == 0 || !self.ptr.is_null()); - if self.len == 0 { - slice::from_raw_parts(NonNull::dangling().as_ptr(), 0) - } else { - slice::from_raw_parts(self.ptr, self.len) - } - } -} diff --git a/rust/test/cpp/interop/main.rs b/rust/test/cpp/interop/main.rs index ef84d99512..006d8d4b84 100644 --- a/rust/test/cpp/interop/main.rs +++ b/rust/test/cpp/interop/main.rs @@ -48,10 +48,10 @@ macro_rules! proto_assert_eq { extern "C" { fn DeserializeTestAllTypes(data: *const u8, len: usize) -> NonNull; fn MutateTestAllTypes(msg: NonNull); - fn SerializeTestAllTypes(msg: NonNull) -> protobuf_cpp::SerializedData; + fn SerializeTestAllTypes(msg: NonNull) -> protobuf_cpp::__runtime::SerializedData; fn NewWithExtension() -> NonNull; - fn GetBytesExtension(msg: NonNull) -> protobuf_cpp::PtrAndLen; + fn GetBytesExtension(msg: NonNull) -> protobuf_cpp::__internal::PtrAndLen; } #[test] @@ -91,7 +91,7 @@ fn deserialize_in_cpp() { let msg2 = unsafe { TestAllTypes::__unstable_wrap_cpp_grant_permission_to_break(DeserializeTestAllTypes( - data.as_ptr(), + (*data).as_ptr(), data.len(), )) }; @@ -110,8 +110,7 @@ fn smuggle_extension() { let mut msg2 = TestAllExtensions::new(); msg2.deserialize(&data).unwrap(); - let bytes = unsafe { - GetBytesExtension(msg2.__unstable_cpp_repr_grant_permission_to_break()).as_ref() - }; + let bytes = + unsafe { GetBytesExtension(msg2.__unstable_cpp_repr_grant_permission_to_break()).as_ref() }; assert_eq!(&*bytes, b"smuggled"); } diff --git a/src/google/protobuf/compiler/rust/accessors/singular_bytes.cc b/src/google/protobuf/compiler/rust/accessors/singular_bytes.cc index f8b2ef9248..7f57d8396c 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_bytes.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_bytes.cc @@ -84,7 +84,7 @@ class SingularBytes final : public AccessorGenerator { }, R"rs( fn $hazzer_thunk$(raw_msg: $NonNull$) -> bool; - fn $getter_thunk$(raw_msg: $NonNull$) -> $pb$::PtrAndLen; + fn $getter_thunk$(raw_msg: $NonNull$) -> $pbi$::PtrAndLen; fn $setter_thunk$(raw_msg: $NonNull$, val: *const u8, len: usize); fn $clearer_thunk$(raw_msg: $NonNull$); )rs"); diff --git a/src/google/protobuf/compiler/rust/generator.cc b/src/google/protobuf/compiler/rust/generator.cc index cb39238657..5b6340ce8f 100644 --- a/src/google/protobuf/compiler/rust/generator.cc +++ b/src/google/protobuf/compiler/rust/generator.cc @@ -97,7 +97,8 @@ bool RustGenerator::Generate(const FileDescriptor* file_desc, auto v = file.printer().WithVars({ {"std", "::__std"}, {"pb", "::__pb"}, - {"pbi", "::__pb::__runtime"}, + {"pbi", "::__pb::__internal"}, + {"pbr", "::__pb::__runtime"}, {"NonNull", "::__std::ptr::NonNull"}, }); diff --git a/src/google/protobuf/compiler/rust/message.cc b/src/google/protobuf/compiler/rust/message.cc index 24b0fe3631..65466b0f08 100644 --- a/src/google/protobuf/compiler/rust/message.cc +++ b/src/google/protobuf/compiler/rust/message.cc @@ -59,7 +59,7 @@ void MessageStructFields(Context msg) { //~ rustc incorrectly thinks this field is never read, even though //~ it has a destructor! #[allow(dead_code)] - arena: $pbi$::Arena, + arena: $pbr$::Arena, )rs"); return; } @@ -77,7 +77,7 @@ void MessageNew(Context msg) { case Kernel::kUpb: msg.Emit({{"new_thunk", Thunk(msg, "new")}}, R"rs( - let arena = $pbi$::Arena::new(); + let arena = $pbr$::Arena::new(); Self { msg: unsafe { $new_thunk$(arena.raw()) }, arena, @@ -99,11 +99,11 @@ void MessageSerialize(Context msg) { case Kernel::kUpb: msg.Emit({{"serialize_thunk", Thunk(msg, "serialize")}}, R"rs( - let arena = $pbi$::Arena::new(); + let arena = $pbr$::Arena::new(); let mut len = 0; unsafe { let data = $serialize_thunk$(self.msg, arena.raw(), &mut len); - $pb$::SerializedData::from_raw_parts(arena, data, len) + $pbr$::SerializedData::from_raw_parts(arena, data, len) } )rs"); return; @@ -121,7 +121,7 @@ void MessageDeserialize(Context msg) { }, R"rs( let success = unsafe { - let data = $pb$::SerializedData::from_raw_parts( + let data = $pbr$::SerializedData::from_raw_parts( $NonNull$::new(data.as_ptr() as *mut _).unwrap(), data.len(), ); @@ -134,7 +134,7 @@ void MessageDeserialize(Context msg) { case Kernel::kUpb: msg.Emit({{"deserialize_thunk", Thunk(msg, "parse")}}, R"rs( - let arena = $pbi$::Arena::new(); + let arena = $pbr$::Arena::new(); let msg = unsafe { $NonNull$::::new( $deserialize_thunk$(data.as_ptr(), data.len(), arena.raw()) @@ -171,8 +171,8 @@ void MessageExterns(Context msg) { R"rs( fn $new_thunk$() -> $NonNull$; fn $delete_thunk$(raw_msg: $NonNull$); - fn $serialize_thunk$(raw_msg: $NonNull$) -> $pb$::SerializedData; - fn $deserialize_thunk$(raw_msg: $NonNull$, data: $pb$::SerializedData) -> bool; + fn $serialize_thunk$(raw_msg: $NonNull$) -> $pbr$::SerializedData; + fn $deserialize_thunk$(raw_msg: $NonNull$, data: $pbr$::SerializedData) -> bool; )rs"); return; @@ -184,9 +184,9 @@ void MessageExterns(Context msg) { {"deserialize_thunk", Thunk(msg, "parse")}, }, R"rs( - fn $new_thunk$(arena: $pbi$::RawArena) -> $NonNull$; - fn $serialize_thunk$(msg: $NonNull$, arena: $pbi$::RawArena, len: &mut usize) -> $NonNull$; - fn $deserialize_thunk$(data: *const u8, size: usize, arena: $pbi$::RawArena) -> *mut u8; + fn $new_thunk$(arena: $pbr$::RawArena) -> $NonNull$; + fn $serialize_thunk$(msg: $NonNull$, arena: $pbr$::RawArena, len: &mut usize) -> $NonNull$; + fn $deserialize_thunk$(data: *const u8, size: usize, arena: $pbr$::RawArena) -> *mut u8; )rs"); return; } @@ -293,7 +293,7 @@ void MessageGenerator::GenerateRs(Context msg) { $Msg::new$ } - pub fn serialize(&self) -> $pb$::SerializedData { + pub fn serialize(&self) -> $pbr$::SerializedData { $Msg::serialize$ } pub fn deserialize(&mut self, data: &[u8]) -> Result<(), $pb$::ParseError> {