Tests forcing wrappers

.github/workflows/ci.yml

@@ -177,6 +177,17 @@ jobs:
           RUST_BACKTRACE: "0"
         run: cargo -Z build-std test --workspace --target x86_64-unknown-linux-gnu
 
+  force-wrapper-generation:
+    name: Test forcing wrapper generation
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: Swatinem/rust-cache@v1
+      - name: Tests generating all possible shims
+        env:
+          AUTOCXX_FORCE_WRAPPER_GENERATION: 1
+        run: cargo test --workspace
+
   # Clippy check
   clippy:
     name: Clippy

book/src/contributing.md

@@ -71,7 +71,11 @@ RUST_BACKTRACE=1 RUST_LOG=autocxx_engine=info cargo test --all test_cycle_string
 This is especially valuable to see the `bindgen` output Rust code, and then the converted Rust code which we pass into cxx. Usually, most problems are due to some mis-conversion somewhere
 in `engine/src/conversion`. See [here](https://docs.rs/autocxx-engine/latest/autocxx_engine/struct.IncludeCppEngine.html) for documentation and diagrams on how the engine works.
 
-You may also wish to set `AUTOCXX_ASAN=1` on Linux when running tests.
+You may also wish to set `AUTOCXX_ASAN=1` on Linux when running tests. To exercise all
+the code paths related to generating both C++ and Rust side shims, you can set
+`AUTOCXX_FORCE_WRAPPER_GENERATION=1`. The test suite doesn't do this by default because
+we also want to test the normal code paths. (In the future we might want to
+parameterize the test suite to do both.)
 
 ## Reporting bugs
 

engine/src/builder.rs

@@ -10,7 +10,7 @@ use autocxx_parser::file_locations::FileLocationStrategy;
 use miette::Diagnostic;
 use thiserror::Error;
 
-use crate::generate_rs_single;
+use crate::{generate_rs_single, CodegenOptions};
 use crate::{strip_system_headers, CppCodegenOptions, ParseError, RebuildDependencyRecorder};
 use std::ffi::OsStr;
 use std::ffi::OsString;
@@ -73,7 +73,7 @@ pub struct Builder<'a, BuilderContext> {
     dependency_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
     custom_gendir: Option<PathBuf>,
     auto_allowlist: bool,
-    cpp_codegen_options: CppCodegenOptions<'a>,
+    codegen_options: CodegenOptions<'a>,
     // This member is to ensure that this type is parameterized
     // by a BuilderContext. The goal is to balance three needs:
     // (1) have most of the functionality over in autocxx_engine,
@@ -108,7 +108,7 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
             dependency_recorder: CTX::get_dependency_recorder(),
             custom_gendir: None,
             auto_allowlist: false,
-            cpp_codegen_options: CppCodegenOptions::default(),
+            codegen_options: CodegenOptions::default(),
             ctx: PhantomData,
         }
     }
@@ -130,7 +130,7 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
     where
         F: FnOnce(&mut CppCodegenOptions),
     {
-        modifier(&mut self.cpp_codegen_options);
+        modifier(&mut self.codegen_options.cpp_codegen_options);
         self
     }
 
@@ -152,20 +152,33 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
         self
     }
 
+    #[doc(hidden)]
+    /// Whether to force autocxx always to generate extra Rust and C++
+    /// side shims. This is only used by the integration test suite to
+    /// exercise more code paths - don't use it!
+    pub fn force_wrapper_generation(mut self, do_it: bool) -> Self {
+        self.codegen_options.force_wrapper_gen = do_it;
+        self
+    }
+
     /// Whether to suppress inclusion of system headers (`memory`, `string` etc.)
     /// from generated C++ bindings code. This should not normally be used,
     /// but can occasionally be useful if you're reducing a test case and you
     /// have a preprocessed header file which already contains absolutely everything
     /// that the bindings could ever need.
     pub fn suppress_system_headers(mut self, do_it: bool) -> Self {
-        self.cpp_codegen_options.suppress_system_headers = do_it;
+        self.codegen_options
+            .cpp_codegen_options
+            .suppress_system_headers = do_it;
         self
     }
 
     /// An annotation optionally to include on each C++ function.
     /// For example to export the symbol from a library.
     pub fn cxx_impl_annotations(mut self, cxx_impl_annotations: Option<String>) -> Self {
-        self.cpp_codegen_options.cxx_impl_annotations = cxx_impl_annotations;
+        self.codegen_options
+            .cpp_codegen_options
+            .cxx_impl_annotations = cxx_impl_annotations;
         self
     }
 
