From 65fb99a5dc9284b181f1e0872f0700e2764f5447 Mon Sep 17 00:00:00 2001 From: Devin Jean Date: Tue, 22 Nov 2022 10:23:24 -0600 Subject: [PATCH 1/4] add derive bound option --- src/gc-arena-derive/src/lib.rs | 51 ++++++++++++++++++++++++++-------- src/gc-arena/src/collect.rs | 30 ++++++++++++++++++++ 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/src/gc-arena-derive/src/lib.rs b/src/gc-arena-derive/src/lib.rs index 92be5f8..489137b 100644 --- a/src/gc-arena-derive/src/lib.rs +++ b/src/gc-arena-derive/src/lib.rs @@ -19,6 +19,7 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { } let mut mode = None; + let mut override_bound = None; for attr in &s.ast().attrs { match attr.parse_meta() { @@ -33,15 +34,32 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { panic!("`Collect` mode was already specified with `#[collect({})]`, cannot specify twice", prev_mode_str); } - if let Some(syn::NestedMeta::Meta(syn::Meta::Path(path))) = nested.first() { - if path.is_ident("require_static") { - mode = Some(Mode::RequireStatic); - } else if path.is_ident("no_drop") { - mode = Some(Mode::NoDrop); - } else if path.is_ident("unsafe_drop") { - mode = Some(Mode::UnsafeDrop); - } else { - panic!("`#[collect]` requires one of: \"require_static\", \"no_drop\", or \"unsafe_drop\" as an argument"); + for nested in nested { + match nested { + syn::NestedMeta::Meta(syn::Meta::Path(path)) => { + if path.is_ident("require_static") { + mode = Some(Mode::RequireStatic); + } else if path.is_ident("no_drop") { + mode = Some(Mode::NoDrop); + } else if path.is_ident("unsafe_drop") { + mode = Some(Mode::UnsafeDrop); + } else { + panic!("`#[collect]` requires one of: \"require_static\", \"no_drop\", or \"unsafe_drop\" as an argument"); + } + } + syn::NestedMeta::Meta(syn::Meta::NameValue(value)) => { + if value.path.is_ident("bound") { + match value.lit { + syn::Lit::Str(x) => override_bound = Some(x), + _ => { + panic!("`#[collect]` bound expression must be a string") + } + } + } else { + panic!("`#[collect]` encountered an unknown nested item"); + } + } + _ => panic!("`#[collect]` encountered an unknown nested item"), } } } @@ -55,7 +73,13 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { let where_clause = if mode == Mode::RequireStatic { quote!(where Self: 'static) } else { - quote!() + override_bound + .as_ref() + .map(|x| { + x.parse() + .expect("`#[collect]` failed to parse explicit trait bound expression") + }) + .unwrap_or_else(|| quote!()) }; let mut errors = vec![]; @@ -192,7 +216,12 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { ) }); - s.clone().add_bounds(AddBounds::Fields).gen_impl(quote! { + let bounds_type = if override_bound.is_some() { + AddBounds::None + } else { + AddBounds::Fields + }; + s.clone().add_bounds(bounds_type).gen_impl(quote! { gen unsafe impl gc_arena::Collect for @Self #where_clause { #[inline] fn needs_trace() -> bool { diff --git a/src/gc-arena/src/collect.rs b/src/gc-arena/src/collect.rs index 8a24813..9a17bd6 100644 --- a/src/gc-arena/src/collect.rs +++ b/src/gc-arena/src/collect.rs @@ -18,6 +18,36 @@ use crate::context::CollectionContext; /// barrier requirement cannot be broken when procedurally deriving `Collect`. A safe way of /// providing internal mutability in this case is to use `GcCell`, which provides internal /// mutability while ensuring that the write barrier is always executed. +/// +/// ## Deriving `Collect` +/// +/// To derive `Collect`, an additional attribute is required on the struct/enum called `collect`. +/// This has several optional arguments, but the only required argument is the derive strategy. +/// This can be one of +/// +/// - `#[collect(require_static)]` - Adds a `'static` bound, which allows for a no-op trace implementation. +/// This is the ideal choice where possible. +/// - `#[collect(no_drop)]` - The typical safe tracing derive strategy which only has to add a requirement +/// that your struct/enum does not have a custom implementation of `Drop`. +/// - `#[collect(unsafe_drop)]` - The most versatile tracing derive strategy which allows a custom drop implementation. +/// However, this strategy can lead to unsoundness if care is not taken (see the above explanation of `Drop` interactions). +/// +/// The `collect` attribute also accepts a number of optional configuration settings: +/// +/// - `#[collect(bound = "")]` - Replaces the default generated `where` clause with the given code. +/// This can be an empty string to add no `where` clause, or otherwise must start with `"where"`, +/// e.g., `#[collect(bound = "where T: Collect")]`. +/// Note that this option is ignored for `require_static` mode since the only bound it produces is `Self: 'static`. +/// Also note that providing an explicit bound in this way is safe, and only changes the trait bounds used +/// to enable the implementation of `Collect`. +/// +/// Options may be passed to the `collect` attribute together, e.g., `#[collect(no_drop, bound = "")]`. +/// +/// The `collect` attribute may also be used on any field of an enum or struct, however the only allowed usage +/// is to specify the strategy as `require_static` (no other strategies are allowed, and no optional settings can be specified). +/// This will add a `'static` bound to the type of the field (regardless of an explicit `bound` setting) +/// in exchange for not having to trace into the given field (the ideal choice where possible). +/// Note that if the entire struct/enum is marked with `require_static` then this is unnecessary. pub unsafe trait Collect { /// As an optimization, if this type can never hold a `Gc` pointer and `trace` is unnecessary to /// call, you may implement this method and return false. The default implementation returns From df54c765752ae1dda074c572036059b1861cdf16 Mon Sep 17 00:00:00 2001 From: Devin Jean Date: Tue, 22 Nov 2022 12:55:28 -0600 Subject: [PATCH 2/4] move derive doc info --- src/gc-arena-derive/src/lib.rs | 34 +++++++++++++++++++++++++++++++++- src/gc-arena/src/collect.rs | 32 +------------------------------- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/src/gc-arena-derive/src/lib.rs b/src/gc-arena-derive/src/lib.rs index 489137b..bc7154d 100644 --- a/src/gc-arena-derive/src/lib.rs +++ b/src/gc-arena-derive/src/lib.rs @@ -252,4 +252,36 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { } } -decl_derive!([Collect, attributes(collect)] => collect_derive); +decl_derive! { + [Collect, attributes(collect)] => + /// Derives the `Collect` trait needed to trace a gc type. + /// + /// To derive `Collect`, an additional attribute is required on the struct/enum called `collect`. + /// This has several optional arguments, but the only required argument is the derive strategy. + /// This can be one of + /// + /// - `#[collect(require_static)]` - Adds a `'static` bound, which allows for a no-op trace implementation. + /// This is the ideal choice where possible. + /// - `#[collect(no_drop)]` - The typical safe tracing derive strategy which only has to add a requirement + /// that your struct/enum does not have a custom implementation of `Drop`. + /// - `#[collect(unsafe_drop)]` - The most versatile tracing derive strategy which allows a custom drop implementation. + /// However, this strategy can lead to unsoundness if care is not taken (see the above explanation of `Drop` interactions). + /// + /// The `collect` attribute also accepts a number of optional configuration settings: + /// + /// - `#[collect(bound = "")]` - Replaces the default generated `where` clause with the given code. + /// This can be an empty string to add no `where` clause, or otherwise must start with `"where"`, + /// e.g., `#[collect(bound = "where T: Collect")]`. + /// Note that this option is ignored for `require_static` mode since the only bound it produces is `Self: 'static`. + /// Also note that providing an explicit bound in this way is safe, and only changes the trait bounds used + /// to enable the implementation of `Collect`. + /// + /// Options may be passed to the `collect` attribute together, e.g., `#[collect(no_drop, bound = "")]`. + /// + /// The `collect` attribute may also be used on any field of an enum or struct, however the only allowed usage + /// is to specify the strategy as `require_static` (no other strategies are allowed, and no optional settings can be specified). + /// This will add a `'static` bound to the type of the field (regardless of an explicit `bound` setting) + /// in exchange for not having to trace into the given field (the ideal choice where possible). + /// Note that if the entire struct/enum is marked with `require_static` then this is unnecessary. + collect_derive +} diff --git a/src/gc-arena/src/collect.rs b/src/gc-arena/src/collect.rs index 9a17bd6..740ef2f 100644 --- a/src/gc-arena/src/collect.rs +++ b/src/gc-arena/src/collect.rs @@ -11,43 +11,13 @@ use crate::context::CollectionContext; /// 3. Internal mutability *must* not be used to adopt new `Gc` pointers without calling /// `Gc::write_barrier` during the same arena mutation. /// -/// It is, however, possible to implement this trait safely by procedurally deriving it, which +/// It is, however, possible to implement this trait safely by procedurally deriving it (see [`gc_arena_derive::Collect`]), which /// requires that every field in the structure also implement `Collect`, and implements a safe, /// empty version of `Drop`. Internally mutable types like `Cell` and `RefCell` do not implement /// `Collect` in such a way that it is possible to store `Gc` pointers inside them, so the write /// barrier requirement cannot be broken when procedurally deriving `Collect`. A safe way of /// providing internal mutability in this case is to use `GcCell`, which provides internal /// mutability while ensuring that the write barrier is always executed. -/// -/// ## Deriving `Collect` -/// -/// To derive `Collect`, an additional attribute is required on the struct/enum called `collect`. -/// This has several optional arguments, but the only required argument is the derive strategy. -/// This can be one of -/// -/// - `#[collect(require_static)]` - Adds a `'static` bound, which allows for a no-op trace implementation. -/// This is the ideal choice where possible. -/// - `#[collect(no_drop)]` - The typical safe tracing derive strategy which only has to add a requirement -/// that your struct/enum does not have a custom implementation of `Drop`. -/// - `#[collect(unsafe_drop)]` - The most versatile tracing derive strategy which allows a custom drop implementation. -/// However, this strategy can lead to unsoundness if care is not taken (see the above explanation of `Drop` interactions). -/// -/// The `collect` attribute also accepts a number of optional configuration settings: -/// -/// - `#[collect(bound = "")]` - Replaces the default generated `where` clause with the given code. -/// This can be an empty string to add no `where` clause, or otherwise must start with `"where"`, -/// e.g., `#[collect(bound = "where T: Collect")]`. -/// Note that this option is ignored for `require_static` mode since the only bound it produces is `Self: 'static`. -/// Also note that providing an explicit bound in this way is safe, and only changes the trait bounds used -/// to enable the implementation of `Collect`. -/// -/// Options may be passed to the `collect` attribute together, e.g., `#[collect(no_drop, bound = "")]`. -/// -/// The `collect` attribute may also be used on any field of an enum or struct, however the only allowed usage -/// is to specify the strategy as `require_static` (no other strategies are allowed, and no optional settings can be specified). -/// This will add a `'static` bound to the type of the field (regardless of an explicit `bound` setting) -/// in exchange for not having to trace into the given field (the ideal choice where possible). -/// Note that if the entire struct/enum is marked with `require_static` then this is unnecessary. pub unsafe trait Collect { /// As an optimization, if this type can never hold a `Gc` pointer and `trace` is unnecessary to /// call, you may implement this method and return false. The default implementation returns From 4af8d946bc17b103bc2887b9f69b6bed1f94a5c0 Mon Sep 17 00:00:00 2001 From: Devin Jean Date: Tue, 22 Nov 2022 22:10:10 -0600 Subject: [PATCH 3/4] switch to generics bounds --- src/gc-arena-derive/src/lib.rs | 2 +- src/gc-sequence/src/and_then.rs | 7 +++++-- src/gc-sequence/src/map.rs | 4 ++-- src/gc-sequence/src/map_result.rs | 6 +++--- src/gc-sequence/src/sequence_fn.rs | 4 ++-- src/gc-sequence/src/then.rs | 10 ++++++++-- 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/gc-arena-derive/src/lib.rs b/src/gc-arena-derive/src/lib.rs index bc7154d..f8c0213 100644 --- a/src/gc-arena-derive/src/lib.rs +++ b/src/gc-arena-derive/src/lib.rs @@ -219,7 +219,7 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { let bounds_type = if override_bound.is_some() { AddBounds::None } else { - AddBounds::Fields + AddBounds::Generics }; s.clone().add_bounds(bounds_type).gen_impl(quote! { gen unsafe impl gc_arena::Collect for @Self #where_clause { diff --git a/src/gc-sequence/src/and_then.rs b/src/gc-sequence/src/and_then.rs index e8d09c6..d9d7e31 100644 --- a/src/gc-sequence/src/and_then.rs +++ b/src/gc-sequence/src/and_then.rs @@ -4,7 +4,7 @@ use crate::Sequence; #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, I: Collect, F: 'static")] pub enum AndThen { First(S, Option>), Second(Option<(I, StaticCollect)>), @@ -47,7 +47,10 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect( + no_drop, + bound = "where S: Collect, C: Collect, I: Collect, F: 'static" +)] pub enum AndThenWith { First(S, Option<(C, StaticCollect)>), Second(Option<(C, I, StaticCollect)>), diff --git a/src/gc-sequence/src/map.rs b/src/gc-sequence/src/map.rs index f13fef0..48c2c0a 100644 --- a/src/gc-sequence/src/map.rs +++ b/src/gc-sequence/src/map.rs @@ -4,7 +4,7 @@ use crate::Sequence; #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, F: 'static")] pub struct Map(S, Option>); impl Map { @@ -32,7 +32,7 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, C: Collect, F: 'static")] pub struct MapWith(S, Option<(C, StaticCollect)>); impl MapWith { diff --git a/src/gc-sequence/src/map_result.rs b/src/gc-sequence/src/map_result.rs index e8778b6..3ef1057 100644 --- a/src/gc-sequence/src/map_result.rs +++ b/src/gc-sequence/src/map_result.rs @@ -4,7 +4,7 @@ use crate::Sequence; #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, F: 'static")] pub struct MapOk(S, Option>); impl MapOk { @@ -35,7 +35,7 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, C: Collect, F: 'static")] pub struct MapOkWith(S, Option<(C, StaticCollect)>); impl MapOkWith { @@ -66,7 +66,7 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where S: Collect, F: 'static")] pub struct MapError(S, Option>); impl MapError { diff --git a/src/gc-sequence/src/sequence_fn.rs b/src/gc-sequence/src/sequence_fn.rs index d1f250e..a775873 100644 --- a/src/gc-sequence/src/sequence_fn.rs +++ b/src/gc-sequence/src/sequence_fn.rs @@ -11,7 +11,7 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where F: 'static")] pub struct SequenceFn(Option>); impl SequenceFn { @@ -43,7 +43,7 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect(no_drop, bound = "where C: Collect, F: 'static")] pub struct SequenceFnWith(Option<(C, StaticCollect)>); impl SequenceFnWith { diff --git a/src/gc-sequence/src/then.rs b/src/gc-sequence/src/then.rs index 05502c3..0e6aa80 100644 --- a/src/gc-sequence/src/then.rs +++ b/src/gc-sequence/src/then.rs @@ -4,7 +4,10 @@ use crate::Sequence; #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect( + no_drop, + bound = "where >::Output: Collect, F: 'static" +)] pub enum Then<'gc, S, F> where S: Sequence<'gc>, @@ -49,7 +52,10 @@ where #[must_use = "sequences do nothing unless stepped"] #[derive(Debug, Collect)] -#[collect(no_drop)] +#[collect( + no_drop, + bound = "where C: Collect, >::Output: Collect, F: 'static" +)] pub enum ThenWith<'gc, S, C, F> where S: Sequence<'gc>, From fc61973cfc1e4a2a8339e0fdd92fdfb9eb5ffb2b Mon Sep 17 00:00:00 2001 From: Devin Jean Date: Wed, 23 Nov 2022 11:38:49 -0600 Subject: [PATCH 4/4] better errors + catch mult modes/bounds --- src/gc-arena-derive/src/lib.rs | 42 ++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/gc-arena-derive/src/lib.rs b/src/gc-arena-derive/src/lib.rs index f8c0213..813e0bc 100644 --- a/src/gc-arena-derive/src/lib.rs +++ b/src/gc-arena-derive/src/lib.rs @@ -18,6 +18,13 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { UnsafeDrop, } + macro_rules! usage_error { + ($($t:tt)*) => {{ + let msg = format!($($t)*); + panic!("{}. `#[collect(...)]` requires one mode (`require_static`, `no_drop`, or `unsafe_drop`) and optionally `bound = \"...\"`.", msg); + }} + } + let mut mode = None; let mut override_bound = None; @@ -25,18 +32,17 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { match attr.parse_meta() { Ok(syn::Meta::List(syn::MetaList { path, nested, .. })) => { if path.is_ident("collect") { - if let Some(prev_mode) = mode { - let prev_mode_str = match prev_mode { - Mode::RequireStatic => "require_static", - Mode::NoDrop => "no_drop", - Mode::UnsafeDrop => "unsafe_drop", - }; - panic!("`Collect` mode was already specified with `#[collect({})]`, cannot specify twice", prev_mode_str); + if mode.is_some() { + panic!("multiple `#[collect(...)]` attributes found on the same item. consider combining them."); } for nested in nested { match nested { syn::NestedMeta::Meta(syn::Meta::Path(path)) => { + if mode.is_some() { + usage_error!("multiple modes specified"); + } + if path.is_ident("require_static") { mode = Some(Mode::RequireStatic); } else if path.is_ident("no_drop") { @@ -44,31 +50,39 @@ fn collect_derive(mut s: synstructure::Structure) -> TokenStream { } else if path.is_ident("unsafe_drop") { mode = Some(Mode::UnsafeDrop); } else { - panic!("`#[collect]` requires one of: \"require_static\", \"no_drop\", or \"unsafe_drop\" as an argument"); + usage_error!("unknown option") } } syn::NestedMeta::Meta(syn::Meta::NameValue(value)) => { + if override_bound.is_some() { + usage_error!("multiple bounds specified"); + } + if value.path.is_ident("bound") { match value.lit { syn::Lit::Str(x) => override_bound = Some(x), - _ => { - panic!("`#[collect]` bound expression must be a string") - } + _ => usage_error!("bound must be str"), } } else { - panic!("`#[collect]` encountered an unknown nested item"); + usage_error!("unknown option"); } } - _ => panic!("`#[collect]` encountered an unknown nested item"), + _ => usage_error!("unknown option"), } } + + if mode.is_none() { + usage_error!("missing mode"); + } } } _ => {} } } - let mode = mode.expect("deriving `Collect` requires a `#[collect()]` attribute, where `` is one of \"require_static\", \"no_drop\", or \"unsafe_drop\""); + let mode = mode.unwrap_or_else(|| { + usage_error!("deriving `Collect` requires a `#[collect(...)]` attribute") + }); let where_clause = if mode == Mode::RequireStatic { quote!(where Self: 'static)