Work towards spotting deleted move constructors.

This works but it turns out we need to do the same for copy constructors.
It's also a bit messy thus far.

Work towards #489.

engine/Cargo.toml

@@ -36,8 +36,8 @@ log = "0.4"
 proc-macro2 = "1.0.11"
 quote = "1.0"
 indoc = "1.0"
-autocxx-bindgen = "=0.59.10"
-#autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "rvalue-references" }
+#autocxx-bindgen = "=0.59.10"
+autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "denote-deleted-move-constructors" }
 itertools = "0.10"
 cc = { version = "1.0", optional = true }
 unzip-n = "0.1.2"

engine/src/conversion/analysis/name_check.rs

@@ -76,6 +76,7 @@ pub(crate) fn check_names(apis: Vec<Api<FnPhase>>) -> Vec<Api<FnPhase>> {
         | Api::RustSubclassFn { .. }
         | Api::RustFn { .. }
         | Api::IgnoredItem { .. } => Ok(Box::new(std::iter::once(api))),
+        Api::DeletedMoveConstructor { .. } => Ok(Box::new(std::iter::empty())),
     });
 
     // Reject any names which are duplicates within the cxx bridge mod,

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

@@ -43,6 +43,7 @@ pub(crate) struct PodAnalysis {
     /// abstract or not.
     pub(crate) castable_bases: HashSet<QualifiedName>,
     pub(crate) field_deps: HashSet<QualifiedName>,
+    pub(crate) movable: bool,
 }
 
 pub(crate) struct PodPhase;
@@ -59,14 +60,16 @@ impl AnalysisPhase for PodPhase {
 /// and an object which can be used to query the POD status of any
 /// type whether or not it's one of the [Api]s.
 pub(crate) fn analyze_pod_apis(
-    apis: Vec<Api<TypedefPhase>>,
+    mut apis: Vec<Api<TypedefPhase>>,
     config: &IncludeCppConfig,
 ) -> Result<Vec<Api<PodPhase>>, ConvertError> {
     // This next line will return an error if any of the 'generate_pod'
     // directives from the user can't be met because, for instance,
     // a type contains a std::string or some other type which can't be
     // held safely by value in Rust.
     let byvalue_checker = ByValueChecker::new_from_apis(&apis, config)?;
+    // We'll also note which types have deleted move constructors.
+    let deleted_move_constructors = find_deleted_move_constructors(&mut apis);
     let mut extra_apis = Vec::new();
     let mut type_converter = TypeConverter::new(config, &apis);
     let mut results = Vec::new();
@@ -82,6 +85,7 @@ pub(crate) fn analyze_pod_apis(
                 name,
                 details,
                 config,
+                &deleted_move_constructors,
             )
         },
         analyze_enum,
@@ -103,6 +107,7 @@ pub(crate) fn analyze_pod_apis(
                 name,
                 details,
                 config,
+                &deleted_move_constructors,
             )
         },
         analyze_enum,
@@ -128,7 +133,9 @@ fn analyze_struct(
     name: ApiName,
     mut details: Box<StructDetails>,
     config: &IncludeCppConfig,
+    deleted_move_constructors: &HashSet<QualifiedName>,
 ) -> Result<Box<dyn Iterator<Item = Api<PodPhase>>>, ConvertErrorWithContext> {
+    let movable = !deleted_move_constructors.contains(&name.name);
     let id = name.name.get_final_ident();
     if details.vis != CppVisibility::Public {
         return Err(ConvertErrorWithContext(
@@ -169,6 +176,7 @@ fn analyze_struct(
             bases: bases.into_keys().collect(),
             castable_bases,
             field_deps,
+            movable,
         },
     })))
 }
@@ -206,3 +214,16 @@ fn get_bases(item: &ItemStruct) -> HashMap<QualifiedName, bool> {
         })
         .collect()
 }
+
+fn find_deleted_move_constructors(apis: &mut Vec<Api<TypedefPhase>>) -> HashSet<QualifiedName> {
+    // The nightly API Vec::drain_filter would be nice here
+    let results = apis
+        .iter()
+        .filter_map(|api| match api {
+            Api::DeletedMoveConstructor { name } => Some(name.name.clone()),
+            _ => None,
+        })
+        .collect();
+    apis.retain(|api| !matches!(api, Api::DeletedMoveConstructor { .. }));
+    results
+}