@@ -208,7 +221,11 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
         write_to_file(
             &incdir,
             "cxx.h",
-            &Self::get_cxx_header_bytes(self.cpp_codegen_options.suppress_system_headers),
+            &Self::get_cxx_header_bytes(
+                self.codegen_options
+                    .cpp_codegen_options
+                    .suppress_system_headers,
+            ),
         )?;
 
         let autocxx_inc = build_autocxx_inc(self.autocxx_incs, &incdir);
@@ -221,7 +238,7 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
                 autocxx_inc,
                 clang_args,
                 self.dependency_recorder,
-                &self.cpp_codegen_options,
+                &self.codegen_options,
             )
             .map_err(BuilderError::ParseError)?;
         let mut counter = 0;
@@ -235,7 +252,7 @@ impl<CTX: BuilderContext> Builder<'_, CTX> {
         builder.includes(parsed_file.include_dirs());
         for include_cpp in parsed_file.get_cpp_buildables() {
             let generated_code = include_cpp
-                .generate_h_and_cxx(&self.cpp_codegen_options)
+                .generate_h_and_cxx(&self.codegen_options.cpp_codegen_options)
                 .map_err(BuilderError::InvalidCxx)?;
             for filepair in generated_code.0 {
                 let fname = format!("gen{counter}.cxx");

engine/src/conversion/analysis/fun/mod.rs

@@ -284,13 +284,15 @@ pub(crate) struct FnAnalyzer<'a> {
     generic_types: HashSet<QualifiedName>,
     types_in_anonymous_namespace: HashSet<QualifiedName>,
     existing_superclass_trait_api_names: HashSet<QualifiedName>,
+    force_wrapper_generation: bool,
 }
 
 impl<'a> FnAnalyzer<'a> {
     pub(crate) fn analyze_functions(
         apis: ApiVec<PodPhase>,
         unsafe_policy: &'a UnsafePolicy,
         config: &'a IncludeCppConfig,
+        force_wrapper_generation: bool,
     ) -> ApiVec<FnPrePhase2> {
         let mut me = Self {
             unsafe_policy,
@@ -306,6 +308,7 @@ impl<'a> FnAnalyzer<'a> {
             generic_types: Self::build_generic_type_set(&apis),
             existing_superclass_trait_api_names: HashSet::new(),
             types_in_anonymous_namespace: Self::build_types_in_anonymous_namespace(&apis),
+            force_wrapper_generation,
         };
         let mut results = ApiVec::new();
         convert_apis(
@@ -1249,6 +1252,7 @@ impl<'a> FnAnalyzer<'a> {
             _ if ret_type_conversion_needed => true,
             _ if cpp_name_incompatible_with_cxx => true,
             _ if fun.synthetic_cpp.is_some() => true,
+            _ if self.force_wrapper_generation => true,
             _ => false,
         };
 
@@ -1362,6 +1366,7 @@ impl<'a> FnAnalyzer<'a> {
             _ if any_param_needs_rust_conversion || return_needs_rust_conversion => true,
             FnKind::TraitMethod { .. } => true,
             FnKind::Method { .. } => cxxbridge_name != rust_name,
+            _ if self.force_wrapper_generation => true,
             _ => false,
         };
 

engine/src/conversion/conversion_tests.rs

@@ -11,7 +11,7 @@ use autocxx_parser::UnsafePolicy;
 use syn::parse_quote;
 use syn::ItemMod;
 
-use crate::CppCodegenOptions;
+use crate::CodegenOptions;
 
 use super::BridgeConverter;
 
@@ -33,7 +33,7 @@ fn do_test(input: ItemMod) {
         input,
         UnsafePolicy::AllFunctionsSafe,
         inclusions,
-        &CppCodegenOptions::default(),
+        &CodegenOptions::default(),
         "",
     )
     .unwrap();

engine/src/conversion/mod.rs

@@ -28,7 +28,7 @@ use itertools::Itertools;
 use syn::{Item, ItemMod};
 
 use crate::{
-    conversion::analysis::deps::HasDependencies, CppCodegenOptions, CppFilePair, UnsafePolicy,
+    conversion::analysis::deps::HasDependencies, CodegenOptions, CppFilePair, UnsafePolicy,
 };
 
 use self::{
@@ -122,7 +122,7 @@ impl<'a> BridgeConverter<'a> {
         mut bindgen_mod: ItemMod,
         unsafe_policy: UnsafePolicy,
         inclusions: String,
-        cpp_codegen_options: &CppCodegenOptions,
+        codegen_options: &CodegenOptions,
         source_file_contents: &str,
     ) -> Result<CodegenResults, ConvertError> {
         match &mut bindgen_mod.content {
@@ -158,8 +158,12 @@ impl<'a> BridgeConverter<'a> {
                 // part of `autocxx`. Again, this returns a new set of `Api`s, but
                 // parameterized by a richer set of metadata.
                 Self::dump_apis("adding casts", &analyzed_apis);
-                let analyzed_apis =
-                    FnAnalyzer::analyze_functions(analyzed_apis, &unsafe_policy, self.config);
+                let analyzed_apis = FnAnalyzer::analyze_functions(
+                    analyzed_apis,
+                    &unsafe_policy,
+                    self.config,
+                    codegen_options.force_wrapper_gen,
+                );
                 // If any of those functions turned out to be pure virtual, don't attempt
                 // to generate UniquePtr implementations for the type, since it can't
                 // be instantiated.
@@ -189,12 +193,15 @@ impl<'a> BridgeConverter<'a> {
                 Self::dump_apis_with_deps("GC", &analyzed_apis);
                 // And finally pass them to the code gen phases, which outputs
                 // code suitable for cxx to consume.
-                let cxxgen_header_name = cpp_codegen_options.cxxgen_header_namer.name_header();
+                let cxxgen_header_name = codegen_options
+                    .cpp_codegen_options
+                    .cxxgen_header_namer
+                    .name_header();
                 let cpp = CppCodeGenerator::generate_cpp_code(
                     inclusions,
                     &analyzed_apis,
                     self.config,
-                    cpp_codegen_options,
+                    &codegen_options.cpp_codegen_options,
                     &cxxgen_header_name,
                 )
                 .map_err(ConvertError::Cpp)?;

engine/src/lib.rs

@@ -146,6 +146,16 @@ enum State {
     Generated(Box<GenerationResults>),
 }
 
+/// Code generation options.
+#[derive(Default)]
+pub struct CodegenOptions<'a> {
+    // An option used by the test suite to force a more convoluted
+    // route through our code, to uncover bugs.
+    pub force_wrapper_gen: bool,
+    /// Options about the C++ code generation.
+    pub cpp_codegen_options: CppCodegenOptions<'a>,
+}
+
 const AUTOCXX_CLANG_ARGS: &[&str; 4] = &["-x", "c++", "-std=c++14", "-DBINDGEN"];
 
 /// Implement to learn of header files which get included
@@ -408,7 +418,7 @@ impl IncludeCppEngine {
         inc_dirs: Vec<PathBuf>,
         extra_clang_args: &[&str],
         dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
-        cpp_codegen_options: &CppCodegenOptions,
+        codegen_options: &CodegenOptions,
     ) -> Result<()> {
         // If we are in parse only mode, do nothing. This is used for
         // doc tests to ensure the parsing is valid, but we can't expect
@@ -456,7 +466,7 @@ impl IncludeCppEngine {
                 bindings,
                 self.config.unsafe_policy.clone(),
                 header_contents,
-                cpp_codegen_options,
+                codegen_options,
                 &source_file_contents,
             )
             .map_err(Error::Conversion)?;

engine/src/parse_file.rs

@@ -12,7 +12,7 @@ use crate::{
     cxxbridge::CxxBridge, Error as EngineError, GeneratedCpp, IncludeCppEngine,
     RebuildDependencyRecorder,
 };
-use crate::{CppCodegenOptions, LocatedSynError};
+use crate::{CodegenOptions, CppCodegenOptions, LocatedSynError};
 use autocxx_parser::directive_names::SUBCLASS;
 use autocxx_parser::{AllowlistEntry, RustPath, Subclass, SubclassAttrs};
 use indexmap::set::IndexSet as HashSet;
@@ -372,7 +372,7 @@ impl ParsedFile {
         autocxx_inc: Vec<PathBuf>,
         extra_clang_args: &[&str],
         dep_recorder: Option<Box<dyn RebuildDependencyRecorder>>,
-        cpp_codegen_options: &CppCodegenOptions,
+        codegen_options: &CodegenOptions,
     ) -> Result<(), ParseError> {
         let mut mods_found = HashSet::new();
         let inner_dep_recorder: Option<Rc<dyn RebuildDependencyRecorder>> =
@@ -394,7 +394,7 @@ impl ParsedFile {
                     autocxx_inc.clone(),
                     extra_clang_args,
                     dep_recorder,
-                    cpp_codegen_options,
+                    codegen_options,
                 )
                 .map_err(ParseError::AutocxxCodegenError)?
         }

gen/cmd/src/main.rs

@@ -247,6 +247,10 @@ fn main() -> miette::Result<()> {
         autocxxgen_header_namer,
         cxxgen_header_namer,
     };
+    let codegen_options = autocxx_engine::CodegenOptions {
+        cpp_codegen_options,
+        ..Default::default()
+    };
     let depfile = match matches.value_of("depfile") {
         None => None,
         Some(depfile_path) => {
@@ -277,7 +281,7 @@ fn main() -> miette::Result<()> {
             incs.clone(),
             &extra_clang_args,
             dep_recorder,
-            &cpp_codegen_options,
+            &codegen_options,
         )?;
     }
 
@@ -305,7 +309,7 @@ fn main() -> miette::Result<()> {
             .flat_map(|file| file.get_cpp_buildables())
         {
             let generations = include_cxx
-                .generate_h_and_cxx(&cpp_codegen_options)
+                .generate_h_and_cxx(&codegen_options.cpp_codegen_options)
                 .expect("Unable to generate header and C++ code");
             for pair in generations.0 {
                 let cppname = name_cc_file(counter);
@@ -314,7 +318,7 @@ fn main() -> miette::Result<()> {
                 counter += 1;
             }
         }
-        drop(cpp_codegen_options);
+        drop(codegen_options);
         // Write placeholders to ensure we always make exactly 'n' of each file type.
         writer.write_placeholders(counter, desired_number, name_cc_file)?;
         writer.write_placeholders(

integration-tests/src/lib.rs

@@ -421,6 +421,8 @@ pub fn do_run_test_manual(
     builder_modifier: Option<BuilderModifier>,
     rust_code_checker: Option<CodeChecker>,
 ) -> Result<(), TestError> {
+    let builder_modifier = consider_forcing_wrapper_generation(builder_modifier);
+
     const HEADER_NAME: &str = "input.h";
     // Step 2: Write the C++ header snippet to a temp file
     let tdir = tempdir().unwrap();
@@ -510,3 +512,38 @@ pub fn do_run_test_manual(
     }
     Ok(())
 }
+
+/// If AUTOCXX_FORCE_WRAPPER_GENERATION is set, always force both C++
+/// and Rust side shims, for extra testing of obscure code paths.
+fn consider_forcing_wrapper_generation(
+    existing_builder_modifier: Option<BuilderModifier>,
+) -> Option<BuilderModifier> {
+    if std::env::var("AUTOCXX_FORCE_WRAPPER_GENERATION").is_err() {
+        existing_builder_modifier
+    } else {
+        Some(Box::new(ForceWrapperGeneration(existing_builder_modifier)))
+    }
+}
+
+struct ForceWrapperGeneration(Option<BuilderModifier>);
+
+impl BuilderModifierFns for ForceWrapperGeneration {
+    fn modify_autocxx_builder<'a>(
+        &self,
+        builder: Builder<'a, TestBuilderContext>,
+    ) -> Builder<'a, TestBuilderContext> {
+        let builder = builder.force_wrapper_generation(true);
+        if let Some(modifier) = &self.0 {
+            modifier.modify_autocxx_builder(builder)
+        } else {
+            builder
+        }
+    }
+    fn modify_cc_builder<'a>(&self, builder: &'a mut cc::Build) -> &'a mut cc::Build {
+        if let Some(modifier) = &self.0 {
+            modifier.modify_cc_builder(builder)
+        } else {
+            builder
+        }
+    }
+}