Remove instances of cloning in the Cache that aren't absolutely nee…

…ded (serenity-rs#2606)

src/cache/mod.rs

@@ -3,16 +3,13 @@
 //! Using the cache allows to avoid REST API requests via the [`http`] module where possible.
 //! Issuing too many requests will lead to ratelimits.
 //!
-//! Following a policy to never hand out locks, the cache will clone all values when calling its
-//! methods.
-//!
 //! # Use by Models
 //!
 //! Most models of Discord objects, such as the [`Message`], [`GuildChannel`], or [`Emoji`], have
-//! methods for interacting with that single instance. This feature is only compiled if the
-//! `methods` feature is enabled. An example of this is [`Guild::edit`], which performs a check to
-//! ensure that the current user is the owner of the guild, prior to actually performing the HTTP
-//! request. The cache is involved due to the function's use of unlocking the cache and retrieving
+//! methods for interacting with that single instance. This feature is only compiled if the `model`
+//! feature is enabled. An example of this is [`Guild::edit`], which performs a check to ensure that
+//! the current user is the owner of the guild, prior to actually performing the HTTP request.
+//! The cache is involved due to the function's use of unlocking the cache and retrieving
 //! the Id of the current user, and comparing it to the Id of the user that owns the guild. This is
 //! an inexpensive method of being able to access data required by these sugary methods.
 //!
@@ -109,11 +106,19 @@ impl<K: Eq + Hash, V, T> std::ops::Deref for CacheRef<'_, K, V, T> {
     }
 }
 
+type MappedGuildRef<'a, T> = CacheRef<'a, GuildId, T, Guild>;
+
 pub type UserRef<'a> = CacheRef<'a, UserId, User>;
+pub type MemberRef<'a> = MappedGuildRef<'a, Member>;
 pub type GuildRef<'a> = CacheRef<'a, GuildId, Guild>;
+pub type GuildRoleRef<'a> = MappedGuildRef<'a, Role>;
+pub type SettingsRef<'a> = CacheRef<'a, (), Settings>;
 pub type CurrentUserRef<'a> = CacheRef<'a, (), CurrentUser>;
-pub type GuildChannelRef<'a> = CacheRef<'a, GuildId, GuildChannel, Guild>;
+pub type GuildChannelRef<'a> = MappedGuildRef<'a, GuildChannel>;
+pub type GuildRolesRef<'a> = MappedGuildRef<'a, HashMap<RoleId, Role>>;
+pub type GuildChannelsRef<'a> = MappedGuildRef<'a, HashMap<ChannelId, GuildChannel>>;
 pub type ChannelMessagesRef<'a> = CacheRef<'a, ChannelId, HashMap<MessageId, Message>>;
