Skip to content

Update symbol_canonical_name to return Result. #2008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Sep 15, 2026
Merged

Update symbol_canonical_name to return Result. #2008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cc_bindings_from_rs/generate_bindings/database/code_snippet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,7 @@ impl<'tcx> CcPrerequisites<'tcx> {
/// or this will fail.
pub fn depend_on_def(&mut self, db: &BindingsGenerator<'tcx>, def_id: DefId) -> Result<()> {
let tcx = db.tcx();
let canonical_name = db.symbol_canonical_name(def_id).ok_or_else(|| {
anyhow!("Failed to generate canonical name for `{}`", tcx.def_path_str(def_id))
})?;
let canonical_name = db.symbol_canonical_name(def_id)?;
// Definition with a local canonical name can be immediately added to the `defs` set.
if canonical_name.krate_num == db.source_crate_num() {
self.defs.insert(def_id);
Expand Down
4 changes: 2 additions & 2 deletions cc_bindings_from_rs/generate_bindings/database/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,11 @@ memoized::query_group! {
/// at either `Bar` or `foo::Bar` due to our `use` statements. This method would give `Bar`
/// the canonical name `foo::Bar`, preferring the more specific of our two available paths.
///
/// If no canonical name can be determined, `None` is returned. This will occur when our
/// If no canonical name can be determined, an error is returned. This will occur when our
/// `def_id` has no publicly visible paths, for example.
///
/// Implementation: cc_bindings_from_rs/generate_bindings/lib.rs?q=function:symbol_canonical_name
fn symbol_canonical_name(&self, def_id: DefId) -> Option<FullyQualifiedName>;
fn symbol_canonical_name(&self, def_id: DefId) -> Result<FullyQualifiedName>;

/// Computes a mapping from a `DefId` to a list of public paths that reference it in a given
/// crate. This accounts for `use` statements that reexport, and optionally alias, the same
Expand Down
22 changes: 5 additions & 17 deletions cc_bindings_from_rs/generate_bindings/format_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,10 +942,7 @@ pub fn format_ty_for_cc<'tcx>(
"Generic types are not supported yet (b/259749095)"
);
crate::should_receive_bindings(db, adt.did())?;
ensure!(
db.symbol_canonical_name(adt.did()).is_some(),
"Not a public or a supported reexported type (b/262052635)."
);
db.symbol_canonical_name(adt.did())?;

prereqs.depend_on_def(db, def_id)?;

Expand All @@ -955,9 +952,7 @@ pub fn format_ty_for_cc<'tcx>(
})?;
}

let canonical_name = db
.symbol_canonical_name(def_id)
.ok_or_else(|| anyhow!("Failed to generate canonical name for `{ty}`"))?;
let canonical_name = db.symbol_canonical_name(def_id)?;

