From 82bf029dd072daca32adc88251919afc3a6a5bde Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Tue, 16 Jul 2019 16:44:23 +0200 Subject: [PATCH 1/3] Add `?Sized` relaxation to impl header whenever possible It's only impossible when there is `Self` by value in in parameter or return position in any method. This commit does not yet check for `Self` in non-receiver parameters as this crate cannot deal with that yet anyway. Furthermore, a `where Self: Sized` bound on methods does not help in that we still cannot add the `?Sized` relaxation. This is related to issue #11. --- src/gen.rs | 83 +++++++++++++++++-- tests/compile-fail/trait_obj_value_self.rs | 13 +++ .../compile-pass/trait_obj_default_method.rs | 22 +++++ .../compile-pass/trait_obj_immutable_self.rs | 19 +++++ tests/compile-pass/trait_obj_value_self.rs | 7 ++ 5 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 tests/compile-fail/trait_obj_value_self.rs create mode 100644 tests/compile-pass/trait_obj_default_method.rs create mode 100644 tests/compile-pass/trait_obj_immutable_self.rs create mode 100644 tests/compile-pass/trait_obj_value_self.rs diff --git a/src/gen.rs b/src/gen.rs index 2729624..0b7fc92 100644 --- a/src/gen.rs +++ b/src/gen.rs @@ -2,8 +2,9 @@ use crate::proc_macro::Span; use proc_macro2::TokenStream as TokenStream2; use quote::{ToTokens, TokenStreamExt}; use syn::{ - FnArg, Ident, ItemTrait, Lifetime, MethodSig, Pat, PatIdent, TraitItem, TraitItemConst, - TraitItemMethod, TraitItemType, + FnArg, Ident, ItemTrait, Lifetime, MethodSig, Pat, PatIdent, ReturnType, TraitBound, + TraitBoundModifier, TraitItem, TraitItemConst, TraitItemMethod, TraitItemType, Type, + TypeParamBound, WherePredicate, }; use crate::{ @@ -65,12 +66,84 @@ fn gen_header( // The `'{proxy_lt_param}` in the beginning is only added when the proxy // type is `&` or `&mut`. let impl_generics = { + // Determine whether we can add a `?Sized` relaxation to allow trait + // objects. We can do that as long as there is no method that has a + // `self` by value receiver and no `where Self: Sized` bound. + let sized_required = trait_def.items.iter() + // Only interested in methods + .filter_map(|item| if let TraitItem::Method(m) = item { Some(m) } else { None }) + // We also ignore methods that we will not override. In the case of + // invalid attributes it is save to assume default behavior. + .filter(|m| !should_keep_default_for(m, proxy_type).unwrap_or(false)) + .any(|m| { + // Check if there is a `Self: Sized` bound on the method. + let self_is_bounded_sized = m.sig.decl.generics.where_clause.iter() + .flat_map(|wc| &wc.predicates) + .filter_map(|pred| { + if let WherePredicate::Type(p) = pred { Some(p) } else { None } + }) + .any(|pred| { + // Check if the type is `Self` + match &pred.bounded_ty { + Type::Path(p) if p.path.is_ident("Self") => { + // Check if the bound contains `Sized` + pred.bounds.iter().any(|b| { + match b { + TypeParamBound::Trait(TraitBound { + modifier: TraitBoundModifier::None, + path, + .. + }) => path.is_ident("Sized"), + _ => false, + } + }) + } + _ => false, + } + }); + + // Check if the first parameter is `self` by value. In that + // case, we might require `Self` to be `Sized`. + let self_value_param = match m.sig.decl.inputs.first().map(|p| p.into_value()) { + Some(FnArg::SelfValue(_)) => true, + _ => false, + }; + + // Check if return type is `Self` + let self_value_return = match &m.sig.decl.output { + ReturnType::Type(_, t) => { + if let Type::Path(p) = &**t { + p.path.is_ident("Self") + } else { + false + } + } + _ => false, + }; + + // TODO: check for `Self` parameter in any other argument. + + // If for this method, `Self` is used in a position that + // requires `Self: Sized` or this bound is added explicitly, we + // cannot add the `?Sized` relaxation to the impl body. + self_value_param || self_value_return || self_is_bounded_sized + }); + + let relaxation = if sized_required { + quote! {} + } else { + quote! { + ?::std::marker::Sized } + }; + // Determine if our proxy type needs a lifetime parameter let (mut params, ty_bounds) = match proxy_type { - ProxyType::Ref | ProxyType::RefMut => { - (quote! { #proxy_lt_param, }, quote! { : #proxy_lt_param + #trait_path }) + ProxyType::Ref | ProxyType::RefMut => ( + quote! { #proxy_lt_param, }, + quote! { : #proxy_lt_param + #trait_path #relaxation } + ), + ProxyType::Box | ProxyType::Rc | ProxyType::Arc => { + (quote!{}, quote! { : #trait_path #relaxation }) } - ProxyType::Box | ProxyType::Rc | ProxyType::Arc => (quote!{}, quote! { : #trait_path }), ProxyType::Fn | ProxyType::FnMut | ProxyType::FnOnce => { let fn_bound = gen_fn_type_for_trait(proxy_type, trait_def)?; (quote!{}, quote! { : #fn_bound }) diff --git a/tests/compile-fail/trait_obj_value_self.rs b/tests/compile-fail/trait_obj_value_self.rs new file mode 100644 index 0000000..2e818d0 --- /dev/null +++ b/tests/compile-fail/trait_obj_value_self.rs @@ -0,0 +1,13 @@ +use auto_impl::auto_impl; + + +#[auto_impl(Box)] +trait Trait { + fn foo(self); +} + +fn assert_impl() {} + +fn main() { + assert_impl::>(); +} diff --git a/tests/compile-pass/trait_obj_default_method.rs b/tests/compile-pass/trait_obj_default_method.rs new file mode 100644 index 0000000..9ee6962 --- /dev/null +++ b/tests/compile-pass/trait_obj_default_method.rs @@ -0,0 +1,22 @@ +use auto_impl::auto_impl; + + +#[auto_impl(Box)] +trait Trait { + fn bar(&self); + + #[auto_impl(keep_default_for(Box))] + fn foo(self) where Self: Sized {} +} + +fn assert_impl() {} + +struct Foo {} +impl Trait for Foo { + fn bar(&self) {} +} + +fn main() { + assert_impl::(); + assert_impl::>(); +} diff --git a/tests/compile-pass/trait_obj_immutable_self.rs b/tests/compile-pass/trait_obj_immutable_self.rs new file mode 100644 index 0000000..1ac30c0 --- /dev/null +++ b/tests/compile-pass/trait_obj_immutable_self.rs @@ -0,0 +1,19 @@ +use auto_impl::auto_impl; + + +#[auto_impl(&, &mut, Box, Rc, Arc)] +trait Trait { + fn foo(&self); +} + +fn assert_impl() {} + +fn main() { + use std::{rc::Rc, sync::Arc}; + + assert_impl::<&dyn Trait>(); + assert_impl::<&mut dyn Trait>(); + assert_impl::>(); + assert_impl::>(); + assert_impl::>(); +} diff --git a/tests/compile-pass/trait_obj_value_self.rs b/tests/compile-pass/trait_obj_value_self.rs new file mode 100644 index 0000000..ebf30eb --- /dev/null +++ b/tests/compile-pass/trait_obj_value_self.rs @@ -0,0 +1,7 @@ +use auto_impl::auto_impl; + + +#[auto_impl(Box)] +trait Trait { + fn foo(self); +} From cf6c9ccdc1d3239442a23e6c568cc3e7bd623abc Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Tue, 16 Jul 2019 17:29:15 +0200 Subject: [PATCH 2/3] Check for `Self` bounds on methods and add those bounds to impl header See #11 --- src/gen.rs | 32 +++++++++++++++-- tests/compile-pass/self_bound.rs | 23 ++++++++++++ .../compile-pass/self_bound_default_method.rs | 24 +++++++++++++ tests/compile-pass/self_bound_multiple.rs | 35 +++++++++++++++++++ 4 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 tests/compile-pass/self_bound.rs create mode 100644 tests/compile-pass/self_bound_default_method.rs create mode 100644 tests/compile-pass/self_bound_multiple.rs diff --git a/src/gen.rs b/src/gen.rs index 0b7fc92..91ed537 100644 --- a/src/gen.rs +++ b/src/gen.rs @@ -135,14 +135,42 @@ fn gen_header( quote! { + ?::std::marker::Sized } }; + // Check if there are some `Self: Foo` bounds on methods. If so, we + // need to add those bounds to `T` as well. See issue #11 for more + // information, but in short: there is no better solution. Method where + // clauses with `Self: Foo` force us to add `T: Foo` to the impl + // header, as we otherwise cannot generate a valid impl block. + let additional_bounds = trait_def.items.iter() + // Only interested in methods + .filter_map(|item| if let TraitItem::Method(m) = item { Some(m) } else { None }) + // We also ignore methods that we will not override. In the case of + // invalid attributes it is save to assume default behavior. + .filter(|m| !should_keep_default_for(m, proxy_type).unwrap_or(false)) + // Exact all relevant bounds + .flat_map(|m| { + m.sig.decl.generics.where_clause.iter() + .flat_map(|wc| &wc.predicates) + .filter_map(|pred| { + if let WherePredicate::Type(p) = pred { Some(p) } else { None } + }) + .filter(|p| { + // Only `Self:` bounds + match &p.bounded_ty { + Type::Path(p) => p.path.is_ident("Self"), + _ => false, + } + }) + .flat_map(|p| &p.bounds) + }); + // Determine if our proxy type needs a lifetime parameter let (mut params, ty_bounds) = match proxy_type { ProxyType::Ref | ProxyType::RefMut => ( quote! { #proxy_lt_param, }, - quote! { : #proxy_lt_param + #trait_path #relaxation } + quote! { : #proxy_lt_param + #trait_path #relaxation #(+ #additional_bounds)* } ), ProxyType::Box | ProxyType::Rc | ProxyType::Arc => { - (quote!{}, quote! { : #trait_path #relaxation }) + (quote!{}, quote! { : #trait_path #relaxation #(+ #additional_bounds)* }) } ProxyType::Fn | ProxyType::FnMut | ProxyType::FnOnce => { let fn_bound = gen_fn_type_for_trait(proxy_type, trait_def)?; diff --git a/tests/compile-pass/self_bound.rs b/tests/compile-pass/self_bound.rs new file mode 100644 index 0000000..6947d5c --- /dev/null +++ b/tests/compile-pass/self_bound.rs @@ -0,0 +1,23 @@ +use auto_impl::auto_impl; + + +#[auto_impl(&)] +trait Trait { + fn foo(&self) + where Self: Clone; +} + +#[derive(Clone)] +struct Foo {} +impl Trait for Foo { + fn foo(&self) + where Self: Clone, + {} +} + +fn assert_impl() {} + +fn main() { + assert_impl::(); + assert_impl::<&Foo>(); +} diff --git a/tests/compile-pass/self_bound_default_method.rs b/tests/compile-pass/self_bound_default_method.rs new file mode 100644 index 0000000..3dd9209 --- /dev/null +++ b/tests/compile-pass/self_bound_default_method.rs @@ -0,0 +1,24 @@ +use auto_impl::auto_impl; + + +#[auto_impl(Box)] +trait Trait { + fn bar(&self); + + #[auto_impl(keep_default_for(Box))] + fn foo(&self) + where Self: Clone + {} +} + +fn assert_impl() {} + +struct Foo {} +impl Trait for Foo { + fn bar(&self) {} +} + +fn main() { + assert_impl::(); + assert_impl::>(); +} diff --git a/tests/compile-pass/self_bound_multiple.rs b/tests/compile-pass/self_bound_multiple.rs new file mode 100644 index 0000000..2359ee8 --- /dev/null +++ b/tests/compile-pass/self_bound_multiple.rs @@ -0,0 +1,35 @@ +use std::fmt; +use auto_impl::auto_impl; + + +#[auto_impl(&)] +trait Trait { + fn foo(&self) + where Self: Clone; + fn bar(&self) + where Self: Default + fmt::Display; +} + +#[derive(Clone, Default)] +struct Foo {} +impl Trait for Foo { + fn foo(&self) + where Self: Clone, + {} + fn bar(&self) + where Self: Default + fmt::Display, + {} +} + +impl fmt::Display for Foo { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + unimplemented!() + } +} + +fn assert_impl() {} + +fn main() { + assert_impl::(); + assert_impl::<&Foo>(); +} From 90f17b69350204689730de3a5c4cc17d038d8d94 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Fri, 19 Jul 2019 17:21:33 +0200 Subject: [PATCH 3/3] Add `compile-pass` test about `Self: Sized` bounds on the trait Currently, the `impl` block we generate for those traits is a bit stupid as it contains `T: Trait + ?Sized` (where `Trait: Sized`), but it compiles fine. So I don't see a reason to make our code more complicated just to avoid this strangeness. --- tests/compile-pass/trait_obj_self_sized.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 tests/compile-pass/trait_obj_self_sized.rs diff --git a/tests/compile-pass/trait_obj_self_sized.rs b/tests/compile-pass/trait_obj_self_sized.rs new file mode 100644 index 0000000..46cb566 --- /dev/null +++ b/tests/compile-pass/trait_obj_self_sized.rs @@ -0,0 +1,17 @@ +use auto_impl::auto_impl; + + +#[auto_impl(Box)] +trait Foo: Sized { + fn foo(&self); +} + +#[auto_impl(Box)] +trait Bar where Self: Sized { + fn foo(&self); +} + +#[auto_impl(Box)] +trait Baz: Sized where Self: Sized { + fn foo(&self); +}