Merge pull request #911 from google/never-panic-on-push-partial

Never panic on pushing API

Author: adetaylor

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

@@ -24,8 +24,8 @@ use crate::{
             UnsafetyNeeded, Virtualness,
         },
         apivec::ApiVec,
-        convert_error::ConvertErrorWithContext,
         convert_error::ErrorContext,
+        convert_error::{ConvertErrorWithContext, ErrorContextType},
         error_reporter::{convert_apis, report_any_error},
     },
     known_types::known_types,
@@ -963,7 +963,7 @@ impl<'a> FnAnalyzer<'a> {
             let rust_name = self.get_function_overload_name(ns, ideal_rust_name);
             (
                 FnKind::Function,
-                ErrorContext::Item(make_ident(&rust_name)),
+                ErrorContext::new_for_item(make_ident(&rust_name)),
                 rust_name,
             )
         };
@@ -1489,7 +1489,7 @@ impl<'a> FnAnalyzer<'a> {
                             trait_call_is_unsafe: false,
                         }),
                     },
-                    ErrorContext::Item(make_ident(&rust_name)),
+                    ErrorContext::new_for_item(make_ident(&rust_name)),
                     rust_name,
                 ))
             }
@@ -1534,7 +1534,7 @@ impl<'a> FnAnalyzer<'a> {
                 }),
                 kind,
             },
-            ErrorContext::Item(make_ident(&rust_name)),
+            ErrorContext::new_for_item(make_ident(&rust_name)),
             rust_name,
         ))
     }
