From 2d7fb6392ea2e964272bea70afa954e6136fcd58 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 26 Mar 2021 15:08:15 -0700 Subject: [PATCH 1/2] Fix const virtual fns --- engine/Cargo.toml | 2 +- engine/src/conversion/analysis/fun/mod.rs | 8 +++++++- engine/src/integration_tests.rs | 1 - 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/engine/Cargo.toml b/engine/Cargo.toml index 061af3ac2..1d9b79b2d 100644 --- a/engine/Cargo.toml +++ b/engine/Cargo.toml @@ -32,7 +32,7 @@ proc-macro2 = "1.0.11" quote = "1.0" lazy_static = "1.4" indoc = "1.0" -autocxx-bindgen = "0.57.2" +autocxx-bindgen = "0.57.3" itertools = "0.9" cc = { version = "1.0", optional = true } unzip-n = "0.1.2" diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 6c5da7ce4..2e5da5c8a 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -20,6 +20,7 @@ use crate::known_types::KNOWN_TYPES; use std::collections::{HashMap, HashSet}; use autocxx_parser::{TypeConfig, UnsafePolicy}; +use proc_macro2::Span; use syn::{ parse_quote, punctuated::Punctuated, FnArg, ForeignItemFn, Ident, LitStr, Pat, ReturnType, Type, TypePtr, Visibility, @@ -643,8 +644,13 @@ impl<'a> FnAnalyzer<'a> { ) })?; let this_type_path = this_type.to_type_path(); + let const_token = if mutability.is_some() { + None + } else { + Some(syn::Token![const](Span::call_site())) + }; pt.ty = Box::new(parse_quote! { - * #mutability #this_type_path + * #mutability #const_token #this_type_path }); } Ok(this_type) diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs index 3b428923e..bc5cfb38b 100644 --- a/engine/src/integration_tests.rs +++ b/engine/src/integration_tests.rs @@ -3696,7 +3696,6 @@ fn test_virtual_fns() { } #[test] -#[ignore] // https://github.com/google/autocxx/issues/313 fn test_const_virtual_fns() { let hdr = indoc! {" #include From 0a69faebf34959c859d372aa3a40e5fd88756e1d Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Fri, 26 Mar 2021 13:16:04 -0700 Subject: [PATCH 2/2] Attempt to spot PV classes --- .../src/conversion/analysis/abstract_types.rs | 50 +++++++++++++++++++ engine/src/conversion/analysis/fun/mod.rs | 22 +++++--- engine/src/conversion/analysis/mod.rs | 1 + engine/src/conversion/api.rs | 10 ++++ engine/src/conversion/codegen_rs/mod.rs | 7 +-- engine/src/conversion/mod.rs | 10 ++-- engine/src/integration_tests.rs | 3 +- 7 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 engine/src/conversion/analysis/abstract_types.rs diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs new file mode 100644 index 000000000..5bce613a3 --- /dev/null +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -0,0 +1,50 @@ +// 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 super::fun::{FnAnalysis, FnAnalysisBody}; +use crate::conversion::api::ApiDetail; +use crate::conversion::api::{Api, TypeKind}; +use std::collections::HashSet; + +/// Spot types with pure virtual functions and mark them abstract. +pub(crate) fn mark_types_abstract(apis: &mut Vec>) { + let abstract_types: HashSet<_> = apis + .iter() + .filter_map(|api| match &api.detail { + ApiDetail::Function { + fun: _, + analysis: + FnAnalysisBody { + is_pure_virtual: true, + self_ty: Some(self_ty_name), + .. + }, + } => Some(self_ty_name.clone()), + _ => None, + }) + .collect(); + if abstract_types.is_empty() { + return; + } + + for api in apis { + let tyname = api.typename(); + match &mut api.detail { + ApiDetail::Type { analysis, .. } if abstract_types.contains(&tyname) => { + *analysis = TypeKind::Abstract; + } + _ => {} + } + } +} diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 2e5da5c8a..f89816d93 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -62,6 +62,7 @@ pub(crate) struct FnAnalysisBody { pub(crate) id_for_allowlist: Option, pub(crate) use_stmt: Use, pub(crate) additional_cpp: Option, + pub(crate) is_pure_virtual: bool, } pub(crate) struct ArgumentAnalysis { @@ -70,7 +71,7 @@ pub(crate) struct ArgumentAnalysis { pub(crate) self_type: Option, was_reference: bool, deps: HashSet, - virtual_this_encountered: bool, + is_virtual: bool, requires_unsafe: bool, } @@ -305,6 +306,7 @@ impl<'a> FnAnalyzer<'a> { ) -> Result, ConvertError> { let fun = &func_information.item; let virtual_this = &func_information.virtual_this_type; + let is_pure_virtual = Self::has_attr(&fun, "bindgen_pure_virtual"); // This function is one of the most complex parts of our conversion. // It needs to consider: // 1. Rejecting destructors entirely. @@ -357,7 +359,10 @@ impl<'a> FnAnalyzer<'a> { .filter_map(|pd| pd.self_type.as_ref()) .next() .cloned(); - let virtual_this_encountered = param_details.iter().any(|pd| pd.virtual_this_encountered); + let is_virtual = param_details.iter().any(|pd| pd.is_virtual); + if is_pure_virtual { + assert!(is_virtual); + } let requires_unsafe = param_details.iter().any(|pd| pd.requires_unsafe); let is_static_method = if self_ty.is_none() { @@ -485,7 +490,7 @@ impl<'a> FnAnalyzer<'a> { || ret_type_conversion_needed || is_static_method || differently_named_method - || virtual_this_encountered; + || is_virtual; let mut additional_cpp = None; @@ -603,6 +608,7 @@ impl<'a> FnAnalyzer<'a> { id_for_allowlist, use_stmt, additional_cpp, + is_pure_virtual, }, id, deps, @@ -625,7 +631,7 @@ impl<'a> FnAnalyzer<'a> { let mut pt = pt.clone(); let mut self_type = None; let old_pat = *pt.pat; - let mut virtual_this_encountered = false; + let mut is_virtual = false; let mut treat_as_reference = false; let new_pat = match old_pat { syn::Pat::Ident(mut pp) if pp.ident == "this" => { @@ -636,7 +642,7 @@ impl<'a> FnAnalyzer<'a> { Type::Path(typ) => { let mut this_type = TypeName::from_type_path(typ); if this_type.is_cvoid() { - virtual_this_encountered = true; + is_virtual = true; this_type = virtual_this.ok_or_else(|| { ConvertError::VirtualThisType( ns.clone(), @@ -687,7 +693,7 @@ impl<'a> FnAnalyzer<'a> { conversion, was_reference, deps, - virtual_this_encountered, + is_virtual, requires_unsafe, }, ) @@ -782,6 +788,10 @@ impl<'a> FnAnalyzer<'a> { } (ref_params, ref_return) } + + fn has_attr(fun: &ForeignItemFn, attr_name: &str) -> bool { + fun.attrs.iter().any(|at| at.path.is_ident(attr_name)) + } } impl Api { diff --git a/engine/src/conversion/analysis/mod.rs b/engine/src/conversion/analysis/mod.rs index 32f4bbd6c..e6b7101a8 100644 --- a/engine/src/conversion/analysis/mod.rs +++ b/engine/src/conversion/analysis/mod.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +pub(crate) mod abstract_types; pub(crate) mod ctypes; pub(crate) mod fun; pub(crate) mod gc; diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index 049659512..1973272e0 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -24,6 +24,16 @@ pub(crate) enum TypeKind { Pod, // trivial. Can be moved and copied in Rust. NonPod, // has destructor or non-trivial move constructors. Can only hold by UniquePtr ForwardDeclaration, // no full C++ declaration available - can't even generate UniquePtr + Abstract, // has pure virtual members - can't even generate UniquePtr +} + +impl TypeKind { + pub(crate) fn can_be_instantiated(&self) -> bool { + match self { + TypeKind::Pod | TypeKind::NonPod => true, + TypeKind::ForwardDeclaration | TypeKind::Abstract => false, + } + } } /// Whether and how this type should be exposed in the mods constructed diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 495d6d0ec..27c3544a3 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -348,9 +348,10 @@ impl<'a> RsCodeGenerator<'a> { } => RsCodegenResult { global_items: Self::generate_extern_type_impl(analysis, &ty_details), impl_entry: None, - bridge_items: match analysis { - TypeKind::ForwardDeclaration => Vec::new(), - _ => create_impl_items(&ty_details.final_ident), + bridge_items: if analysis.can_be_instantiated() { + create_impl_items(&ty_details.final_ident) + } else { + Vec::new() }, extern_c_mod_item: Some(ForeignItem::Verbatim(for_extern_c_ts)), bindgen_mod_item, diff --git a/engine/src/conversion/mod.rs b/engine/src/conversion/mod.rs index e7ebf3873..b4424a344 100644 --- a/engine/src/conversion/mod.rs +++ b/engine/src/conversion/mod.rs @@ -34,8 +34,8 @@ use crate::UnsafePolicy; use self::{ analysis::{ - gc::filter_apis_by_following_edges_from_allowlist, pod::analyze_pod_apis, - remove_ignored::filter_apis_by_ignored_dependents, + abstract_types::mark_types_abstract, gc::filter_apis_by_following_edges_from_allowlist, + pod::analyze_pod_apis, remove_ignored::filter_apis_by_ignored_dependents, }, codegen_rs::RsCodeGenerator, parse::ParseBindgen, @@ -110,12 +110,16 @@ impl<'a> BridgeConverter<'a> { // require C++ wrapper functions. This is probably the most complex // part of `autocxx`. Again, this returns a new set of `Api`s, but // parameterized by a richer set of metadata. - let analyzed_apis = FnAnalyzer::analyze_functions( + let mut analyzed_apis = FnAnalyzer::analyze_functions( analyzed_apis, unsafe_policy, &mut type_converter, self.type_config, )?; + // If any of those functions turned out to be pure virtual, don't attempt + // to generate UniquePtr implementations for the type, since it can't + // be instantiated. + mark_types_abstract(&mut analyzed_apis); // During parsing or subsequent processing we might have encountered // items which we couldn't process due to as-yet-unsupported features. // There might be other items depending on such things. Let's remove them diff --git a/engine/src/integration_tests.rs b/engine/src/integration_tests.rs index bc5cfb38b..24028c112 100644 --- a/engine/src/integration_tests.rs +++ b/engine/src/integration_tests.rs @@ -4186,7 +4186,6 @@ fn test_unexpected_use() { } #[test] -#[ignore] // https://github.com/google/autocxx/issues/305 fn test_get_pure_virtual() { let hdr = indoc! {" #include @@ -4206,7 +4205,7 @@ fn test_get_pure_virtual() { let a_ref = unsafe { a.as_ref() }.unwrap(); assert_eq!(a_ref.get_val(), 3); }; - run_test("", hdr, rs, &["get_a"], &[]); + run_test("", hdr, rs, &["A", "get_a"], &[]); } #[test]