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.

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

Mutable assignments variables #120

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

novafacing
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

novafacing commented Sep 26, 2023

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
Copy link
Owner

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

@idanarye
Copy link
Owner

The new attribute should go in FieldBuilderAttr.

@novafacing
Copy link
Contributor Author

novafacing commented Sep 27, 2023

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 :)

Copy link
Owner

@idanarye idanarye left a comment

Choose a reason for hiding this comment

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

If any of these remarks are too difficult, I can do them myself after I merge.

typed-builder-macro/src/struct_info.rs Outdated Show resolved Hide resolved
typed-builder-macro/src/struct_info.rs Outdated Show resolved Hide resolved
typed-builder-macro/src/field_info.rs Outdated Show resolved Hide resolved
@novafacing
Copy link
Contributor Author

Resolved those three notes, thanks!

@idanarye idanarye merged commit 969d6ae into idanarye:master Sep 27, 2023
9 checks passed
@novafacing novafacing deleted the mutable-assignments branch September 27, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants