From 06ba4610882cb937c064567ec3ddf330e81b5f74 Mon Sep 17 00:00:00 2001 From: Ted Driggs Date: Thu, 22 Feb 2024 14:23:06 -0800 Subject: [PATCH] Refactor visibility options with #[darling(flatten)] This reduces code duplication and makes it impossible for the options structs to see an invalid visibility state. --- .../tests/compile-fail/vis_conflict.stderr | 20 +- derive_builder_core/Cargo.toml | 2 +- .../src/macro_options/darling_opts.rs | 269 ++++++------------ 3 files changed, 96 insertions(+), 195 deletions(-) diff --git a/derive_builder/tests/compile-fail/vis_conflict.stderr b/derive_builder/tests/compile-fail/vis_conflict.stderr index c8c2ed82..8d0b2e06 100644 --- a/derive_builder/tests/compile-fail/vis_conflict.stderr +++ b/derive_builder/tests/compile-fail/vis_conflict.stderr @@ -1,28 +1,16 @@ -error: `public` and `private` cannot be used together - --> tests/compile-fail/vis_conflict.rs:7:15 - | -7 | #[builder(public, private)] - | ^^^^^^ - -error: `vis="..."` cannot be used with `public` or `private` +error: `public` and `vis` cannot be used together --> tests/compile-fail/vis_conflict.rs:5:25 | 5 | #[builder(public, vis = "pub(crate)")] | ^^^^^^^^^^^^ error: `public` and `private` cannot be used together - --> tests/compile-fail/vis_conflict.rs:12:57 + --> tests/compile-fail/vis_conflict.rs:12:48 | 12 | #[builder(public, vis = "pub(crate)", build_fn(private, public))] - | ^^^^^^ - -error: `vis="..."` cannot be used with `public` or `private` - --> tests/compile-fail/vis_conflict.rs:14:30 - | -14 | #[builder(private, vis = "pub")] - | ^^^^^ + | ^^^^^^^ -error: `vis="..."` cannot be used with `public` or `private` +error: `public` and `vis` cannot be used together --> tests/compile-fail/vis_conflict.rs:12:25 | 12 | #[builder(public, vis = "pub(crate)", build_fn(private, public))] diff --git a/derive_builder_core/Cargo.toml b/derive_builder_core/Cargo.toml index ec9cf2a6..dcb5218c 100644 --- a/derive_builder_core/Cargo.toml +++ b/derive_builder_core/Cargo.toml @@ -21,7 +21,7 @@ clippy = [] lib_has_std = [] [dependencies] -darling = "0.20.6" +darling = "0.20.7" proc-macro2 = "1.0.37" quote = "1.0.35" syn = { version = "2.0.15", features = ["full", "extra-traits"] } diff --git a/derive_builder_core/src/macro_options/darling_opts.rs b/derive_builder_core/src/macro_options/darling_opts.rs index 062e8e51..e9e03705 100644 --- a/derive_builder_core/src/macro_options/darling_opts.rs +++ b/derive_builder_core/src/macro_options/darling_opts.rs @@ -12,58 +12,79 @@ use crate::{ DeprecationNotes, Each, FieldConversion, Initializer, Setter, }; -/// `derive_builder` uses separate sibling keywords to represent -/// mutually-exclusive visibility states. This trait requires implementers to -/// expose those property values and provides a method to compute any explicit visibility -/// bounds. -trait Visibility { - fn public(&self) -> &Flag; - fn private(&self) -> &Flag; - fn explicit(&self) -> Option<&syn::Visibility>; - - /// Get the explicitly-expressed visibility preference from the attribute. - /// This returns `None` if the input didn't include either keyword. - /// - /// # Panics - /// This method panics if the input specifies both `public` and `private`. - fn as_expressed_vis(&self) -> Option> { - let declares_public = self.public().is_present(); - let declares_private = self.private().is_present(); - let declares_explicit = self.explicit().is_some(); - - if declares_private { - assert!(!declares_public && !declares_explicit); - Some(Cow::Owned(syn::Visibility::Inherited)) - } else if let Some(vis) = self.explicit() { - assert!(!declares_public); - Some(Cow::Borrowed(vis)) - } else if declares_public { - Some(Cow::Owned(syn::parse_quote!(pub))) - } else { - None +#[derive(Debug, Clone)] +enum VisibilityAttr { + /// `public` + Public(Span), + /// `private` + Private, + /// `vis = "pub(crate)"` + Explicit(syn::Visibility), + None, +} + +impl VisibilityAttr { + pub fn to_explicit_visibility(&self) -> Option> { + match self { + Self::Public(span) => Some(Cow::Owned(syn::Visibility::Public( + parse_quote_spanned!(*span=> pub), + ))), + Self::Private => Some(Cow::Owned(syn::Visibility::Inherited)), + Self::Explicit(v) => Some(Cow::Borrowed(v)), + Self::None => None, } } } -fn no_visibility_conflict(v: &T) -> darling::Result<()> { - let declares_public = v.public().is_present(); - let declares_private = v.private().is_present(); - if let Some(vis) = v.explicit() { - if declares_public || declares_private { - Err( - Error::custom(r#"`vis="..."` cannot be used with `public` or `private`"#) - .with_span(vis), - ) +impl Default for VisibilityAttr { + fn default() -> Self { + Self::None + } +} + +impl FromMeta for VisibilityAttr { + fn from_list(items: &[darling::ast::NestedMeta]) -> darling::Result { + #[derive(FromMeta)] + struct VisibilityAttrInternal { + public: Flag, + private: Flag, + vis: Option, + } + + let VisibilityAttrInternal { + public, + private, + vis: explicit, + } = VisibilityAttrInternal::from_list(items)?; + + let mut conflicts = Error::accumulator(); + + if public.is_present() { + if private.is_present() { + conflicts.push( + Error::custom("`public` and `private` cannot be used together") + .with_span(&private.span()), + ); + } + + if let Some(vis) = explicit { + conflicts.push( + Error::custom("`public` and `vis` cannot be used together").with_span(&vis), + ); + } + + conflicts.finish_with(Self::Public(public.span())) + } else if let Some(vis) = explicit { + if private.is_present() { + conflicts.push(Error::custom("`vis` and `private` cannot be used together")); + } + + conflicts.finish_with(Self::Explicit(vis)) + } else if private.is_present() { + conflicts.finish_with(Self::Private) } else { - Ok(()) + conflicts.finish_with(Self::None) } - } else if declares_public && declares_private { - Err( - Error::custom(r#"`public` and `private` cannot be used together"#) - .with_span(&v.public().span()), - ) - } else { - Ok(()) } } @@ -115,9 +136,8 @@ pub struct BuildFn { skip: bool, name: Ident, validate: Option, - public: Flag, - private: Flag, - vis: Option, + #[darling(flatten)] + visibility: VisibilityAttr, /// Either the path to an existing error type that the build method should return or a meta /// list of options to modify the generated error. /// @@ -169,60 +189,19 @@ impl Default for BuildFn { skip: false, name: Ident::new("build", Span::call_site()), validate: None, - public: Default::default(), - private: Default::default(), - vis: None, + visibility: Default::default(), error: None, } } } -impl Visibility for BuildFn { - fn public(&self) -> &Flag { - &self.public - } - - fn private(&self) -> &Flag { - &self.private - } - - fn explicit(&self) -> Option<&syn::Visibility> { - self.vis.as_ref() - } -} - -/// Contents of the `field` meta in `builder` attributes at the struct level. -#[derive(Debug, Clone, Default, FromMeta)] -pub struct StructLevelFieldMeta { - public: Flag, - private: Flag, - vis: Option, -} - -impl Visibility for StructLevelFieldMeta { - fn public(&self) -> &Flag { - &self.public - } - - fn private(&self) -> &Flag { - &self.private - } - - fn explicit(&self) -> Option<&syn::Visibility> { - self.vis.as_ref() - } -} - /// Contents of the `field` meta in `builder` attributes at the field level. // // This is a superset of the attributes permitted in `field` at the struct level. -// Perhaps in the future we will be able to use `#[darling(flatten)]`, but -// that does not exist right now: https://github.com/TedDriggs/darling/issues/146 #[derive(Debug, Clone, Default, FromMeta)] pub struct FieldLevelFieldMeta { - public: Flag, - private: Flag, - vis: Option, + #[darling(flatten)] + visibility: VisibilityAttr, /// Custom builder field type #[darling(rename = "ty")] builder_type: Option, @@ -230,20 +209,6 @@ pub struct FieldLevelFieldMeta { build: Option, } -impl Visibility for FieldLevelFieldMeta { - fn public(&self) -> &Flag { - &self.public - } - - fn private(&self) -> &Flag { - &self.private - } - - fn explicit(&self) -> Option<&syn::Visibility> { - self.vis.as_ref() - } -} - #[derive(Debug, Clone, Default, FromMeta)] pub struct StructLevelSetter { prefix: Option, @@ -355,14 +320,8 @@ pub struct Field { /// Field-level override for builder pattern. /// Note that setting this may force the builder to derive `Clone`. pattern: Option, - public: Flag, - private: Flag, - /// Declared visibility for the field in the builder, e.g. `#[builder(vis = "...")]`. - /// - /// This cannot be named `vis` or `darling` would put the deriving field's visibility into the - /// field instead. - #[darling(rename = "vis")] - visibility: Option, + #[darling(flatten)] + visibility: VisibilityAttr, // See the documentation for `FieldSetterMeta` to understand how `darling` // is interpreting this field. #[darling(default, with = field_setter)] @@ -388,13 +347,6 @@ pub struct Field { } impl Field { - fn no_visibility_conflicts(&self) -> darling::Result<()> { - let mut errors = Error::accumulator(); - errors.handle(no_visibility_conflict(&self.field)); - errors.handle(no_visibility_conflict(self)); - errors.finish() - } - /// Resolve and check (post-parsing) options which come from multiple darling options /// /// * Check that we don't have a custom field type or builder *and* a default value @@ -527,20 +479,6 @@ fn unnest_from_one_attribute(attr: syn::Attribute) -> darling::Result } } -impl Visibility for Field { - fn public(&self) -> &Flag { - &self.public - } - - fn private(&self) -> &Flag { - &self.private - } - - fn explicit(&self) -> Option<&syn::Visibility> { - self.visibility.as_ref() - } -} - fn default_crate_root() -> Path { parse_quote!(::derive_builder) } @@ -610,15 +548,11 @@ pub struct Options { /// Struct-level value to use in place of any unfilled fields default: Option, - public: Flag, - - private: Flag, - /// Desired visibility of the builder struct. /// /// Do not confuse this with `Options::vis`, which is the visibility of the deriving struct. - #[darling(rename = "vis")] - visibility: Option, + #[darling(flatten)] + visibility: VisibilityAttr, /// The parsed body of the derived struct. data: darling::ast::Data, @@ -630,50 +564,24 @@ pub struct Options { try_setter: Flag, #[darling(default)] - field: StructLevelFieldMeta, + field: VisibilityAttr, #[darling(skip, default)] deprecation_notes: DeprecationNotes, } -impl Visibility for Options { - fn public(&self) -> &Flag { - &self.public - } - - fn private(&self) -> &Flag { - &self.private - } - - fn explicit(&self) -> Option<&syn::Visibility> { - self.visibility.as_ref() - } -} - impl Options { /// Populate `self.struct_attrs` and `self.impl_attrs` by draining `self.attrs` fn unnest_attrs(mut self) -> darling::Result { - let mut errors = Error::accumulator(); - - errors.handle(distribute_and_unnest_attrs( + distribute_and_unnest_attrs( &mut self.attrs, &mut [ ("builder_struct_attr", &mut self.struct_attrs), ("builder_impl_attr", &mut self.impl_attrs), ], - )); + )?; - // Check for conflicting visibility declarations. These cannot be pushed - // down into `FieldMeta` et al because of the call to `no_visibility_conflict(&self)`, - // as all sub-fields must be valid for this `Options` function to run. - errors.handle(no_visibility_conflict(&self.field)); - errors.handle(no_visibility_conflict(&self.build_fn)); - self.data - .as_ref() - .map_struct_fields(|f| errors.handle(f.no_visibility_conflicts())); - errors.handle(no_visibility_conflict(&self)); - - errors.finish_with(self) + Ok(self) } } @@ -701,14 +609,17 @@ impl Options { /// If a visibility was declared in attributes, that will be used; /// otherwise the struct's own visibility will be used. pub fn builder_vis(&self) -> Cow { - self.as_expressed_vis().unwrap_or(Cow::Borrowed(&self.vis)) + self.visibility + .to_explicit_visibility() + .unwrap_or_else(|| Cow::Borrowed(&self.vis)) } /// Get the visibility of the emitted `build` method. /// This defaults to the visibility of the parent builder, but can be overridden. pub fn build_method_vis(&self) -> Cow { self.build_fn - .as_expressed_vis() + .visibility + .to_explicit_visibility() .unwrap_or_else(|| self.builder_vis()) } @@ -876,8 +787,9 @@ impl<'a> FieldWithDefaults<'a> { /// Get the visibility of the emitted setter, if there will be one. pub fn setter_vis(&self) -> Cow { self.field - .as_expressed_vis() - .or_else(|| self.parent.as_expressed_vis()) + .visibility + .to_explicit_visibility() + .or_else(|| self.parent.visibility.to_explicit_visibility()) .unwrap_or_else(|| Cow::Owned(syn::parse_quote!(pub))) } @@ -893,7 +805,8 @@ impl<'a> FieldWithDefaults<'a> { pub fn field_vis(&self) -> Cow { self.field .field - .as_expressed_vis() + .visibility + .to_explicit_visibility() .or_else( // Disabled fields become a PhantomData in the builder. We make that field // non-public, even if the rest of the builder is public, since this field is just @@ -906,7 +819,7 @@ impl<'a> FieldWithDefaults<'a> { } }, ) - .or_else(|| self.parent.field.as_expressed_vis()) + .or_else(|| self.parent.field.to_explicit_visibility()) .unwrap_or(Cow::Owned(syn::Visibility::Inherited)) }