From 9ecb5389d1c12b3d18e1a37e66c0f958454f551e Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 16:59:36 -0800 Subject: [PATCH 1/6] First step towards better errors. Now control printing of output in builder.rs. --- engine/src/builder.rs | 24 ++++++++++++++++++++++-- engine/src/lib.rs | 2 +- gen/build/src/lib.rs | 16 +++++++++++++++- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/engine/src/builder.rs b/engine/src/builder.rs index b4e4cc4ad..28a354d4a 100644 --- a/engine/src/builder.rs +++ b/engine/src/builder.rs @@ -14,10 +14,10 @@ use crate::Error as EngineError; use crate::{ParseError, ParsedFile}; -use std::ffi::OsStr; use std::fs::File; use std::io::Write; use std::path::{Path, PathBuf}; +use std::{ffi::OsStr, io, process}; /// Errors returned during creation of a cc::Build from an include_cxx /// macro. @@ -41,8 +41,10 @@ pub enum BuilderError { UnableToCreateDirectory(std::io::Error, PathBuf), } +pub type BuilderSuccess = cc::Build; + /// Results of a build. -pub type BuilderResult = Result; +pub type BuilderResult = Result; /// Build autocxx C++ files and return a cc::Build you can use to build /// more from a build.rs file. @@ -57,6 +59,24 @@ where build_to_custom_directory(rs_file, autocxx_incs, out_dir()) } +/// Builds successfully, or exits the process displaying a suitable +/// message. +pub fn expect_build(rs_file: P1, autocxx_incs: I) -> cc::Build +where + P1: AsRef, + I: IntoIterator, + T: AsRef, +{ + build(rs_file, autocxx_incs).unwrap_or_else(|err| { + let _ = writeln!(io::stderr(), "\n\nautocxx error: {}\n\n", report(err)); + process::exit(1); + }) +} + +fn report(err: BuilderError) -> String { + format!("{:?}", err) +} + /// Like build, but you can specify the location where files should be generated. /// Not generally recommended for use in build scripts. pub(crate) fn build_to_custom_directory( diff --git a/engine/src/lib.rs b/engine/src/lib.rs index 9f08e301e..d6448c7ce 100644 --- a/engine/src/lib.rs +++ b/engine/src/lib.rs @@ -41,7 +41,7 @@ use log::{info, warn}; use types::TypeName; #[cfg(any(test, feature = "build"))] -pub use builder::{build, BuilderError, BuilderResult}; +pub use builder::{build, expect_build, BuilderError, BuilderResult, BuilderSuccess}; pub use parse::{parse_file, parse_token_stream, ParseError, ParsedFile}; pub use cxx_gen::HEADER; diff --git a/gen/build/src/lib.rs b/gen/build/src/lib.rs index 1de353c77..590d7c739 100644 --- a/gen/build/src/lib.rs +++ b/gen/build/src/lib.rs @@ -12,7 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. -use autocxx_engine::{build as engine_build, BuilderResult}; +use autocxx_engine::{ + build as engine_build, expect_build as engine_expect_build, BuilderResult, BuilderSuccess, +}; use std::io::Write; use std::{ffi::OsStr, path::Path}; @@ -27,3 +29,15 @@ where .init(); engine_build(rs_file, autocxx_incs) } + +pub fn expect_build(rs_file: P1, autocxx_incs: I) -> BuilderSuccess +where + P1: AsRef, + I: IntoIterator, + T: AsRef, +{ + env_logger::builder() + .format(|buf, record| writeln!(buf, "cargo:warning=MESSAGE:{}", record.args())) + .init(); + engine_expect_build(rs_file, autocxx_incs) +} From aed5b99c5f574baf41ae42cf328500b3de659190 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 21:59:35 -0800 Subject: [PATCH 2/6] Adding formatters for error types. --- engine/src/bridge_converter.rs | 13 ++++++++++- engine/src/builder.rs | 22 +++++++++++++----- engine/src/lib.rs | 41 ++++++++++++++-------------------- engine/src/parse.rs | 15 +++++++++++-- 4 files changed, 59 insertions(+), 32 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index 46e30fc49..f825d90ef 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashMap; +use std::{fmt::Display, collections::HashMap}; use crate::{ additional_cpp_generator::{AdditionalNeed, ArgumentConversion, ByValueWrapper}, @@ -45,6 +45,17 @@ pub enum ConvertError { UnknownForeignItem, } +impl Display for ConvertError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ConvertError::NoContent => write!(f, "The initial run of 'bindgen' did not generate any content. This might be because none of the requested items for generation could be converted.")?, + ConvertError::UnsafePODType(err) => write!(f, "An item was requested using 'generate_pod' which was not safe to hold by value in Rust. {}", err)?, + ConvertError::UnknownForeignItem => write!(f, "Bindgen generated some unexpected code. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + } + Ok(()) + } +} + /// Results of a conversion. pub(crate) struct BridgeConversionResults { pub items: Vec, diff --git a/engine/src/builder.rs b/engine/src/builder.rs index 28a354d4a..e9d260c09 100644 --- a/engine/src/builder.rs +++ b/engine/src/builder.rs @@ -14,7 +14,7 @@ use crate::Error as EngineError; use crate::{ParseError, ParsedFile}; -use std::fs::File; +use std::{fmt::Display, fs::File}; use std::io::Write; use std::path::{Path, PathBuf}; use std::{ffi::OsStr, io, process}; @@ -25,11 +25,9 @@ use std::{ffi::OsStr, io, process}; pub enum BuilderError { /// The cxx module couldn't parse the code generated by autocxx. /// This could well be a bug in autocxx. - InvalidCxx(EngineError), + InvalidCxx(cxx_gen::Error), /// The .rs file didn't exist or couldn't be parsed. ParseError(ParseError), - /// We couldn't create a temporary directory to store the c++ code. - TempDirCreationFailed(std::io::Error), /// We couldn't write the c++ code to disk. FileWriteFail(std::io::Error, PathBuf), /// No `include_cxx` macro was found anywhere. @@ -41,6 +39,20 @@ pub enum BuilderError { UnableToCreateDirectory(std::io::Error, PathBuf), } +impl Display for BuilderError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + BuilderError::ParseError(pe) => write!(f, "Unable to parse .rs file: {}", pe)?, + BuilderError::InvalidCxx(ee) => write!(f, "cxx was unable to understand the code generated by autocxx. {}", ee)?, + BuilderError::FileWriteFail(ee, pb) => write!(f, "Unable to write to {}: {}", pb.to_string_lossy(), ee)?, + BuilderError::NoIncludeCxxMacrosFound => write!(f, "No include_cpp! macro found")?, + BuilderError::IncludeDirProblem(ee) => write!(f, "Unable to set up C++ include direcories as requested: {}", ee)?, + BuilderError::UnableToCreateDirectory(ee, pb) => write!(f, "Unable to create directory {}: {}", pb.to_string_lossy(), ee)?, + } + Ok(()) + } +} + pub type BuilderSuccess = cc::Build; /// Results of a build. @@ -74,7 +86,7 @@ where } fn report(err: BuilderError) -> String { - format!("{:?}", err) + err.to_string() } /// Like build, but you can specify the location where files should be generated. diff --git a/engine/src/lib.rs b/engine/src/lib.rs index d6448c7ce..6695130a6 100644 --- a/engine/src/lib.rs +++ b/engine/src/lib.rs @@ -29,7 +29,7 @@ mod builder; mod integration_tests; use proc_macro2::TokenStream as TokenStream2; -use std::path::PathBuf; +use std::{fmt::Display, path::PathBuf}; use quote::ToTokens; use syn::parse::{Parse, ParseStream, Result as ParseResult}; @@ -65,16 +65,9 @@ pub struct GeneratedCpp(pub Vec); /// functions. #[derive(Debug)] pub enum Error { - /// A file reading error, most likely on the .rs file. - Io(std::io::Error), /// Any error reported by bindgen, generating the C++ bindings. /// Any C++ parsing errors, etc. would be reported this way. Bindgen(()), - /// Any problem reported by the cxx crate in generating - /// safe bindings. This would encompass errors where bindgen - /// has generated unsafe bindings, but we're unable to convert - /// them to safe `cxx::bridge` style bindings. - CxxGen(cxx_gen::Error), /// Any problem parsing the Rust file. Parsing(syn::Error), /// No `include_cpp!` macro could be found. @@ -92,6 +85,20 @@ pub enum Error { NoGenerationRequested, } +impl Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Error::Bindgen(_) => write!(f, "Bindgen was unable to generate the initial .rs bindings for this file. This may indicate a parsing problem with the C++ headers.")?, + Error::Parsing(err) => write!(f, "The Rust file could not be parsede: {}", err)?, + Error::NoAutoCxxInc => write!(f, "No C++ include directory was provided. Consider setting AUTOCXX_INC.")?, + Error::CouldNotCanoncalizeIncludeDir(pb) => write!(f, "One of the C++ include directories provided ({}) did not appear to exist or could otherwise not be made into a canonical path.", pb.to_string_lossy())?, + Error::Conversion(err) => write!(f, "autocxx could not generate the requested bindings. {}", err)?, + Error::NoGenerationRequested => write!(f, "No 'generate' or 'generate_pod' directives were found, so we would not generate any Rust bindings despite the inclusion of C++ headers.")?, + } + Ok(()) + } +} + pub type Result = std::result::Result; pub enum CppInclusion { @@ -128,18 +135,6 @@ impl Parse for IncludeCpp { } } -fn dump_generated_code(gen: cxx_gen::GeneratedCode) -> Result { - info!( - "CXX:\n{}", - String::from_utf8(gen.implementation.clone()).unwrap() - ); - info!( - "header:\n{}", - String::from_utf8(gen.header.clone()).unwrap() - ); - Ok(gen) -} - impl IncludeCpp { fn new_from_parse_stream(input: ParseStream) -> syn::Result { // Takes as inputs: @@ -377,7 +372,7 @@ impl IncludeCpp { } /// Generate C++-side bindings for these APIs. Call `generate` first. - pub fn generate_h_and_cxx(&self) -> Result { + pub fn generate_h_and_cxx(&self) -> Result { let mut files = Vec::new(); match &self.state { State::ParseOnly => panic!("Cannot generate C++ in parse-only mode"), @@ -386,9 +381,7 @@ impl IncludeCpp { State::Generated(itemmod, additional_cpp_generator) => { let rs = itemmod.into_token_stream(); let opt = cxx_gen::Opt::default(); - let cxx_generated = cxx_gen::generate_header_and_cc(rs, &opt) - .map_err(Error::CxxGen) - .and_then(dump_generated_code)?; + let cxx_generated = cxx_gen::generate_header_and_cc(rs, &opt)?; files.push(CppFilePair { header: cxx_generated.header, header_name: "cxxgen.h".to_string(), diff --git a/engine/src/parse.rs b/engine/src/parse.rs index f1a7882f3..c816d06b7 100644 --- a/engine/src/parse.rs +++ b/engine/src/parse.rs @@ -16,7 +16,7 @@ use crate::Error as EngineError; use crate::IncludeCpp; use proc_macro2::TokenStream; use quote::ToTokens; -use std::io::Read; +use std::{fmt::Display, io::Read}; use std::path::Path; use syn::Item; @@ -32,7 +32,18 @@ pub enum ParseError { Syntax(syn::Error), /// The include CPP macro could not be parsed. MacroParseFail(EngineError), - GenerateRsError(EngineError), +} + +impl Display for ParseError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ParseError::FileOpen(err) => write!(f, "Unable to open file: {}", err)?, + ParseError::FileRead(err) => write!(f, "Unable to read file: {}", err)?, + ParseError::Syntax(err) => write!(f, "Syntax error parsing Rust file: {}", err)?, + ParseError::MacroParseFail(err) => write!(f, "Unable to parse include_cpp! macro: {}", err)?, + } + Ok(()) + } } /// Parse a Rust file, and spot any include_cpp macros within it. From 42d7c826bbbd16d7bcb937ad8f4a7988b7f4ad26 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 22:35:12 -0800 Subject: [PATCH 3/6] More error handling from deep inside bridge converter. --- engine/src/bridge_converter.rs | 121 +++++++++++++++++++-------------- engine/src/builder.rs | 4 +- engine/src/parse.rs | 6 +- 3 files changed, 75 insertions(+), 56 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index f825d90ef..657a3f92e 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{fmt::Display, collections::HashMap}; +use std::{collections::HashMap, fmt::Display}; use crate::{ additional_cpp_generator::{AdditionalNeed, ArgumentConversion, ByValueWrapper}, @@ -43,6 +43,9 @@ pub enum ConvertError { NoContent, UnsafePODType(String), UnknownForeignItem, + UnexpectedOuterItem, + UnexpectedItemInMod, + ComplexTypedefTarget(String), } impl Display for ConvertError { @@ -50,7 +53,10 @@ impl Display for ConvertError { match self { ConvertError::NoContent => write!(f, "The initial run of 'bindgen' did not generate any content. This might be because none of the requested items for generation could be converted.")?, ConvertError::UnsafePODType(err) => write!(f, "An item was requested using 'generate_pod' which was not safe to hold by value in Rust. {}", err)?, - ConvertError::UnknownForeignItem => write!(f, "Bindgen generated some unexpected code. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + ConvertError::UnknownForeignItem => write!(f, "Bindgen generated some unexpected code in a foreign mod section. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + ConvertError::UnexpectedOuterItem => write!(f, "Bindgen generated some unexpected code in its outermost mod section. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + ConvertError::UnexpectedItemInMod => write!(f, "Bindgen generated some unexpected code in an inner namespace mod. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + ConvertError::ComplexTypedefTarget(ty) => write!(f, "autocxx was unable to produce a typdef pointing to the complex type {}.", ty)?, } Ok(()) } @@ -200,7 +206,7 @@ impl<'a> BridgeConversion<'a> { self.convert_mod_items(items, root_ns, &mut bindgen_root_items)?; } } - _ => panic!("Unexpected outer item"), + _ => return Err(ConvertError::UnexpectedOuterItem), } } self.extern_c_mod_items @@ -342,12 +348,7 @@ impl<'a> BridgeConversion<'a> { output_items.push(Item::Type(ity)); self.typedefs.insert(tyname, target); } - _ => { - // TODO it would be nice to enable this, but at the moment - // we hit it too often. Also, item is not Debug so - // it's a bit annoying trying to work out what's up. - //panic!("Unhandled item"); - } + _ => return Err(ConvertError::UnexpectedItemInMod), } } Ok(()) @@ -625,12 +626,20 @@ impl<'a> BridgeConversion<'a> { } // Now let's analyze all the parameters. We do this first // because we'll use this to determine whether this is a method. - let (mut params, param_details): (Punctuated<_, syn::Token![,]>, Vec<_>) = fun + let (goods, bads): (Vec<_>, Vec<_>) = fun .sig .inputs .into_iter() .map(|i| self.convert_fn_arg(i)) - .unzip(); + .partition(Result::is_ok); + if let Some(problem) = bads.into_iter().next() { + match problem { + Err(e) => return Err(e), + _ => panic!("Err didn't contain en err"), + } + } + let (mut params, param_details): (Punctuated<_, syn::Token![,]>, Vec<_>) = + goods.into_iter().map(Result::unwrap).unzip(); let is_a_method = param_details.iter().any(|b| b.was_self); @@ -665,7 +674,7 @@ impl<'a> BridgeConversion<'a> { // Analyze the return type, just as we previously did for the // parameters. - let (mut ret_type, ret_type_conversion) = self.convert_return_type(fun.sig.output); + let (mut ret_type, ret_type_conversion) = self.convert_return_type(fun.sig.output)?; // Do we need to convert either parameters or return type? let param_conversion_needed = param_details.iter().any(|b| b.conversion.work_needed()); @@ -806,8 +815,8 @@ impl<'a> BridgeConversion<'a> { /// methodWhichTakesValue_up_wrapper) is just a function. /// 2) Don't give the first parameter a special name. In which case /// we will generate a standalone function on the Rust side. - fn convert_fn_arg(&self, arg: FnArg) -> (FnArg, ArgumentAnalysis) { - match arg { + fn convert_fn_arg(&self, arg: FnArg) -> Result<(FnArg, ArgumentAnalysis), ConvertError> { + Ok(match arg { FnArg::Typed(mut pt) => { let mut found_this = false; let old_pat = *pt.pat; @@ -822,7 +831,7 @@ impl<'a> BridgeConversion<'a> { } _ => old_pat, }; - let new_ty = self.convert_boxed_type(pt.ty); + let new_ty = self.convert_boxed_type(pt.ty)?; let conversion = self.conversion_required(&new_ty); pt.pat = Box::new(new_pat.clone()); pt.ty = new_ty; @@ -835,8 +844,8 @@ impl<'a> BridgeConversion<'a> { }, ) } - _ => panic!("FnArg::Receiver not yet handled"), - } + _ => panic!("Did not expect FnArg::Receiver to be generated by bindgen"), + }) } fn conversion_required(&self, ty: &Type) -> ArgumentConversion { @@ -864,11 +873,14 @@ impl<'a> BridgeConversion<'a> { } } - fn convert_return_type(&self, rt: ReturnType) -> (ReturnType, Option) { - match rt { + fn convert_return_type( + &self, + rt: ReturnType, + ) -> Result<(ReturnType, Option), ConvertError> { + let result = match rt { ReturnType::Default => (ReturnType::Default, None), ReturnType::Type(rarrow, boxed_type) => { - let boxed_type = self.convert_boxed_type(boxed_type); + let boxed_type = self.convert_boxed_type(boxed_type)?; let conversion = if self.requires_conversion(boxed_type.as_ref()) { ArgumentConversion::new_to_unique_ptr(*boxed_type.clone()) } else { @@ -876,17 +888,18 @@ impl<'a> BridgeConversion<'a> { }; (ReturnType::Type(rarrow, boxed_type), Some(conversion)) } - } + }; + Ok(result) } - fn convert_boxed_type(&self, ty: Box) -> Box { - Box::new(self.convert_type(*ty)) + fn convert_boxed_type(&self, ty: Box) -> Result, ConvertError> { + Ok(Box::new(self.convert_type(*ty)?)) } - fn convert_type(&self, ty: Type) -> Type { - match ty { + fn convert_type(&self, ty: Type) -> Result { + let result = match ty { Type::Path(p) => { - let newp = self.convert_type_path(p); + let newp = self.convert_type_path(p)?; // Special handling because rust_Str (as emitted by bindgen) // doesn't simply get renamed to a different type _identifier_. // This plain type-by-value (as far as bindgen is concerned) @@ -900,56 +913,57 @@ impl<'a> BridgeConversion<'a> { } } Type::Reference(mut r) => { - r.elem = self.convert_boxed_type(r.elem); + r.elem = self.convert_boxed_type(r.elem)?; Type::Reference(r) } - Type::Ptr(ptr) => Type::Reference(self.convert_ptr_to_reference(ptr)), + Type::Ptr(ptr) => Type::Reference(self.convert_ptr_to_reference(ptr)?), _ => ty, - } + }; + Ok(result) } - fn convert_ptr_to_reference(&self, ptr: TypePtr) -> TypeReference { + fn convert_ptr_to_reference(&self, ptr: TypePtr) -> Result { let mutability = ptr.mutability; - let elem = self.convert_boxed_type(ptr.elem); + let elem = self.convert_boxed_type(ptr.elem)?; // TODO - in the future, we should check if this is a rust::Str and throw // a wobbler if not. rust::Str should only be seen _by value_ in C++ // headers; it manifests as &str in Rust but on the C++ side it must // be a plain value. We should detect and abort. - parse_quote! { + Ok(parse_quote! { & #mutability #elem - } + }) } - fn convert_type_path(&self, mut typ: TypePath) -> TypePath { + fn convert_type_path(&self, mut typ: TypePath) -> Result { if typ.path.segments.iter().next().unwrap().ident == "root" { typ.path.segments = typ .path .segments .into_iter() .skip(1) // skip root - .map(|s| -> PathSegment { + .map(|s| -> Result { let ident = &s.ident; let args = match s.arguments { PathArguments::AngleBracketed(mut ab) => { - ab.args = self.convert_punctuated(ab.args); + ab.args = self.convert_punctuated(ab.args)?; PathArguments::AngleBracketed(ab) } _ => s.arguments, }; - parse_quote!( #ident #args ) + Ok(parse_quote!( #ident #args )) }) - .collect(); + .collect::>()?; } self.replace_cpp_with_cxx(typ) } - fn replace_cpp_with_cxx(&self, typ: TypePath) -> TypePath { + fn replace_cpp_with_cxx(&self, typ: TypePath) -> Result { let mut last_seg_args = None; let mut seg_iter = typ.path.segments.iter().peekable(); while let Some(seg) = seg_iter.next() { if !seg.arguments.is_empty() { if seg_iter.peek().is_some() { - panic!("Found a type with path arguments not on the last segment") + panic!("Did not expect bindgen to create a type with path arguments on a non-final segment") } else { last_seg_args = Some(seg.arguments.clone()); } @@ -958,7 +972,7 @@ impl<'a> BridgeConversion<'a> { drop(seg_iter); let tn = TypeName::from_cxx_type_path(&typ); // Let's see if this is a typedef. - let typ = match self.resolve_typedef(&tn) { + let typ = match self.resolve_typedef(&tn)? { Some(newid) => newid.to_cxx_type_path(), None => typ, }; @@ -970,37 +984,40 @@ impl<'a> BridgeConversion<'a> { let last_seg = typ.path.segments.last_mut().unwrap(); last_seg.arguments = last_seg_args; } - typ + Ok(typ) } - fn resolve_typedef<'b>(&'b self, tn: &'b TypeName) -> Option<&'b TypeName> { + fn resolve_typedef<'b>( + &'b self, + tn: &'b TypeName, + ) -> Result, ConvertError> { match self.typedefs.get(&tn) { - None => None, + None => Ok(None), Some(TypedefTarget::NoArguments(original_tn)) => { - match self.resolve_typedef(original_tn) { - None => Some(original_tn), - Some(further_resolution) => Some(further_resolution) + match self.resolve_typedef(original_tn)? { + None => Ok(Some(original_tn)), + Some(further_resolution) => Ok(Some(further_resolution)), } - }, - _ => panic!("Asked to resolve typedef {} but it leads to something complex which autocxx cannot yet handle", tn.to_cpp_name()) + } + _ => Err(ConvertError::ComplexTypedefTarget(tn.to_cpp_name())), } } fn convert_punctuated

