search

colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.32k stars 88 forks source link

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

Closed Xion closed 2 years ago

Xion commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Sure, check out https://github.com/Xion/derive-builder-issue-269 .

TedDriggs commented 2 years ago

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?