let mut tokens = canonical_name.format_for_cc(db)?;
// Add generic arguments for a generic ADT.
Expand Down Expand Up @@ -1597,9 +1592,7 @@ pub fn format_ty_for_rs<'tcx>(db: &BindingsGenerator<'tcx>, ty: Ty<'tcx>) -> Res
has_cpp_type || is_supported_generic_type || has_composable_bridging,
"Generic types without composable bridging are not supported yet (b/259749095)"
);
let canonical_name = db
.symbol_canonical_name(adt.did())
.ok_or_else(|| anyhow!("Failed to get canonical name for {:?}", adt.did()))?;
let canonical_name = db.symbol_canonical_name(adt.did())?;
let type_name = canonical_name.format_for_rs();
let generic_params = if substs.is_empty() {
quote! {}
Expand Down Expand Up @@ -1824,10 +1817,7 @@ pub fn crubit_abi_type_from_ty<'tcx>(
include_paths,
cpp_type,
} => {
let fully_qualified_name =
db.symbol_canonical_name(adt.did()).ok_or_else(|| {
anyhow!("Failed to get canonical name for {:?}", adt.did())
})?;
let fully_qualified_name = db.symbol_canonical_name(adt.did())?;
let mut prereqs = CcPrerequisites::default();
for path in &include_paths {
prereqs.includes.insert(CcInclude::from_path(path.as_str()));
Expand Down Expand Up @@ -1875,9 +1865,7 @@ pub fn crubit_abi_type_from_ty<'tcx>(
return Ok(CrubitAbiTypeWithCcPrereqs { crubit_abi_type, prereqs });
}

let fully_qualified_name = db
.symbol_canonical_name(adt.did())
.ok_or_else(|| anyhow!("Failed to get canonical name for {:?}", adt.did()))?;
let fully_qualified_name = db.symbol_canonical_name(adt.did())?;

// It's just a regular old type.
// Question: do we need to check that it doesn't have any generics?
Expand Down
2 changes: 1 addition & 1 deletion cc_bindings_from_rs/generate_bindings/generate_bindings_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2509,7 +2509,7 @@ fn test_trait_operator_without_core_crate_header_returns_error() {
let bindings = generate_bindings::generate_bindings(&db).unwrap();
let cc_api = cc_tokens_to_formatted_string_for_tests(bindings.cc_api).unwrap();
assert!(
cc_api.contains("trait does not have a canonical"),
cc_api.contains("public or a supported reexported type"),
"Expected unsupported error message in cc_api, got:\n{cc_api}"
);
});
Expand Down
14 changes: 4 additions & 10 deletions cc_bindings_from_rs/generate_bindings/generate_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,7 @@ fn format_trait_ref_for_cc<'tcx>(
) -> Result<CcSnippet<'tcx>> {
let trait_name = db
.symbol_canonical_name(trait_ref.def_id)
.and_then(|fully_qualified_name| fully_qualified_name.format_for_cc(db).ok())
.and_then(|fully_qualified_name| fully_qualified_name.format_for_cc(db))
.expect("Generated trait method for a trait with an invalid cc name");
let mut trait_args = trait_ref.args[1..].iter().filter_map(|arg| arg.as_type()).peekable();
let mut prereqs = CcPrerequisites::default();
Expand All @@ -954,13 +954,7 @@ fn format_trait_ref_for_rs<'tcx>(
) -> Result<TokenStream> {
let trait_name = db
.symbol_canonical_name(trait_ref.def_id)
.map(|fully_qualified_name| fully_qualified_name.format_for_rs())
.ok_or_else(|| {
anyhow!(
"Failed to format trait name `{}`: trait does not have a canonical name",
db.tcx().def_path_str(trait_ref.def_id)
)
})?;
.map(|fully_qualified_name| fully_qualified_name.format_for_rs())?;
let mut trait_args = trait_ref.args[1..].iter().filter_map(|arg| arg.as_type()).peekable();
if trait_args.peek().is_none() {
Ok(quote! { #trait_name })
Expand Down Expand Up @@ -1165,7 +1159,7 @@ pub fn generate_function<'tcx>(
Some(ty) => match ty.kind() {
ty::TyKind::Adt(adt, substs) => {
assert!(!has_non_lifetime_substs(substs), "Callers should filter out generics");
db.symbol_canonical_name(adt.did())
db.symbol_canonical_name(adt.did()).ok()
}
_ => panic!("Non-ADT `impl`s should be filtered by caller"),
},
Expand Down Expand Up @@ -1344,7 +1338,7 @@ pub fn generate_function<'tcx>(
let fn_name = make_rs_ident(unqualified_rust_fn_name.as_str());
let struct_name = struct_name.format_for_rs();
quote! { #struct_name :: #fn_name }
} else if let Some(canonical) = db.symbol_canonical_name(def_id) {
} else if let Ok(canonical) = db.symbol_canonical_name(def_id) {
canonical.format_for_rs()
} else {
panic!(
Expand Down
13 changes: 3 additions & 10 deletions cc_bindings_from_rs/generate_bindings/generate_function_thunk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,7 @@ fn format_ty_for_closure_param_rs<'tcx>(
_ => {}
}
}
let canonical_name = db
.symbol_canonical_name(adt.did())
.ok_or_else(|| anyhow!("Failed to get canonical name for {:?}", adt.did()))?;
let canonical_name = db.symbol_canonical_name(adt.did())?;
let type_name = canonical_name.format_for_rs();
let generic_params = if substs.is_empty() {
quote! {}
Expand Down Expand Up @@ -980,7 +978,7 @@ pub fn generate_trait_thunks<'tcx>(
type_args.iter().copied().map(ty::GenericArg::from),
) {
let display_name = def_id
.and_then(|id| db.symbol_canonical_name(id))
.and_then(|id| db.symbol_canonical_name(id).ok())
.map(|canon| {
let parts = canon.rs_name_parts().map(|s| format!("{}", s)).collect::<Vec<_>>();
parts.join("::")
Expand Down Expand Up @@ -1073,12 +1071,7 @@ pub fn generate_trait_thunks<'tcx>(
}
})
} else {
let fully_qualified_trait_name = db
.symbol_canonical_name(trait_id)
.ok_or_else(|| {
anyhow!("Failed to get canonical name for {}", tcx.def_path_str(trait_id))
})?
.format_for_rs();
let fully_qualified_trait_name = db.symbol_canonical_name(trait_id)?.format_for_rs();
let method_name = make_rs_ident(method.name().as_str());
let args = type_args
.iter()
Expand Down
16 changes: 4 additions & 12 deletions cc_bindings_from_rs/generate_bindings/generate_struct_and_union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ fn generate_constructor_impls<'tcx>(
let is_src_local_adt = match src_ty.kind() {
ty::TyKind::Adt(adt_def, _) => db
.symbol_canonical_name(adt_def.did())
.is_none_or(|name| name.krate_num == db.source_crate_num()),
.map_or(true, |name| name.krate_num == db.source_crate_num()),
_ => false,
};
if is_src_local_adt {
Expand Down Expand Up @@ -2144,9 +2144,7 @@ pub fn adt_needs_bindings<'tcx>(
let tcx = db.tcx();
let attributes = crubit_attr::get_attrs(tcx, def_id).unwrap();

let Some(fully_qualified_name) = db.symbol_canonical_name(def_id) else {
bail!("No public path could be found for type {}", tcx.def_path_str(def_id));
};
let fully_qualified_name = db.symbol_canonical_name(def_id)?;
if let Some(cpp_type) = fully_qualified_name.unqualified.cpp_type {
let item_name = tcx.def_path_str(def_id);
bail!(
Expand Down Expand Up @@ -2178,9 +2176,7 @@ pub fn generate_generic_adt_declaration<'tcx>(
def_id: DefId,
) -> Result<ApiSnippets<'tcx>> {
let tcx = db.tcx();
let Some(fully_qualified_name) = db.symbol_canonical_name(def_id) else {
bail!("No public path could be found for type {}", tcx.def_path_str(def_id));
};
let fully_qualified_name = db.symbol_canonical_name(def_id)?;

let attributes = crubit_attr::get_attrs(tcx, def_id).unwrap_or_default();
if let Some(cpp_type) = fully_qualified_name.unqualified.cpp_type {
Expand Down Expand Up @@ -2269,11 +2265,7 @@ pub fn generate_adt_core<'tcx>(
crate::normalize_ty(tcx, tcx.param_env(def_id), tcx.type_of(def_id).instantiate_identity()),
);
assert!(self_ty.is_adt());
assert!(db.symbol_canonical_name(def_id).is_some(), "Caller should verify");

let Some(fully_qualified_name) = db.symbol_canonical_name(def_id) else {
bail!("`generate_adt_core` called on non-reachable type {}", tcx.def_path_str(def_id));
};
let fully_qualified_name = db.symbol_canonical_name(def_id)?;
let rs_fully_qualified_name = fully_qualified_name.format_for_rs();
let cpp_name = format_cc_ident(db, fully_qualified_name.unqualified.cpp_name.as_str())
.context("Error formatting item name")?;
Expand Down
4 changes: 2 additions & 2 deletions cc_bindings_from_rs/generate_bindings/generate_template_specialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,7 +1674,7 @@ fn append_explicit_trait_impls<'tcx>(
continue;
};
// Only bind implementations for supported ADTs.
let Some(canonical_name) = db.symbol_canonical_name(*did) else {
let Ok(canonical_name) = db.symbol_canonical_name(*did) else {
continue;
};
// We explicitly want to allow ADTs that specify cpp_type.
Expand Down Expand Up @@ -1719,7 +1719,7 @@ fn append_negative_auto_trait_impls<'tcx>(
}) {
continue;
}
let Some(canonical_name) = db.symbol_canonical_name(self_def_id) else {
let Ok(canonical_name) = db.symbol_canonical_name(self_def_id) else {
continue;
};
if canonical_name.krate_num != db.source_crate_num() {
Expand Down
46 changes: 24 additions & 22 deletions cc_bindings_from_rs/generate_bindings/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,7 @@ fn renamed_crate_original_name(db: &BindingsGenerator<'_>, krate_id: CrateNum) -
}

/// Implementation of `BindingsGenerator::symbol_canonical_name`.
fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Option<FullyQualifiedName> {
fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Result<FullyQualifiedName> {
let tcx = db.tcx();

// TODO: b/433286909 - We shouldn't pass DefKind::Use to this method and instead should keep what our use
Expand All @@ -908,12 +908,13 @@ fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Option<Fu
// Symbols that should not receive bindings should not have a canonical name, so that we do not
// attempt to bind other items that depend on them (functions that use them in their signature
// etc.).
if should_receive_bindings(db, def_id).is_err() {
return None;
}
should_receive_bindings(db, def_id)?;

let (full_path_strs, type_alias_def_id, krate_num) = {
let paths = db.all_public_paths_by_def_id().get(&def_id).cloned()?;
let paths =
db.all_public_paths_by_def_id().get(&def_id).cloned().ok_or_else(|| {
anyhow!("Not a public or a supported reexported type (b/262052635).")
})?;

// Select a canonical path for this symbol from available paths.
// Our paths are kept in sorted order, so the canonical path will be the first one.
Expand All @@ -938,7 +939,10 @@ fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Option<Fu
db.symbol_unqualified_name(alias_def_id)
}
})
.or_else(|| db.symbol_unqualified_name(def_id))?;
.or_else(|| db.symbol_unqualified_name(def_id))
.ok_or_else(|| {
anyhow!("Failed to get unqualified name for `{}`", tcx.def_path_str(def_id))
})?;

// `crate_name` gets the crate name written out in the rmeta file, which is not always the name
// we want to spell out in our generated bindings. Proto targets, for example, rename their crate
Expand All @@ -962,7 +966,7 @@ fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Option<Fu
// See https://github.com/rust-lang/rust/issues/144333 for details.
let path_strs: Vec<&str> = full_path_strs.iter().map(|x| &**x).collect();
if matches!(&*path_strs, ["dsl"]) {
return None;
bail!("Unsupported ambiguous re-export in polars_plan::dsl");
}
}

Expand All @@ -976,7 +980,7 @@ fn symbol_canonical_name(db: &BindingsGenerator<'_>, def_id: DefId) -> Option<Fu
use_leading_colons,
);
let cpp_top_level_ns = format_top_level_ns_for_crate(db, krate_num);
Some(FullyQualifiedName {
Ok(FullyQualifiedName {
krate,
krate_num,
cpp_top_level_ns,
Expand Down Expand Up @@ -1132,9 +1136,9 @@ fn generate_using<'tcx>(
bail!("Unable to `use` function whose bindings failed: {err:?}");
}
};
let fully_qualified_fn_name = db
.symbol_canonical_name(def_id)
.unwrap_or_else(|| panic!("Failed to get canonical name for {:?}", def_id));
let fully_qualified_fn_name = db.symbol_canonical_name(def_id).unwrap_or_else(|err| {
panic!("Failed to get canonical name for {:?}: {err}", def_id)
});
let formatted_fully_qualified_fn_name = fully_qualified_fn_name.format_for_cc(db)?;
let main_api_fn_name =
format_cc_ident(db, fully_qualified_fn_name.unqualified.cpp_name.as_str())
Expand Down Expand Up @@ -1378,7 +1382,7 @@ fn supported_traits(db: &BindingsGenerator<'_>) -> Rc<[DefId]> {
.visible_traits()
.filter(|trait_id| {
// Does the trait get bindings?
db.symbol_canonical_name(*trait_id).is_some()
db.symbol_canonical_name(*trait_id).is_ok()
})
.filter(|trait_id| {
// Traits do not support const generics.
Expand Down Expand Up @@ -1536,9 +1540,7 @@ fn create_type_alias<'tcx>(
alias_name: &str,
alias_type: Ty<'tcx>,
) -> Result<CcSnippet<'tcx>> {
let fully_qualified_name = db
.symbol_canonical_name(def_id)
.ok_or_else(|| anyhow!("Failed to get canonical name for {:?}", def_id))?;
let fully_qualified_name = db.symbol_canonical_name(def_id)?;
let rs_type = format!("{}", fully_qualified_name.format_for_rs());
create_type_alias_with_rs_type(db, def_id, &rs_type, alias_name, alias_type)
}
Expand Down Expand Up @@ -1783,7 +1785,7 @@ fn copy_codegen_style_to_snippets<'tcx>(
None => {
let display_name = core
.def_id
.and_then(|id| db.symbol_canonical_name(id))
.and_then(|id| db.symbol_canonical_name(id).ok())
.map(|canon| {
let parts =
canon.rs_name_parts().map(|s| format!("{}", s)).collect::<Vec<_>>();
Expand Down Expand Up @@ -2200,7 +2202,7 @@ fn generate_item_impl<'tcx>(
def_id: DefId,
) -> Result<Option<ApiSnippets<'tcx>>> {
let tcx = db.tcx();
if db.symbol_canonical_name(def_id).is_none() {
if db.symbol_canonical_name(def_id).is_err() {
return Ok(None);
};
let item = match tcx.def_kind(def_id) {
Expand Down Expand Up @@ -2456,7 +2458,7 @@ fn formatted_items_in_crate<'tcx>(
.into_iter()
.filter_map(|(def_id, paths)| {
let mut snippets = None;
let canonical_name = db.symbol_canonical_name(def_id)?;
let canonical_name = db.symbol_canonical_name(def_id).ok()?;
let aliases = if canonical_name.krate_num == db.source_crate_num() {
// We only want to call `generate_item` on DefIds from our source crate. External
// crate DefIds might appear in this map if our crate re-exports them, but we don't
Expand Down Expand Up @@ -2613,9 +2615,9 @@ fn generate_crate(db: &BindingsGenerator) -> Result<BindingsTokens> {
cc_details.push(CcDetails::new(
def_id,
db.symbol_canonical_name(def_id)
.unwrap_or_else(|| {
.unwrap_or_else(|err| {
panic!(
"Exported item {} should have a canonical name",
"Exported item {} should have a canonical name: {err}",
db.tcx().def_path_str(def_id)
)
})
Expand Down Expand Up @@ -2784,9 +2786,9 @@ fn generate_crate(db: &BindingsGenerator) -> Result<BindingsTokens> {
NamespaceQualifier::new(
cpp_top_level_ns.iter().cloned().chain({
db.symbol_canonical_name(def_id)
.unwrap_or_else(|| {
.unwrap_or_else(|err| {
panic!(
"Exported item {} should have a canonical name",
"Exported item {} should have a canonical name: {err}",
tcx.def_path_str(def_id),
)
})
Expand Down
Loading