+pub type MessageRef<'a> = CacheRef<'a, ChannelId, Message, HashMap<MessageId, Message>>;
 
 #[derive(Debug)]
 pub(crate) struct CachedShardData {
@@ -446,9 +451,6 @@ impl Cache {
 
     /// Retrieves a [`Guild`]'s member from the cache based on the guild's and user's given Ids.
     ///
-    /// **Note**: This will clone the entire member. Instead, retrieve the guild and retrieve from
-    /// the guild's [`members`] map to avoid this.
-    ///
     /// # Examples
     ///
     /// Retrieving the member object of the user that posted a message, in a
@@ -461,7 +463,7 @@ impl Cache {
     /// #
     /// # async fn run(http: Http, cache: Cache, message: Message) {
     /// #
-    /// let member = {
+    /// let roles_len = {
     ///     let channel = match cache.channel(message.channel_id) {
     ///         Some(channel) => channel,
     ///         None => {
@@ -472,20 +474,17 @@ impl Cache {
     ///         },
     ///     };
     ///
-    ///     match cache.member(channel.guild_id, message.author.id) {
-    ///         Some(member) => member,
-    ///         None => {
-    ///             if let Err(why) = message.channel_id.say(&http, "Error finding member data").await {
-    ///                 println!("Error sending message: {:?}", why);
-    ///             }
-    ///             return;
-    ///         },
-    ///     }
+    ///     cache.member(channel.guild_id, message.author.id).map(|m| m.roles.len())
     /// };
     ///
-    /// let msg = format!("You have {} roles", member.roles.len());
+    /// let message_res = if let Some(roles_len) = roles_len {
+    ///     let msg = format!("You have {} roles", roles_len);
+    ///     message.channel_id.say(&http, &msg).await
+    /// } else {
+    ///     message.channel_id.say(&http, "Error finding member data").await
+    /// };
     ///
-    /// if let Err(why) = message.channel_id.say(&http, &msg).await {
+    /// if let Err(why) = message_res {
     ///     println!("Error sending message: {:?}", why);
     /// }
     /// # }
@@ -494,71 +493,27 @@ impl Cache {
     /// [`EventHandler::message`]: crate::client::EventHandler::message
     /// [`members`]: crate::model::guild::Guild::members
     #[inline]
-    pub fn member<G, U>(&self, guild_id: G, user_id: U) -> Option<Member>
-    where
-        G: Into<GuildId>,
-        U: Into<UserId>,
-    {
-        self._member(guild_id.into(), user_id.into())
-    }
-
-    fn _member(&self, guild_id: GuildId, user_id: UserId) -> Option<Member> {
-        match self.guilds.get(&guild_id) {
-            Some(guild) => guild.members.get(&user_id).cloned(),
-            None => None,
-        }
-    }
-
-    /// This method allows to only clone a field of a member instead of the entire member by
-    /// providing a `field_selector`-closure picking what you want to clone.
-    ///
-    /// ```rust,no_run
-    /// # use serenity::cache::Cache;
-    /// #
-    /// # fn run() -> Result<(), Box<dyn std::error::Error>> {
-    /// # let cache = Cache::default();
-    /// // We clone only the `name` instead of the entire channel.
-    /// if let Some(Some(nick)) = cache.member_field(7, 8, |member| member.nick.clone()) {
-    ///     println!("Member's nick: {}", nick);
-    /// }
-    /// # Ok(())
-    /// # }
-    /// ```
-    #[inline]
-    pub fn member_field<Ret, Fun>(
+    pub fn member(
         &self,
         guild_id: impl Into<GuildId>,
         user_id: impl Into<UserId>,
-        field_selector: Fun,
-    ) -> Option<Ret>
-    where
-        Fun: FnOnce(&Member) -> Ret,
-    {
-        self._member_field(guild_id.into(), user_id.into(), field_selector)
+    ) -> Option<MemberRef<'_>> {
+        self._member(guild_id.into(), user_id.into())
     }
 
-    fn _member_field<Ret, Fun>(
-        &self,
-        guild_id: GuildId,
-        user_id: UserId,
-        field_selector: Fun,
-    ) -> Option<Ret>
-    where
-        Fun: FnOnce(&Member) -> Ret,
-    {
-        let guild = self.guilds.get(&guild_id)?;
-        let member = guild.members.get(&user_id)?;
-
-        Some(field_selector(member))
+    fn _member(&self, guild_id: GuildId, user_id: UserId) -> Option<MemberRef<'_>> {
+        let member = self.guilds.get(&guild_id)?.try_map(|g| g.members.get(&user_id)).ok()?;
+        Some(CacheRef::from_mapped_ref(member))
     }
 
     #[inline]
-    pub fn guild_roles(&self, guild_id: impl Into<GuildId>) -> Option<HashMap<RoleId, Role>> {
+    pub fn guild_roles(&self, guild_id: impl Into<GuildId>) -> Option<GuildRolesRef<'_>> {
         self._guild_roles(guild_id.into())
     }
 
-    fn _guild_roles(&self, guild_id: GuildId) -> Option<HashMap<RoleId, Role>> {
-        self.guilds.get(&guild_id).map(|g| g.roles.clone())
+    fn _guild_roles(&self, guild_id: GuildId) -> Option<GuildRolesRef<'_>> {
+        let roles = self.guilds.get(&guild_id)?.map(|g| &g.roles);
+        Some(CacheRef::from_mapped_ref(roles))
     }
 
     /// This method clones and returns all unavailable guilds.
@@ -569,18 +524,13 @@ impl Cache {
 
     /// This method returns all channels from a guild of with the given `guild_id`.
     #[inline]
-    pub fn guild_channels(
-        &self,
-        guild_id: impl Into<GuildId>,
-    ) -> Option<DashMap<ChannelId, GuildChannel, BuildHasher>> {
+    pub fn guild_channels(&self, guild_id: impl Into<GuildId>) -> Option<GuildChannelsRef<'_>> {
         self._guild_channels(guild_id.into())
     }
 
-    fn _guild_channels(
-        &self,
-        guild_id: GuildId,
-    ) -> Option<DashMap<ChannelId, GuildChannel, BuildHasher>> {
-        self.guilds.get(&guild_id).map(|g| g.channels.clone().into_iter().collect())
+    fn _guild_channels(&self, guild_id: GuildId) -> Option<GuildChannelsRef<'_>> {
+        let channels = self.guilds.get(&guild_id)?.map(|g| &g.channels);
+        Some(CacheRef::from_mapped_ref(channels))
     }
 
     /// Returns the number of guild channels in the cache.
@@ -618,16 +568,18 @@ impl Cache {
     ///
     /// [`EventHandler::message`]: crate::client::EventHandler::message
     #[inline]
-    pub fn message<C, M>(&self, channel_id: C, message_id: M) -> Option<Message>
+    pub fn message<C, M>(&self, channel_id: C, message_id: M) -> Option<MessageRef<'_>>
     where
         C: Into<ChannelId>,
         M: Into<MessageId>,
     {
         self._message(channel_id.into(), message_id.into())
     }
 
-    fn _message(&self, channel_id: ChannelId, message_id: MessageId) -> Option<Message> {
-        self.messages.get(&channel_id).and_then(|messages| messages.get(&message_id).cloned())
+    fn _message(&self, channel_id: ChannelId, message_id: MessageId) -> Option<MessageRef<'_>> {
+        let channel_messages = self.messages.get(&channel_id)?;
+        let message = channel_messages.try_map(|messages| messages.get(&message_id)).ok()?;
+        Some(CacheRef::from_mapped_ref(message))
     }
 
     /// Retrieves a [`Guild`]'s role by their Ids.
@@ -646,22 +598,23 @@ impl Cache {
     /// // assuming the cache is in scope, e.g. via `Context`
     /// if let Some(role) = cache.role(7, 77) {
     ///     println!("Role with Id 77 is called {}", role.name);
-    /// }
+    /// };
     /// ```
     ///
     /// [`Guild`]: crate::model::guild::Guild
     /// [`roles`]: crate::model::guild::Guild::roles
     #[inline]
-    pub fn role<G, R>(&self, guild_id: G, role_id: R) -> Option<Role>
+    pub fn role<G, R>(&self, guild_id: G, role_id: R) -> Option<GuildRoleRef<'_>>
     where
         G: Into<GuildId>,
         R: Into<RoleId>,
     {
         self._role(guild_id.into(), role_id.into())
     }
 
-    fn _role(&self, guild_id: GuildId, role_id: RoleId) -> Option<Role> {
-        self.guilds.get(&guild_id).and_then(|g| g.roles.get(&role_id).cloned())
+    fn _role(&self, guild_id: GuildId, role_id: RoleId) -> Option<GuildRoleRef<'_>> {
+        let role = self.guilds.get(&guild_id)?.try_map(|g| g.roles.get(&role_id)).ok()?;
+        Some(CacheRef::from_mapped_ref(role))
     }
 
     /// Returns the settings.
@@ -678,8 +631,8 @@ impl Cache {
     /// println!("Max settings: {}", cache.settings().max_messages);
     /// # }
     /// ```
-    pub fn settings(&self) -> Settings {
-        self.settings.read().clone()
+    pub fn settings(&self) -> SettingsRef<'_> {
+        CacheRef::from_guard(self.settings.read())
     }
 
     /// Sets the maximum amount of messages per channel to cache.

src/client/dispatch.rs

@@ -224,7 +224,8 @@ fn update_cache_with_event(ctx: Context, event: Event) -> Option<(FullEvent, Opt
         },
         Event::GuildMemberUpdate(mut event) => {
             let before = if_cache!(update_cache(&ctx, &mut event));
-            let after: Option<Member> = if_cache!(ctx.cache.member(event.guild_id, event.user.id));
+            let after: Option<Member> =
+                if_cache!(ctx.cache.member(event.guild_id, event.user.id).map(|m| m.clone()));
 
             FullEvent::GuildMemberUpdate {
                 ctx,
@@ -316,7 +317,7 @@ fn update_cache_with_event(ctx: Context, event: Event) -> Option<(FullEvent, Opt
         },
         Event::MessageUpdate(mut event) => {
             let before = if_cache!(update_cache(&ctx, &mut event));
-            let after = if_cache!(ctx.cache.message(event.channel_id, event.id));
+            let after = if_cache!(ctx.cache.message(event.channel_id, event.id).map(|m| m.clone()));
 
             FullEvent::MessageUpdate {
                 ctx,

src/model/channel/channel_id.rs

@@ -452,7 +452,7 @@ impl ChannelId {
         #[cfg(feature = "cache")]
         if let Some(cache) = cache_http.cache() {
             if let Some(message) = cache.message(self, message_id) {
-                return Ok(message);
+                return Ok(message.clone());
             }
         }
 

src/model/guild/guild_id.rs

@@ -1080,7 +1080,7 @@ impl GuildId {
         {
             if let Some(cache) = cache_http.cache() {
                 if let Some(member) = cache.member(self, user_id) {
-                    return Ok(member);
+                    return Ok(member.clone());
                 }
             }
         }

src/model/guild/mod.rs

@@ -439,9 +439,7 @@ impl Guild {
         let name = name.as_ref();
         let guild_channels = cache.as_ref().guild_channels(self.id)?;
 
-        for channel_entry in &guild_channels {
-            let (id, channel) = channel_entry.pair();
-
+        for (id, channel) in guild_channels.iter() {
             if channel.name == name {
                 return Some(*id);
             }

src/model/guild/partial_guild.rs

@@ -388,9 +388,7 @@ impl PartialGuild {
         let name = name.as_ref();
         let guild_channels = cache.as_ref().guild_channels(self.id)?;
 
-        for channel_entry in &guild_channels {
-            let (id, channel) = channel_entry.pair();
-
+        for (id, channel) in guild_channels.iter() {
             if channel.name == name {
                 return Some(*id);
             }

src/utils/argument_convert/message.rs

@@ -71,7 +71,7 @@ impl ArgumentConvert for Message {
         #[cfg(feature = "cache")]
         if let Some(cache) = ctx.cache() {
             if let Some(msg) = cache.message(channel_id, message_id) {
-                return Ok(msg);
+                return Ok(msg.clone());
             }
         }