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

Default values are ignored when using `skip(build_fn)` #182

Closed nicholastmosher closed 3 years ago

nicholastmosher commented 3 years ago

I have a scenario where I would like to use the #[builder(default = "..")] attribute, but I would also like to use #[builder(build_fn(skip))]. I have code like the following that I would expect to work:

use derive_builder::Builder;

const DEFAULT_PREFIX: &str = "/usr/local/";

#[derive(Builder, Debug)]
#[builder(build_fn(skip))]
struct Config {
    #[builder(default = "DEFAULT_PREFIX.to_string()")]
    pub prefix: String,
}

impl ConfigBuilder {
    pub fn build(&self) -> Result<Config, String> {
        // I would expect this to work, becuase `prefix` is marked with default value
        let prefix = self.prefix.clone()
            .expect("Missing default prefix");

        Ok(Config {
            prefix,
        })
    }
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn test_default_values() {
        let config = ConfigBuilder::default().build().unwrap();
        assert_eq!(config.prefix, DEFAULT_PREFIX);
    }
}

What I get instead is a panic with "Missing default prefix".

It seems that the builder macro must place the code for handling default values in the generated build function, and since I'm not using it, I'm not seeing that behavior occur. I can get around this by using unwrap_or in the custom builder, but it was a bit confusing that the default values I'd defined weren't anywhere to be seen.

I wonder if it'd be possible to add the default-value generation into the implementation of Default for the builder struct instead. That way, using ConfigBuilder::default() would pre-populate those fields with Some(<value provided in attribute>), and a custom builder method could simply unwrap them without needing to duplicate the default-value declarations.

TedDriggs commented 3 years ago

Moving defaults to be eagerly created would make the common-case performance worse: We'd run default functions whose return values we ended up not needing because the caller invoked the appropriate setter method.

If you're going down the path of a custom builder, can you skip using #[builder(default)] at all, and instead put those defaults directly into your build function? That way they're still declared once. Alternatively, can you provide a custom implementation of CustomBuilder::default that has the values you want?

nicholastmosher commented 3 years ago

Sure, I think that those are reasonable workarounds. I think it might be worth adding a note to the docs that the #[builder(default)] attribute doesn't work with a custom builder function, that was the big part that tripped me up. If I have a free moment I'll see if I can make a quick PR for that.

TedDriggs commented 3 years ago

That'd be a great docs update, thanks.