Repository: google/autocxx (rust)

Issue:
Pointer pointed to something unsupported: say what type wasn't supported.
I've just tried generating an FFI for a function like:
```
void *ykllvmwrap_irtrace_compile(
    char *funcnames[], size_t bbs[], size_t tracelen, char *faddrkeys[],
    void *faddrvals[], size_t faddrlen, void *bitcodedata, uint64_t bitcodelen,
    int debuginfofd, char *debuginfopath);
```

And got (in the html docs):

> autocxx bindings couldn’t be generated: Pointer pointed to something unsupported

It would be useful if the error message could tell me which type(s) is/are the problem.

Cheers

Pull request:
Better diagnostics for pointer-to-pointer cases
Fixes #1270.

Patch:
diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs
index f7b2b977..947b582e 100644
--- a/engine/src/conversion/analysis/fun/mod.rs
+++ b/engine/src/conversion/analysis/fun/mod.rs
@@ -40,8 +40,8 @@ use itertools::Itertools;
 use proc_macro2::Span;
 use quote::quote;
 use syn::{
-    parse_quote, punctuated::Punctuated, token::Comma, FnArg, Ident, Pat, ReturnType, Type,
-    TypePath, TypePtr, TypeReference, Visibility,
+    parse_quote, punctuated::Punctuated, token::Comma, FnArg, Ident, Pat, PatType, ReturnType,
+    Type, TypePath, TypePtr, TypeReference, Visibility,
 };
 
 use crate::{
@@ -748,6 +748,10 @@ impl<'a> FnAnalyzer<'a> {
                     sophistication,
                     false,
                 )
+                .map_err(|err| ConvertErrorFromCpp::Argument {
+                    arg: describe_arg(i),
+                    err: Box::new(err),
+                })
             })
             .partition(Result::is_ok);
         let (mut params, mut param_details): (Punctuated<_, Comma>, Vec<_>) =
@@ -2327,3 +2331,14 @@ impl HasFieldsAndBases for Api<FnPrePhase2> {
         }
     }
 }
