From ebd8aba2fa95530dbb878941845fa74360f38bde Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Sun, 25 Jul 2021 10:46:06 +0200 Subject: [PATCH] Disallow `Option<_>` type for fields with `#[nested]` or `#[default]` Regarding nested fields: I cannot imagine a situation where that distinction is useful. Also, specifying an empty nested object looks stupid in TOML and YAML anyway. Regarding default fields: If there is a default value, then the field should not be declared as optional to begin with. --- macro/src/gen.rs | 144 ++++++++++++++++------------------------------ macro/src/ir.rs | 44 +++++++++++--- macro/src/lib.rs | 1 + macro/src/util.rs | 46 +++++++++++++++ src/meta.rs | 4 +- src/toml.rs | 25 ++++---- 6 files changed, 146 insertions(+), 118 deletions(-) create mode 100644 macro/src/util.rs diff --git a/macro/src/gen.rs b/macro/src/gen.rs index 6306ee9..b9318fc 100644 --- a/macro/src/gen.rs +++ b/macro/src/gen.rs @@ -23,18 +23,23 @@ fn gen_config_impl(input: &ir::Input) -> TokenStream { let from_exprs = input.fields.iter().map(|f| { let field_name = &f.name; let path = field_name.to_string(); - if !f.is_leaf() { - quote! { - confique::Config::from_partial(partial.#field_name).map_err(|e| { - confique::internal::prepend_missing_value_error(e, #path) - })? + match f.kind { + FieldKind::Nested { .. } => { + quote! { + confique::Config::from_partial(partial.#field_name).map_err(|e| { + confique::internal::prepend_missing_value_error(e, #path) + })? + } } - } else if !is_option(&f.ty) { - quote! { - partial.#field_name.ok_or(confique::internal::missing_value_error(#path.into()))? + FieldKind::OptionalLeaf { .. } => { + quote! { partial.#field_name } + } + FieldKind::RequiredLeaf { .. } => { + quote! { + partial.#field_name + .ok_or(confique::internal::missing_value_error(#path.into()))? + } } - } else { - quote! { partial.#field_name } } }); @@ -69,23 +74,22 @@ fn partial_names(original_name: &Ident) -> (Ident, Ident) { fn gen_partial_mod(input: &ir::Input) -> TokenStream { let (mod_name, struct_name) = partial_names(&input.name); let visibility = &input.visibility; - let inner_visibility = inner_visibility(&input.visibility); + let inner_vis = inner_visibility(&input.visibility); // Prepare some tokens per field. let field_names = input.fields.iter().map(|f| &f.name).collect::>(); let struct_fields = input.fields.iter().map(|f| { let name = &f.name; - if f.is_leaf() { - let inner = unwrap_option(&f.ty).unwrap_or(&f.ty); - quote! { #inner_visibility #name: Option<#inner>, } - } else { - let ty = &f.ty; - quote! { + match &f.kind { + FieldKind::RequiredLeaf { ty, .. } => quote! { #inner_vis #name: Option<#ty> }, + FieldKind::OptionalLeaf { inner_ty } => quote! { #inner_vis #name: Option<#inner_ty> }, + FieldKind::Nested { ty } => quote! { #[serde(default = "confique::Partial::empty")] - #inner_visibility #name: <#ty as confique::Config>::Partial, - } + #inner_vis #name: <#ty as confique::Config>::Partial + }, } }); + let empty_values = input.fields.iter().map(|f| { if f.is_leaf() { quote! { None } @@ -93,10 +97,13 @@ fn gen_partial_mod(input: &ir::Input) -> TokenStream { quote! { confique::Partial::empty() } } }); + let defaults = input.fields.iter().map(|f| { match &f.kind { - FieldKind::Leaf { default: None } => quote! { None }, - FieldKind::Leaf { default: Some(default) } => { + FieldKind::RequiredLeaf { default: None, .. } | FieldKind::OptionalLeaf { .. } => { + quote! { None } + } + FieldKind::RequiredLeaf { default: Some(default), .. } => { let msg = format!( "default config value for `{}::{}` cannot be deserialized", input.name, @@ -107,15 +114,10 @@ fn gen_partial_mod(input: &ir::Input) -> TokenStream { Some(confique::internal::deserialize_default(#default).expect(#msg)) } } - FieldKind::Nested => { - if is_option(&f.ty) { - quote! { Some(confique::Partial::default_values()) } - } else { - quote! { confique::Partial::default_values() } - } - } + FieldKind::Nested { .. } => quote! { confique::Partial::default_values() }, } }); + let fallbacks= input.fields.iter().map(|f| { let name = &f.name; if f.is_leaf() { @@ -124,24 +126,22 @@ fn gen_partial_mod(input: &ir::Input) -> TokenStream { quote! { self.#name.with_fallback(fallback.#name) } } }); + let is_empty_exprs = input.fields.iter().map(|f| { let name = &f.name; - match f.kind { - FieldKind::Leaf { .. } => quote! { self.#name.is_none() }, - FieldKind::Nested => quote! { self.#name.is_empty() }, + if f.is_leaf() { + quote! { self.#name.is_none() } + } else { + quote! { self.#name.is_empty() } } }); + let is_complete_expr = input.fields.iter().map(|f| { let name = &f.name; match f.kind { - FieldKind::Leaf { .. } => { - if is_option(&f.ty) { - quote! { true } - } else { - quote! { self.#name.is_some() } - } - } - FieldKind::Nested => quote! { self.#name.is_complete() }, + FieldKind::OptionalLeaf { .. } => quote! { true }, + FieldKind::RequiredLeaf { .. } => quote! { self.#name.is_some() }, + FieldKind::Nested { .. } => quote! { self.#name.is_complete() }, } }); @@ -150,8 +150,8 @@ fn gen_partial_mod(input: &ir::Input) -> TokenStream { use super::*; #[derive(confique::serde::Deserialize)] - #inner_visibility struct #struct_name { - #( #struct_fields )* + #inner_vis struct #struct_name { + #( #struct_fields, )* } impl confique::Partial for #struct_name { @@ -194,26 +194,28 @@ fn gen_meta(input: &ir::Input) -> TokenStream { let name = f.name.to_string(); let doc = &f.doc; let kind = match &f.kind { - FieldKind::Nested => { - let ty = &f.ty; + FieldKind::Nested { ty }=> { quote! { confique::meta::FieldKind::Nested { meta: &<#ty as confique::Config>::META } } } - FieldKind::Leaf { default } => { - let default_value = gen_meta_default(default, &f.ty); + FieldKind::OptionalLeaf { .. } => { quote! { - confique::meta::FieldKind::Leaf { default: #default_value } + confique::meta::FieldKind::OptionalLeaf + } + } + FieldKind::RequiredLeaf { default, ty } => { + let default_value = gen_meta_default(default, &ty); + quote! { + confique::meta::FieldKind::RequiredLeaf { default: #default_value } } } }; - let is_optional = is_option(&f.ty); quote! { confique::meta::Field { name: #name, doc: &[ #(#doc),* ], - optional: #is_optional, kind: #kind, } } @@ -301,52 +303,6 @@ fn gen_meta_default(default: &Option, ty: &syn::Type) -> TokenStream { } } -/// Checks if the given type is an `Option` and if so, return the inner type. -/// -/// Note: this function clearly shows one of the major shortcomings of proc -/// macros right now: we do not have access to the compiler's type tables and -/// can only check if it "looks" like an `Option`. Of course, stuff can go -/// wrong. But that's the best we can do and it's highly unlikely that someone -/// shadows `Option`. -fn unwrap_option(ty: &syn::Type) -> Option<&syn::Type> { - let ty = match ty { - syn::Type::Path(path) => path, - _ => return None, - }; - - if ty.qself.is_some() || ty.path.leading_colon.is_some() { - return None; - } - - let valid_paths = [ - &["Option"] as &[_], - &["std", "option", "Option"], - &["core", "option", "Option"], - ]; - if !valid_paths.iter().any(|vp| ty.path.segments.iter().map(|s| &s.ident).eq(*vp)) { - return None; - } - - let args = match &ty.path.segments.last().unwrap().arguments { - syn::PathArguments::AngleBracketed(args) => args, - _ => return None, - }; - - if args.args.len() != 1 { - return None; - } - - match &args.args[0] { - syn::GenericArgument::Type(t) => Some(t), - _ => None, - } -} - -/// Returns `true` if the given type is `Option<_>`. -fn is_option(ty: &syn::Type) -> bool { - unwrap_option(ty).is_some() -} - impl ToTokens for ir::Expr { fn to_tokens(&self, tokens: &mut TokenStream) { match self { diff --git a/macro/src/ir.rs b/macro/src/ir.rs index c5d2e1f..6818994 100644 --- a/macro/src/ir.rs +++ b/macro/src/ir.rs @@ -2,6 +2,8 @@ use syn::{Error, Token, parse::{Parse, ParseStream}, spanned::Spanned}; +use crate::util::{is_option, unwrap_option}; + /// The parsed input to the `gen_config` macro. #[derive(Debug)] @@ -16,7 +18,6 @@ pub(crate) struct Input { pub(crate) struct Field { pub(crate) doc: Vec, pub(crate) name: syn::Ident, - pub(crate) ty: syn::Type, pub(crate) kind: FieldKind, // TODO: @@ -27,10 +28,21 @@ pub(crate) struct Field { #[derive(Debug)] pub(crate) enum FieldKind { - Leaf { + /// A non-optional leaf. `ty` is not `Option<_>`. + RequiredLeaf { default: Option, + ty: syn::Type, + }, + + /// A leaf with type `Option<_>`. + OptionalLeaf { + inner_ty: syn::Type, + }, + + /// A nested configuration. The type is never `Option<_>`. + Nested { + ty: syn::Type, }, - Nested, } /// The kinds of expressions (just literals) we allow for default or example @@ -76,6 +88,12 @@ impl Field { // TODO: check no other attributes are here let kind = if attrs.nested { + if is_option(&field.ty) { + return Err(Error::new( + field.ident.span(), + "nested configurations cannot be optional (type `Option<_>`)", + )); + } if attrs.default.is_some() { return Err(Error::new( field.ident.span(), @@ -83,23 +101,33 @@ impl Field { )); } - FieldKind::Nested + FieldKind::Nested { ty: field.ty } } else { - FieldKind::Leaf { - default: attrs.default, + match unwrap_option(&field.ty) { + None => FieldKind::RequiredLeaf { default: attrs.default, ty: field.ty }, + Some(inner) => { + if attrs.default.is_some() { + return Err(Error::new( + field.ident.span(), + "optional fields (type `Option<_>`) cannot have default \ + values (`#[config(default = ...)]`)", + )); + } + + FieldKind::OptionalLeaf { inner_ty: inner.clone() } + } } }; Ok(Self { doc, name: field.ident.expect("bug: expected named field"), - ty: field.ty, kind, }) } pub(crate) fn is_leaf(&self) -> bool { - matches!(self.kind, FieldKind::Leaf { .. }) + matches!(self.kind, FieldKind::RequiredLeaf { .. } | FieldKind::OptionalLeaf { .. }) } } diff --git a/macro/src/lib.rs b/macro/src/lib.rs index 228e646..51eaba7 100644 --- a/macro/src/lib.rs +++ b/macro/src/lib.rs @@ -3,6 +3,7 @@ use proc_macro::TokenStream as TokenStream1; mod gen; mod ir; +mod util; #[proc_macro_derive(Config, attributes(config))] diff --git a/macro/src/util.rs b/macro/src/util.rs new file mode 100644 index 0000000..d01d174 --- /dev/null +++ b/macro/src/util.rs @@ -0,0 +1,46 @@ + +/// Checks if the given type is an `Option` and if so, return the inner type. +/// +/// Note: this function clearly shows one of the major shortcomings of proc +/// macros right now: we do not have access to the compiler's type tables and +/// can only check if it "looks" like an `Option`. Of course, stuff can go +/// wrong. But that's the best we can do and it's highly unlikely that someone +/// shadows `Option`. +pub(crate) fn unwrap_option(ty: &syn::Type) -> Option<&syn::Type> { + let ty = match ty { + syn::Type::Path(path) => path, + _ => return None, + }; + + if ty.qself.is_some() || ty.path.leading_colon.is_some() { + return None; + } + + let valid_paths = [ + &["Option"] as &[_], + &["std", "option", "Option"], + &["core", "option", "Option"], + ]; + if !valid_paths.iter().any(|vp| ty.path.segments.iter().map(|s| &s.ident).eq(*vp)) { + return None; + } + + let args = match &ty.path.segments.last().unwrap().arguments { + syn::PathArguments::AngleBracketed(args) => args, + _ => return None, + }; + + if args.args.len() != 1 { + return None; + } + + match &args.args[0] { + syn::GenericArgument::Type(t) => Some(t), + _ => None, + } +} + +/// Returns `true` if the given type is `Option<_>`. +pub(crate) fn is_option(ty: &syn::Type) -> bool { + unwrap_option(ty).is_some() +} diff --git a/src/meta.rs b/src/meta.rs index ab78207..4f3d256 100644 --- a/src/meta.rs +++ b/src/meta.rs @@ -22,15 +22,15 @@ pub struct Meta { pub struct Field { pub name: &'static str, pub doc: &'static [&'static str], - pub optional: bool, pub kind: FieldKind, } #[derive(Clone, Copy, Debug)] pub enum FieldKind { - Leaf { + RequiredLeaf { default: Option, }, + OptionalLeaf, Nested { meta: &'static Meta, }, diff --git a/src/toml.rs b/src/toml.rs index cca84d7..a592abc 100644 --- a/src/toml.rs +++ b/src/toml.rs @@ -141,24 +141,19 @@ fn format_impl( } match &field.kind { - FieldKind::Leaf { default } => { + FieldKind::RequiredLeaf { default } => { // Emit comment about default value or the value being required if options.comments { - match default { - Some(v) => { - if !field.doc.is_empty() { - emit!("#"); - } - emit!("# Default value: {}", PrintExpr(v)); + if let Some(v) = default { + if !field.doc.is_empty() { + emit!("#"); } - None => { - if !field.optional { - if !field.doc.is_empty() { - emit!("#"); - } - emit!("# Required! This value must be specified."); - } + emit!("# Default value: {}", PrintExpr(v)); + } else { + if !field.doc.is_empty() { + emit!("#"); } + emit!("# Required! This value must be specified."); } } @@ -169,6 +164,8 @@ fn format_impl( } } + FieldKind::OptionalLeaf => emit!("#{} =", field.name), + FieldKind::Nested { meta } => { let child_path = path.iter().copied().chain([field.name]).collect(); format_impl(s, meta, child_path, options);