extern_rust_function types: diagnostics

Previously extern_rust_function support was highly basic and
didn't usually work at all. This change improevs matters:

* it allows us to emit errors with miette diagnostics from
  this part of the codebase, giving spans in case of trouble
* it validates that the parameters to such a function are
  valid to be used by cxx, as is the return type
* it detects types used as parameters and return type and
  ensures they are not garbage collected elsewhere in autocxx
* it adds a code example.
* we remove a field from RustFun which we didn't actually need
  and replace it with a simple Boolean.

Builds on test case from #1166 (thanks!) and there's a bit of
discussion in #1167.

Cargo.toml

@@ -32,7 +32,7 @@ moveit = { version = "0.5", features = [ "cxx" ] }
 
 [workspace]
 members = ["parser", "engine", "gen/cmd", "gen/build", "macro", "demo", "tools/reduce", "tools/mdbook-preprocessor", "integration-tests"]
-exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/chromium-fake-render-frame-host", "examples/pod", "examples/non-trivial-type-on-stack", "examples/llvm", "examples/reference-wrappers", "tools/stress-test"]
+exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/chromium-fake-render-frame-host", "examples/pod", "examples/non-trivial-type-on-stack", "examples/llvm", "examples/reference-wrappers", "examples/cpp_calling_rust", "tools/stress-test"]
 
 #[patch.crates-io]
 #cxx = { path="../cxx" }

engine/src/ast_discoverer.rs

@@ -103,7 +103,7 @@ impl<'b> PerModDiscoveries<'b> {
                     self.discoveries.extern_rust_funs.push(RustFun {
                         path: self.deeper_path(&fun.sig.ident),
                         sig: fun.sig.clone(),
-                        receiver: None,
+                        has_receiver: false,
                     });
                 }
             }
@@ -347,7 +347,7 @@ impl<'b> PerModDiscoveries<'b> {
                     self.discoveries.extern_rust_funs.push(RustFun {
                         path: self.deeper_path(&itm.sig.ident),
                         sig,
-                        receiver: Some(receiver.get_final_ident().clone()),
+                        has_receiver: true,
                     });
                     self.discoveries.extern_rust_types.push(receiver.clone())
                 } else {

engine/src/conversion/analysis/deps.rs

@@ -52,7 +52,7 @@ impl HasDependencies for Api<FnPrePhase1> {
                 superclass,
             } => Box::new(std::iter::once(superclass)),
             Api::RustSubclassFn { details, .. } => Box::new(details.dependencies.iter()),
-            Api::RustFn { receiver, .. } => Box::new(receiver.iter()),
+            Api::RustFn { deps, .. } => Box::new(deps.iter()),
             _ => Box::new(std::iter::empty()),
         }
     }
@@ -105,7 +105,7 @@ impl HasDependencies for Api<FnPhase> {
                 superclass,
             } => Box::new(std::iter::once(superclass)),
             Api::RustSubclassFn { details, .. } => Box::new(details.dependencies.iter()),
-            Api::RustFn { receiver, .. } => Box::new(receiver.iter()),
+            Api::RustFn { deps, .. } => Box::new(deps.iter()),
             _ => Box::new(std::iter::empty()),
         }
     }

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