( &self, pun: Punctuated, - ) -> Punctuated + ) -> Result, ConvertError> where P: Default, { let mut new_pun = Punctuated::new(); for arg in pun.into_iter() { new_pun.push(match arg { - GenericArgument::Type(t) => GenericArgument::Type(self.convert_type(t)), + GenericArgument::Type(t) => GenericArgument::Type(self.convert_type(t)?), _ => arg, }); } - new_pun + Ok(new_pun) } /// Generate lots of 'use' statements to pull cxxbridge items into the output diff --git a/engine/src/builder.rs b/engine/src/builder.rs index e9d260c09..ac789e5e3 100644 --- a/engine/src/builder.rs +++ b/engine/src/builder.rs @@ -14,10 +14,10 @@ use crate::Error as EngineError; use crate::{ParseError, ParsedFile}; -use std::{fmt::Display, fs::File}; use std::io::Write; use std::path::{Path, PathBuf}; use std::{ffi::OsStr, io, process}; +use std::{fmt::Display, fs::File}; /// Errors returned during creation of a cc::Build from an include_cxx /// macro. @@ -43,7 +43,7 @@ impl Display for BuilderError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { BuilderError::ParseError(pe) => write!(f, "Unable to parse .rs file: {}", pe)?, - BuilderError::InvalidCxx(ee) => write!(f, "cxx was unable to understand the code generated by autocxx. {}", ee)?, + BuilderError::InvalidCxx(ee) => write!(f, "cxx was unable to understand the code generated by autocxx (likely a bug in autocxx; please report.) {}", ee)?, BuilderError::FileWriteFail(ee, pb) => write!(f, "Unable to write to {}: {}", pb.to_string_lossy(), ee)?, BuilderError::NoIncludeCxxMacrosFound => write!(f, "No include_cpp! macro found")?, BuilderError::IncludeDirProblem(ee) => write!(f, "Unable to set up C++ include direcories as requested: {}", ee)?, diff --git a/engine/src/parse.rs b/engine/src/parse.rs index c816d06b7..1ae7161dc 100644 --- a/engine/src/parse.rs +++ b/engine/src/parse.rs @@ -16,8 +16,8 @@ use crate::Error as EngineError; use crate::IncludeCpp; use proc_macro2::TokenStream; use quote::ToTokens; -use std::{fmt::Display, io::Read}; use std::path::Path; +use std::{fmt::Display, io::Read}; use syn::Item; /// Errors which may occur when parsing a Rust source file to discover @@ -40,7 +40,9 @@ impl Display for ParseError { ParseError::FileOpen(err) => write!(f, "Unable to open file: {}", err)?, ParseError::FileRead(err) => write!(f, "Unable to read file: {}", err)?, ParseError::Syntax(err) => write!(f, "Syntax error parsing Rust file: {}", err)?, - ParseError::MacroParseFail(err) => write!(f, "Unable to parse include_cpp! macro: {}", err)?, + ParseError::MacroParseFail(err) => { + write!(f, "Unable to parse include_cpp! macro: {}", err)? + } } Ok(()) } From ea0fd3f0038ee64b5a390f458e784062334832ee Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 22:35:55 -0800 Subject: [PATCH 4/6] Rename for consistency. --- engine/src/bridge_converter.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index 657a3f92e..a51c338d0 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -42,7 +42,7 @@ use syn::{punctuated::Punctuated, ItemImpl}; pub enum ConvertError { NoContent, UnsafePODType(String), - UnknownForeignItem, + UnexpectedForeignItem, UnexpectedOuterItem, UnexpectedItemInMod, ComplexTypedefTarget(String), @@ -53,7 +53,7 @@ impl Display for ConvertError { match self { ConvertError::NoContent => write!(f, "The initial run of 'bindgen' did not generate any content. This might be because none of the requested items for generation could be converted.")?, ConvertError::UnsafePODType(err) => write!(f, "An item was requested using 'generate_pod' which was not safe to hold by value in Rust. {}", err)?, - ConvertError::UnknownForeignItem => write!(f, "Bindgen generated some unexpected code in a foreign mod section. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, + ConvertError::UnexpectedForeignItem => write!(f, "Bindgen generated some unexpected code in a foreign mod section. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, ConvertError::UnexpectedOuterItem => write!(f, "Bindgen generated some unexpected code in its outermost mod section. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, ConvertError::UnexpectedItemInMod => write!(f, "Bindgen generated some unexpected code in an inner namespace mod. You may have specified something in a 'generate' directive which is not currently compatible with autocxx.")?, ConvertError::ComplexTypedefTarget(ty) => write!(f, "autocxx was unable to produce a typdef pointing to the complex type {}.", ty)?, @@ -591,7 +591,7 @@ impl<'a> BridgeConversion<'a> { ForeignItem::Fn(f) => { self.convert_foreign_fn(f, ns, output_items)?; } - _ => return Err(ConvertError::UnknownForeignItem), + _ => return Err(ConvertError::UnexpectedForeignItem), } } Ok(()) From c03d0f34a15ba777854cf14799a080f7f3a01823 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 22:37:15 -0800 Subject: [PATCH 5/6] Remove obsolete comment. --- engine/src/bridge_converter.rs | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index a51c338d0..2ccf30707 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -788,33 +788,6 @@ impl<'a> BridgeConversion<'a> { /// Returns additionally a Boolean indicating whether an argument was /// 'this' and another one indicating whether we took a type by value /// and that type was non-trivial. - /// - /// Regarding autocxx_gen_this, this pertains to what happens if - /// we come across a *method* which takes or returns a non-POD type - /// by value (for example a std::string or a struct containing a - /// std::string). For normal functions, additional_cpp_generator - /// generates a wrapper method which effectively boxes/unboxes - /// values from unique_ptr to the desired by-value type. - /// - /// For methods, we generate a similar wrapper, but it's not a - /// member of the original class - it's just a standalone function. - /// On the C++ side this method is happy to call the original - /// member function of the class, and all is well. But then we come - /// back here during the second invocation of bridge_converter, - /// and discover the new function we generated. We then have to - /// decide how to teach cxx about that function, and neither - /// option is satisfactory: - /// 1) If we rename the first parameter to 'self' then cxx - /// will treat it as a method. This is what we want because - /// it means we can call it from Rust like this: - /// my_object.methodWhichTakesValue(uniquePtrToArg) - /// But, in cxx's own generated code, it will insist on calling - /// Type::methodWhichTakesValue(...) - /// That doesn't work, since our autogenerated - /// methodWhichTakesValue (actually called - /// methodWhichTakesValue_up_wrapper) is just a function. - /// 2) Don't give the first parameter a special name. In which case - /// we will generate a standalone function on the Rust side. fn convert_fn_arg(&self, arg: FnArg) -> Result<(FnArg, ArgumentAnalysis), ConvertError> { Ok(match arg { FnArg::Typed(mut pt) => { From 400507f7e017312578279aff428fe6b74f0a9674 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 13 Nov 2020 22:44:31 -0800 Subject: [PATCH 6/6] Remove some duplication. --- engine/src/bridge_converter.rs | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs index 2ccf30707..58ccb799c 100644 --- a/engine/src/bridge_converter.rs +++ b/engine/src/bridge_converter.rs @@ -805,7 +805,7 @@ impl<'a> BridgeConversion<'a> { _ => old_pat, }; let new_ty = self.convert_boxed_type(pt.ty)?; - let conversion = self.conversion_required(&new_ty); + let conversion = self.argument_conversion_details(&new_ty); pt.pat = Box::new(new_pat.clone()); pt.ty = new_ty; ( @@ -821,7 +821,10 @@ impl<'a> BridgeConversion<'a> { }) } - fn conversion_required(&self, ty: &Type) -> ArgumentConversion { + fn conversion_details(&self, ty: &Type, conversion_direction: F) -> ArgumentConversion + where + F: FnOnce(Type) -> ArgumentConversion, + { match ty { Type::Path(p) => { if self @@ -830,20 +833,19 @@ impl<'a> BridgeConversion<'a> { { ArgumentConversion::new_unconverted(ty.clone()) } else { - ArgumentConversion::new_from_unique_ptr(ty.clone()) + conversion_direction(ty.clone()) } } _ => ArgumentConversion::new_unconverted(ty.clone()), } } - fn requires_conversion(&self, ty: &Type) -> bool { - match ty { - Type::Path(typ) => !self - .byvalue_checker - .is_pod(&TypeName::from_cxx_type_path(typ)), - _ => false, - } + fn argument_conversion_details(&self, ty: &Type) -> ArgumentConversion { + self.conversion_details(ty, ArgumentConversion::new_from_unique_ptr) + } + + fn return_type_conversion_details(&self, ty: &Type) -> ArgumentConversion { + self.conversion_details(ty, ArgumentConversion::new_to_unique_ptr) } fn convert_return_type( @@ -854,11 +856,7 @@ impl<'a> BridgeConversion<'a> { ReturnType::Default => (ReturnType::Default, None), ReturnType::Type(rarrow, boxed_type) => { let boxed_type = self.convert_boxed_type(boxed_type)?; - let conversion = if self.requires_conversion(boxed_type.as_ref()) { - ArgumentConversion::new_to_unique_ptr(*boxed_type.clone()) - } else { - ArgumentConversion::new_unconverted(*boxed_type.clone()) - }; + let conversion = self.return_type_conversion_details(boxed_type.as_ref()); (ReturnType::Type(rarrow, boxed_type), Some(conversion)) } };