Improving Http and surrounding APIs

[mkrasnitski]

There was some discussion about some areas of the API where there is currently room for improvement; things like taking more advantage of Rust's type system to ensure API compatibility, plus removing duplicated code or method signatures as well as in general decreasing API surface area to prevent future internal changes from being breaking. I've gone and listed some of them below, so that they don't become lost:

1. While the methods on Http might be considered the "canonical" wrappers around the Discord API, they also don't take advantage of Rust's type system at all, since they take a Json object, which is basically just a HashMap with Strings as keys. The user interacts with the Http struct quite often, mainly to facilitate requests to Discord endpoints on their behalf. So, they also have access to these methods.
   It would be nice to mark all such methods as pub(crate) instead of pub, as this would require users go through the various model types instead, which only allow specific fields to be set in the hashmap. Plus, if Discord changes how an endpoint works in some way, then the underlying Http implementation could be changed without introducing breaking changes, while methods on model types would remain unaffected (methods would only be added, not changed). This would allow features to release quicker and reduce technical debt.
   • Some of the methods on Http take an optional audit_log_reason parameter, which currently all the wrapper methods on model types just set to None. Should we keep these methods exposed, or add methods to the model types to allow passing an optional audit_log_reason? Better yet, is it worth considering adding builders for these specific function calls? As in, take as argument a closure that constructs a builder struct. This might not be worth doing for methods with very few arguments.
2. The model types are full of duplicated method signatures. For example, the GuildId/Guild/PartialGuild types. Many methods in GuildId are wrapped by methods on the other two guild types. When combined with (1), the worry is that this code duplication could introduce inconsistency. So, it would be ideal if there were only a single possible callsite. The logical choice is to just keep as many methods on GuildId as possible. However, semantically, these methods operate on guilds, not on the id itself, so there's a discrepancy there.
   The alternative would be to auto-generate wrappers, perhaps using a proc-macro. Many of these wrappers are simple thin wrappers. Others add some functionality though, such as checking the cache to validate user permissions. These too could probably be auto-impl'd using a proc-macro, as long as the required permissions are specified in each case.
3. The current builders allow users to add any combination of fields to them. However, it would be great if we could enforce API compliance, such as the inclusion of required fields, at compile time using the type system. There are some rare cases too where certain fields are mutually exclusive based on value. We could possibly use the typestate pattern to encode this relationship into the type system as well.

Any thoughts on how to tackle these issues would be greatly appreciated!

[mkrasnitski]

I've come up with a potential solution to the 3rd point above. It involves changing the builder methods to be called on the result of the relevant method on the corresponding model type, rather than inside a closure passed as argument to that method. As an example:

The following code:

let msg = channel_id
    .send_message(&ctx.http, |m| {
        m.content("Hello, World!")
            .embed(|e| {
                e.title("This is a title")
                    .description("This is a description")
                    .image("attachment://ferris_eyes.png")
            })
            .add_file("./ferris_eyes.png")
    })
    .await?;

would turn into this:

let msg = channel_id
    .send_message(&ctx.http)
    .content("Hello, World!")
    .embed(|e| {
        e.title("This is a title")
            .description("This is a description")
            .image("attachment://ferris_eyes.png")
    })
    .add_file("./ferris_eyes.png")
    .await?;

or possibly even this:

let e = Embed::builder()
    .title("This is a title")
    .description("This is a description")
    .image("attachment://ferris_eyes.png");
let msg = channel_id
    .send_message(&ctx.http)
    .content("Hello, World!")
    .add_file("./ferris_eyes.png")
    .embed(e)
    .await?;

This would require changing the return types of all the current model methods that take builder closures as arguments from this:

pub async fn send_message<'a, F>(self, http: impl AsRef<Http>, f: F) -> Result<Message>
where
    for<'b> F: FnOnce(&'b mut CreateMessage<'a>) -> &'b mut CreateMessage<'a>,

to this:

pub fn send_message<'a>(self, http: impl AsRef<Http>) -> CreateMessage<'a> {
    CreateMessage::new(http.as_ref())
}

Note that send_message is now synchronous. Then, we would either implement Future directly for CreateMessage:

impl<'a> Future for CreateMessage<'a> {
    type Output = Result<Message>;
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        ...
    }
}

Or simpler would be to add an execute method on the builder in which case instead of calling .await, you'd call .execute().await:

impl<'a> CreateMessage<'a> {
    async fn execute() -> Result<Message> {
        ...
   }
}

Besides allowing for less-nested code, this opens the door to allow encoding API-level invariants into the type system. For example, the CreateMessage endpoint requires at least one of the content, embeds, files[n], or sticker_ids fields to be populated, but none of those fields are individually required. So, we can enforce this by having ChannelId::send_message return a non-Future type, but calling content or the other corresponding methods returns a type that is Future. This way you can't actually await the result unless you've added at least one required field. This would work regardless if we implemented Future directly on CreateMessage or not. What's more, we could use this for enforcing mutually exclusive fields and so on.

Thoughts?

[kangalio]

Linking these for completeness:

#2024 (initial builder paradigm overhaul)
#2069 (Send fix)
#2078, #2086 (remove now-redundant builders)
#2087 (statically enforce required fields)
#2105 (audit log reason support)

(Thank you @mkrasnitski !)

Points 1.1 and 3 are covered with those

[kangalio]

About point 2, I think it's best to just remove the methods from the model types and have users call them on Id instead. It's better anyways if users are aware they only need the ID for most things. The only remaining methods on model types should be those that actually require the stored model data. That also makes it more obvious what the methods are doing: a method on Id is just a dumb request to Discord, a method on a model type does something more sophisticated.

A lot (the majority?) of model type methods really just plainly delegate to their inner ID:

(regex \{[\r\s]\s*self\.id\..+\.await[\r\s]\s*\} searches for { \n self.id.[...].await \n })

So it would be quite worth it to eliminate those

Opinions? If people agree this is not an awful idea, the next thing to do would be deprecate these methods, and remove them in the next breaking release

[mkrasnitski]

I think semantically it would be a little confusing. For example, the methods on GuildId, which all do operations on Guilds, would have to be called like guild.id.method() which would make it seem like the guild id itself is responsible somehow for those methods, but that's counterintuitive. On the other hand, having methods be duplicated three times can also be confusing in its own way, and changing something requires changing it in three places.

I did think some more about using a proc-macro to autogenerate the wrapper methods. You would define a method once on GuildId, and decorate it with an attribute, like so:

#[wrapped_by(Guild, PartialGuild, field=id)]
/// Gets all auto moderation [`Rule`]s of this guild via HTTP.
///
/// **Note**: Requires the [Manage Guild] permission.
///
/// # Errors
///
/// Returns an [`Error::Http`] if the guild is unavailable.
///
/// [Manage Guild]: Permissions::MANAGE_GUILD
#[inline]
pub async fn automod_rules(self, http: impl AsRef<Http>) -> Result<Vec<Rule>> {
    http.as_ref().get_automod_rules(self.get()).await
}

Then, the macro could duplicate the docs, the signature, and the return type, and the body would simply be self.{field}.{method}({args},*), and an extra .await at the end if the fn is async (which they all are iirc). This would decrease maintenance load, though I'm worried what impact it would have on compile times.

[GnomedDev]

This seems like quite a large issue tracking multiple concepts. Certain parts of it are also already fixed. Should this just be closed as it's not really useful?

[mkrasnitski]

A lot of this has already been addressed. If you feel like some parts are worth tracking still, you can file another issue, but this one can be closed.