Potential refactor ideas for requests

[WaffleLapkin]

Motivation

The current design of the Bot/Requester/requests is cool and is working, however it has a number of issues and I'm not sure it's the best one.

Now thinking about it again, it seem like the design tries to put "performance" above usability/simplicity. I don't think that's generally a bad call, but I do think that for teloxide specifically it is a bad decision. My reasoning is that 1. performance is bound at IO/telegram anyway most of the time; 2. we already sacrifice performance for ease of use in dptree; 3. teloxide is used a lot by programmer beginners for whom it is often hard to learn, in some cases because of the current Bot/... design.

Would be nice to improve things here. Below are some problem with the current design:

API updates

API updates are currently extremely cumbersome, as they require changing a lot of things:

• The schema (+regenerating things)
• Requester trait
• Forward macro calls in adaptors (those are very annoying, since they are macros)
• Types (not related to this issue)

Defining new adaptors is painful

Related to the previous point: defining new adaptors requires overriding a lot of methods and is very painful.

Too many traits / trait items

We ended up with the following methods in traits (I include different monomorphizations):

• Requester methods: api_methods × adaptors
• *Setters methods: api_methods × parameters × adaptors ¹²
• Request methods: 2 × api_methods × adaptors²
• Payload methods: api_methods
• HasPayload methods: 3 × api_methods × (adaptors + 1) ³
• Download methods: 2 × adaptors

I feel like this might be contributing to the relatively slow compilation time and bad r-a suggestions sometimes.

There is also the fact that Requester has a ton of

Wrong bot type / long bot type

This is something that trips beginners all the time. When you are using things like .parse_mode, you get DefaultParseMode<Bot>, which breaks dptree deps.

type Bot = ...; kind of alias mostly fixes the issue, but it still bad for beginners and in error messages.

Goals / ideas of the refactor

• Use more dynamic dispatch (nice for beginners who don't want to look at long nested types, good for compilation time since it removes some monomorphization copies)
• Use less traits / use more concrete items

Proposal

My main idea is to have concrete types Bot and Request<P>⁴ types. To accommodate the ability to have custom behavior via adaptors, both Bot and Request<P> will store something like Arc<dyn BotEngine> inside.

Types implementing BotEngine can either wrap other BotEngine types or have type erasure themselves⁵, I don't think this matters much, as they only exist to the user while creating Bot.

Since Bot and Request are single types now, you can implement current Requester, *Setters, Request, HasPayload methods on them directly.

Since payloads are a closed set, we might also want to make BotEngine have just a single fn execute<R>(&self, payload: Payload) -> impl Future<Output = Result<R, Error>> where Payload is an enum of all payloads. Alternatively maybe accept a generic payload⁶.

API / sage / implementation sketch

Creating a new bot with a custom engine:

let engine = Reqwest::new().throttle(...).parse_mode(...);
let bot = Bot::with_engine(token, engine);

Bot API methods⁷⁸:

impl Bot {
    pub fn send_message(&self, chat_id: impl Into<Recipient>, text: impl Into<String>) -> Request<'_, SendMessage>;
}

Setters:

impl Request<SendMessage> {
    pub fn chat_id(self, value: impl Into<Recipient>) -> Self { ... }
    pub fn text(self, value: impl Into<String>) -> Self { ... }
    pub fn message_thread_id(self, value: i32) -> Self { ... }
    pub fn parse_mode(self, value: ParseMode) -> Self { ... }
    // ...
}

Pros

• Should solve most of the problems of the current design
  • More user-friendly interface
  • Less methods for the compiler to chew through
  • Hopefully easier to read docs
  • Hopefully nicer to implement/change

Cons

• It's not fully clear how to best handle state in requests that depend on the request type of the payload
  • Worst case scenatio: dynamic dispatch/enums
• Unclear how to handle error types (Bot<E>? Bot always returns the same error type?)
• Less static things
  • I think it's fine though
• Requires a lot of work (rewriting the core of the library for the N-th time! ><)

Alternatives

Do nothing

The always present: leave the status quo as-is.

I don't think it's a good approach, since it leaves all the problems described at the beginning intact. Causing problem to both users and maintainers.

Do this, but will less type erasure

With GATs being stabilized now, we could have Bot<Engine> with all the same interface I'm pretty sure. I don't think that we need that, but we could.

Footnotes

1. api_methods × fields signifies "total number of parameters of requests" ↩

2. some adaptors don't count here, since they don't have their own request types ↩ ↩²

3. +1 for the HasPayload implementations on payloads ↩

4. P for the payload, we still need to be generic over the return type :) ↩

5. Or just implement the functionality themselves, obv ↩

6. Requires us to define a single trait that allows to send pretty much any payload (should be possible though, something like Serialize + GetInputFiles + Any + ...) ↩

7. I want to suggest changing style guide and using APIT more since they make better documentation and the downsides are rare & unimportant (but it's a topic for another time) ↩

8. The lifetime can be removed from Request, but I don't think it's a big deal either way ↩

[WaffleLapkin]

For some context: the two main reasons why we did not do something like that before are

• you can't impl setters on Request<P: HasPayload<Payload = SendMessage>> since they will overlap¹
• we did not think of Request<SendMessage> which solves the overlap
• I wanted to keep the concrete types everywhere
• With concrete types some of these designs require GATs, that weren't stable at the time

Footnotes

1. it shouldn't overlap, but it's a current limitation of rustc ↩

[WaffleLapkin]

For context /2: the idea is to keep the way users call requests (bot.method(x).optional(y).await?) and to keep all the features (opt-in things like throttling, default parse mode, etc) but make the actual interface and implementation simpler and as such, nicer to use, especially for beginners.

[Hirrolot]

Thanks for the proposal, I'll read it through in the coming week and reply here.