@@ -1363,7 +1363,9 @@ impl<'a> FnAnalyzer<'a> {
 
         // Naming, part two.
         // Work out our final naming strategy.
-        validate_ident_ok_for_cxx(&cxxbridge_name.to_string()).unwrap_or_else(set_ignore_reason);
+        validate_ident_ok_for_cxx(&cxxbridge_name.to_string())
+            .map_err(ConvertErrorFromCpp::InvalidIdent)
+            .unwrap_or_else(set_ignore_reason);
         let rust_name_ident = make_ident(&rust_name);
         let rust_rename_strategy = match kind {
             _ if rust_wrapper_needed => RustRenameStrategy::RenameUsingWrapperFunction,
@@ -1645,7 +1647,8 @@ impl<'a> FnAnalyzer<'a> {
                         syn::Pat::Ident(pp)
                     }
                     syn::Pat::Ident(pp) => {
-                        validate_ident_ok_for_cxx(&pp.ident.to_string())?;
+                        validate_ident_ok_for_cxx(&pp.ident.to_string())
+                            .map_err(ConvertErrorFromCpp::InvalidIdent)?;
                         pointer_treatment = references.param_treatment(&pp.ident);
                         syn::Pat::Ident(pp)
                     }

engine/src/conversion/analysis/name_check.rs

@@ -109,7 +109,7 @@ fn validate_all_segments_ok_for_cxx(
     items: impl Iterator<Item = String>,
 ) -> Result<(), ConvertErrorFromCpp> {
     for seg in items {
-        validate_ident_ok_for_cxx(&seg)?;
+        validate_ident_ok_for_cxx(&seg).map_err(ConvertErrorFromCpp::InvalidIdent)?;
     }
     Ok(())
 }

engine/src/conversion/analysis/type_converter.rs

@@ -256,7 +256,9 @@ impl<'a> TypeConverter<'a> {
         }
 
         let original_tn = QualifiedName::from_type_path(&typ);
-        original_tn.validate_ok_for_cxx()?;
+        original_tn
+            .validate_ok_for_cxx()
+            .map_err(ConvertErrorFromCpp::InvalidIdent)?;
         if self.config.is_on_blocklist(&original_tn.to_cpp_name()) {
             return Err(ConvertErrorFromCpp::Blocked(original_tn));
         }
@@ -455,7 +457,7 @@ impl<'a> TypeConverter<'a> {
     ) -> Result<Annotated<Type>, ConvertErrorFromCpp> {
         match pointer_treatment {
             PointerTreatment::Pointer => {
-                crate::known_types::ensure_pointee_is_valid(&ptr)?;
+                Self::ensure_pointee_is_valid(&ptr)?;
                 let innerty =
                     self.convert_boxed_type(ptr.elem, ns, &TypeConversionContext::WithinReference)?;
                 ptr.elem = innerty.ty;
@@ -490,7 +492,7 @@ impl<'a> TypeConverter<'a> {
                 Ok(outer)
             }
             PointerTreatment::RValueReference => {
-                crate::known_types::ensure_pointee_is_valid(&ptr)?;
+                Self::ensure_pointee_is_valid(&ptr)?;
                 let innerty =
                     self.convert_boxed_type(ptr.elem, ns, &TypeConversionContext::WithinReference)?;
                 ptr.elem = innerty.ty;
@@ -504,6 +506,13 @@ impl<'a> TypeConverter<'a> {
         }
     }
 
+    fn ensure_pointee_is_valid(ptr: &TypePtr) -> Result<(), ConvertErrorFromCpp> {
+        match *ptr.elem {
+            Type::Path(..) => Ok(()),
+            _ => Err(ConvertErrorFromCpp::InvalidPointee),
+        }
+    }
+
     fn get_templated_typename(
         &mut self,
         rs_definition: &Type,

engine/src/conversion/api.rs

@@ -531,7 +531,7 @@ pub(crate) enum Api<T: AnalysisPhase> {
     RustFn {
         name: ApiName,
         details: RustFun,
-        receiver: Option<QualifiedName>,
+        deps: Vec<QualifiedName>,
     },
     /// Some function for the extern "Rust" block.
     RustSubclassFn {

engine/src/conversion/codegen_rs/mod.rs

@@ -685,34 +685,28 @@ impl<'a> RsCodeGenerator<'a> {
                 details:
                     RustFun {
                         path,
-                        sig,
-                        receiver: None,
+                        mut sig,
+                        has_receiver,
                         ..
                     },
                 ..
-            } => RsCodegenResult {
-                global_items: vec![parse_quote! {
-                    use super::#path;
-                }],
-                extern_rust_mod_items: vec![parse_quote! {
-                    #sig;
-                }],
-                ..Default::default()
-            },
-            Api::RustFn {
-                details:
-                    RustFun {
-                        sig,
-                        receiver: Some(_),
-                        ..
+            } => {
+                sig.inputs = unqualify_params(sig.inputs);
+                sig.output = unqualify_ret_type(sig.output);
+                RsCodegenResult {
+                    global_items: if !has_receiver {
+                        vec![parse_quote! {
+                            use super::#path;
+                        }]
+                    } else {
+                        Vec::new()
                     },
-                ..
-            } => RsCodegenResult {
-                extern_rust_mod_items: vec![parse_quote! {
-                    #sig;
-                }],
-                ..Default::default()
-            },
+                    extern_rust_mod_items: vec![parse_quote! {
+                        #sig;
+                    }],
+                    ..Default::default()
+                }
+            }
             Api::RustSubclassFn {
                 details, subclass, ..
             } => Self::generate_subclass_fn(id, *details, subclass),

engine/src/conversion/conversion_tests.rs

@@ -34,6 +34,7 @@ fn do_test(input: ItemMod) {
         UnsafePolicy::AllFunctionsSafe,
         inclusions,
         &CppCodegenOptions::default(),
+        "",
     )
     .unwrap();
 }

engine/src/conversion/convert_error.rs

@@ -9,18 +9,34 @@
 use indexmap::set::IndexSet as HashSet;
 
 use itertools::Itertools;
+use miette::{Diagnostic, SourceSpan};
+use proc_macro2::Span;
 use syn::Ident;
 use thiserror::Error;
 
 use crate::{
-    known_types,
-    types::{make_ident, Namespace, QualifiedName},
+    known_types, proc_macro_span_to_miette_span,
+    types::{make_ident, InvalidIdentError, Namespace, QualifiedName},
 };
 
-#[derive(Debug, Clone, Error)]
-pub enum ConvertErrorFromCpp {
+/// Errors which can occur during conversion
+#[derive(Debug, Clone, Error, Diagnostic)]
+pub enum ConvertError {
     #[error("The initial run of 'bindgen' did not generate any content. This might be because none of the requested items for generation could be converted.")]
     NoContent,
+    #[error(transparent)]
+    Cpp(ConvertErrorFromCpp),
+    #[error(transparent)]
+    #[diagnostic(transparent)]
+    Rust(LocatedConvertErrorFromRust),
+}
+
+/// Errors that can occur during conversion which are detected from some C++
+/// source code. Currently, we do not gain span information from bindgen
+/// so these errors are presented without useful source code snippets.
+/// We hope to change this in future.
+#[derive(Debug, Clone, Error)]
+pub enum ConvertErrorFromCpp {
     #[error("An item was requested using 'generate_pod' which was not safe to hold by value in Rust. {0}")]
     UnsafePodType(String),
     #[error("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.")]
@@ -63,14 +79,12 @@ pub enum ConvertErrorFromCpp {
     Blocked(QualifiedName),
     #[error("This function or method uses a type where one of the template parameters was incomprehensible to bindgen/autocxx - probably because it uses template specialization.")]
     UnusedTemplateParam,
-    #[error("Names containing __ are reserved by C++ so not acceptable to cxx")]
-    TooManyUnderscores,
     #[error("This item relies on a type not known to autocxx ({})", .0.to_cpp_name())]
     UnknownDependentType(QualifiedName),
     #[error("This item depends on some other type(s) which autocxx could not generate, some of them are: {}", .0.iter().join(", "))]
     IgnoredDependent(HashSet<QualifiedName>),
-    #[error("The item name '{0}' is a reserved word in Rust.")]
-    ReservedName(String),
+    #[error(transparent)]
+    InvalidIdent(InvalidIdentError),
     #[error("This item name is used in multiple namespaces. At present, autocxx and cxx allow only one type of a given name. This limitation will be fixed in future. (Items found with this name: {})", .0.iter().join(", "))]
     DuplicateCxxBridgeName(Vec<String>),
     #[error("This is a method on a type which can't be used as the receiver in Rust (i.e. self/this). This is probably because some type involves template specialization.")]
@@ -123,8 +137,6 @@ pub enum ConvertErrorFromCpp {
     MethodInAnonymousNamespace,
     #[error("We're unable to make a concrete version of this template, because we found an error handling the template.")]
     ConcreteVersionOfIgnoredTemplate,
-    #[error("bindgen decided to call this type _bindgen_ty_N because it couldn't deduce the correct name for it. That means we can't generate C++ bindings to it.")]
-    BindgenTy,
     #[error("This is a typedef to a type in an anonymous namespace, not currently supported.")]
     TypedefToTypeInAnonymousNamespace,
     #[error("This type refers to a generic type parameter of an outer type, which is not yet supported.")]
@@ -133,6 +145,44 @@ pub enum ConvertErrorFromCpp {
     ForwardDeclaredNestedType,
 }
 
+/// Error types derived from Rust code. This is separate from [`ConvertError`] because these
+/// may have spans attached for better diagnostics.
+#[derive(Debug, Clone, Error)]
+pub enum ConvertErrorFromRust {
+    #[error("extern_rust_function only supports limited parameter and return types. This is not such a supported type")]
+    UnsupportedTypeForExternFun,
+    #[error("extern_rust_function requires a fully qualified receiver, that is: fn a(self: &SomeType) as opposed to fn a(&self)")]
+    ExternRustFunRequiresFullyQualifiedReceiver,
+    #[error("extern_rust_function cannot support &mut T references; instead use Pin<&mut T> (see cxx documentation for more details")]
+    PinnedReferencesRequiredForExternFun,
+    #[error("extern_rust_function cannot currently support qualified type paths (that is, foo::bar::Baz). All type paths must be within the current module, imported using 'use'. This restriction may be lifted in future.")]
+    NamespacesNotSupportedForExternFun,
+    #[error("extern_rust_function signatures must never reference Self: instead, spell out the type explicitly.")]
+    ExplicitSelf,
+}
+
+/// A [`ConvertErrorFromRust`] which also implements [`miette::Diagnostic`] so can be pretty-printed
+/// to show the affected span of code.
+#[derive(Error, Debug, Diagnostic, Clone)]
+#[error("{err}")]
+pub struct LocatedConvertErrorFromRust {
+    err: ConvertErrorFromRust,
+    #[source_code]
+    file: String,
+    #[label("error here")]
+    span: SourceSpan,
+}
+
+impl LocatedConvertErrorFromRust {
+    pub(crate) fn new(err: ConvertErrorFromRust, span: &Span, file: &str) -> Self {
+        Self {
+            err,
+            span: proc_macro_span_to_miette_span(span),
+            file: file.to_string(),
+        }
+    }
+}
+
 /// Ensures that error contexts are always created using the constructors in this
 /// mod, therefore undergoing identifier sanitation.
 #[derive(Clone)]

engine/src/conversion/error_reporter.rs

@@ -127,11 +127,11 @@ pub(crate) fn convert_apis<FF, SF, EF, TF, A, B: 'static>(
             Api::RustFn {
                 name,
                 details,
-                receiver,
+                deps,
             } => Ok(Box::new(std::iter::once(Api::RustFn {
                 name,
                 details,
-                receiver,
+                deps,
             }))),
             Api::RustSubclassFn {
                 name,

engine/src/conversion/mod.rs

@@ -22,7 +22,8 @@ mod utilities;
 use analysis::fun::FnAnalyzer;
 use autocxx_parser::IncludeCppConfig;
 pub(crate) use codegen_cpp::CppCodeGenerator;
-pub(crate) use convert_error::ConvertErrorFromCpp;
+pub(crate) use convert_error::ConvertError;
+use convert_error::ConvertErrorFromCpp;
 use itertools::Itertools;
 use syn::{Item, ItemMod};
 
@@ -122,14 +123,15 @@ impl<'a> BridgeConverter<'a> {
         unsafe_policy: UnsafePolicy,
         inclusions: String,
         cpp_codegen_options: &CppCodegenOptions,
-    ) -> Result<CodegenResults, ConvertErrorFromCpp> {
+        source_file_contents: &str,
+    ) -> Result<CodegenResults, ConvertError> {
         match &mut bindgen_mod.content {
-            None => Err(ConvertErrorFromCpp::NoContent),
+            None => Err(ConvertError::NoContent),
             Some((_, items)) => {
                 // Parse the bindgen mod.
                 let items_to_process = items.drain(..).collect();
                 let parser = ParseBindgen::new(self.config);
-                let apis = parser.parse_items(items_to_process)?;
+                let apis = parser.parse_items(items_to_process, source_file_contents)?;
                 Self::dump_apis("parsing", &apis);
                 // Inside parse_results, we now have a list of APIs.
                 // We now enter various analysis phases.
@@ -144,7 +146,8 @@ impl<'a> BridgeConverter<'a> {
                 // POD really are POD, and duly mark any dependent types.
                 // This returns a new list of `Api`s, which will be parameterized with
                 // the analysis results.
-                let analyzed_apis = analyze_pod_apis(apis, self.config)?;
+                let analyzed_apis =
+                    analyze_pod_apis(apis, self.config).map_err(ConvertError::Cpp)?;
                 Self::dump_apis("pod analysis", &analyzed_apis);
                 let analyzed_apis = replace_hopeless_typedef_targets(self.config, analyzed_apis);
                 let analyzed_apis = add_casts(analyzed_apis);
@@ -193,7 +196,8 @@ impl<'a> BridgeConverter<'a> {
                     self.config,
                     cpp_codegen_options,
                     &cxxgen_header_name,
-                )?;
+                )
+                .map_err(ConvertError::Cpp)?;
                 let rs = RsCodeGenerator::generate_rs_code(
                     analyzed_apis,
                     &unsafe_policy,