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

Combination of #[builder(default = "foo", field(...)] should error out #269

Closed
Xion opened this issue Oct 24, 2022 · 3 comments · Fixed by #271
Closed

Combination of #[builder(default = "foo", field(...)] should error out #269

Xion opened this issue Oct 24, 2022 · 3 comments · Fixed by #271
Assignees

Comments

@Xion
Copy link

Xion commented Oct 24, 2022

Right now, the default value is silently ignored.

This is only indirectly hinted at in the docs where it says that a type T in field(type="T") needs to implement Default. It is not stated that the provided custom default would be ignored, however.

@TedDriggs
Copy link
Collaborator

TedDriggs commented Oct 24, 2022

Is it ignored by the build method? I'm a bit surprised by that.

According to the code, we have a check already that will complain if field(build = "...") is used in conjunction with default. However, I would expect that it's fine to change a field's type in the builder using the #[builder(field(type = "..."))] and still have the default apply at the right time in build_fn.

The reason that T needs to impl Default is because we need to create it "empty" when we start the builder. By default, we use Option wrappers around all fields and that's sufficient to guarantee we can give those fields default values at builder creation. We don't use the body of the #[builder(default = "...")] expression until we're in the build method, since we don't want to do potentially-expensive initialization that won't be needed.

Can you provide a repro?

@Xion
Copy link
Author

Xion commented Oct 25, 2022

@TedDriggs
Copy link
Collaborator

Thanks for that!

It looks like the issue is here, where the presence of a custom type will push us to use the Move strategy, which would preclude the optionality check.

@ijackson do you remember if there’s a reason we didn’t initially check for this?

@TedDriggs TedDriggs self-assigned this Oct 25, 2022
TedDriggs added a commit that referenced this issue Oct 26, 2022
If `field.type` is used without `field.build`, a Move conversion is used instead of the OptionOrDefault conversion.
This means that the presence of `field.type` prevents a field-level `default` from doing anything.
Standard practice in derive_builder is to produce an error when parameters are not going to be used.

Fixes #269
TedDriggs added a commit that referenced this issue Oct 26, 2022
If `field.type` is used without `field.build`, a Move conversion is used instead of the OptionOrDefault conversion.
This means that the presence of `field.type` prevents a field-level `default` from doing anything.
Standard practice in derive_builder is to produce an error when parameters are not going to be used.

Fixes #269
TedDriggs added a commit that referenced this issue Oct 26, 2022
If `field.type` is used without `field.build`, a Move conversion is used instead of the OptionOrDefault conversion.
This means that the presence of `field.type` prevents a field-level `default` from doing anything.
Standard practice in derive_builder is to produce an error when parameters are not going to be used.

Fixes #269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants