Assign Return Value Of Delegated Calls To Delegated Field

[the-wondersmith]

I'm working on an API client that's implemented (essentially) as a newtype wrapper around a reqwest::Client. For convenience, I'd like to mirror reqwest's API as much as possible (no point in reinventing the wheel, right?) so I've also included a newtype wrapper for reqwest::ClientBuilder as well.

The client and builder wrappers basically look like this:

ClientBuilder

#[derive(Debug, Default)]
pub struct APIClientBuilder {
    api_key: Option<reqwest::header::HeaderValue>,
    builder: reqwest::ClientBuilder,
}

impl Deref for APIClientBuilder {
    type Target = reqwest::ClientBuilder;

    fn deref(&self) -> &Self::Target {
        &self.builder
    }
}

impl APIClientBuilder {
    pub fn new() -> Self {
        // ...
    }

    pub fn build(self) -> Result<APIClient, crate::errors::ClientError> {
        // ...
    }

    /// Set the API key to use for authenticating requests
    pub fn api_key<ApiKey: ToString>(mut self, api_key: ApiKey) -> Self {
        // ...
    }
}

APIClient

#[derive(Debug)]
pub struct APIClient(reqwest::Client);

impl Deref for APIClient {
    type Target = reqwest::Client;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl APIClient {
    /// Create a new [`APIClient`][APIClient] instance
    pub fn new<ApiKey: ToString>(api_key: ApiKey) -> Result<Self, crate::errors::APIClientError> {
        Self::builder()  // -> APIClientBuilder  // this is fine 
            .api_key(api_key)  // -> APIClientBuilder  // this is *also* fine
            .cookie_store(true)  // -> reqwest::ClientBuilder  // problems start here
            .timeout(Duration::from_secs(360))  // -> reqwest::ClientBuilder
            .connect_timeout(Duration::from_secs(360))  // -> reqwest::ClientBuilder
            .build()  // -> reqwest::Client  // this *should* be crate::client::APIClient
    }
}

I've annotated the code block above to illustrate the issue - the return type of the builder methods changes depending on whether or not they're "overridden" by the wrapper type.

My question is - would it be possible to instruct delegate to "capture" (re-assign) the return value of delegated calls back to the delegated field? At the moment, doing:

// ...

delegate! {
    to self.builder {
        pub fn timeout(mut self, timeout: Duration) -> APIClientBuilder;
        pub fn cookie_store(mut self, enable: bool) -> APIClientBuilder;
        pub fn connect_timeout(mut self, timeout: Duration) -> APIClientBuilder;
    }
}

// ...

causes delegate to generate:

// ...

#[inline(always)]
pub fn timeout(mut self, timeout: Duration) -> APIClientBuilder {  // incorrect return type
    self.builder.timeout(timeout)
}

#[inline(always)]
pub fn cookie_store(mut self, enable: bool) -> APIClientBuilder {  // incorrect return type
    self.builder.cookie_store(enable)
}

#[inline(always)]
pub fn connect_timeout(mut self, timeout: Duration) -> APIClientBuilder {  // incorrect return type
    self.builder.connect_timeout(timeout)
}

// ...

Ideally (and with the correct attribute / annotation presumably) though, it would generate:

// ...

#[inline(always)]
pub fn timeout(mut self, timeout: Duration) -> APIClientBuilder {
    self.builder = self.builder.timeout(timeout);
    self
}

#[inline(always)]
pub fn cookie_store(mut self, enable: bool) -> APIClientBuilder {
    self.builder = self.builder.cookie_store(enable);
    self
}

#[inline(always)]
pub fn connect_timeout(mut self, timeout: Duration) -> APIClientBuilder {
    self.builder = self.builder.connect_timeout(timeout);
    self
}

// ...

[Kobzol]

Hi :) If it was only required to return self, then we could solve it by examining if the return type is Self (we currently already examine the return type and do not return anything if there is no explicit return type). But the part with self.builder = ... seems more problematic, and I'm not sure how to solve it other than introducing an explicit attribute like #[builder], which I would be a bit reluctant to do.

To solve your specific use-case: I assume that APIClientBuilder implements (or could implement) From<reqwest::ClientBuilder>? In that case, you could solve this by adding #[into] on top of the delegated method.

[the-wondersmith]

@Kobzol Isn't the macro aware of the delegated field?

APIClientBuilder does implement From<reqwest::ClientBuilder>, the problem is the explicit api_key field. It's required by the service the client is for, but is not explicitly a requirement of a generic reqwest::Client, so the "point" of the wrapper type for reqwest::ClientBuilder is to enforce the compile-time guarantee that you can't build a valid APIClient without having set a value for the required key without losing the ability to customize the wrapped client.

Using #[into] absolutely would work, but consider:

    // ...
    let mut builder = APIClient::builder();

    builder  // builder instance is effectively "empty" here
      .api_key("SOME API KEY")  // builder.api_key is now Some("SOME API KEY")
      .some_delegated_method()  // what happens to the value of builder.api_key here?
      .build()
}

Assuming the macro is aware of which field a given method is being delegated to, the "capture" behavior could be implemented either with an explicit directive (like you said), or implicitly by examining the arguments to the delegated method. The logic would be something like "if the delegated method takes (mutable?) ownership of self and returns Self, capture the mutated field value by assigning it before returning the 'outer' self instance".

[Kobzol]

@Kobzol Isn't the macro aware of the delegated field?

It is. It's not a problem to implement it, I'd just like to avoid special casing too many specific use-cases, and would rather come up with a more generic approach.

Guessing what the user wanted to do from the self and return type in such a complicated way seems even worse than adding a #[builder] attribute, I really wouldn't like to do that.

We cannot also just return self if the return type is Self, as there can be patterns where that is not the intended behavior, e.g.

struct Tree { left: Box<Tree>, right: Box<Tree> }
impl Tree {
    delegate! {
        to self.left {
             #[call(foo)]
             fn foo_left(&self) -> Self;
        }
    }

    fn foo(&self) -> Self { Tree { ... } }
}

In this case the user probably wants

fn foo_left(&self) -> Self {
    self.left.foo()
}

and not

fn foo_left(&self) -> Self {
    self.left.foo();
    self
}

I was thinking about adding some #[return(<expr>)] attribute that could be used to modify the return expression. You could e.g. wrap it in a function call that would store the returned builder instance and then return self. But sadly, for your use-case it wouldn't be enough (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=115f3a664cb5b1d4b773b1850b43ade6), because the inner builder is consumed/moved on each method call.

I wonder if there are other builder patterns. If the inner builder used &mut self instead of self for the builder methods, then probably you would want something like this instead:

#[inline(always)]
pub fn cookie_store(mut self, enable: bool) -> Self {
    self.builder.cookie_store(enable);
    self
}

This case could be solved just with the return self.

I would propose adding two new attributes:

• #[return(expr)] will allow modifying what is returned from the delegated function. For now I wouldn't add any placeholders (so it won't be possible to return the result of the delegated function call), and mostly just support #[return(self)].
• #[store], which will a binary attribute that will instruct to store the result of the delegated method (+ things like .into() etc.) into the delegated expression and not return it from the delegated method.

Your code would then have to look like this:

delegate! {
    to self.builder {
        #[return(self)]
        #[store]
        pub fn timeout(mut self, timeout: Duration) -> APIClientBuilder;
        #[return(self)]
        #[store]
        pub fn cookie_store(mut self, enable: bool) -> APIClientBuilder;
        #[return(self)]
        #[store]
        pub fn connect_timeout(mut self, timeout: Duration) -> APIClientBuilder;
    }
}

which seems a bit verbose.

I wonder if the delegate macro is even that useful for your use-case? Does it really save a lot of boilerplate and provide value for you?

If the answers are yes, then I will try to implement these attributes and see how practical it is to use them, and combine them with other, existing attributes.

But probably even before that I should make "block" attributes work to remove all this duplication for each method. It would be a nice use-case for "block" attributes that are effective for all delegated methods, so that it does not need to be repeated needlessly.

delegate! {
    #[return(self)]
    #[store]
    to self.builder {
        pub fn timeout(mut self, timeout: Duration) -> APIClientBuilder;
        pub fn cookie_store(mut self, enable: bool) -> APIClientBuilder;
        pub fn connect_timeout(mut self, timeout: Duration) -> APIClientBuilder;
    }
}

Something like this.

[the-wondersmith]

@Kobzol Those are all excellent and salient points. I hadn't considered whether &mut self might be viable (though I don't think it is, because the builder pattern (generally) consumes and returns self). I'll check shortly though.

For clarity's sake, I had figured that a generic approach would be best and was ~90% certain that guessing user intentions is pretty counter to the Rust mindset. I had imagined that something like a #[capture] attribute or somesuch might work, but your proposals make much more sense.

