Skip to content

Commit

Permalink
Cease to modify bindgen output.
Browse files Browse the repository at this point in the history
This is a major change to the function of autocxx. Previously, it ran bindgen,
edited the output, appended cxx instructions, and packaged the results in a
hierarchy of mods for the user.

With this change, we NO LONGER edit the bindgen output. That's passed onto
the user as-is. We simply augment it with cxx wrappers.

This has pros and cons. Pros:
* It makes the function of autocxx conceptually easier to understand.
* We're more likely to be able to accommodate upstream bindgen changes
  without falling over. Similarly, as bindgen improves, we should
  automatically pass those improvements onto autocxx users.
* With subsequent cleanups, it should reduce the complexity of autocxx.

Cons:
* We're more sensitive to bugs within bindgen. Four tests have been
  disabled because of bindgen bugs (which have been reported upstream).
* We are unable to hide some of the original bindgen methods attached
  to types; users should not normally call them.

This is a compatibility break in two known respects:
* A wider selection of bindgen issues may be passed on, so this might
  break autocxx for some users.
* For C++ functions that were Rust keywords (e.g. "async", "move")
  previously it was necessary to give their name with a _ in the
  `generate!()` directive, e.g. `generate!("async_")`. This is no
  longer the case (which is an improvement but nevertheless a
  compatibility break).

This code needs quite a bit of cleanup.

Fixes #1450.
  • Loading branch information