engine/src/conversion/analysis/type_converter.rs

@@ -509,6 +509,7 @@ impl<'a> TypeConverter<'a> {
                 | Api::CType { .. }
                 | Api::RustSubclassFn { .. }
                 | Api::IgnoredItem { .. }
+                | Api::DeletedMoveConstructor { .. }
                 | Api::RustFn { .. } => None,
             })
             .cloned()

engine/src/conversion/api.rs

@@ -391,6 +391,9 @@ pub(crate) enum Api<T: AnalysisPhase> {
         name: SubclassName,
         superclass: QualifiedName,
     },
+    /// A C++ function which is marked =delete
+    /// For now we only worry about move constructors here.
+    DeletedMoveConstructor { name: ApiName },
 }
 
 pub(crate) struct RustSubclassFnDetails {
@@ -422,6 +425,7 @@ impl<T: AnalysisPhase> Api<T> {
             Api::RustFn { name, .. } => name,
             Api::RustSubclassFn { name, .. } => name,
             Api::Subclass { name, .. } => &name.0,
+            Api::DeletedMoveConstructor { name } => name,
         }
     }
 

engine/src/conversion/codegen_rs/impl_item_creator.rs

@@ -15,11 +15,11 @@
 use autocxx_parser::IncludeCppConfig;
 use syn::{parse_quote, Ident, Item};
 