Honestly, it's entirely possible that I'm approaching the problem with the wrong mindset. I'm a shameless Rust convert but I am nevertheless a convert, having come to Rust from Python. My impulse to just wrap the underlying type and add the required functionality comes from Python's inheritance being the goto for these types of things. I've read up on Rust's opinion of inheritance, and while I don't disagree with the decision, I'm absolutely having a hard time figuring out the idiomatic Rust way of expressing:

import httpx

class APIClient(httpx.AsyncClient):
    """A custom `httpx`-based API client."""

    async def get_some_resource(self, resource: str) -> Resource:
        """Get the specified resource from the upstream API."""
        return await self.get(f"resources/{resource}")

The newtype pattern + delegate is (so far) the cleanest way I've found to express what I'm trying to get at, i.e reqwest already has an excellent client builder and there's no sense in reinventing the wheel from scratch just to add an extra spoke. I'm just a bit... lost in the sauce 🥲

[Kobzol]

I scarcely have the need to actually delegate methods like this (which might seem ironic since I maintain this crate, but I got to it by chance mostly, I'm not the original author :D ). Usually when I wrap things, I either provide the original interface using the Deref trait (which doesn't really work for builder methods that consume self though), or I expose a completely different API.

Regarding the client: do you really want to just add new methods and allow using all existing methods of the reqwest client? When I design my own network client, it's usually for some specific service, and I only want to expose functions/endpoints that are specific to that service. And if some user has the need to actually do something more complicated, we can expose post/get/etc. methods that set some API key or target URL internally, but allow the user to do a generic request.

Example: octocrab (GitHub API). It has structs that expose GitHub specific functionality (https://docs.rs/octocrab/latest/octocrab/struct.Octocrab.html#method.post), but when users need something that is not exposed by the API, they can also use provided get/post methods.

Regarding the builder: wouldn't it be enough to simply allow users to build your custom builder from the reqwest builder? Like this:

struct MyBuilder { builder: reqwest::Builder, api_key: String }
impl MyBuilder {
    fn from_builder(builder: reqwest::Builder, api_key: String) -> Self {
        Self { builder, api_key }
    }
}

If you only set the API key and nothing else in your builder, then this should be enough. And it has the added benefit that you cannot forget to set the API key (or set it multiple times by accident), otherwise you would not even be able to create the builder. This can be extended for more complex scenarios with the typestate pattern.

If you need to set multiple builder attributes, I still think that it's fine to provide the constructor. For builders, you usually just set all the attributes and then call build, so I think that it would be fine if you just set all reqwest attributes first, and then all your custom attributes after that. If you don't need to interleave setting these attributes (why would you?), then it should be fine and you don't need delegation.

let builder = reqwest::Builder()
    .set_a(1)
    .set_b(2)
    .set_c(3);
let builder = MyBuilder::from(builder)
    .api_key("...")
    .my_url("...")
    .build();

You could even implement a custom trait method to add a method to reqwest::Builder to do all this in one step (sadly it's not straightforward to create such method chain only with From/Into):

let result = reqwest::Builder()
    .set_a(1)
    .set_b(2)
    .set_c(3)
    .into_my_builder()
    .api_key("...")
    .my_url("...")
    .build();

[ryanolson]

This could be very useful:

delegate! {
    #[return(self)]
    #[store]
    to self.builder {
        pub fn timeout(&mut self, timeout: Duration) -> APIClientBuilder;
        pub fn cookie_store(&mut self, enable: bool) -> APIClientBuilder;
        pub fn connect_timeout(&mut self, timeout: Duration) -> APIClientBuilder;
    }
}

Imagine a scenario where you have a builder composed of two other builders.

pub struct ClientBuilder {
  client: Client,
  request: ApiRequestBuilder,  // original api
  extension: ApiExtensionBuilder, // extensions to the original api maintained separately
}

impl ClientBuilder {
  pub async fn execute(&self) -> Result<Response> {
    let request = self.request.build()?
    let ext = self.extension.build()?
    let request = ActualRequest { request, ext }
    let response = client.execute(request).await?;
    Ok(response)
  }
}

Perhaps there is a more elegant way of doing this. Or perhaps a procedural macro to auto generate builders setter methods, i.e. any method that uses &mut self. I personally don't have the skill set to do procedural macros yet, but writing out the methods in the via delegate with the returns overrides would be a great start.

[Kobzol]

I probably didn't exactly understand the use-case. I think that builders are better left for specialized crates, like derive_builder, since this would require several new attributes for rust-delegate.