Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanup. Make build method generate as const #123

Closed
wants to merge 6 commits into from
Closed

Code cleanup. Make build method generate as const #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
92ea6a5
Code cleanup. Make build method generate as const
DanikVitek Oct 15, 2023
0c3e609
Fix builder method being const when impossible
DanikVitek Oct 15, 2023
a4e2490
Unwrap `Rc` and `OnceCell`
DanikVitek Oct 15, 2023
822c46f
Unwrap `Rc` and `OnceCell`
DanikVitek Oct 15, 2023
c0c5d77
Merge branch 'master' of https://github.com/DanikVitek/rust-typed-bui…
DanikVitek Oct 15, 2023
3702691
Make `fields` a boxed slice
DanikVitek Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions typed-builder-macro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![forbid(rust_2018_idioms)]

use proc_macro2::TokenStream;
use quote::quote;
use syn::{parse::Error, parse_macro_input, spanned::Spanned, DeriveInput};
Expand Down
2 changes: 1 addition & 1 deletion typed-builder-macro/src/mutator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl ApplyMeta for MutatorAttribute {
}

impl Parse for Mutator {
fn parse(input: ParseStream) -> syn::Result<Self> {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant. Is this because of that #![forbid(rust_2018_idioms)] thing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This lets to see at a glance where a borrowing occurs

let mut fun: ItemFn = input.parse()?;

let mut attribute = MutatorAttribute::default();
Expand Down
66 changes: 39 additions & 27 deletions typed-builder-macro/src/struct_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use std::cell::OnceCell;
use std::rc::Rc;

use proc_macro2::{Ident, Span, TokenStream};
use quote::{format_ident, quote, quote_spanned, ToTokens};
use syn::parse::Error;
use syn::punctuated::Punctuated;
use syn::{parse_quote, GenericArgument, ItemFn, Token};
use syn::{parse_quote, Error, GenericArgument, ItemFn, Token};

use crate::field_info::{FieldBuilderAttr, FieldInfo};
use crate::mutator::Mutator;
Expand All @@ -16,7 +18,7 @@ pub struct StructInfo<'a> {
pub vis: &'a syn::Visibility,
pub name: &'a syn::Ident,
pub generics: &'a syn::Generics,
pub fields: Vec<FieldInfo<'a>>,
pub fields: Box<[FieldInfo<'a>]>,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this really necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is not very usefull, but the field is effectively immutable and so there is no need to take away additional 8 bytes for capacity. But the struct is already >2KiB in size so tbh no much difference


pub builder_attr: TypeBuilderAttr<'a>,
pub builder_name: syn::Ident,
Expand Down Expand Up @@ -49,7 +51,7 @@ impl<'a> StructInfo<'a> {
.collect()
}

pub fn new(ast: &'a syn::DeriveInput, fields: impl Iterator<Item = &'a syn::Field>) -> Result<StructInfo<'a>, Error> {
pub fn new(ast: &'a syn::DeriveInput, fields: impl Iterator<Item = &'a syn::Field>) -> syn::Result<Self> {
let builder_attr = TypeBuilderAttr::new(&ast.attrs)?;
let builder_name = builder_attr
.builder_type
Expand All @@ -70,7 +72,7 @@ impl<'a> StructInfo<'a> {
})
}

pub fn builder_creation_impl(&self) -> Result<TokenStream, Error> {
pub fn builder_creation_impl(&self) -> syn::Result<TokenStream> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH this is the only change I like in this PR.

let StructInfo {
vis,
ref name,
Expand All @@ -85,15 +87,27 @@ impl<'a> StructInfo<'a> {
empty_type()
}
}));
let init_fields_expr = self.included_fields().map(|f| {
f.builder_attr.via_mutators.as_ref().map_or_else(
|| quote!(()),
|via_mutators| {
let init = &via_mutators.init;
quote!((#init,))
},
)
});
let builder_method_const = Rc::new(OnceCell::new());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Respectfully WTF? Looking at the code, it seems like a simple:

let mut builder_method_const = true;

would suffice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Borrow checker was not happy about the moved value. It could be done with Rc<Cell<bool>> though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it locally and it worked. Had to remove the move from the closure in the map, of course.

I speculate that it didn't work for you because you tried it before adding the collect. The compiler, in a way, was complaining about the very same bug that the collect solved.

Another option is to just do a second iteration:

let builder_method_const = self
    .included_fields()
    .filter_map(|field| field.builder_attr.via_mutators.as_ref())
    .all(|via_mutators| matches!(via_mutators.init, syn::Expr::Lit(_)))
    .then(|| quote!(const));

But of course - all of this is irrelevant because #123 (comment).

let init_fields_expr = self
.included_fields()
.map({
let builder_method_const = builder_method_const.clone();
move |f| {
f.builder_attr.via_mutators.as_ref().map_or_else(
idanarye marked this conversation as resolved.
Show resolved Hide resolved
|| quote!(()),
|via_mutators| {
let init = &via_mutators.init;
if !matches!(init, syn::Expr::Lit(_)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to make things explicit. If the build method is to be const, it should be marked as such in the configuration attributes - not implicitly deduced based on some incomplete (literals aren't the only const expressions) heuristics.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, makes sense. It was just my compulsive thought to make it const when I saw the macro expansion for my usecase. But now I know that it is not so simple

_ = builder_method_const.set(quote!());
}
quote!((#init,))
},
)
}
})
.collect::<Box<[_]>>();
let builder_method_const = Rc::into_inner(builder_method_const).unwrap();
let builder_method_const = OnceCell::into_inner(builder_method_const).unwrap_or_else(|| quote!(const));
let mut all_fields_param_type: syn::TypeParam =
syn::Ident::new("TypedBuilderFields", proc_macro2::Span::call_site()).into();
let all_fields_param = syn::GenericParam::Type(all_fields_param_type.clone());
Expand Down Expand Up @@ -177,10 +191,10 @@ impl<'a> StructInfo<'a> {
impl #impl_generics #name #ty_generics #where_clause {
#builder_method_doc
#[allow(dead_code, clippy::default_trait_access)]
#builder_method_visibility fn #builder_method_name() -> #builder_name #generics_with_empty {
#builder_method_visibility #builder_method_const fn #builder_method_name() -> #builder_name #generics_with_empty {
#builder_name {
fields: (#(#init_fields_expr,)*),
phantom: ::core::default::Default::default(),
phantom: ::core::marker::PhantomData,
}
}
}
Expand All @@ -206,7 +220,7 @@ impl<'a> StructInfo<'a> {
})
}

pub fn field_impl(&self, field: &FieldInfo) -> Result<TokenStream, Error> {
pub fn field_impl(&self, field: &FieldInfo<'_>) -> syn::Result<TokenStream> {
let StructInfo { ref builder_name, .. } = *self;

let descructuring = self.included_fields().map(|f| {
Expand Down Expand Up @@ -324,12 +338,10 @@ impl<'a> StructInfo<'a> {
})
}

pub fn required_field_impl(&self, field: &FieldInfo) -> TokenStream {
let StructInfo { ref builder_name, .. } = self;
pub fn required_field_impl(&self, field: &FieldInfo<'_>) -> TokenStream {
let StructInfo { builder_name, .. } = self;

let FieldInfo {
name: ref field_name, ..
} = field;
let FieldInfo { name: field_name, .. } = *field;
let mut builder_generics: Vec<syn::GenericArgument> = self
.generics
.params
Expand Down Expand Up @@ -637,7 +649,7 @@ pub struct CommonDeclarationSettings {
}

impl ApplyMeta for CommonDeclarationSettings {
fn apply_meta(&mut self, expr: AttrArg) -> Result<(), Error> {
fn apply_meta(&mut self, expr: AttrArg) -> syn::Result<()> {
match expr.name().to_string().as_str() {
"vis" => {
let expr_str = expr.key_value()?.parse_value::<syn::LitStr>()?.value();
Expand Down Expand Up @@ -701,7 +713,7 @@ pub struct BuildMethodSettings {
}

impl ApplyMeta for BuildMethodSettings {
fn apply_meta(&mut self, expr: AttrArg) -> Result<(), Error> {
fn apply_meta(&mut self, expr: AttrArg) -> syn::Result<()> {
match expr.name().to_string().as_str() {
"into" => match expr {
AttrArg::Flag(_) => {
Expand Down Expand Up @@ -756,8 +768,8 @@ impl Default for TypeBuilderAttr<'_> {
}
}

impl<'a> TypeBuilderAttr<'a> {
pub fn new(attrs: &[syn::Attribute]) -> Result<Self, Error> {
impl TypeBuilderAttr<'_> {
pub fn new(attrs: &[syn::Attribute]) -> syn::Result<Self> {
let mut result = Self::default();

for attr in attrs {
Expand All @@ -784,7 +796,7 @@ impl<'a> TypeBuilderAttr<'a> {
}

impl ApplyMeta for TypeBuilderAttr<'_> {
fn apply_meta(&mut self, expr: AttrArg) -> Result<(), Error> {
fn apply_meta(&mut self, expr: AttrArg) -> syn::Result<()> {
match expr.name().to_string().as_str() {
"crate_module_path" => {
let crate_module_path = expr.key_value()?.parse_value::<syn::ExprPath>()?;
Expand Down
16 changes: 10 additions & 6 deletions typed-builder-macro/src/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn empty_type_tuple() -> syn::TypeTuple {
}
}

pub fn modify_types_generics_hack<F>(ty_generics: &syn::TypeGenerics, mut mutator: F) -> syn::AngleBracketedGenericArguments
pub fn modify_types_generics_hack<F>(ty_generics: &syn::TypeGenerics<'_>, mut mutator: F) -> syn::AngleBracketedGenericArguments
where
F: FnMut(&mut syn::punctuated::Punctuated<syn::GenericArgument, syn::token::Comma>),
{
Expand Down Expand Up @@ -212,7 +212,7 @@ impl ToTokens for KeyValue {
}

impl Parse for KeyValue {
fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
fn parse(input: syn::parse::ParseStream<'_>) -> syn::Result<Self> {
Ok(Self {
name: input.parse()?,
eq: input.parse()?,
Expand All @@ -232,7 +232,8 @@ impl SubAttr {
Punctuated::<T, Token![,]>::parse_terminated.parse2(self.args)
}
pub fn undelimited<T: Parse>(self) -> syn::Result<impl IntoIterator<Item = T>> {
(|p: ParseStream| iter::from_fn(|| (!p.is_empty()).then(|| p.parse())).collect::<syn::Result<Vec<T>>>()).parse2(self.args)
(|p: ParseStream<'_>| iter::from_fn(|| (!p.is_empty()).then(|| p.parse())).collect::<syn::Result<Vec<T>>>())
.parse2(self.args)
}
}

Expand All @@ -243,7 +244,7 @@ impl ToTokens for SubAttr {
}
}

fn get_cursor_after_parsing<P: Parse + Spanned>(input: syn::parse::ParseBuffer) -> syn::Result<syn::buffer::Cursor> {
fn get_cursor_after_parsing<P: Parse + Spanned>(input: syn::parse::ParseBuffer<'_>) -> syn::Result<syn::buffer::Cursor<'_>> {
let parse_attempt: P = input.parse()?;
let cursor = input.cursor();
if cursor.eof() || input.peek(Token![,]) {
Expand All @@ -256,7 +257,10 @@ fn get_cursor_after_parsing<P: Parse + Spanned>(input: syn::parse::ParseBuffer)
}
}

fn get_token_stream_up_to_cursor(input: syn::parse::ParseStream, cursor: syn::buffer::Cursor) -> syn::Result<TokenStream> {
fn get_token_stream_up_to_cursor(
input: syn::parse::ParseStream<'_>,
cursor: syn::buffer::Cursor<'_>,
) -> syn::Result<TokenStream> {
Ok(core::iter::from_fn(|| {
if input.cursor() < cursor {
input.parse::<TokenTree>().ok()
Expand All @@ -268,7 +272,7 @@ fn get_token_stream_up_to_cursor(input: syn::parse::ParseStream, cursor: syn::bu
}

impl Parse for AttrArg {
fn parse(input: syn::parse::ParseStream) -> syn::Result<Self> {
fn parse(input: syn::parse::ParseStream<'_>) -> syn::Result<Self> {
if input.peek(Token![!]) {
Ok(Self::Not {
not: input.parse()?,
Expand Down
Loading