Skip to content
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

Further name tweaks #1437

Merged
merged 6 commits into from
Feb 10, 2025
Merged

Further name tweaks #1437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7689cfa
Investigate a couple of FIXMEs.
adetaylor Feb 10, 2025
2b1ba93
Fix one more FIXME.
adetaylor Feb 10, 2025
69f484f
Improve diagnostic display name.
adetaylor Feb 10, 2025
a084090
More name fixes.
adetaylor Feb 10, 2025
067ed2d
Cease to rely on API name for function codegen.
adetaylor Feb 10, 2025
52a8430
Clippy fix.
adetaylor Feb 10, 2025
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
2 changes: 2 additions & 0 deletions engine/src/conversion/analysis/fun/function_wrapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,8 @@ pub(crate) enum CppFunctionKind {
pub(crate) struct CppFunction {
pub(crate) payload: CppFunctionBody,
pub(crate) wrapper_function_name: crate::minisyn::Ident,
/// The following field is apparently only used for
/// subclass calls.
pub(crate) original_cpp_name: CppEffectiveName,
pub(crate) return_conversion: Option<TypeConversionPolicy>,
pub(crate) argument_conversion: Vec<TypeConversionPolicy>,
Expand Down
109 changes: 64 additions & 45 deletions engine/src/conversion/analysis/fun/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ pub(crate) struct FnAnalysis {
pub(crate) cxxbridge_name: crate::minisyn::Ident,
/// ... so record also the name under which we wish to expose it in Rust.
pub(crate) rust_name: String,
/// And also the name of the underlying C++ function if it differs
/// from the cxxbridge_name.
pub(crate) cpp_call_name: Option<CppOriginalName>,
pub(crate) rust_rename_strategy: RustRenameStrategy,
pub(crate) params: Punctuated<FnArg, Comma>,
pub(crate) kind: FnKind,
Expand Down Expand Up @@ -722,21 +725,19 @@ impl<'a> FnAnalyzer<'a> {
sophistication: TypeConversionSophistication,
predetermined_rust_name: Option<String>,
) -> (FnAnalysis, ApiName) {
let mut cpp_name = name
.cpp_name_if_present()
.cloned()
.map(|n| n.to_effective_name());
let cpp_original_name = name.cpp_name_if_present();
let ns = name.name.get_namespace();

// Let's gather some pre-wisdom about the name of the function.
// We're shortly going to plunge into analyzing the parameters,
// and it would be nice to have some idea of the function name
// for diagnostics whilst we do that.
let initial_rust_name = fun.ident.to_string();
let diagnostic_display_name = cpp_name
let diagnostic_name = cpp_original_name
.as_ref()
.map(|n| n.diagnostic_display_name())
.unwrap_or(&initial_rust_name);
let diagnostic_name = QualifiedName::new(ns, make_ident(diagnostic_name));

// Now let's analyze all the parameters.
// See if any have annotations which our fork of bindgen has craftily inserted...
Expand All @@ -747,7 +748,7 @@ impl<'a> FnAnalyzer<'a> {
self.convert_fn_arg(
i,
ns,
diagnostic_display_name,
&diagnostic_name,
&fun.synthesized_this_type,
&fun.references,
true,
Expand Down Expand Up @@ -788,15 +789,15 @@ impl<'a> FnAnalyzer<'a> {
// method, IRN=A_foo, CN=foo output: foo case 4
// method, IRN=A_move, CN=move (keyword problem) output: move_ case 5
// method, IRN=A_foo1, CN=foo (overload) output: foo case 6
let ideal_rust_name = match &cpp_name {
let ideal_rust_name = match cpp_original_name {
None => initial_rust_name, // case 1
Some(cpp_name) => {
Some(cpp_original_name) => {
if initial_rust_name.ends_with('_') {
initial_rust_name // case 2
} else if validate_ident_ok_for_rust(cpp_name).is_err() {
format!("{}_", cpp_name.to_string_for_rust_name()) // case 5
} else if validate_ident_ok_for_rust(cpp_original_name).is_err() {
format!("{}_", cpp_original_name.to_string_for_rust_name()) // case 5
} else {
cpp_name.to_string_for_rust_name() // cases 3, 4, 6
cpp_original_name.to_string_for_rust_name() // cases 3, 4, 6
}
}
};
Expand Down Expand Up @@ -1018,7 +1019,7 @@ impl<'a> FnAnalyzer<'a> {
0,
fun,
ns,
&rust_name,
&diagnostic_name,
&mut params,
&mut param_details,
None,
Expand All @@ -1037,7 +1038,7 @@ impl<'a> FnAnalyzer<'a> {
0,
fun,
ns,
&rust_name,
&diagnostic_name,
&mut params,
&mut param_details,
Some(RustConversionType::FromTypeToPtr),
Expand All @@ -1061,7 +1062,7 @@ impl<'a> FnAnalyzer<'a> {
0,
fun,
ns,
&rust_name,
&diagnostic_name,
&mut params,
&mut param_details,
Some(RustConversionType::FromPinMaybeUninitToPtr),
Expand All @@ -1086,7 +1087,7 @@ impl<'a> FnAnalyzer<'a> {
0,
fun,
ns,
&rust_name,
&diagnostic_name,
&mut params,
&mut param_details,
Some(RustConversionType::FromPinMaybeUninitToPtr),
Expand All @@ -1099,7 +1100,7 @@ impl<'a> FnAnalyzer<'a> {
1,
fun,
ns,
&rust_name,
&diagnostic_name,
&mut params,
&mut param_details,
Some(RustConversionType::FromPinMoveRefToPtr),
Expand Down Expand Up @@ -1192,15 +1193,30 @@ impl<'a> FnAnalyzer<'a> {
&rust_name,
ns,
);
if cxxbridge_name != rust_name && cpp_name.is_none() {
cpp_name = Some(CppEffectiveName::from_rust_name(rust_name.clone()));
}

let api_name_cpp_override = match cpp_original_name {
Some(name) => Some(name.clone()),
None if cxxbridge_name != rust_name => {
Some(CppOriginalName::from_rust_name(rust_name.clone()))
}
None => None,
};
let underlying_cpp_function_name = cpp_original_name
.cloned()
.map(|n| n.to_effective_name())
.unwrap_or_else(|| CppEffectiveName::from_rust_name(rust_name.clone()));
let mut cxxbridge_name = make_ident(&cxxbridge_name);

// Analyze the return type, just as we previously did for the
// parameters.
let mut return_analysis = self
.convert_return_type(&fun.output, ns, &fun.references, sophistication)
.convert_return_type(
&fun.output,
ns,
&diagnostic_name,
&fun.references,
sophistication,
)
.unwrap_or_else(|err| {
set_ignore_reason(err);
ReturnTypeAnalysis::default()
Expand Down Expand Up @@ -1272,10 +1288,9 @@ impl<'a> FnAnalyzer<'a> {
.unwrap_or_default();

// See https://github.com/dtolnay/cxx/issues/878 for the reason for this next line.
let effective_cpp_name =
CppEffectiveName::from_cpp_name_and_rust_name(cpp_name.as_ref(), &rust_name);
let cpp_name_incompatible_with_cxx =
validate_ident_ok_for_rust(&effective_cpp_name).is_err();
let cpp_name_incompatible_with_cxx = cpp_original_name
.map(|n| validate_ident_ok_for_rust(n).is_err())
.unwrap_or_default();
// If possible, we'll put knowledge of the C++ API directly into the cxx::bridge
// mod. However, there are various circumstances where cxx can't work with the existing
// C++ API and we need to create a C++ wrapper function which is more cxx-compliant.
Expand Down Expand Up @@ -1350,16 +1365,16 @@ impl<'a> FnAnalyzer<'a> {
CppFunctionBody::StaticMethodCall(
ns.clone(),
impl_for.get_final_ident(),
effective_cpp_name,
underlying_cpp_function_name,
),
CppFunctionKind::Function,
),
FnKind::Method { .. } => (
CppFunctionBody::FunctionCall(ns.clone(), effective_cpp_name),
CppFunctionBody::FunctionCall(ns.clone(), underlying_cpp_function_name),
CppFunctionKind::Method,
),
_ => (
CppFunctionBody::FunctionCall(ns.clone(), effective_cpp_name),
CppFunctionBody::FunctionCall(ns.clone(), underlying_cpp_function_name),
CppFunctionKind::Function,
),
},
Expand Down Expand Up @@ -1388,12 +1403,15 @@ impl<'a> FnAnalyzer<'a> {
));
}

let original_cpp_name = cpp_original_name
.cloned()
.map(|n| n.to_effective_name())
.unwrap_or_else(|| CppEffectiveName::from_cxxbridge_name(&cxxbridge_name));

Some(CppFunction {
payload,
wrapper_function_name: cxxbridge_name.clone(),
original_cpp_name: cpp_name.clone().unwrap_or_else(|| {
CppEffectiveName::from_cxxbridge_name(cxxbridge_name.clone())
}),
original_cpp_name,
return_conversion: ret_type_conversion.clone(),
argument_conversion: param_details.iter().map(|d| d.conversion.clone()).collect(),
kind: cpp_function_kind,
Expand Down Expand Up @@ -1435,6 +1453,7 @@ impl<'a> FnAnalyzer<'a> {
let analysis = FnAnalysis {
cxxbridge_name: cxxbridge_name.clone(),
rust_name: rust_name.clone(),
cpp_call_name: api_name_cpp_override,
rust_rename_strategy,
params,
ret_conversion: ret_type_conversion,
Expand All @@ -1449,11 +1468,11 @@ impl<'a> FnAnalyzer<'a> {
externally_callable,
rust_wrapper_needed,
};
let name = ApiName::new_with_cpp_name(
ns,
cxxbridge_name,
cpp_name.map(CppOriginalName::from_effective_name),
);
// For everything other than functions, the API name is immutable.
// It would be nice to get to that point with functions, but at present
// the API name is used in Rust codegen to generate "use" statements,
// so we override it.
let name = ApiName::new_with_cpp_name(ns, cxxbridge_name, cpp_original_name.cloned());
(analysis, name)
}

Expand All @@ -1478,7 +1497,7 @@ impl<'a> FnAnalyzer<'a> {
param_idx: usize,
fun: &FuncToConvert,
ns: &Namespace,
rust_name: &str,
diagnostic_name: &QualifiedName,
params: &mut Punctuated<FnArg, Comma>,
param_details: &mut [ArgumentAnalysis],
force_rust_conversion: Option<RustConversionType>,
Expand All @@ -1489,7 +1508,7 @@ impl<'a> FnAnalyzer<'a> {
self.convert_fn_arg(
fun.inputs.iter().nth(param_idx).unwrap(),
ns,
rust_name,
diagnostic_name,
&fun.synthesized_this_type,
&fun.references,
false,
Expand Down Expand Up @@ -1645,7 +1664,7 @@ impl<'a> FnAnalyzer<'a> {
&mut self,
arg: &FnArg,
ns: &Namespace,
fn_name: &str,
diagnostic_name: &QualifiedName,
virtual_this: &Option<QualifiedName>,
references: &References,
treat_this_as_reference: bool,
Expand Down Expand Up @@ -1691,13 +1710,12 @@ impl<'a> FnAnalyzer<'a> {
Ok((this_type, receiver_mutability))
}
_ => Err(ConvertErrorFromCpp::UnexpectedThisType(
QualifiedName::new(ns, make_ident(fn_name)),
diagnostic_name.clone(),
)),
},
_ => Err(ConvertErrorFromCpp::UnexpectedThisType(QualifiedName::new(
ns,
make_ident(fn_name),
))),
_ => Err(ConvertErrorFromCpp::UnexpectedThisType(
diagnostic_name.clone(),
)),
}?;
self_type = Some(this_type);
is_placement_return_destination = construct_into_self;
Expand Down Expand Up @@ -1924,6 +1942,7 @@ impl<'a> FnAnalyzer<'a> {
&mut self,
rt: &ReturnType,
ns: &Namespace,
diagnostic_name: &QualifiedName,
references: &References,
sophistication: TypeConversionSophistication,
) -> Result<ReturnTypeAnalysis, ConvertErrorFromCpp> {
Expand Down Expand Up @@ -1955,7 +1974,7 @@ impl<'a> FnAnalyzer<'a> {
let (fnarg, analysis) = self.convert_fn_arg(
&fnarg,
ns,
"",
diagnostic_name,
&None,
&References::default(),
false,
Expand Down Expand Up @@ -2148,7 +2167,7 @@ impl<'a> FnAnalyzer<'a> {
.get(&self_ty.name)
.cloned()
.or_else(|| Some(self_ty.name.get_final_item().to_string()))
.map(CppOriginalName::from_string_for_constructor)
.map(CppOriginalName::from_type_name_for_constructor)
} else {
None
};
Expand Down
7 changes: 4 additions & 3 deletions engine/src/conversion/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,17 +359,18 @@ impl ApiName {
Self::new(&Namespace::new(), id)
}

/// Return the C++ name to use here, which will use the Rust
/// name unless it's been overridden by a C++ name.
pub(crate) fn cpp_name(&self) -> CppEffectiveName {
CppEffectiveName::from_api_details(&self.cpp_name, &self.name)
}

pub(crate) fn qualified_cpp_name(&self) -> String {
let cpp_name = self.cpp_name();
self.name
.ns_segment_iter()
.chain(std::iter::once(
cpp_name
.to_string_to_make_qualified_name()
self.cpp_name()
.to_string_for_cpp_generation()
.to_string()
.as_str(),
))
Expand Down
14 changes: 8 additions & 6 deletions engine/src/conversion/codegen_rs/fun_codegen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ use crate::{
MethodKind, RustRenameStrategy, TraitMethodDetails,
},
api::UnsafetyNeeded,
CppEffectiveName,
},
minisyn::minisynize_vec,
types::{Namespace, QualifiedName},
Expand Down Expand Up @@ -89,14 +88,14 @@ pub(super) fn gen_function(
ns: &Namespace,
fun: FuncToConvert,
analysis: FnAnalysis,
cpp_call_name: CppEffectiveName,
non_pod_types: &HashSet<QualifiedName>,
) -> RsCodegenResult {
if analysis.ignore_reason.is_err() || !analysis.externally_callable {
return RsCodegenResult::default();
}
let cxxbridge_name = analysis.cxxbridge_name;
let rust_name = &analysis.rust_name;
let cpp_call_name = &analysis.cpp_call_name;
let ret_type = analysis.ret_type;
let ret_conversion = analysis.ret_conversion;
let param_details = analysis.param_details;
Expand Down Expand Up @@ -177,10 +176,13 @@ pub(super) fn gen_function(
_ => Some(Use::UsedFromCxxBridge),
},
};
if cpp_call_name.does_not_match_cxxbridge_name(&cxxbridge_name) && !wrapper_function_needed {
cpp_name_attr = Attribute::parse_outer
.parse2(cpp_call_name.generate_cxxbridge_name_attribute())
.unwrap();
if let Some(cpp_call_name) = cpp_call_name {
if cpp_call_name.does_not_match_cxxbridge_name(&cxxbridge_name) && !wrapper_function_needed
{
cpp_name_attr = Attribute::parse_outer
.parse2(cpp_call_name.generate_cxxbridge_name_attribute())
.unwrap();
}
}

// Finally - namespace support. All the Types in everything
Expand Down
11 changes: 3 additions & 8 deletions engine/src/conversion/codegen_rs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,6 @@ impl<'a> RsCodeGenerator<'a> {
) -> RsCodegenResult {
let name = api.name().clone();
let id = name.get_final_ident();
let cpp_call_name = api.effective_cpp_name();
match api {
Api::StringConstructor { .. } => {
let make_string_name = make_ident(self.config.get_makestring_name());
Expand All @@ -479,13 +478,9 @@ impl<'a> RsCodeGenerator<'a> {
..Default::default()
}
}
Api::Function { fun, analysis, .. } => gen_function(
name.get_namespace(),
*fun,
analysis,
cpp_call_name,
non_pod_types,
),
Api::Function { fun, analysis, .. } => {
gen_function(name.get_namespace(), *fun, analysis, non_pod_types)
}
Api::Const { const_item, .. } => RsCodegenResult {
bindgen_mod_items: vec![Item::Const(const_item.into())],
materializations: vec![Use::UsedFromBindgen],
Expand Down
Loading
Loading