Clean up ShardManager/Queuer/Runner

[GnomedDev]

Just some basic cleanups I noticed.

[mkrasnitski]

Since ShardManager::{restart, restart_shard} are public functions, relying on the user to make sure that ShardInfo::total is correct seems brittle. Maybe ShardManager::shard_total could be kept and changed to type Arc<u32>? It's only set on initialization and never changed, and therefore could be kept immutable this way.

[mkrasnitski]

Ok, so rather than an Arc<u32>, I figured the best way to avoid breaking the signatures of restart and restart_shard is to have shard_total be a OnceLock<u32> that gets filled inside ShardManager::initialize and is fetched inside ShardManager::boot.

[GnomedDev]

Right then, fixed your concerns by making ShardQueuer just store the total shard count and stop using ShardInfo everywhere redundantly.

[mkrasnitski]

Changing to u16 is fine, but making shard_total a NonZeroU16 I think is a bit awkward, since it infects the public API for Client.

[GnomedDev]

It infecting the API for Client is somewhat intentional, it is an invariant of the API that was previously not encoded and is now in the type system.

[mkrasnitski]

It infecting the API for Client is somewhat intentional, it is an invariant of the API that was previously not encoded and is now in the type system.

That's true, but do you think the panic could be moved inside the function by converting from a u16? The affected functions are called early on in the life of the program, and aren't called as often as start_autosharded (I'm just assuming here).

[GnomedDev]

If I wanted mistakes in my code to become problems at runtime, instead of compile time, I would be writing in python. I can make it panic if 0 inside the serenity code but I very much recommend against. Id::new panicking is fine enough because it's done so often and isn't likely to have a person give it 0, but this is a one time thing for each program, it just looks like more of a migration because we have 50 bots in the repo in the form of doc examples and actual examples.

[mkrasnitski]

Initial brief review of the new changes. The shard total is being incorrectly initialized IMO. The intention is that it should be either specified manually, or default to the value provided by the gateway. Right now it can't be manually overridden.

One way to fix this is to introduce a new ShardQueuerMessage variant that can override the value of ShardQueuer::shard_total. Then, Client::start_autosharded won't have to fire a duplicate request to the gateway.

I don't like that ShardManagerOptions::shard_total exists solely to get the value into the ShardQueuer, but I guess changing to using a OnceLock would mean that Client::start_autosharded would be the place that initializes it, meaning a duplicate gateway request.

As for the NonZeroU16 thing, all I'll say is that in the string NonZeroU16::new(1).expect("1 != 0"), out of the 35 total characters, only a single one is relevant. And, I'm sure there are plenty of other values serenity receives from the API that can never be 0 which aren't tracked as non-zero. Ultimately, the tradeoff of noisiness here is pretty stark to me, and not worth it.

[GnomedDev]

Followed through with all your suggestions.

[mkrasnitski]

Thank you for all your patience and work on this PR.