+
+/// Stringify a function argument for diagnostics
+fn describe_arg(arg: &FnArg) -> String {
+    match arg {
+        FnArg::Receiver(_) => "the function receiver (this/self paramter)".into(),
+        FnArg::Typed(PatType { pat, .. }) => match pat.as_ref() {
+            Pat::Ident(pti) => pti.ident.to_string(),
+            _ => "another argument we don't know how to describe".into(),
+        },
+    }
+}
diff --git a/engine/src/conversion/analysis/type_converter.rs b/engine/src/conversion/analysis/type_converter.rs
index ada512be..a7adaf8d 100644
--- a/engine/src/conversion/analysis/type_converter.rs
+++ b/engine/src/conversion/analysis/type_converter.rs
@@ -509,7 +509,11 @@ impl<'a> TypeConverter<'a> {
     fn ensure_pointee_is_valid(ptr: &TypePtr) -> Result<(), ConvertErrorFromCpp> {
         match *ptr.elem {
             Type::Path(..) => Ok(()),
-            _ => Err(ConvertErrorFromCpp::InvalidPointee),
+            Type::Array(..) => Err(ConvertErrorFromCpp::InvalidArrayPointee),
+            Type::Ptr(..) => Err(ConvertErrorFromCpp::InvalidPointerPointee),
+            _ => Err(ConvertErrorFromCpp::InvalidPointee(
+                ptr.elem.to_token_stream().to_string(),
+            )),
         }
     }
 
diff --git a/engine/src/conversion/convert_error.rs b/engine/src/conversion/convert_error.rs
index 46845cd7..097d79d2 100644
--- a/engine/src/conversion/convert_error.rs
+++ b/engine/src/conversion/convert_error.rs
@@ -75,8 +75,12 @@ pub enum ConvertErrorFromCpp {
     UnexpectedUseStatement(Option<String>),
     #[error("Type {} was parameterized over something complex which we don't yet support", .0.to_cpp_name())]
     TemplatedTypeContainingNonPathArg(QualifiedName),
-    #[error("Pointer pointed to something unsupported")]
-    InvalidPointee,
+    #[error("Pointer pointed to an array, which is not yet supported")]
+    InvalidArrayPointee,
+    #[error("Pointer pointed to another pointer, which is not yet supported")]
+    InvalidPointerPointee,
+    #[error("Pointer pointed to something unsupported (autocxx only supports pointers to named types): {0}")]
+    InvalidPointee(String),
     #[error("The 'generate' or 'generate_pod' directive for '{0}' did not result in any code being generated. Perhaps this was mis-spelled or you didn't qualify the name with any namespaces? Otherwise please report a bug.")]
     DidNotGenerateAnything(String),
     #[error("Found an attempt at using a forward declaration ({}) inside a templated cxx type such as UniquePtr or CxxVector. If the forward declaration is a typedef, perhaps autocxx wasn't sure whether or not it involved a forward declaration. If you're sure it didn't, then you may be able to solve this by using instantiable!.", .0.to_cpp_name())]
@@ -149,6 +153,12 @@ pub enum ConvertErrorFromCpp {
     ReferringToGenericTypeParam,
     #[error("This forward declaration was nested within another struct/class. autocxx is unable to represent inner types if they are forward declarations.")]
     ForwardDeclaredNestedType,
+    #[error("Problem handling function argument {arg}: {err}")]
+    Argument {
+        arg: String,
+        #[source]
+        err: Box<ConvertErrorFromCpp>,
+    },
 }
 
 /// Error types derived from Rust code. This is separate from [`ConvertError`] because these


Test patch:
diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs
index 4f86b114..3954e35c 100644
--- a/integration-tests/tests/integration_test.rs
+++ b/integration-tests/tests/integration_test.rs
@@ -18,7 +18,7 @@ use crate::{
 };
 use autocxx_integration_tests::{
     directives_from_lists, do_run_test, do_run_test_manual, run_generate_all_test, run_test,
-    run_test_ex, run_test_expect_fail, run_test_expect_fail_ex, BuilderModifier, TestError,
+    run_test_ex, run_test_expect_fail, run_test_expect_fail_ex, TestError,
 };
 use indoc::indoc;
 use itertools::Itertools;
@@ -842,7 +842,8 @@ fn test_take_nonpod_by_ptr_in_wrapped_method() {
     run_test("", hdr, rs, &["A", "Bob", "C"], &[]);
 }
 
-fn run_char_test(builder_modifier: Option<BuilderModifier>) {
+#[test]
+fn test_take_char_by_ptr_in_wrapped_method() {
     let hdr = indoc! {"
         #include <cstdint>
         #include <memory>
@@ -872,25 +873,7 @@ fn run_char_test(builder_modifier: Option<BuilderModifier>) {
         assert_eq!(unsafe { ch.as_ref()}.unwrap(), &104i8);
         assert_eq!(unsafe { a.as_ref().unwrap().take_char(ch, c2) }, 104);
     };
-    run_test_ex(
-        "",
-        hdr,
-        rs,
-        directives_from_lists(&["A", "C"], &[], None),
-        builder_modifier,
-        None,
-        None,
-    );
-}
-
-#[test]
-fn test_take_char_by_ptr_in_wrapped_method() {
-    run_char_test(None)
-}
-
-#[test]
-fn test_take_char_by_ptr_in_wrapped_method_with_unsigned_chars() {
-    run_char_test(make_clang_arg_adder(&["-funsigned-char"]))
+    run_test("", hdr, rs, &["A", "C"], &[]);
 }
 
 #[test]
@@ -12082,20 +12065,6 @@ fn test_issue_1229() {
     do_run_test_manual("", hdr, rs, None, None).unwrap();
 }
 
-#[test]
-fn test_ignore_va_list() {
-    let hdr = indoc! {"
-        #include <stdarg.h>
-        class A {
-        public:
-            A() {}
-            void fn(va_list) {}
-        };
-    "};
-    let rs = quote! {};
-    run_test("", hdr, rs, &["A"], &[]);
-}
-
 // Yet to test:
 // - Ifdef
 // - Out param pointers