build(cargo): update typed-builder requirement from 0.19 to 0.20

dependabot[bot] commented 3 weeks ago

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

EdJoPaTo commented 3 weeks ago

What about switching to bon, which is inspired by typed-builder and has taken some learnings from it? Havnt looked deeper into it, just thought again about it. https://crates.io/crates/bon

Its v2 Release also sparks the question… do we want the Into conversions?

ayrat555 commented 3 weeks ago

@EdJoPaTo I'm ok with switching if features are the same. what do you mean by "do we want the Into conversions?"? is it automatic in bon? I don't see anything against it

EdJoPaTo commented 3 weeks ago

bon v1 did Into automatically and removed this with v2. More reasoning here: https://elastio.github.io/bon/blog/bon-builder-generator-v2-release#removed-magical-automatic-into-conversions

Which got me thinking whether it's a good idea to use the Into stuff or not.

Veetaha commented 3 weeks ago

Hey, I found this mention of bon here 👀 . Yes, you are right Into conversions are a double-edged sword. I don't say they are strictly evil, and there is a range of types where Into conversion makes sense to make the regular usage of the API cleaner.

I described all the motivations and demotivations for using Into conversions in this article.

I looked at your usage of Into with typed-builder and found some cases when Into is enabled on u32. For example: https://github.com/ayrat555/frankenstein/blob/7d7b024de6169dd630b318a8bfe3c464c1cfcb7e/src/api_params.rs#L255-L257

I can confidently say that u32 is one of those types, that shouldn't have Into enabled for the reason of weakened primitive numeric literals type inference described in the article.

Note that typed-builder docs also mention this problem briefly, but they don't explain it in-depth, so when I, for example, was using typed-builder, I also missed that part in the docs, and had to learn it by myself later.

into: automatically convert the argument of the setter method to the type of the field. Note that this conversion interferes with Rust’s type inference and integer literal detection, so this may reduce ergonomics if the field type is generic or an unsigned integer.

ayrat555 commented 3 weeks ago

I'm ok with switching to bon @EdJoPaTo

maybe @Veetaha can help us to migrate?

Veetaha commented 3 weeks ago

I created a draft PR https://github.com/ayrat555/frankenstein/pull/196 with the migration. In that PR I just updated from the TypedBuilder syntax to bon::builder (the diff is positive because #[builder] is placed on a line separate from derive(...).

Right now, I have preserved the same builder configurations for Into conversions. And I have several points to discuss. All of them depend on the amount of breaking changes you are willing to accept.

`Into` conversions

Will it be fine to do a breaking change and remove Into conversions from primitive types? I think Into makes sense mainly only for String, PathBuf and some enums that implement From<Variant> (if there are any) in this case.

`Clone` for builder type

I've noticed that in your tests you created a builder and cloned it in a loop using the following pattern:

let builder = T::builder();
loop {
     let val = builder.clone().member("value").build();
}

Note that this is another problem of typed-builder, which is minor, but yet could lead to subtle unintentional breaking changes. typed-builder implicitly derives Clone for its generated builder. The builder generated by TypedBuilder implements Clone only if all the members that were set in the builder implement Clone too. This may be a problem, because this behavior is implicit and undocumented in typed-builder, and it means you, as the user of typed-builder can unintentionally introduce a breaking change by using a non-cloneable type inside of the builder, when before you used a cloneable type.

I say it's a minor thing in typed-builder, because it doesn't lead to breaking changes that easily. For example, if you add a new optional member that doesn't implement Clone, the existing code that uses the builder won't break immediately. The builder will keep being cloneable so far as the new optional field isn't set on the builder. This is because typed-builder's impl Clone uses a where AllSetMembers: Clone bound, so if all members that were set on the builder implement Clone, the builder type will also implement Clone.

The breakage can appear only in subtle cases such as when you change the type inside of the builder to a non-cloneable one, but use #[builder(transform = |param: PrevType| ..)] to continue accepting the previous type in the setter. In this case the existing usages of the builder that set the field using the PrevType that was cloneable may break, because the builder stops implementing Clone.

With bon, the generated builder doesn't derive Clone by default at the time of this writing, and there isn't an attribute option to configure it to derive Clone as well.

Instead, if you'd like to clone the builder generated by bon today, you just need to wrap its creation code in a closure like this:

let builder = || T::builder();
loop {
     let val = builder().member("value").build();
}

So.. There are two options that we could do about this:

Give up on having Clone for all builders. Suggest using the closure pattern to create new builders instead of cloning them to anyone depending on the builder type deriving Clone
Don't give up on having Clone for all builders. Wait for bon to add a feature like #[builder(derive(Clone))] to be able to enable Clone for builders.

So what do you think is the right direction?

dependabot[bot] commented 1 week ago

Looks like typed-builder is no longer a dependency, so this is no longer needed.

ayrat555 / frankenstein