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

Make generated identifiers of private fields deterministic #219

Merged
merged 6 commits into from
Nov 27, 2024
Merged

Make generated identifiers of private fields deterministic #219

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
17530c1
Remove randomly generated private idents
Veetaha Nov 27, 2024
46f4072
Add a UI test for deprecation warnings
Veetaha Nov 27, 2024
8d4a94d
Add `unsafe` to field names
Veetaha Nov 27, 2024
739f4c6
Update trybuild snapshot test
Veetaha Nov 27, 2024
5f63ff6
Add a comment
Veetaha Nov 27, 2024
37be4f8
Update test snapshot
Veetaha Nov 27, 2024
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
15 changes: 6 additions & 9 deletions bon-macros/src/builder/builder_gen/builder_decl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ impl super::BuilderGenCtx {
let where_clause = &self.generics.where_clause;
let phantom_data = self.phantom_data();
let state_mod = &self.state_mod.ident;
let phantom_field = &self.ident_pool.phantom;
let receiver_field = &self.ident_pool.receiver;
let start_fn_args_field = &self.ident_pool.start_fn_args;
let named_members_field = &self.ident_pool.named_members;

// The fields can't be hidden using Rust's privacy syntax.
// The details about this are described in the blog post:
Expand All @@ -27,7 +23,8 @@ impl super::BuilderGenCtx {
// generated code. This simplifies the task of removing unnecessary
// attributes from the generated code when preparing for demo purposes.
let deprecated_msg = "\
this field should not be used directly; it's an implementation detail \
this field should not be used directly; it's an implementation detail, and \
if you access it directly, you may break some internal unsafe invariants; \
if you found yourself needing it, then you are probably doing something wrong; \
feel free to open an issue/discussion in our GitHub repository \
(https://github.com/elastio/bon) or ask for help in our Discord server \
Expand All @@ -43,7 +40,7 @@ impl super::BuilderGenCtx {
let ty = &receiver.without_self_keyword;
quote! {
#private_field_attrs
#receiver_field: #ty,
__unsafe_private_receiver: #ty,
}
});