@@ -1956,14 +1956,11 @@ impl<'a> FnAnalyzer<'a> {
 }
 
 fn error_context_for_method(self_ty: &QualifiedName, rust_name: &str) -> ErrorContext {
-    ErrorContext::Method {
-        self_ty: self_ty.get_final_ident(),
-        method: make_ident(rust_name),
-    }
+    ErrorContext::new_for_method(self_ty.get_final_ident(), make_ident(rust_name))
 }
 
 impl Api<FnPhase> {
-    pub(crate) fn typename_for_allowlist(&self) -> QualifiedName {
+    pub(crate) fn name_for_allowlist(&self) -> QualifiedName {
         match &self {
             Api::Function { analysis, .. } => match analysis.kind {
                 FnKind::Method { ref impl_for, .. } => impl_for.clone(),
@@ -1973,6 +1970,19 @@ impl Api<FnPhase> {
                 }
             },
             Api::RustSubclassFn { subclass, .. } => subclass.0.name.clone(),
+            Api::IgnoredItem {
+                name,
+                ctx: Some(ctx),
+                ..
+            } => match ctx.get_type() {
+                ErrorContextType::Method { self_ty, .. } => {
+                    QualifiedName::new(name.name.get_namespace(), self_ty.clone())
+                }
+                ErrorContextType::Item(id) => {
+                    QualifiedName::new(name.name.get_namespace(), id.clone())
+                }
+                _ => name.name.clone(),
+            },
             _ => self.name().clone(),
         }
     }

engine/src/conversion/analysis/gc.rs

@@ -41,7 +41,7 @@ pub(crate) fn filter_apis_by_following_edges_from_allowlist(
     let mut todos: Vec<QualifiedName> = apis
         .iter()
         .filter(|api| {
-            let tnforal = api.typename_for_allowlist();
+            let tnforal = api.name_for_allowlist();
             config.is_on_allowlist(&tnforal.to_cpp_name())
         })
         .map(Api::name)

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

@@ -135,7 +135,7 @@ fn analyze_struct(
     if details.vis != CppVisibility::Public {
         return Err(ConvertErrorWithContext(
             ConvertError::NonPublicNestedType,
-            Some(ErrorContext::Item(id)),
+            Some(ErrorContext::new_for_item(id)),
         ));
     }
     let metadata = BindgenSemanticAttributes::new_retaining_others(&mut details.item.attrs);
@@ -157,11 +157,14 @@ fn analyze_struct(
         if details.has_rvalue_reference_fields {
             return Err(ConvertErrorWithContext(
                 ConvertError::RValueReferenceField,
-                Some(ErrorContext::Item(id)),
+                Some(ErrorContext::new_for_item(id)),
             ));
         }
         if let Some(err) = field_conversion_errors.into_iter().next() {
-            return Err(ConvertErrorWithContext(err, Some(ErrorContext::Item(id))));
+            return Err(ConvertErrorWithContext(
+                err,
+                Some(ErrorContext::new_for_item(id)),
+            ));
         }
         TypeKind::Pod
     } else {

engine/src/conversion/analysis/remove_ignored.rs

@@ -19,15 +19,9 @@ use crate::{conversion::api::Api, known_types};
 /// know about at all. In either case, we don't simply remove the type, but instead
 /// replace it with an error marker.
 pub(crate) fn filter_apis_by_ignored_dependents(mut apis: ApiVec<FnPhase>) -> ApiVec<FnPhase> {
-    let (ignored_items, valid_items): (Vec<&Api<_>>, Vec<&Api<_>>) = apis.iter().partition(|api| {
-        matches!(
-            api,
-            Api::IgnoredItem {
-                ctx: ErrorContext::Item(..),
-                ..
-            }
-        )
-    });
+    let (ignored_items, valid_items): (Vec<&Api<_>>, Vec<&Api<_>>) = apis
+        .iter()
+        .partition(|api| matches!(api, Api::IgnoredItem { .. }));
     let mut ignored_items: HashSet<_> = ignored_items
         .into_iter()
         .map(|api| api.name().clone())
@@ -83,7 +77,7 @@ fn create_ignore_item(api: Api<FnPhase>, err: ConvertError) -> Api<FnPhase> {
                         ..
                     },
                 ..
-            } => ErrorContext::NoCode,
+            } => None,
             Api::Function {
                 analysis:
                     FnAnalysis {
@@ -94,11 +88,8 @@ fn create_ignore_item(api: Api<FnPhase>, err: ConvertError) -> Api<FnPhase> {
                         ..
                     },
                 ..
-            } => ErrorContext::Method {
-                self_ty: self_ty.get_final_ident(),
-                method: id,
-            },
-            _ => ErrorContext::Item(id),
+            } => Some(ErrorContext::new_for_method(self_ty.get_final_ident(), id)),
+            _ => Some(ErrorContext::new_for_item(id)),
         },
     }
 }

engine/src/conversion/analysis/tdef.rs

@@ -96,14 +96,14 @@ fn get_replacement_typedef(
     match type_conversion_results {
         Err(err) => Err(ConvertErrorWithContext(
             err,
-            Some(ErrorContext::Item(name.name.get_final_ident())),
+            Some(ErrorContext::new_for_item(name.name.get_final_ident())),
         )),
         Ok(Annotated {
             ty: syn::Type::Path(ref typ),
             ..
         }) if QualifiedName::from_type_path(typ) == name.name => Err(ConvertErrorWithContext(
             ConvertError::InfinitelyRecursiveTypedef(name.name.clone()),
-            Some(ErrorContext::Item(name.name.get_final_ident())),
+            Some(ErrorContext::new_for_item(name.name.get_final_ident())),
         )),
         Ok(mut final_type) => {
             converted_type.ty = Box::new(final_type.ty.clone());

engine/src/conversion/api.rs

@@ -467,7 +467,7 @@ pub(crate) enum Api<T: AnalysisPhase> {
     IgnoredItem {
         name: ApiName,
         err: ConvertError,
-        ctx: ErrorContext,
+        ctx: Option<ErrorContext>,
     },
     /// A Rust type which is not a C++ type.
     RustType { name: ApiName, path: RustPath },

engine/src/conversion/apivec.rs

@@ -53,32 +53,21 @@ impl<P: AnalysisPhase> ApiVec<P> {
         let already_contains = self.already_contains(name);
         if already_contains {
             if api.discard_duplicates() {
+                // This is already an IgnoredItem or something else where
+                // we can silently drop it.
                 log::info!("Discarding duplicate API for {}", name);
             } else {
-                panic!(
-                    "Already have an API with that name: {}. API was {:?}",
-                    name, api
+                log::info!(
+                    "Duplicate API for {} - removing all of them and replacing with an IgnoredItem.",
+                    name
                 );
+                self.retain(|api| api.name() != name);
+                self.push(Api::IgnoredItem {
+                    name: ApiName::new_from_qualified_name(name.clone()),
+                    err: ConvertError::DuplicateItemsFoundInParsing,
+                    ctx: Some(ErrorContext::new_for_item(name.get_final_ident())),
+                })
             }
-        }
-        self.names.insert(name.clone());
-        self.apis.push(api)
-    }
-
-    pub(crate) fn push_eliminating_duplicates(&mut self, api: Api<P>) {
-        let name = api.name();
-        let already_contains = self.already_contains(name);
-        if already_contains {
-            log::info!(
-                "Duplicate API for {} - removing all of them and replacing with an IgnoredItem.",
-                name
-            );
-            self.retain(|api| api.name() != name);
-            self.push(Api::IgnoredItem {
-                name: ApiName::new_from_qualified_name(name.clone()),
-                err: ConvertError::DuplicateItemsFoundInParsing,
-                ctx: ErrorContext::Item(name.get_final_ident()),
-            })
         } else {
             self.names.insert(name.clone());
             self.apis.push(api)

engine/src/conversion/codegen_rs/mod.rs

@@ -33,7 +33,6 @@ use crate::{
         },
         doc_attr::get_doc_attr,
     },
-    known_types::known_types,
     types::{make_ident, Namespace, QualifiedName},
 };
 use impl_item_creator::create_impl_items;
@@ -46,6 +45,7 @@ use self::{
 use super::{
     analysis::fun::{FnPhase, ReceiverMutability},
     api::{AnalysisPhase, Api, SubclassName, TypeKind, TypedefKind},
+    convert_error::ErrorContextType,
 };
 use super::{
     api::{Layout, Provenance, RustSubclassFnDetails, SuperclassMethod, TraitImplSignature},
@@ -564,8 +564,12 @@ impl<'a> RsCodeGenerator<'a> {
                     subclasses_with_a_single_trivial_constructor.contains(&name.0.name);
                 self.generate_subclass(name, &superclass, methods, generate_peer_constructor)
             }
-            Api::IgnoredItem { err, ctx, .. } => Self::generate_error_entry(err, ctx),
-            Api::SubclassTraitItem { .. } => RsCodegenResult::default(),
+            Api::IgnoredItem {
+                err,
+                ctx: Some(ctx),
+                ..
+            } => Self::generate_error_entry(err, ctx),
+            Api::IgnoredItem { .. } | Api::SubclassTraitItem { .. } => RsCodegenResult::default(),
         }
     }
 
@@ -930,67 +934,46 @@ impl<'a> RsCodeGenerator<'a> {
     /// generated.
     fn generate_error_entry(err: ConvertError, ctx: ErrorContext) -> RsCodegenResult {
         let err = format!("autocxx bindings couldn't be generated: {}", err);
-        let (impl_entry, materialization) = match ctx {
-            ErrorContext::Item(id) => {
-                let id = Self::sanitize_error_ident(&id).unwrap_or(id);
-                (
-                    None,
-                    Some(Use::Custom(Box::new(parse_quote! {
-                        #[doc = #err]
-                        pub struct #id;
-                    }))),
-                )
-            }
-            ErrorContext::Method { self_ty, method }
-                if Self::sanitize_error_ident(&self_ty).is_none() =>
-            {
-                // This could go wrong if a separate error has caused
-                // us to drop an entire type as well as one of its individual items.
-                // Then we'll be applying an impl to a thing which doesn't exist. TODO.
-                let method = Self::sanitize_error_ident(&method).unwrap_or(method);
-                (
-                    Some(Box::new(ImplBlockDetails {
-                        item: parse_quote! {
-                            #[doc = #err]
-                            fn #method(_uhoh: autocxx::BindingGenerationFailure) {
-                            }
-                        },
-                        ty: self_ty,
-                    })),
-                    None,
-                )
-            }
-            ErrorContext::Method { self_ty, method } => {
-                // If the type can't be represented (e.g. u8) this would get fiddly.
-                let id = make_ident(format!("{}_method_{}", self_ty, method));
-                (
-                    None,
-                    Some(Use::Custom(Box::new(parse_quote! {
+        let (impl_entry, bindgen_mod_item, materialization) = match ctx.into_type() {
+            ErrorContextType::Item(id) => (
+                // Populate within bindgen mod because impl blocks may attach.
+                None,
+                Some(parse_quote! {
+                    #[doc = #err]
+                    pub struct #id;
+                }),
+                Some(Use::SpecificNameFromBindgen(id)),
+            ),
+            ErrorContextType::SanitizedItem(id) => (
+                // Guaranteed to be no impl blocks - populate directly in output mod.
+                None,
+                None,
+                Some(Use::Custom(Box::new(parse_quote! {
+                    #[doc = #err]
+                    pub struct #id;
+                }))),
+            ),
+            ErrorContextType::Method { self_ty, method } => (
+                Some(Box::new(ImplBlockDetails {
+                    item: parse_quote! {
                         #[doc = #err]
-                        pub struct #id;
-                    }))),
-                )
-            }
-            ErrorContext::NoCode => (None, None),
+                        fn #method(_uhoh: autocxx::BindingGenerationFailure) {
+                        }
+                    },
+                    ty: self_ty,
+                })),
+                None,
+                None,
+            ),
         };
         RsCodegenResult {
             impl_entry,
+            bindgen_mod_items: bindgen_mod_item.into_iter().collect(),
             materializations: materialization.into_iter().collect(),
             ..Default::default()
         }
     }
 
-    /// Because errors may be generated for invalid types or identifiers,
-    /// we may need to scrub the name
-    fn sanitize_error_ident(id: &Ident) -> Option<Ident> {
-        let qn = QualifiedName::new(&Namespace::new(), id.clone());
-        if known_types().conflicts_with_built_in_type(&qn) {
-            Some(make_ident(format!("{}_autocxx_error", qn.get_final_item())))
-        } else {
-            None
-        }
-    }
-
     fn generate_cxx_use_stmt(name: &QualifiedName, alias: Option<&Ident>) -> Item {
         let segs = Self::find_output_mod_root(name.get_namespace())
             .chain(std::iter::once(make_ident("cxxbridge")))