Skip to content

Commit

Permalink
Merge pull request #1264 from google/reject-unions
Browse files Browse the repository at this point in the history 
Fix namespaced destructors and reject unions
  • Loading branch information
@adetaylor
adetaylor authored Apr 14, 2023
2 parents b0275ff + a44f398 commit bec2301
Show file tree
Hide file tree
Showing 8 changed files with 236 additions and 134 deletions.
6 changes: 6 additions & 0 deletions engine/src/conversion/analysis/pod/byvalue_checker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ impl ByValueChecker {
let fieldlist = Self::get_field_types(def);
for ty_id in &fieldlist {
match self.results.get(ty_id) {
None if ty_id.get_final_item() == "__BindgenUnionField" => {
field_safety_problem = PodState::UnsafeToBePod(format!(
"Type {tyname} could not be POD because it is a union"
));
break;
}
None => {
field_safety_problem = PodState::UnsafeToBePod(format!(
"Type {tyname} could not be POD because its dependent type {ty_id} isn't known"
Expand Down
6 changes: 4 additions & 2 deletions engine/src/conversion/analysis/type_converter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
conversion::{
api::{AnalysisPhase, Api, ApiName, NullPhase, TypedefKind, UnanalyzedApi},
apivec::ApiVec,
codegen_cpp::type_to_cpp::type_to_cpp,
codegen_cpp::type_to_cpp::CppNameMap,
ConvertErrorFromCpp,
},
known_types::{known_types, CxxGenericType},
Expand Down Expand Up @@ -130,6 +130,7 @@ pub(crate) struct TypeConverter<'a> {
forward_declarations: HashSet<QualifiedName>,
ignored_types: HashSet<QualifiedName>,
config: &'a IncludeCppConfig,
original_name_map: CppNameMap,
}

impl<'a> TypeConverter<'a> {
Expand All @@ -144,6 +145,7 @@ impl<'a> TypeConverter<'a> {
forward_declarations: Self::find_incomplete_types(apis),
ignored_types: Self::find_ignored_types(apis),
config,
original_name_map: CppNameMap::new_from_apis(apis),
}
}

Expand Down Expand Up @@ -525,7 +527,7 @@ impl<'a> TypeConverter<'a> {
// We just use this as a hash key, essentially.
// TODO: Once we've completed the TypeConverter refactoring (see #220),
// pass in an actual original_name_map here.
let cpp_definition = type_to_cpp(rs_definition, &HashMap::new())?;
let cpp_definition = self.original_name_map.type_to_cpp(rs_definition)?;
let e = self.concrete_templates.get(&cpp_definition);
match e {
Some(tn) => Ok((tn.clone(), None)),
Expand Down
6 changes: 3 additions & 3 deletions engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::conversion::{
ConvertErrorFromCpp,
};

use super::type_to_cpp::{type_to_cpp, CppNameMap};
use super::type_to_cpp::CppNameMap;

impl TypeConversionPolicy {
pub(super) fn unconverted_type(
Expand Down Expand Up @@ -49,7 +49,7 @@ impl TypeConversionPolicy {
Ok(format!(
"{}{}*",
const_string,
type_to_cpp(ty, cpp_name_map)?
cpp_name_map.type_to_cpp(ty)?
))
}
_ => self.unwrapped_type_as_string(cpp_name_map),
Expand All @@ -60,7 +60,7 @@ impl TypeConversionPolicy {
&self,
cpp_name_map: &CppNameMap,
) -> Result<String, ConvertErrorFromCpp> {
type_to_cpp(self.cxxbridge_type(), cpp_name_map)
cpp_name_map.type_to_cpp(self.cxxbridge_type())
}

pub(crate) fn is_a_pointer(&self) -> Pointerness {
Expand Down
45 changes: 33 additions & 12 deletions engine/src/conversion/codegen_cpp/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@ use indexmap::map::IndexMap as HashMap;
use indexmap::set::IndexSet as HashSet;
use itertools::Itertools;
use std::borrow::Cow;
use type_to_cpp::{original_name_map_from_apis, type_to_cpp, CppNameMap};

use self::type_to_cpp::{
final_ident_using_original_name_map, namespaced_name_using_original_name_map,
};
use type_to_cpp::CppNameMap;

use super::{
analysis::{
Expand Down Expand Up @@ -124,7 +120,7 @@ impl<'a> CppCodeGenerator<'a> {
let mut gen = CppCodeGenerator {
additional_functions: Vec::new(),
inclusions,
original_name_map: original_name_map_from_apis(apis),
original_name_map: CppNameMap::new_from_apis(apis),
config,
cpp_codegen_options,
cxxgen_header_name,
Expand Down Expand Up @@ -172,7 +168,7 @@ impl<'a> CppCodeGenerator<'a> {
} => {
let effective_cpp_definition = match rs_definition {
Some(rs_definition) => {
Cow::Owned(type_to_cpp(rs_definition, &self.original_name_map)?)
Cow::Owned(self.original_name_map.type_to_cpp(rs_definition)?)
}
None => Cow::Borrowed(cpp_definition),
};
Expand Down Expand Up @@ -486,9 +482,34 @@ impl<'a> CppCodeGenerator<'a> {
)
}
CppFunctionBody::Destructor(ns, id) => {
let ty_id = QualifiedName::new(ns, id.clone());
let ty_id = final_ident_using_original_name_map(&ty_id, &self.original_name_map);
(format!("{arg_list}->~{ty_id}()"), "".to_string(), false)
let full_name = QualifiedName::new(ns, id.clone());
let ty_id = self.original_name_map.get_final_item(&full_name);
let is_a_nested_struct = self.original_name_map.get(&full_name).is_some();
// This is all super duper fiddly.
// All we want to do is call a destructor. Constraints:
// * an unnamed struct, e.g. typedef struct { .. } A, does not
// have any way of fully qualifying its destructor name.
// We have to use a 'using' statement.
// * we don't get enough information from bindgen to distinguish
// typedef struct { .. } A // unnamed struct
// from
// struct A { .. } // named struct
// * we can only do 'using A::B::C' if 'B' is a namespace,
// as opposed to a type with an inner type.
// * we can always do 'using C = A::B::C' but then SOME C++
// compilers complain that it's unused, iff it's a named struct.
let destructor_call = format!("{arg_list}->{ty_id}::~{ty_id}()");
let destructor_call = if ns.is_empty() {
destructor_call
} else {
let path = self.original_name_map.map(&full_name);
if is_a_nested_struct {
format!("{{ using {ty_id} = {path}; {destructor_call}; {ty_id}* pointless; (void)pointless; }}")
} else {
format!("{{ using {path}; {destructor_call}; }}")
}
};
(destructor_call, "".to_string(), false)
}
CppFunctionBody::FunctionCall(ns, id) => match receiver {
Some(receiver) => (
Expand Down Expand Up @@ -554,7 +575,7 @@ impl<'a> CppCodeGenerator<'a> {

underlying_function_call = match placement_param {
Some(placement_param) => {
let tyname = type_to_cpp(ret.cxxbridge_type(), &self.original_name_map)?;
let tyname = self.original_name_map.type_to_cpp(ret.cxxbridge_type())?;
format!("new({placement_param}) {tyname}({call_itself})")
}
None => format!("return {call_itself}"),
Expand Down Expand Up @@ -600,7 +621,7 @@ impl<'a> CppCodeGenerator<'a> {
}

fn namespaced_name(&self, name: &QualifiedName) -> String {
namespaced_name_using_original_name_map(name, &self.original_name_map)
self.original_name_map.map(name)
}

fn generate_ctype_typedef(&mut self, tn: &QualifiedName) {
Expand Down
214 changes: 111 additions & 103 deletions engine/src/conversion/codegen_cpp/type_to_cpp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,121 +19,129 @@ use syn::{Token, Type};
/// Map from QualifiedName to original C++ name. Original C++ name does not
/// include the namespace; this can be assumed to be the same as the namespace
/// in the QualifiedName.
pub(crate) type CppNameMap = HashMap<QualifiedName, String>;
/// The "original C++ name" is mostly relevant in the case of nested types,
/// where the typename might be A::B within a namespace C::D.
pub(crate) struct CppNameMap(HashMap<QualifiedName, String>);

pub(crate) fn original_name_map_from_apis<T: AnalysisPhase>(apis: &ApiVec<T>) -> CppNameMap {
apis.iter()
.filter_map(|api| {
api.cpp_name()
.as_ref()
.map(|cpp_name| (api.name().clone(), cpp_name.clone()))
})
.collect()
}
impl CppNameMap {
/// Look through the APIs we've found to assemble the original name
/// map.
pub(crate) fn new_from_apis<T: AnalysisPhase>(apis: &ApiVec<T>) -> Self {
Self(
apis.iter()
.filter_map(|api| {
api.cpp_name()
.as_ref()
.map(|cpp_name| (api.name().clone(), cpp_name.clone()))
})
.collect(),
)
}

pub(crate) fn namespaced_name_using_original_name_map(
qual_name: &QualifiedName,
original_name_map: &CppNameMap,
) -> String {
if let Some(cpp_name) = original_name_map.get(qual_name) {
qual_name
.get_namespace()
.iter()
.chain(once(cpp_name))
.join("::")
} else {
qual_name.to_cpp_name()
/// Imagine a nested struct in namespace::outer::inner
/// This function converts from the bindgen name, namespace::outer_inner,
/// to namespace::outer::inner.
pub(crate) fn map(&self, qual_name: &QualifiedName) -> String {
if let Some(cpp_name) = self.0.get(qual_name) {
qual_name
.get_namespace()
.iter()
.chain(once(cpp_name))
.join("::")
} else {
qual_name.to_cpp_name()
}
}
}

pub(crate) fn final_ident_using_original_name_map(
qual_name: &QualifiedName,
original_name_map: &CppNameMap,
) -> String {
match original_name_map.get(qual_name) {
Some(original_name) => {
// If we have an original name, this may be a nested struct
// (e.g. A::B). The final ident here is just 'B' so...
original_name
.rsplit_once("::")
.map_or(original_name.clone(), |(_, original_name)| {
original_name.to_string()
})
/// Get a stringified version of the last ident in the name.
/// e.g. for namespace::outer_inner this will return inner.
/// This is useful for doing things such as calling constructors
/// such as inner() or destructors such as ~inner()
pub(crate) fn get_final_item<'b>(&'b self, qual_name: &'b QualifiedName) -> &'b str {
match self.get(qual_name) {
Some(n) => match n.rsplit_once("::") {
Some((_, suffix)) => suffix,
None => qual_name.get_final_item(),
},
None => qual_name.get_final_item(),
}
None => qual_name.get_final_cpp_item(),
}
}

pub(crate) fn type_to_cpp(
ty: &Type,
cpp_name_map: &CppNameMap,
) -> Result<String, ConvertErrorFromCpp> {
match ty {
Type::Path(typ) => {
// If this is a std::unique_ptr we do need to pass
// its argument through.
let qual_name = QualifiedName::from_type_path(typ);
let root = namespaced_name_using_original_name_map(&qual_name, cpp_name_map);
if root == "Pin" {
// Strip all Pins from type names when describing them in C++.
let inner_type = &typ.path.segments.last().unwrap().arguments;
if let syn::PathArguments::AngleBracketed(ab) = inner_type {
let inner_type = ab.args.iter().next().unwrap();
if let syn::GenericArgument::Type(gat) = inner_type {
return type_to_cpp(gat, cpp_name_map);
/// Convert a type to its C++ spelling.
pub(crate) fn type_to_cpp(&self, ty: &Type) -> Result<String, ConvertErrorFromCpp> {
match ty {
Type::Path(typ) => {
// If this is a std::unique_ptr we do need to pass
// its argument through.
let qual_name = QualifiedName::from_type_path(typ);
let root = self.map(&qual_name);
if root == "Pin" {
// Strip all Pins from type names when describing them in C++.
let inner_type = &typ.path.segments.last().unwrap().arguments;
if let syn::PathArguments::AngleBracketed(ab) = inner_type {
let inner_type = ab.args.iter().next().unwrap();
if let syn::GenericArgument::Type(gat) = inner_type {
return self.type_to_cpp(gat);
}
}
panic!("Pin<...> didn't contain the inner types we expected");
}
panic!("Pin<...> didn't contain the inner types we expected");
}
let suffix = match &typ.path.segments.last().unwrap().arguments {
syn::PathArguments::AngleBracketed(ab) => {
let results: Result<Vec<_>, _> = ab
.args
.iter()
.map(|x| match x {
syn::GenericArgument::Type(gat) => type_to_cpp(gat, cpp_name_map),
_ => Ok("".to_string()),
})
.collect();
Some(results?.join(", "))
let suffix = match &typ.path.segments.last().unwrap().arguments {
syn::PathArguments::AngleBracketed(ab) => {
let results: Result<Vec<_>, _> = ab
.args
.iter()
.map(|x| match x {
syn::GenericArgument::Type(gat) => self.type_to_cpp(gat),
_ => Ok("".to_string()),
})
.collect();
Some(results?.join(", "))
}
syn::PathArguments::None | syn::PathArguments::Parenthesized(_) => None,
};
match suffix {
None => Ok(root),
Some(suffix) => Ok(format!("{root}<{suffix}>")),
}
syn::PathArguments::None | syn::PathArguments::Parenthesized(_) => None,
};
match suffix {
None => Ok(root),
Some(suffix) => Ok(format!("{root}<{suffix}>")),
}
}
Type::Reference(typr) => match &*typr.elem {
Type::Path(typ) if typ.path.is_ident("str") => Ok("rust::Str".into()),
_ => Ok(format!(
"{}{}&",
get_mut_string(&typr.mutability),
type_to_cpp(typr.elem.as_ref(), cpp_name_map)?
Type::Reference(typr) => match &*typr.elem {
Type::Path(typ) if typ.path.is_ident("str") => Ok("rust::Str".into()),
_ => Ok(format!(
"{}{}&",
get_mut_string(&typr.mutability),
self.type_to_cpp(typr.elem.as_ref())?
)),
},
Type::Ptr(typp) => Ok(format!(
"{}{}*",
get_mut_string(&typp.mutability),
self.type_to_cpp(typp.elem.as_ref())?
)),
Type::Array(_)
| Type::BareFn(_)
| Type::Group(_)
| Type::ImplTrait(_)
| Type::Infer(_)
| Type::Macro(_)
| Type::Never(_)
| Type::Paren(_)
| Type::Slice(_)
| Type::TraitObject(_)
| Type::Tuple(_)
| Type::Verbatim(_) => Err(ConvertErrorFromCpp::UnsupportedType(
ty.to_token_stream().to_string(),
)),
_ => Err(ConvertErrorFromCpp::UnknownType(
ty.to_token_stream().to_string(),
)),
},
Type::Ptr(typp) => Ok(format!(
"{}{}*",
get_mut_string(&typp.mutability),
type_to_cpp(typp.elem.as_ref(), cpp_name_map)?
)),
Type::Array(_)
| Type::BareFn(_)
| Type::Group(_)
| Type::ImplTrait(_)
| Type::Infer(_)
| Type::Macro(_)
| Type::Never(_)
| Type::Paren(_)
| Type::Slice(_)
| Type::TraitObject(_)
| Type::Tuple(_)
| Type::Verbatim(_) => Err(ConvertErrorFromCpp::UnsupportedType(
ty.to_token_stream().to_string(),
)),
_ => Err(ConvertErrorFromCpp::UnknownType(
ty.to_token_stream().to_string(),
)),
}
}

/// Check an individual item in the name map. Returns a thing if
/// it's an inner type, otherwise returns none.
pub(crate) fn get(&self, name: &QualifiedName) -> Option<&String> {
self.0.get(name)
}
}

Expand Down
Loading

0 comments on commit bec2301

Please sign in to comment.