Make payload of `NamespaceQualifier` private.
This CL prevents direct construction of `NamespaceQualifier` and forces
using explicit `code_gen_utils` APIs to construct a
`NamespaceQualifier`. This is a step toward a future where the provided
construction API performs *early* verification of the input (e.g.
panicking or returning an error early when the inputs cannot be
formatted as C++ namespace identifiers or Rust module names).
This CL refactors places that used to depend on a public payload
of `NamespaceQualifier` in the following way:
* Construction of `NamespaceQualifier` now goes through an explicit
`NamespaceQualifier::new` API.
* `CratePath` has been refactored to avoid having to rip-open and
deconstruct a `NamespaceQualifier`.
PiperOrigin-RevId: 499270950
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index 7a6ece2..8c976f6 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -22,6 +22,7 @@
use rustc_target::spec::PanicStrategy;
use std::collections::{BTreeSet, HashMap, HashSet};
use std::ops::AddAssign;
+use std::rc::Rc;
pub struct GeneratedBindings {
pub h_body: TokenStream,
@@ -211,8 +212,9 @@
let name = full_path.pop().expect("At least the item's name should be present");
let name = get_symbol(name);
- let mod_path = full_path.into_iter().map(get_symbol).map(|s| s.as_str().into()).collect();
- let mod_path = NamespaceQualifier(mod_path);
+ let mod_path = NamespaceQualifier::new(
+ full_path.into_iter().map(get_symbol).map(|s| Rc::<str>::from(s.as_str())),
+ );
Self { krate, mod_path, name }
}
diff --git a/common/code_gen_utils.rs b/common/code_gen_utils.rs
index cc81138..3f38d94 100644
--- a/common/code_gen_utils.rs
+++ b/common/code_gen_utils.rs
@@ -45,22 +45,30 @@
}
}
-/// Representation of `foo::bar::baz::` where each component is either the name
+/// Representation of `foo::bar::baz` where each component is either the name
/// of a C++ namespace, or the name of a Rust module.
#[derive(Debug, PartialEq, Eq, Clone, Hash, PartialOrd, Ord)]
-// TODO(b/258265044): Make the `Vec<String>` payload private + guarantee
-// additional invariants in an explicit, public `new` method. This will help to
-// catch some error conditions early (e.g. an empty path component may trigger a
-// panic in `make_rs_ident`; a reserved C++ keyword might trigger a late error
-// in `format_for_cc` / `format_cc_ident`).
-pub struct NamespaceQualifier(pub Vec<Rc<str>>);
+pub struct NamespaceQualifier(Vec<Rc<str>>);
impl NamespaceQualifier {
+ /// Constructs a new `NamespaceQualifier` from a sequence of names.
+ pub fn new<T: Into<Rc<str>>>(iter: impl IntoIterator<Item = T>) -> Self {
+ // TODO(b/258265044): Catch most (all if possible) error conditions early. For
+ // example:
+ // - Panic early if any strings are empty, or are not Rust identifiers
+ // - Report an error early if any strings are C++ reserved keywords
+ // This may make `format_for_cc`, `format_with_cc_body`, and
+ // `format_namespace_bound_cc_tokens` infallible.
+ Self(iter.into_iter().map(Into::into).collect())
+ }
+
+ /// Returns `foo::bar::baz::` (escaping Rust keywords as needed).
pub fn format_for_rs(&self) -> TokenStream {
let namespace_rs_idents = self.0.iter().map(|ns| make_rs_ident(ns));
quote! { #(#namespace_rs_idents::)* }
}
+ /// Returns `foo::bar::baz::` (reporting errors for C++ keywords).
pub fn format_for_cc(&self) -> Result<TokenStream> {
let namespace_cc_idents = self.cc_idents()?;
Ok(quote! { #(#namespace_cc_idents::)* })
@@ -497,13 +505,9 @@
);
}
- fn create_namespace_qualifier_for_tests(input: &[&str]) -> NamespaceQualifier {
- NamespaceQualifier(input.into_iter().map(|&s| s.into()).collect())
- }
-
#[test]
fn test_namespace_qualifier_empty() {
- let ns = create_namespace_qualifier_for_tests(&[]);
+ let ns = NamespaceQualifier::new::<&str>([]);
let actual_rs = ns.format_for_rs();
assert!(actual_rs.is_empty());
let actual_cc = ns.format_for_cc().unwrap();
@@ -512,7 +516,7 @@
#[test]
fn test_namespace_qualifier_basic() {
- let ns = create_namespace_qualifier_for_tests(&["foo", "bar"]);
+ let ns = NamespaceQualifier::new(["foo", "bar"]);
let actual_rs = ns.format_for_rs();
assert_rs_matches!(actual_rs, quote! { foo::bar:: });
let actual_cc = ns.format_for_cc().unwrap();
@@ -521,7 +525,7 @@
#[test]
fn test_namespace_qualifier_reserved_cc_keyword() {
- let ns = create_namespace_qualifier_for_tests(&["foo", "impl", "bar"]);
+ let ns = NamespaceQualifier::new(["foo", "impl", "bar"]);
let actual_rs = ns.format_for_rs();
assert_rs_matches!(actual_rs, quote! { foo :: r#impl :: bar :: });
let actual_cc = ns.format_for_cc().unwrap();
@@ -530,7 +534,7 @@
#[test]
fn test_namespace_qualifier_reserved_rust_keyword() {
- let ns = create_namespace_qualifier_for_tests(&["foo", "reinterpret_cast", "bar"]);
+ let ns = NamespaceQualifier::new(["foo", "reinterpret_cast", "bar"]);
let actual_rs = ns.format_for_rs();
assert_rs_matches!(actual_rs, quote! { foo :: reinterpret_cast :: bar :: });
let cc_error = ns.format_for_cc().unwrap_err();
@@ -541,7 +545,7 @@
#[test]
fn test_namespace_qualifier_format_with_cc_body_top_level_namespace() {
- let ns = create_namespace_qualifier_for_tests(&[]);
+ let ns = NamespaceQualifier::new::<&str>([]);
assert_cc_matches!(
ns.format_with_cc_body(quote! { cc body goes here }).unwrap(),
quote! { cc body goes here },
@@ -550,7 +554,7 @@
#[test]
fn test_namespace_qualifier_format_with_cc_body_nested_namespace() {
- let ns = create_namespace_qualifier_for_tests(&["foo", "bar", "baz"]);
+ let ns = NamespaceQualifier::new(["foo", "bar", "baz"]);
assert_cc_matches!(
ns.format_with_cc_body(quote! { cc body goes here }).unwrap(),
quote! {
@@ -563,10 +567,10 @@
#[test]
fn test_format_namespace_bound_cc_tokens() {
- let top_level = create_namespace_qualifier_for_tests(&[]);
- let m1 = create_namespace_qualifier_for_tests(&["m1"]);
- let m2 = create_namespace_qualifier_for_tests(&["m2"]);
- let input = vec![
+ let top_level = NamespaceQualifier::new::<&str>([]);
+ let m1 = NamespaceQualifier::new(["m1"]);
+ let m2 = NamespaceQualifier::new(["m2"]);
+ let input = [
(top_level.clone(), quote! { void f0a(); }),
(m1.clone(), quote! { void f1a(); }),
(m1.clone(), quote! { void f1b(); }),
diff --git a/rs_bindings_from_cc/ir.rs b/rs_bindings_from_cc/ir.rs
index 9b2cc88..efc6f34 100644
--- a/rs_bindings_from_cc/ir.rs
+++ b/rs_bindings_from_cc/ir.rs
@@ -872,8 +872,8 @@
}
}
- pub fn crate_root_path(&self) -> Option<&str> {
- self.flat_ir.crate_root_path.as_deref()
+ pub fn crate_root_path(&self) -> Option<Rc<str>> {
+ self.flat_ir.crate_root_path.clone()
}
}
@@ -934,6 +934,6 @@
}
"#;
let ir = deserialize_ir(input.as_bytes()).unwrap();
- assert_eq!(ir.crate_root_path(), Some("__cc_template_instantiations_rs_api"));
+ assert_eq!(ir.crate_root_path().as_deref(), Some("__cc_template_instantiations_rs_api"));
}
}
diff --git a/rs_bindings_from_cc/src_code_gen.rs b/rs_bindings_from_cc/src_code_gen.rs
index f50f5ac..169d376 100644
--- a/rs_bindings_from_cc/src_code_gen.rs
+++ b/rs_bindings_from_cc/src_code_gen.rs
@@ -1905,7 +1905,7 @@
}
}
}
- Ok(NamespaceQualifier(namespaces.into_iter().rev().collect_vec()))
+ Ok(NamespaceQualifier::new(namespaces.into_iter().rev()))
}
/// Generates Rust source code for a given incomplete record declaration.
@@ -2849,46 +2849,33 @@
/// Qualified path from the root of the crate to the module containing the type.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct CratePath {
- idents: Vec<String>,
+ /// `Some("other_crate")` or `None` for paths within the current crate.
+ crate_ident: Option<Ident>,
+
+ crate_root_path: NamespaceQualifier,
+ namespace_qualifier: NamespaceQualifier,
}
impl CratePath {
fn new(
ir: &IR,
- namespace_qualifier: &NamespaceQualifier,
+ namespace_qualifier: NamespaceQualifier,
crate_ident: Option<Ident>,
) -> CratePath {
- let namespace_qualifiers = &namespace_qualifier.0;
- let crate_ident = match crate_ident {
- Some(ci) => ci.to_string(),
- None => "crate".to_string(),
- };
- let idents = std::iter::once(crate_ident)
- .chain(ir.crate_root_path().into_iter().map(ToOwned::to_owned))
- .chain(namespace_qualifiers.iter().map(|s| s.to_string()))
- .collect();
- CratePath { idents }
+ let crate_root_path = NamespaceQualifier::new(ir.crate_root_path());
+ CratePath { crate_ident, crate_root_path, namespace_qualifier }
}
}
impl ToTokens for CratePath {
fn to_tokens(&self, tokens: &mut TokenStream) {
- use quote::TokenStreamExt;
- for (i, ident) in self.idents.iter().enumerate() {
- if i > 0 {
- // Double colon `::`
- tokens.append(proc_macro2::Punct::new(':', proc_macro2::Spacing::Joint));
- tokens.append(proc_macro2::Punct::new(':', proc_macro2::Spacing::Alone));
- tokens.append(make_rs_ident(ident))
- } else {
- tokens.append(if ident == "crate" {
- // leading token `crate` should be interpreted as a keyword, not escaped.
- format_ident!("{}", ident)
- } else {
- make_rs_ident(ident)
- })
- }
- }
+ let crate_ident = match self.crate_ident.as_ref() {
+ None => quote! { crate },
+ Some(ident) => quote! { #ident },
+ };
+ let crate_root_path = self.crate_root_path.format_for_rs();
+ let namespace_qualifier = self.namespace_qualifier.format_for_rs();
+ quote! { #crate_ident :: #crate_root_path #namespace_qualifier }.to_tokens(tokens)
}
}
@@ -2939,7 +2926,7 @@
pub fn new_record(record: Rc<Record>, ir: &IR) -> Result<Self> {
let crate_path = Rc::new(CratePath::new(
ir,
- &namespace_qualifier_of_item(record.id, ir)?,
+ namespace_qualifier_of_item(record.id, ir)?,
rs_imported_crate_name(&record.owning_target, ir),
));
Ok(RsTypeKind::Record { record, crate_path })
@@ -3171,15 +3158,15 @@
}
RsTypeKind::IncompleteRecord { incomplete_record, crate_path } => {
let record_ident = make_rs_ident(incomplete_record.rs_name.as_ref());
- quote! { #crate_path :: #record_ident }
+ quote! { #crate_path #record_ident }
}
RsTypeKind::Record { record, crate_path } => {
let ident = make_rs_ident(record.rs_name.as_ref());
- quote! { #crate_path :: #ident }
+ quote! { #crate_path #ident }
}
RsTypeKind::TypeAlias { type_alias, crate_path, .. } => {
let ident = make_rs_ident(&type_alias.identifier.identifier);
- quote! { #crate_path :: #ident }
+ quote! { #crate_path #ident }
}
// This doesn't affect void in function return values, as those are special-cased to be
// omitted.
@@ -3275,7 +3262,7 @@
incomplete_record: incomplete_record.clone(),
crate_path: Rc::new(CratePath::new(
&ir,
- &namespace_qualifier_of_item(incomplete_record.id, &ir)?,
+ namespace_qualifier_of_item(incomplete_record.id, &ir)?,
rs_imported_crate_name(&incomplete_record.owning_target, &ir),
)),
},
@@ -3290,7 +3277,7 @@
type_alias: type_alias.clone(),
crate_path: Rc::new(CratePath::new(
&ir,
- &namespace_qualifier_of_item(type_alias.id, &ir)?,
+ namespace_qualifier_of_item(type_alias.id, &ir)?,
rs_imported_crate_name(&type_alias.owning_target, &ir),
)),
underlying_type: Rc::new(
@@ -3569,11 +3556,9 @@
}
fn crate_root_path_tokens(ir: &IR) -> TokenStream {
- if let Some(crate_root_path) = ir.crate_root_path() {
- let crate_root_path = make_rs_ident(crate_root_path);
- quote! {crate::#crate_root_path}
- } else {
- quote! {crate}
+ match ir.crate_root_path().as_deref().map(make_rs_ident) {
+ None => quote! { crate },
+ Some(crate_root_path) => quote! { crate :: #crate_root_path },
}
}
