Repository: google/autocxx (rust)

Issue:
Overloaded functions not translated properly
bindgen outputs overloaded functions as `func1`, `func2` etc. since Rust doesn't support them. That's good enough for us, for now, but right now we just crash and burn instead.

Pull request:
Overloads
Fixes #120.

Patch:
diff --git a/engine/src/bridge_converter.rs b/engine/src/bridge_converter.rs
index 94f34d81..5951ef57 100644
--- a/engine/src/bridge_converter.rs
+++ b/engine/src/bridge_converter.rs
@@ -18,6 +18,7 @@ use crate::{
     additional_cpp_generator::AdditionalNeed,
     function_wrapper::{ArgumentConversion, FunctionWrapper, FunctionWrapperPayload},
     known_types::{is_known_type, known_type_substitute_path, should_dereference_in_cpp},
+    overload_tracker::OverloadTracker,
     types::make_ident,
 };
 use crate::{
@@ -65,6 +66,11 @@ impl Display for ConvertError {
     }
 }
 
+struct NamespaceContext {
+    overload_tracker: OverloadTracker,
+    method_impl_blocks: HashMap<String, ItemImpl>,
+}
+
 /// Results of a conversion.
 pub(crate) struct BridgeConversionResults {
     pub items: Vec<Item>,
@@ -248,7 +254,10 @@ impl<'a> BridgeConversion<'a> {
         ns: Namespace,
         output_items: &mut Vec<Item>,
     ) -> Result<(), ConvertError> {
-        let mut method_impl_blocks = HashMap::new();
+        let mut namespace_context = NamespaceContext {
+            method_impl_blocks: HashMap::new(),
+            overload_tracker: OverloadTracker::new(),
+        };
         for item in items {
             match item {
                 Item::ForeignMod(mut fm) => {
@@ -261,7 +270,7 @@ impl<'a> BridgeConversion<'a> {
                         // the contents of all bindgen 'extern "C"' mods into this
                         // one.
                     }
-                    self.convert_foreign_mod_items(items, &ns, &mut method_impl_blocks)?;
+                    self.convert_foreign_mod_items(items, &ns, &mut namespace_context)?;
                 }
                 Item::Struct(mut s) => {
                     let tyname = TypeName::new(&ns, &s.ident.to_string());
@@ -317,7 +326,12 @@ impl<'a> BridgeConversion<'a> {
                 _ => return Err(ConvertError::UnexpectedItemInMod),
             }
         }
-        output_items.extend(method_impl_blocks.drain().map(|(_, v)| Item::Impl(v)));
+        output_items.extend(
+            namespace_context
+                .method_impl_blocks
+                .drain()
+                .map(|(_, v)| Item::Impl(v)),
+        );
         let supers = std::iter::repeat(make_ident("super")).take(ns.depth() + 2);
         output_items.push(Item::Use(parse_quote! {
             #[allow(unused_imports)]
@@ -564,12 +578,12 @@ impl<'a> BridgeConversion<'a> {
         &mut self,
         foreign_mod_items: Vec<ForeignItem>,
         ns: &Namespace,
-        method_impl_blocks: &mut HashMap<String, ItemImpl>,
+        namespace_context: &mut NamespaceContext,
     ) -> Result<(), ConvertError> {
         for i in foreign_mod_items {
             match i {
                 ForeignItem::Fn(f) => {
-                    self.convert_foreign_fn(f, ns, method_impl_blocks)?;
+                    self.convert_foreign_fn(f, ns, namespace_context)?;
                 }
                 _ => return Err(ConvertError::UnexpectedForeignItem),
             }
@@ -581,7 +595,7 @@ impl<'a> BridgeConversion<'a> {
         &mut self,
         fun: ForeignItemFn,
         ns: &Namespace,
-        method_impl_blocks: &mut HashMap<String, ItemImpl>,
+        namespace_context: &mut NamespaceContext,
     ) -> Result<(), ConvertError> {
         // This function is one of the most complex parts of bridge_converter.
         // It needs to consider:
@@ -622,18 +636,25 @@ impl<'a> BridgeConversion<'a> {
         let is_a_method = self_ty.is_some();
 
         // Work out naming.
-        let mut rust_name = fun.sig.ident.to_string();
+        let initial_rust_name = fun.sig.ident.to_string();
+        let mut rust_name;
         let mut is_constructor = false;
+        let cpp_call_name;
         if let Some(self_ty) = &self_ty {
-            let method_prefix = self_ty.get_final_ident().to_string();
+            // Method.
+            let type_ident = self_ty.get_final_ident().to_string();
             // bindgen generates methods with the name:
             // {class}_{method name}
             // It then generates an impl section for the Rust type
             // with the original name, but we currently discard that impl section.
             // We want to feed cxx methods with just the method name, so let's
             // strip off the class name.
-            rust_name = rust_name[method_prefix.len() + 1..].to_string();
-            if rust_name.starts_with(&method_prefix) {
+            let overload_details = namespace_context
+                .overload_tracker
+                .get_method_real_name(&type_ident, &initial_rust_name);
+            cpp_call_name = overload_details.cpp_method_name;
+            rust_name = overload_details.rust_method_name;
+            if rust_name.starts_with(&type_ident) {
                 // It's a constructor. bindgen generates
                 // fn new(this: *Type, ...args)
                 // We want
@@ -642,13 +663,22 @@ impl<'a> BridgeConversion<'a> {
                 // fn make_unique(...args) -> UniquePtr<Type>
                 // If there are multiple constructors, bindgen generates
                 // new, new1, new2 etc. and we'll keep those suffixes.
-                let constructor_suffix = &rust_name[method_prefix.len()..];
+                let constructor_suffix = &rust_name[type_ident.len()..];
                 rust_name = format!("make_unique{}", constructor_suffix);
                 // Strip off the 'this' arg.
                 params = params.into_iter().skip(1).collect();
                 param_details.remove(0);
                 is_constructor = true;
             }
+        } else {
+            // Not a method.
+            // What's the name of the underlying C++ function call?
+            // If bindgen found overloaded methods, it may not be what it seems.
+            let overload_details = namespace_context
+                .overload_tracker
+                .get_function_real_name(&initial_rust_name);
+            cpp_call_name = overload_details.cpp_method_name;
+            rust_name = overload_details.rust_method_name;
         }
 
         // Analyze the return type, just as we previously did for the
@@ -693,6 +723,7 @@ impl<'a> BridgeConversion<'a> {
         // put something different into here if we have to do argument or
         // return type conversion, so get some mutable variables ready.
         let mut rust_name_attr = Vec::new();
+        let mut cpp_name_attr = Vec::new();
         let rust_name_ident = make_ident(&rust_name);
         let mut cxxbridge_name = rust_name_ident.clone();
 
@@ -700,7 +731,7 @@ impl<'a> BridgeConversion<'a> {
             // Generate a new layer of C++ code to wrap/unwrap parameters
             // and return values into/out of std::unique_ptrs.
             // First give instructions to generate the additional C++.
-            let cpp_construction_ident = cxxbridge_name;
+            let cpp_construction_ident = make_ident(&cpp_call_name);
             cxxbridge_name = make_ident(&if let Some(type_name) = &self_ty {
                 format!(
                     "{}_{}_up_wrapper",
@@ -759,7 +790,8 @@ impl<'a> BridgeConversion<'a> {
                         cxxbridge::#cxxbridge_name ( #(#arg_list),* )
                     }
                 });
-                let e = method_impl_blocks
+                let e = namespace_context
+                    .method_impl_blocks
                     .entry(type_name.to_string())
                     .or_insert_with(|| {
                         parse_quote! {
@@ -777,7 +809,13 @@ impl<'a> BridgeConversion<'a> {
                     ))
                     .unwrap();
             }
-        };
+        } else {
+            cpp_name_attr = Attribute::parse_outer
+                .parse2(quote!(
+                    #[cxx_name = #cpp_call_name]
+                ))
+                .unwrap();
+        }
         // Finally - namespace support. All the Types in everything
         // above this point are fully qualified. We need to unqualify them.
         // We need to do that _after_ the above wrapper_function_needed
@@ -805,6 +843,7 @@ impl<'a> BridgeConversion<'a> {
         self.extern_c_mod_items.push(ForeignItem::Fn(parse_quote!(
             #(#namespace_attr)*
             #(#rust_name_attr)*
+            #(#cpp_name_attr)*
             #vis fn #cxxbridge_name ( #params ) #ret_type;
         )));
         if !is_a_method {
diff --git a/engine/src/lib.rs b/engine/src/lib.rs
index cc624837..05752bda 100644
--- a/engine/src/lib.rs
+++ b/engine/src/lib.rs
@@ -18,6 +18,7 @@ mod byvalue_checker;
 mod function_wrapper;
 mod known_types;
 mod namespace_organizer;
+mod overload_tracker;
 mod parse;
 mod rust_pretty_printer;
 mod types;
@@ -245,6 +246,7 @@ impl IncludeCpp {
                 non_exhaustive: false,
             })
             .enable_cxx_namespaces()
+            .disable_nested_struct_naming()
             .generate_inline_functions(true)
             .layout_tests(false); // TODO revisit later
         for item in known_types::get_initial_blocklist() {
diff --git a/engine/src/overload_tracker.rs b/engine/src/overload_tracker.rs
new file mode 100644
index 00000000..debfc356
--- /dev/null
+++ b/engine/src/overload_tracker.rs
@@ -0,0 +1,182 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::collections::HashMap;
+
+#[derive(PartialEq, Debug)]
+pub(crate) struct MethodOverload {
+    pub(crate) cpp_method_name: String,
+    pub(crate) rust_method_name: String,
+}
+
+impl MethodOverload {
+    fn new(cpp_method_name: String, rust_method_name: String) -> Self {
+        Self {
+            cpp_method_name,
+            rust_method_name,
+        }
+    }
+}
+
+/// Registry of all the overloads of a function found within a given
+/// namespace (i.e. mod in bindgen's output).
+/// The idea here is that bindgen will output a series of overridden
+/// 'foo' functions as foo, foo1, foo2.
+/// We will recognize that sequence and call the correct underlying
+/// C++ function ("foo" in all cases).
+/// For extra complexity, if multiple types each have 'foo' methods
+/// it's part of the same global numbering series within bindgen
+/// output, whereas we would like to use plain old 'foo' as the method
+/// names in the impl blocks we generate. This is more important than
+/// it seems, because otherwise two different types with a 'get()'
+/// method would instead have a 'get()' and 'get1()' method in the
+/// bindings we generate.
+pub(crate) struct OverloadTracker {
+    offset_by_type_and_name: HashMap<String, HashMap<String, usize>>,
+    expected_next_by_name: HashMap<String, usize>,
+}
+
+static NULL_TYPE: &str = "<null>"; // for global functions not methods
+
+impl OverloadTracker {
+    pub(crate) fn new() -> Self {
+        Self {
+            offset_by_type_and_name: HashMap::new(),
+            expected_next_by_name: HashMap::new(),
+        }
+    }
+
+    fn split_name(found_name: &str) -> (&str, usize) {
+        for (pos, ch) in found_name.chars().rev().enumerate() {
+            if !ch.is_numeric() {
+                let split = found_name.len() - pos;
+                let prefix = &found_name[0..split];
+                let suffix = &found_name[split..];
+                let counter = if suffix.is_empty() {
+                    0
+                } else {
+                    suffix.parse::<usize>().unwrap()
+                };
+                return (prefix, counter);
+            }
+        }
+        panic!("Identifier was entirely numeric");
+    }
+
+    pub(crate) fn get_function_real_name<'a>(&mut self, found_name: &'a str) -> MethodOverload {
+        self.get_method_real_name(NULL_TYPE, found_name)
+    }
+
+    pub(crate) fn get_method_real_name(
+        &mut self,
+        type_name: &str,
+        found_name: &str,
+    ) -> MethodOverload {
+        let (fn_name, counter) = Self::split_name(found_name);
+        let expected_next_suffix = self
+            .expected_next_by_name
+            .entry(fn_name.to_owned())
+            .or_insert(0usize);
+        if counter != *expected_next_suffix {
+            // This is not some kind of overload thing.
+            // Instead, this is probably some function legitimately called 'insert2' or somesuch.
+            MethodOverload::new(found_name.to_owned(), found_name.to_owned())
+        } else {
+            // Possibly part of an overload sequence. We have no way to be sure
+            // but let's assume so.
+            *expected_next_suffix += 1;
+            let type_e = self
+                .offset_by_type_and_name
+                .entry(type_name.to_string())
+                .or_insert_with(HashMap::new);
+            let offset = type_e.entry(fn_name.to_string()).or_insert(counter);
+            let effective_count = counter - *offset;
+            MethodOverload::new(
+                fn_name.to_string(),
+                if effective_count == 0 {
+                    fn_name.to_string()
+                } else {
+                    format!("{}{}", fn_name, effective_count)
+                },
+            )
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{MethodOverload, OverloadTracker};
+
+    #[test]
+    fn test_by_function() {
+        let mut ot = OverloadTracker::new();
+        assert_eq!(
+            ot.get_function_real_name("job"),
+            MethodOverload::new("job".into(), "job".into())
+        );
+        assert_eq!(
+            ot.get_function_real_name("job1"),
+            MethodOverload::new("job".into(), "job1".into())
+        );
+        assert_eq!(
+            ot.get_function_real_name("job2"),
+            MethodOverload::new("job".into(), "job2".into())
+        );
+        assert_eq!(
+            ot.get_function_real_name("job24"),
+            MethodOverload::new("job24".into(), "job24".into())
+        );
+        assert_eq!(
+            ot.get_function_real_name("fish1"),
+            MethodOverload::new("fish1".into(), "fish1".into())
+        );
+        assert_eq!(
+            ot.get_function_real_name("fish2"),
+            MethodOverload::new("fish2".into(), "fish2".into())
+        );
+    }
+
+    #[test]
+    fn test_by_method() {
+        let mut ot = OverloadTracker::new();
+        assert_eq!(
+            ot.get_method_real_name("A", "do"),
+            MethodOverload::new("do".into(), "do".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("A", "do1"),
+            MethodOverload::new("do".into(), "do1".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("A", "dog"),
+            MethodOverload::new("dog".into(), "dog".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("A", "dog1"),
+            MethodOverload::new("dog".into(), "dog1".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("B", "do2"),
+            MethodOverload::new("do".into(), "do".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("B", "do3"),
+            MethodOverload::new("do".into(), "do1".into())
+        );
+        assert_eq!(
+            ot.get_method_real_name("C", "do2"),
+            MethodOverload::new("do2".into(), "do2".into())
+        );
+    }
+}
diff --git a/src/lib.rs b/src/lib.rs
index c99f0ef3..7d5a70f7 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -165,6 +165,25 @@
 /// ffi mod. However, at present there is an internal limitation that
 /// autocxx can't handle multiple symbols with the same identifier, even
 /// if they're in different namespaces. This will be fixed in future.
+///
+/// # Overloads - and identifiers ending in digits
+///
+/// C++ allows function overloads; Rust doesn't. `autocxx` follows the lead
+/// of `bindgen` here and generating overloads as `func`, `func1`, `func2` etc.
+///
+/// Unfortunately bindgen does not offer a means to distinguish such overloaded
+/// functions from those which are legitimately called `func`, `func1` etc.
+/// and so `autocxx` has to guess. Consider this a known limitation of autocxx
+/// and - for now - avoid functions ending in digits. This restriction may
+/// be solved in future if we can add metadata to the bindgen output or otherwise
+/// arrange for this information to be made available to autocxx.
+///
+/// # C++ classes - why do I get warnings?
+///
+/// autocxx is not currently able to distinguish a C++ struct from a C++ class.
+/// It currently assumes they're all structs. This results in warnings
+/// from most compilers, but could cause actual binary mismatches
+/// on some ABIs. This is a temporary known limitation.
 #[macro_export]
 macro_rules! include_cpp {
     (


Test patch:
diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs
index 562f055a..8ea1dd8a 100644
--- a/engine/src/integration_tests.rs
+++ b/engine/src/integration_tests.rs
@@ -1940,7 +1940,6 @@ fn test_overload_constructors() {
 }
 
 #[test]
-#[ignore] // https://github.com/google/autocxx/issues/120
 fn test_overload_functions() {
     let cxx = indoc! {"
         void daft(uint32_t a) {}
@@ -1956,10 +1955,57 @@ fn test_overload_functions() {
             uint32_t a;
         };
         struct Norma {
+            Norma() {}
             uint32_t a;
         };
-        void daft(uint32_t a);
-        void daft(uint8_t a);
+        void daft(uint32_t);
+        void daft(uint8_t);
+        void daft(std::string);
+        void daft(Fred);
+        void daft(Norma);
+    "};
+    let rs = quote! {
+        ffi::daft(32);
+        ffi::daft1(8);
+        ffi::daft2(ffi::make_string("hello"));
+        let b = ffi::Fred { a: 3 };
+        ffi::daft3(b);
+        let c = ffi::Norma::make_unique();
+        ffi::daft4(c);
+    };
+    run_test(
+        cxx,
+        hdr,
+        rs,
+        &["Norma", "daft", "daft1", "daft2", "daft3", "daft4"],
+        &["Fred"],
+    );
+}
+
+#[test]
+#[ignore] // At present, bindgen generates two separate 'daft1'
+          // functions here, and there's not much we can do about that.
+fn test_overload_numeric_functions() {
+    // Because bindgen deals with conflicting overloaded functions by
+    // appending a numeric suffix, let's see if we can cope.
+    let cxx = indoc! {"
+        void daft1(uint32_t) {}
+        void daft2(uint8_t) {}
+        void daft(std::string) {}
+        void daft(Fred) {}
+        void daft(Norma) {}
+    "};
+    let hdr = indoc! {"
+        #include <cstdint>
+        #include <string>
+        struct Fred {
+            uint32_t a;
+        };
+        struct Norma {
+            uint32_t a;
+        };
+        void daft1(uint32_t a);
+        void daft2(uint8_t a);
         void daft(std::string a);
         void daft(Fred a);
         void daft(Norma a);
@@ -1973,18 +2019,23 @@ fn test_overload_functions() {
         let c = ffi::Norma::make_unique();
         ffi::daft4(c);
     };
-    run_test(cxx, hdr, rs, &["Norma", "daft", "daft1", "daft2", "daft3", "daft4"], &["Fred"]);
+    run_test(
+        cxx,
+        hdr,
+        rs,
+        &["Norma", "daft", "daft1", "daft2", "daft3", "daft4"],
+        &["Fred"],
+    );
 }
 
 #[test]
-#[ignore] // https://github.com/google/autocxx/issues/120
 fn test_overload_methods() {
     let cxx = indoc! {"
-        void Bob::daft(uint32_t a) {}
-        void Bob::daft(uint8_t a) {}
-        void Bob::daft(std::string a) {}
-        void Bob::daft(Fred a) {}
-        void Bob::daft(Norma a) {}
+        void Bob::daft(uint32_t) const {}
+        void Bob::daft(uint8_t) const {}
+        void Bob::daft(std::string) const {}
+        void Bob::daft(Fred) const {}
+        void Bob::daft(Norma) const {}
     "};
     let hdr = indoc! {"
         #include <cstdint>
@@ -1993,15 +2044,16 @@ fn test_overload_methods() {
             uint32_t a;
         };
         struct Norma {
+            Norma() {}
             uint32_t a;
         };
         struct Bob {
             uint32_t a;
-            void daft(uint32_t a);
-            void daft(uint8_t a);
-            void daft(std::string a);
-            void daft(Fred a);
-            void daft(Norma a);
+            void daft(uint32_t) const;
+            void daft(uint8_t) const;
+            void daft(std::string) const;
+            void daft(Fred) const;
+            void daft(Norma) const;
         };
     "};
     let rs = quote! {