-pub(crate) fn create_impl_items(id: &Ident, config: &IncludeCppConfig) -> Vec<Item> {
+pub(crate) fn create_impl_items(id: &Ident, movable: bool, config: &IncludeCppConfig) -> Vec<Item> {
     if config.exclude_impls {
         return vec![];
     }
-    vec![
+    let mut results = vec![
         Item::Impl(parse_quote! {
             impl UniquePtr<#id> {}
         }),
@@ -29,8 +29,11 @@ pub(crate) fn create_impl_items(id: &Ident, config: &IncludeCppConfig) -> Vec<It
         Item::Impl(parse_quote! {
             impl WeakPtr<#id> {}
         }),
-        Item::Impl(parse_quote! {
+    ];
+    if movable {
+        results.push(Item::Impl(parse_quote! {
             impl CxxVector<#id> {}
-        }),
-    ]
+        }))
+    }
+    results
 }

engine/src/conversion/codegen_rs/mod.rs

@@ -521,6 +521,7 @@ impl<'a> RsCodeGenerator<'a> {
                     &name,
                     id,
                     analysis.kind,
+                    analysis.movable,
                     || Some((Item::Struct(details.item), doc_attr)),
                     associated_methods,
                     layout,
@@ -532,6 +533,7 @@ impl<'a> RsCodeGenerator<'a> {
                     &name,
                     id,
                     TypeKind::Pod,
+                    true,
                     || Some((Item::Enum(item), doc_attr)),
                     associated_methods,
                     None,
@@ -541,6 +543,7 @@ impl<'a> RsCodeGenerator<'a> {
                 &name,
                 id,
                 TypeKind::Abstract,
+                false, // assume for now that these types can't be kept in a Vector
                 || None,
                 associated_methods,
                 None,
@@ -594,6 +597,7 @@ impl<'a> RsCodeGenerator<'a> {
                 self.generate_subclass(name, &superclass, methods, generate_peer_constructor)
             }
             Api::IgnoredItem { err, ctx, .. } => Self::generate_error_entry(err, ctx),
+            Api::DeletedMoveConstructor { .. } => RsCodegenResult::default(),
         }
     }
 
@@ -719,7 +723,9 @@ impl<'a> RsCodeGenerator<'a> {
         });
         RsCodegenResult {
             extern_c_mod_items,
-            bridge_items: create_impl_items(&cpp_id, self.config),
+            // For now we just assume we can't keep subclasses in vectors.
+            // That's the reason for the 'false'
+            bridge_items: create_impl_items(&cpp_id, false, self.config),
             bindgen_mod_items,
             materializations: vec![Use::Custom(Box::new(parse_quote! {
                 pub use cxxbridge::#cpp_id;
@@ -810,6 +816,7 @@ impl<'a> RsCodeGenerator<'a> {
         name: &QualifiedName,
         id: Ident,
         type_kind: TypeKind,
+        movable: bool,
         item_creator: F,
         associated_methods: &HashMap<QualifiedName, Vec<SuperclassMethod>>,
         layout: Option<Layout>,
@@ -857,7 +864,7 @@ impl<'a> RsCodeGenerator<'a> {
                 RsCodegenResult {
                     global_items: self.generate_extern_type_impl(type_kind, name),
                     impl_entry: None,
-                    bridge_items: create_impl_items(&id, self.config),
+                    bridge_items: create_impl_items(&id, movable, self.config),
                     extern_c_mod_items: vec![self.generate_cxxbridge_type(name, true, None)],
                     bindgen_mod_items,
                     materializations,

engine/src/conversion/error_reporter.rs

@@ -112,6 +112,11 @@ pub(crate) fn convert_apis<FF, SF, EF, TF, A, B: 'static>(
             Api::RustFn { name, sig, path } => {
                 Ok(Box::new(std::iter::once(Api::RustFn { name, sig, path })))
             }
+            Api::DeletedMoveConstructor { name } => {
+                Ok(Box::new(std::iter::once(Api::DeletedMoveConstructor {
+                    name,
+                })))
+            }
             Api::RustSubclassFn {
                 name,
                 subclass,

engine/src/conversion/parse/parse_foreign_mod.rs

@@ -12,14 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::conversion::api::ApiName;
+use crate::conversion::api::{ApiName, Api};
 use crate::conversion::doc_attr::get_doc_attr;
 use crate::conversion::error_reporter::report_any_error;
 use crate::conversion::{
     api::{FuncToConvert, UnanalyzedApi},
     convert_error::ConvertErrorWithContext,
     convert_error::ErrorContext,
 };
+use crate::types::make_ident;
 use crate::{
     conversion::ConvertError,
     types::{Namespace, QualifiedName},
@@ -45,6 +46,7 @@ pub(crate) struct ParseForeignMod {
     // function name to type name.
     method_receivers: HashMap<Ident, QualifiedName>,
     ignored_apis: Vec<UnanalyzedApi>,
+    deleted_move_constructors: Vec<Ident>,
 }
 
 impl ParseForeignMod {
@@ -54,6 +56,7 @@ impl ParseForeignMod {
             funcs_to_convert: Vec::new(),
             method_receivers: HashMap::new(),
             ignored_apis: Vec::new(),
+            deleted_move_constructors: Vec::new(),
         }
     }
 
@@ -73,6 +76,17 @@ impl ParseForeignMod {
         match i {
             ForeignItem::Fn(item) => {
                 let annotations = BindgenSemanticAttributes::new(&item.attrs);
+                let is_move_constructor = annotations.is_move_constructor();
+                let original_name = annotations.get_original_name();
+                let is_deleted = annotations.has_attr("deleted_fn");
+                if is_deleted {
+                    if is_move_constructor {
+                        self.deleted_move_constructors
+                            .push(original_name.map(make_ident).unwrap_or(item.sig.ident));
+                    }
+                    // Do not generate actual functions for deleted functions.
+                    return Ok(());
+                }
                 let doc_attr = get_doc_attr(&item.attrs);
                 self.funcs_to_convert.push(FuncToConvert {
                     self_ty: None,
@@ -83,11 +97,11 @@ impl ParseForeignMod {
                     vis: item.vis,
                     virtualness: annotations.get_virtualness(),
                     cpp_vis: annotations.get_cpp_visibility(),
-                    is_move_constructor: annotations.is_move_constructor(),
+                    is_move_constructor,
                     unused_template_param: annotations
                         .has_attr("unused_template_param_in_arg_or_return"),
                     references: annotations.get_reference_parameters_and_return(),
-                    original_name: annotations.get_original_name(),
+                    original_name,
                     synthesized_this_type: None,
                     synthesis: None,
                 });
@@ -148,6 +162,11 @@ impl ParseForeignMod {
                 name_for_gc: None,
             })
         }
+        apis.extend(self.deleted_move_constructors.into_iter().map(
+            |id| Api::DeletedMoveConstructor {
+                name: ApiName::new(&self.ns, id),
+            }, // TODO consider whether we need to respect original name annotations
+        ))
     }
 }
 

integration-tests/src/tests.rs

@@ -4897,7 +4897,6 @@ fn test_issue_490() {
 }
 
 #[test]
-#[ignore] // https://github.com/google/autocxx/issues/489
 fn test_immovable_object() {
     let hdr = indoc! {"
         class A {