Strengthen the recommendation to use &mut self builders

[dtolnay]

This seems like almost always the better alternative. For cases where the terminal method needs ownership of some state in the builder, the state can be stored in an Option that the terminal method can take() out.

[Kixunil]

Why? Consuming builder can avoid misuse and unwrapping.

[dtolnay]

Explanation from Aaron Turon in the reqwest meeting:

In general, there are many many places where we could use the type system in principle to rule out some potential bug, but it's either a bug that is very unlikely to arise in practice or where we can generate a panic that people are almost certainly going to see if they ever test their code. I personally feel that in cases like that, we should err on the side of ergonomics—provide a panic if there is some risk of people getting things wrong, but not kill ourselves trying to get the type system to catch every last possible fault.

Here are some ergonomic issues with consuming builders. A consuming builder would be protecting against largely hypothetical misuse, while a &mut self builder would be providing very practical ergonomic benefits.

Reassignment is annoying

#86 makes this harder to forget, but no less inconvenient.

let mut builder = Builder::new();

if let Some(x) = x {
    // &mut self builder:
    builder.x(x);

    // self builder:
    builder = builder.x(x);
}

builder.build()

Hard to give back ownership of the builder after error

It might be possible to enable this by having an accessor on the error type that gives back ownership of the builder, but then you tend to run into Send+Sync trouble. See #82.

let mut builder = Builder::new();

// &mut self builder
if let Err(err) = builder.x(x) {
    warn!("ignoring x: {}", err);
}

// self builder
builder = match builder.x(x) {
    Ok(builder) => builder,
    Err(err) => {
        warn!("ignoring x: {}", err);
        /* ??? */
    }
};

builder.build()

[Kixunil]

My perfectionism dislikes it but you're probably right.

[dtantsur]

A counter-example (reflecting my actual issues with reqwest):

With self (pseudo-rust):

 fn custom_request(method: Method, url: Url) -> RequestBuilder {
   let extra_hdrs = calculate_some_headers();
   make_client().request(method, url).headers(extra_hdrs)
 }

With &mut self:

 fn custom_request(method: Method, url: Url) -> RequestBuilder {
   let extra_hdrs = calculate_some_headers();
   let mut builder = make_client().request(method, url);
   {
     // make unused_results lint happy; we cannot return _unused because it's a reference
     let _unused = builder.headers(extra_hdrs);
   }
   builder
 }

[KamilaBorowska]

I disagree with this guideline, I already wrote my issues with it as far reqwest is concerned at seanmonstar/reqwest#260 (comment).

[seanmonstar]

I am also no longer convinced that this recommendation makes sense in all cases. Specifically, in http and in reqwest, I'm considering moving the builders back from by-ref to by-value. My reasons:

1. By-ref builders are easy if you chain the whole thing in 1 statement, but as soon as you need to pass the builder anywhere, you need to split up in an initial binding, call your setters, and then pass it in a 3rd step.
2. In both http and reqwest, the setters don't return errors immediately, but store the error internally, to be returned at build. So, there is no concern with losing the builder part way through.
3. It leaves behind a "bomb". The builders absolutely needed to consume internal state, without cloning, but because of it being by-ref, the builder is left behind afterwards. It doesn't make sense to be able to use it again. Note I think builders that can set some options and then build multiple items from the same builder don't suffer from this.

[awestlake87]

Something I've kinda wondered about is whether or not it would be reasonable to make a (for lack of a better word) "boomerang" fn, one that can temporarily take ownership of self, but also ensure that self is valid and usable after the call. If the biggest argument against by-value builders is ergonomics, then the "boomerang" fn might provide the best of both worlds.

struct MyBuilder {
    a: u32,
    b: u32,
    c: u32,
}

impl MyBuilder {
    fn new() -> Self {
        Self { a: 0, b: 0, c: 0 }
    } 

    boomerang a(self, a: u32) {
        self = Self { a: a, ..self }
    }

    boomerang b(self, b: u32) {
        self = Self { b: b, ..self }
    }

    boomerang c(self, c: u32) {
        self = Self { c: c, ..self }
    }

    fn build(self) -> (u32, u32, u32) {
        
    }
}

fn main() {
    let builder: MyBuilder::new();

    // builder gets passed by-value to a(...), by-value to b(...) then returned to the builder variable.
    builder
         .a(34)
         .b(45);
    
    // builder gets passed by-value to c(...), then finally consumed by build(...).
    let tuple = builder
        .c(64)
        .build();
}

I'm pretty opposed to adding new syntax for it, and it also seems a bit ugly to have "boomerang" act like it returns "self" in some contexts, but leaves it unmoved in others. Basically, I don't like it, but I've also found myself wanting something like it multiple times for the sake of convenience.