adetaylor committed Feb 26, 2025
1 parent de89fd9 commit abf08cc
Show file tree
Hide file tree
Showing 21 changed files with 365 additions and 480 deletions.
38 changes: 36 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ log = "0.4"
proc-macro2 = "1.0.11"
quote = "1.0"
indoc = "1.0"
autocxx-bindgen = { version = "=0.72.0", default-features = false, features = ["logging", "which-rustfmt"] }
#autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "switch-from-attributes-to-callbacks", default-features = false, features = ["logging", "which-rustfmt"] }
#autocxx-bindgen = { version = "=0.72.0", default-features = false, features = ["logging", "which-rustfmt"] }
autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "all-modules-raw-line", default-features = false, features = ["logging", "which-rustfmt"] }
itertools = "0.10.3"
cc = { version = "1.0", optional = true }
# Note: Keep the patch-level version of cxx-gen and cxx in sync.
Expand All @@ -47,6 +47,7 @@ serde_json = { version = "1.0", optional = true }
miette = "5"
thiserror = "1"
regex = "1.5"
regex_static = "0.1"
indexmap = "1.8"
prettyplease = { version = "0.2.6", features = ["verbatim"] }
rustversion = "1.0"
Expand Down
2 changes: 1 addition & 1 deletion engine/src/conversion/analysis/constructor_deps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ fn decorate_struct(
) -> Result<Box<dyn Iterator<Item = Api<FnPhase>>>, ConvertErrorWithContext> {
let pod = fn_struct.pod;
let is_abstract = matches!(pod.kind, TypeKind::Abstract);
let constructor_and_allocator_deps = if is_abstract || pod.is_generic {
let constructor_and_allocator_deps = if is_abstract || pod.num_generics > 0 {
Vec::new()
} else {
constructors_and_allocators_by_type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ pub(super) fn find_constructors_present(
PodAnalysis {
bases,
field_info,
is_generic: false,
num_generics: 0usize,
in_anonymous_namespace: false,
..
},
Expand Down
11 changes: 4 additions & 7 deletions engine/src/conversion/analysis/fun/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,12 +394,9 @@ impl<'a> FnAnalyzer<'a> {
apis.iter()
.filter_map(|api| match api {
Api::Struct {
analysis:
PodAnalysis {
is_generic: true, ..
},
analysis: PodAnalysis { num_generics, .. },
..
} => Some(api.name().clone()),
} if *num_generics > 0 => Some(api.name().clone()),
_ => None,
})
.collect()
Expand Down Expand Up @@ -2224,11 +2221,11 @@ fn constructor_with_suffix<'a>(rust_name: &'a str, nested_type_ident: &str) -> O
impl Api<FnPhase> {
pub(crate) fn name_for_allowlist(&self) -> QualifiedName {
match &self {
Api::Function { analysis, .. } => match analysis.kind {
Api::Function { fun, analysis, .. } => match analysis.kind {
FnKind::Method { ref impl_for, .. } => impl_for.clone(),
FnKind::TraitMethod { ref impl_for, .. } => impl_for.clone(),
FnKind::Function => {
QualifiedName::new(self.name().get_namespace(), make_ident(&analysis.rust_name))
QualifiedName::new(self.name().get_namespace(), fun.ident.clone())
}
},
Api::RustSubclassFn { subclass, .. } => subclass.0.name.clone(),
Expand Down
2 changes: 1 addition & 1 deletion engine/src/conversion/analysis/pod/byvalue_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl ByValueChecker {
}
_ => None,
},
TypedefKind::Use(_, ref ty) => match **ty {
TypedefKind::Use(ref ty) => match **ty {
crate::minisyn::Type(Type::Path(ref typ)) => {
let target_tn = QualifiedName::from_type_path(typ);
known_types().consider_substitution(&target_tn)
Expand Down
6 changes: 3 additions & 3 deletions engine/src/conversion/analysis/pod/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub(crate) struct PodAnalysis {
/// std::unique_ptr<A> it would just be std::unique_ptr.
pub(crate) field_definition_deps: HashSet<QualifiedName>,
pub(crate) field_info: Vec<FieldInfo>,
pub(crate) is_generic: bool,
pub(crate) num_generics: usize,
pub(crate) in_anonymous_namespace: bool,
}

Expand Down Expand Up @@ -188,7 +188,7 @@ fn analyze_struct(
.filter(|base| config.is_on_allowlist(&base.to_cpp_name()))
.cloned()
.collect();
let is_generic = !details.item.generics.params.is_empty();
let num_generics = details.item.generics.params.len();
let in_anonymous_namespace = name
.name
.ns_segment_iter()
Expand All @@ -203,7 +203,7 @@ fn analyze_struct(
field_deps,
field_definition_deps,
field_info,
is_generic,
num_generics,
in_anonymous_namespace,
},
})))
Expand Down
22 changes: 17 additions & 5 deletions engine/src/conversion/analysis/type_converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ impl<'a> TypeConverter<'a> {
) -> Result<Annotated<Type>, ConvertErrorFromCpp> {
// First we try to spot if these are the special marker paths that
// bindgen uses to denote references or other things.
// TODO the next two lines can be removed
if let Some(ty) = unwrap_has_unused_template_param(&typ) {
self.convert_type(ty.clone(), ns, ctx)
} else if let Some(ty) = unwrap_has_opaque(&typ) {
Expand Down Expand Up @@ -277,7 +278,8 @@ impl<'a> TypeConverter<'a> {
ctx: &TypeConversionContext,
) -> Result<Annotated<Type>, ConvertErrorFromCpp> {
// First, qualify any unqualified paths.
if typ.path.segments.iter().next().unwrap().ident != "root" {
let first_seg = &typ.path.segments.iter().next().unwrap().ident;
if first_seg != "root" && first_seg != "output" {
let ty = QualifiedName::from_type_path(&typ);
// If the type looks like it is unqualified, check we know it
// already, and if not, qualify it according to the current
Expand All @@ -289,7 +291,7 @@ impl<'a> TypeConverter<'a> {
return Err(ConvertErrorFromCpp::UnsupportedBuiltInType(ty));
}
if !self.types_found.contains(&ty) {
typ.path.segments = std::iter::once("root")
typ.path.segments = std::iter::once("output")
.chain(ns.iter())
.map(|s| {
let i = make_ident(s);
Expand All @@ -313,7 +315,7 @@ impl<'a> TypeConverter<'a> {
// Now convert this type itself.
deps.insert(original_tn.clone());
// First let's see if this is a typedef.
let (typ, tn) = match self.resolve_typedef(&original_tn)? {
let (mut typ, tn) = match self.resolve_typedef(&original_tn)? {
None => (typ, original_tn),
Some(Type::Path(resolved_tp)) => {
let resolved_tn = QualifiedName::from_type_path(resolved_tp);
Expand Down Expand Up @@ -350,7 +352,17 @@ impl<'a> TypeConverter<'a> {
}
substitute_type
}
None => typ,
None => {
// Pass through as-is, but replacing `root` with `output`
// in the first path element. We don't just create a whole
// new `TypePath` because that would discard any generics.
if let Some(first_seg) = typ.path.segments.get_mut(0) {
if first_seg.ident == "root" {
first_seg.ident = make_ident("output").0;
}
}
typ
}
};

let mut extra_apis = ApiVec::new();
Expand Down Expand Up @@ -726,7 +738,7 @@ impl TypedefTarget for TypedefAnalysis {
fn get_target(&self) -> Option<&Type> {
Some(match self.kind {
TypedefKind::Type(ref ty) => &ty.ty,
TypedefKind::Use(_, ref ty) => ty,
TypedefKind::Use(ref ty) => ty,
})
}
}
Expand Down
9 changes: 4 additions & 5 deletions engine/src/conversion/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use autocxx_bindgen::callbacks::{Explicitness, Layout, SpecialMemberKind, Virtualness};
use autocxx_bindgen::callbacks::{Explicitness, SpecialMemberKind, Virtualness};

use syn::{
punctuated::Punctuated,
Expand All @@ -16,8 +16,8 @@ use syn::{
use crate::types::{make_ident, Namespace, QualifiedName};
use crate::{
minisyn::{
Attribute, FnArg, Ident, ItemConst, ItemEnum, ItemStruct, ItemType, ItemUse, Pat,
ReturnType, Type, Visibility,
Attribute, FnArg, Ident, ItemConst, ItemEnum, ItemStruct, ItemType, Pat, ReturnType, Type,
Visibility,
},
parse_callbacks::CppOriginalName,
};
Expand Down Expand Up @@ -51,7 +51,6 @@ pub(crate) enum TypeKind {
#[derive(Debug)]
pub(crate) struct StructDetails {
pub(crate) item: ItemStruct,
pub(crate) layout: Option<Layout>,
pub(crate) has_rvalue_reference_fields: bool,
}

Expand Down Expand Up @@ -204,7 +203,7 @@ impl AnalysisPhase for NullPhase {

#[derive(Clone, Debug)]
pub(crate) enum TypedefKind {
Use(ItemUse, Box<Type>),
Use(Box<Type>),
Type(ItemType),
}

Expand Down
16 changes: 11 additions & 5 deletions engine/src/conversion/codegen_rs/fun_codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ pub(super) fn gen_function(
let mut cpp_name_attr = Vec::new();
let mut impl_entry = None;
let mut trait_impl_entry = None;
let mut bindgen_mod_items = Vec::new();
let always_unsafe_due_to_trait_definition = match kind {
FnKind::TraitMethod { ref details, .. } => details.trait_call_is_unsafe,
_ => false,
Expand All @@ -133,6 +132,8 @@ pub(super) fn gen_function(
&ret_conversion,
);

let mut materializations = Vec::new();

if analysis.rust_wrapper_needed {
match kind {
FnKind::Method {
Expand All @@ -159,23 +160,29 @@ pub(super) fn gen_function(
}
_ => {
// Generate plain old function
bindgen_mod_items.push(fn_generator.generate_function_impl());
materializations.push(Use::Custom(Box::new(fn_generator.generate_function_impl())));
}
}
}

// This and the 'if let' can be simplified. TODO.
let materialization = match kind {
FnKind::Method { .. } | FnKind::TraitMethod { .. } => None,
FnKind::Function => match analysis.rust_rename_strategy {
_ if analysis.rust_wrapper_needed => {
Some(Use::SpecificNameFromBindgen(make_ident(rust_name).into()))
assert!(!materializations.is_empty());
None
}
RustRenameStrategy::RenameInOutputMod(ref alias) => {
Some(Use::UsedFromCxxBridgeWithAlias(alias.clone().into()))
}
_ => Some(Use::UsedFromCxxBridge),
},
};
if let Some(materialization) = materialization {
materializations.push(materialization);
}

if let Some(cpp_call_name) = cpp_call_name {
if cpp_call_name.does_not_match_cxxbridge_name(&cxxbridge_name) && !wrapper_function_needed
{
Expand Down Expand Up @@ -217,10 +224,9 @@ pub(super) fn gen_function(
));
RsCodegenResult {
extern_c_mod_items: vec![extern_c_mod_item],
bindgen_mod_items,
impl_entry,
trait_impl_entry,
materializations: materialization.into_iter().collect(),
materializations,
..Default::default()
}
}
Expand Down
2 changes: 1 addition & 1 deletion engine/src/conversion/codegen_rs/function_wrapper_rs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl TypeConversionPolicy {
let holder_type = sub.holder();
let id = sub.id();
let ty = parse_quote! { autocxx::subclass::CppSubclassRustPeerHolder<
super::super::super:: #id>
super::super:: #id>
};
RustParamConversion::Param {
ty,
Expand Down
Loading

0 comments on commit abf08cc

Please sign in to comment.