Expand All @@ -62,7 +59,7 @@ impl super::BuilderGenCtx {
let start_fn_args_field = start_fn_arg_types.peek().is_some().then(|| {
quote! {
#private_field_attrs
#start_fn_args_field: (#(#start_fn_arg_types,)*),
__unsafe_private_start_fn_args: (#(#start_fn_arg_types,)*),
}
});

Expand Down Expand Up @@ -104,15 +101,15 @@ impl super::BuilderGenCtx {
#where_clause
{
#private_field_attrs
#phantom_field: #phantom_data,
__unsafe_private_phantom: #phantom_data,

#receiver_field
#start_fn_args_field

#( #custom_fields_idents: #custom_fields_types, )*

#private_field_attrs
#named_members_field: (
__unsafe_private_named: (
#(
::core::option::Option<#named_members_types>,
)*
Expand Down
26 changes: 9 additions & 17 deletions bon-macros/src/builder/builder_gen/builder_derives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,12 @@ impl BuilderGenCtx {
let generic_args = &self.generics.args;
let builder_ident = &self.builder_type.ident;

let phantom_field = &self.ident_pool.phantom;
let receiver_field = &self.ident_pool.receiver;
let start_fn_args_field = &self.ident_pool.start_fn_args;
let named_members_field = &self.ident_pool.named_members;

let clone = quote!(::core::clone::Clone);

let clone_receiver = self.receiver().map(|receiver| {
let ty = &receiver.without_self_keyword;
quote! {
#receiver_field: <#ty as #clone>::clone(&self.#receiver_field),
__unsafe_private_receiver: <#ty as #clone>::clone(&self.__unsafe_private_receiver),
}
});

Expand All @@ -96,8 +91,8 @@ impl BuilderGenCtx {
// ```
// required for `(...big type...)` to implement `Clone`
// ```
#start_fn_args_field: (
#( <#types as #clone>::clone(&self.#start_fn_args_field.#indices), )*
__unsafe_private_start_fn_args: (
#( <#types as #clone>::clone(&self.__unsafe_private_start_fn_args.#indices), )*
),
}
});
Expand All @@ -115,7 +110,7 @@ impl BuilderGenCtx {

quote! {
#bon::__::derives::clone_member::<#ty>(
&self.#named_members_field.#member_index
&self.__unsafe_private_named.#member_index
)
}
});
Expand Down Expand Up @@ -148,7 +143,7 @@ impl BuilderGenCtx {
{
fn clone(&self) -> Self {
Self {
#phantom_field: ::core::marker::PhantomData,
__unsafe_private_phantom: ::core::marker::PhantomData,
#clone_receiver
#clone_start_fn_args
#( #clone_fields, )*
Expand All @@ -160,17 +155,14 @@ impl BuilderGenCtx {
// ```
// required for `(...big type...)` to implement `Clone`
// ```
#named_members_field: ( #( #clone_named_members, )* ),
__unsafe_private_named: ( #( #clone_named_members, )* ),
}
}
}
}
}

fn derive_debug(&self, derive: &DeriveConfig) -> TokenStream {
let receiver_field = &self.ident_pool.receiver;
let start_fn_args_field = &self.ident_pool.start_fn_args;
let named_members_field = &self.ident_pool.named_members;
let bon = &self.bon;

let format_members = self.members.iter().filter_map(|member| {
Expand All @@ -183,7 +175,7 @@ impl BuilderGenCtx {
output.field(
#member_ident_str,
#bon::__::derives::as_dyn_debug::<#member_ty>(
&self.#start_fn_args_field.#member_index
&self.__unsafe_private_start_fn_args.#member_index
)
);
})
Expand All @@ -206,7 +198,7 @@ impl BuilderGenCtx {
let member_ident_str = &member.name.snake_raw_str;
let member_ty = member.underlying_norm_ty();
Some(quote! {
if let Some(value) = &self.#named_members_field.#member_index {
if let Some(value) = &self.__unsafe_private_named.#member_index {
output.field(
#member_ident_str,
#bon::__::derives::as_dyn_debug::<#member_ty>(value)
Expand All @@ -228,7 +220,7 @@ impl BuilderGenCtx {
output.field(
"self",
#bon::__::derives::as_dyn_debug::<#ty>(
&self.#receiver_field
&self.__unsafe_private_receiver
)
);
}
Expand Down
10 changes: 4 additions & 6 deletions bon-macros/src/builder/builder_gen/finish_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::member::{Member, PosFnMember};
use crate::util::prelude::*;

impl super::BuilderGenCtx {
fn finish_fn_member_expr(&self, member: &Member) -> TokenStream {
fn finish_fn_member_expr(member: &Member) -> TokenStream {
let member = match member {
Member::Named(member) => member,
Member::Skip(member) => {
Expand All @@ -14,9 +14,8 @@ impl super::BuilderGenCtx {
}
Member::StartFn(member) => {
let index = &member.index;
let start_fn_args_field = &self.ident_pool.start_fn_args;

return quote! { self.#start_fn_args_field.#index };
return quote! { self.__unsafe_private_start_fn_args.#index };
}
Member::FinishFn(member) => {
return member
Expand All @@ -31,9 +30,8 @@ impl super::BuilderGenCtx {

let index = &member.index;

let named_members_field = &self.ident_pool.named_members;
let member_field = quote! {
self.#named_members_field.#index
self.__unsafe_private_named.#index
};

let default = member
Expand Down Expand Up @@ -88,7 +86,7 @@ impl super::BuilderGenCtx {

pub(super) fn finish_fn(&self) -> TokenStream {
let members_vars_decls = self.members.iter().map(|member| {
let expr = self.finish_fn_member_expr(member);
let expr = Self::finish_fn_member_expr(member);
let var_ident = member.orig_ident();

// The type hint is necessary in some cases to assist the compiler
Expand Down
5 changes: 1 addition & 4 deletions bon-macros/src/builder/builder_gen/input_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,10 +454,7 @@ impl FinishFnBody for FnCallBody {
let prefix = self
.sig
.receiver()
.map(|_| {
let receiver_field = &ctx.ident_pool.receiver;
quote!(self.#receiver_field.)
})
.map(|_| quote!(self.__unsafe_private_receiver.))
.or_else(|| {
let self_ty = &self.impl_ctx.as_deref()?.self_ty;
Some(quote!(<#self_ty>::))
Expand Down
84 changes: 0 additions & 84 deletions bon-macros/src/builder/builder_gen/models.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,6 @@ pub(crate) struct BuilderGenCtx {
/// Path to the `bon` crate.
pub(super) bon: syn::Path,

/// Private identifiers that are used in the builder implementation.
/// They are intentionally randomized to prevent users from accessing them.
pub(super) ident_pool: PrivateIdentsPool,

/// Name of the generic variable that holds the builder's state.
pub(super) state_var: syn::Ident,

Expand All @@ -160,22 +156,6 @@ pub(crate) struct BuilderGenCtx {
pub(super) finish_fn: FinishFn,
}

/// Identifiers that are private to the builder implementation. The users should
/// not use them directly. They are randomly generated to prevent users from
/// using them. Even if they try to reference them, they won't be able to re-compile
/// their code because the names of these identifiers are regenerated on every
/// macro run.
///
/// This is an unfortunate workaround due to the limitations of defining the
/// builder type inside of a nested module. See more details on this problem in
/// <https://bon-rs.com/blog/the-weird-of-function-local-types-in-rust>
pub(super) struct PrivateIdentsPool {
pub(super) phantom: syn::Ident,
pub(super) receiver: syn::Ident,
pub(super) start_fn_args: syn::Ident,
pub(super) named_members: syn::Ident,
}

pub(super) struct BuilderGenCtxParams<'a> {
pub(crate) bon: Option<syn::Path>,
pub(super) namespace: Cow<'a, GenericsNamespace>,
Expand Down Expand Up @@ -333,7 +313,6 @@ impl BuilderGenCtx {
Ok(Self {
bon: bon.unwrap_or_else(|| syn::parse_quote!(::bon)),
state_var,
ident_pool: PrivateIdentsPool::new(),
members,
allow_attrs,
on,
Expand All @@ -347,69 +326,6 @@ impl BuilderGenCtx {
}
}

impl PrivateIdentsPool {
fn new() -> Self {
use std::collections::hash_map::RandomState;
use std::hash::{BuildHasher, Hasher};

// Thanks @orhun for the article https://blog.orhun.dev/zero-deps-random-in-rust/
let random = RandomState::new().build_hasher().finish();

// Totally random words. All coincidences are purely accidental 😸
let random_words = [
"amethyst",
"applejack",
"blackjack",
"bon",
"cadance",
"celestia",
"cheerilee",
"derpy",
"fleetfoot",
"flitter",
"fluttershy",
"izzy",
"lilly",
"littlepip",
"luna",
"lyra",
"maud",
"minuette",
"octavia",
"pinkie",
"pipp",
"rainbow",
"rampage",
"rarity",
"roseluck",
"scootaloo",
"seaswirl",
"spitfire",
"starlight",
"sunset",
"sweetie",
"trixie",
"twilight",
"twinkleshine",
"twist",
"velvet",
"vinyl",
];

#[allow(clippy::cast_possible_truncation)]
let random_word = random_words[(random % (random_words.len() as u64)) as usize];

let ident = |name: &str| format_ident!("__private_{random_word}_{name}");

Self {
phantom: ident("phantom"),
receiver: ident("receiver"),
start_fn_args: ident("start_fn_args"),
named_members: ident("named_members"),
}
}
}

impl Generics {
pub(super) fn new(
decl_with_defaults: Vec<syn::GenericParam>,
Expand Down
28 changes: 10 additions & 18 deletions bon-macros/src/builder/builder_gen/setters/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@

let member_type = self.member.ty.norm.as_ref();

if let Some(with) = &self.member.config.with {

Check warning on line 30 in bon-macros/src/builder/builder_gen/setters/mod.rs

View workflow job for this annotation

GitHub Actions / cargo-miri 

`if let` assigns a shorter lifetime since Edition 2024
inputs = self.underlying_inputs_from_with(with)?;
expr = self.member_expr_from_with(with);
} else if self.member.config.into.is_present() {
Expand Down Expand Up @@ -342,39 +342,31 @@
let body = match imp.body {
SetterBody::Forward { body } => body,
SetterBody::SetMember { expr } => {
let idents = &self.base.ident_pool;
let phantom_field = &idents.phantom;
let receiver_field = &idents.receiver;
let start_fn_args_field = &idents.start_fn_args;
let named_members_field = &idents.named_members;

let mut output = if !self.member.is_stateful() {
quote! {
self
}
} else {
let builder_ident = &self.base.builder_type.ident;

let maybe_receiver_field = self
.base
.receiver()
.map(|_| quote!(#receiver_field: self.#receiver_field,));
let maybe_receiver_field = self.base.receiver().map(
|_| quote!(__unsafe_private_receiver: self.__unsafe_private_receiver,),
);

let maybe_start_fn_args_field = self
.base
.start_fn_args()
.next()
.map(|_| quote!(#start_fn_args_field: self.#start_fn_args_field,));
let maybe_start_fn_args_field =
self.base.start_fn_args().next().map(
|_| quote!(__unsafe_private_start_fn_args: self.__unsafe_private_start_fn_args,),
);

let custom_fields_idents = self.base.custom_fields().map(|field| &field.ident);

quote! {
#builder_ident {
#phantom_field: ::core::marker::PhantomData,
__unsafe_private_phantom: ::core::marker::PhantomData,
#( #custom_fields_idents: self.#custom_fields_idents, )*
#maybe_receiver_field
#maybe_start_fn_args_field
#named_members_field: self.#named_members_field,
__unsafe_private_named: self.__unsafe_private_named,
}
}
};
Expand All @@ -393,7 +385,7 @@

let index = &self.member.index;
quote! {
self.#named_members_field.#index = #expr;
self.__unsafe_private_named.#index = #expr;
#output
}
}
Expand Down
Loading
Loading