idanarye / rust-typed-builder

Compile-time type-checked builder derive
https://crates.io/crates/typed-builder
Apache License 2.0
904 stars 52 forks source link

Mutable assignments variables #120

Closed novafacing closed 11 months ago

novafacing commented 11 months ago

Very small change to allow a use case for LibAFL where a default = expression needs to mutate another field. Here's a snippet for example:

/// Sending end on a (unidirectional) sharedmap channel
#[derive(TypedBuilder, Debug)]
#[builder(build_method(into = Result::<LlmpSender<SP>, Error>))]
pub struct LlmpSender<SP>
where
    SP: ShMemProvider,
{
    /// ID of this sender.
    id: ClientId,
    #[builder(default = ptr::null_mut())]
    /// Ref to the last message this sender sent on the last page.
    /// If null, a new page (just) started.
    last_msg_sent: *const LlmpMsg,
    #[builder(default)]
    /// A vec of pages that we previously used, but that have served its purpose
    /// (no potential readers are left).
    /// Instead of freeing them, we keep them around to potentially reuse them later,
    /// if they are still large enough.
    /// This way, the OS doesn't have to spend time zeroing pages, and getting rid of our old pages
    unused_shmem_cache: Vec<LlmpSharedMap<SP::ShMem>>,
    #[builder(default = false)]
    /// If true, pages will never be pruned.
    /// The broker uses this feature.
    /// By keeping the message history around,
    /// new clients may join at any time in the future.
    keep_pages_forever: bool,
    #[builder(default = false)]
    /// True, if we allocatd a message, but didn't call [`Self::send()`] yet
    has_unsent_message: bool,
    /// The sharedmem provider to get new sharaed maps if we're full
    shmem_provider: SP,
    #[builder(
        default = vec![LlmpSharedMap::new(id, shmem_provider.new_shmem(LLMP_CFG_INITIAL_MAP_SIZE).expect("Failed to create a new shared memory"))],
        setter(into))
    ]
    /// A vec of page wrappers, each containing an initialized [`ShMem`]
    out_shmems: Vec<LlmpSharedMap<SP::ShMem>>,
}
idanarye commented 11 months ago

I'm... not 100% sure how I feel about this. I'm still on the fence about allowing default expressions access to the previous fields - this is an undocumented feature, which may prevent the ability to split things out to traits in the future.

I'll allow it, but keep in mind that it might be broken in the future. Probably in favor of something like #75 (which in this case would mean the shmem_provider would be passed as a context, and then maybe moved/cloned into its own field)

One change I must insist on though - one of Rust's basic tenants is that immutable should be the default, and I really don't think this rule should be violated here. The mutability should be triggered by something in the attribute:

/// The sharedmem provider to get new sharaed maps if we're full
#[builder(mutable_during_default_resolution)]
shmem_provider: SP,

Yes, it's long. Ugly features get ugly names.

Also, this means it needs to wait until #116 gets in. Or at least until I cherry-pick some commits from it.

novafacing commented 11 months ago

Sure thing and sound reasoning, I'll make that change! I'll stay in tune with updates as they happen and in progress PRs. Happy to work on making mutability more idiomatic in the future -- for now, this will be a very helpful stopgap!

I will also document the feature unless you'd prefer it remains undocumented 😉

idanarye commented 11 months ago

121 is in. It should now be easier to add new fields.

idanarye commented 11 months ago

The new attribute should go in FieldBuilderAttr.

novafacing commented 11 months ago

Ok, added a doc entry and added that field attribute! I didn't see any negative tests, but I deleted mutable_during_default_resolution manually to make sure I get an error :)

novafacing commented 11 months ago

Resolved those three notes, thanks!