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

Field-level #[builder(default = __)] easier to use wrong than right #65

Closed
TedDriggs opened this issue Apr 7, 2017 · 5 comments
Closed

Comments

@TedDriggs
Copy link
Collaborator

#[derive(Debug, PartialEq, Builder)]
struct Dolor {
    msg: String,
    
    #[builder(default = "self.msg")]
    z_failure: Option<String>
}

This produces the following error:

error[E0507]: cannot move out of borrowed content
    |
169 |     #[derive(Debug, PartialEq, Builder)]
    |                                ^^^^^^^ cannot move out of borrowed content

Alternatively, some really weird things can be accomplished:

#[derive(Debug, PartialEq, Builder)]
struct Dolor {
    
    #[builder(default)]
    msg: String,

    /// Prints 'hi' if not explicitly initialized.
    #[builder(default = "{println!(\"Hi!\"); Some(\"woot\".to_string())}")]
    z_failure: Option<String>
}

The only time the current implementation of this will work at the field level is if the user provides a literal or a value that implements Copy. Serde avoids this by requiring the value of default to be a function with the signature Fn() -> T, which it calls every time it needs a new instance. That feels much more hygienic than the current behavior (and makes using default in conjunction with helper or factory functions much cleaner).

@colin-kiegel, how would you feel about combining this with #61 and documenting the breaking change as "aligned default behavior with serde"?

@colin-kiegel
Copy link
Owner

Hm, thanks.

There was a discussion about this here (comparison with serde / question 1) and in this comment below:

I feel like the semantics of #[serde(default = "some::Function::foo")] would be too restrictive for derive_builder.

  • I suspect a lot of simple cases where delegation to some::Function::foo would be overkill and default="42" is really all you want
  • For complex cases it would seem beneficial to delegate to methods on FooBuilder which can inspect all builder fields.

It would have been nice to share the semantics with a popular crate like serde. But this would have significant drawbacks IMO.

The outcome of that discussion was, please delegate to methods if that makes sense (complex stuff), or use arbitrary expressions if you know what you are doing (only really recommended for simple const expressions). The docs clearly recommends delegating to helper methods.

I still believe that 100% serde-behaviour is not a good fit for derive_builder. E.g. in serde you cannot write default="42", instead you have to wrap const-values in a static function. - at least the last time I checked the documentation.

@TedDriggs
Copy link
Collaborator Author

I still believe that 100% serde-behaviour is not a good fit for derive_builder. E.g. in serde you cannot write default="42", instead you have to wrap const-values in a static function. - at least the last time I checked the documentation.

This is accurate. Is there maybe a way to check at compile time that the user has either passed:

  1. A value literal that implements Copy or
  2. A path to a function?

That could disallow the exotic misuses while still allowing the desired simple case.

@colin-kiegel
Copy link
Owner

A custom derive can only inspect the source code of the item it is applied to. Nothing more - so we don't know what's in scope and what implements what. We're completely blind to the world. :-)

@TedDriggs
Copy link
Collaborator Author

:( bummer. Okay, in that case I'm out of ideas on this one.

@colin-kiegel
Copy link
Owner

I thinks it's the best we can do right now. I'll close in the meantime.

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

No branches or pull requests

2 participants