References on all the things #114

Emilgardis · 2021-04-25T17:51:59Z

Started work on making everything use references instead of allocating on multiple places

https://github.com/twitch-rs/twitch_api/tree/references

This solution should work, but I'm not sure the approach is good.

I'd want requests to be able to take references, and responses return them too.

Suggestions for how to avoid unnecessary allocations in responses hugely appreciated!

Emilgardis · 2021-05-14T09:11:42Z

https://docs.rs/aliri_braid seems like a good way to solve this!

Nerixyz · 2022-08-01T15:49:20Z

I tried to "convert" the GetUsersRequest as a POC to use reference types (see diffs below). Using references in the struct has the consequence, that we can't pass a "static"/immediate reference to a slice, but need to create a variable for the array first:

let req = GetUsersRequest::builder().id(&["44322889".into()]).build();

This will result in the following error:

error[E0716]: temporary value dropped while borrowed
   --> src\helix\endpoints\users\get_users.rs:112:46
    |
112 |     let req = GetUsersRequest::builder().id(&["44322889".into()]).build();
    |                                              ^^^^^^^^^^^^^^^^^^^         - temporary value is freed at the end of this statement
    |                                              |
    |                                              creates a temporary which is freed while still in use
...
139 |     let uri = req.get_uri().unwrap();
    |               ------------- borrow later used here
    |
help: consider using a `let` binding to create a longer lived value
    |
112 ~     let binding = ["44322889".into()];
113 ~     let req = GetUsersRequest::builder().id(&binding).build();
    |

Fix:

let ids = ["44322889".into()];
let req = GetUsersRequest::builder().id(&ids).build();

Diffs:

helix/endpoints/users/get_users.rs

diff --git a/src/helix/endpoints/users/get_users.rs b/src/helix/endpoints/users/get_users.rs
index 49333d8d3..0f5789471 100644
--- a/src/helix/endpoints/users/get_users.rs
+++ b/src/helix/endpoints/users/get_users.rs
@@ -25,9 +25,11 @@
 //! # let client: helix::HelixClient<'static, client::DummyHttpClient> = helix::HelixClient::default();
 //! # let token = twitch_oauth2::AccessToken::new("validtoken".to_string());
 //! # let token = twitch_oauth2::UserToken::from_existing(&client, token, None, None).await?;
+//! let user_ids = ["1234".into()];
+//! let usernames = ["justintvfan".into()];
 //! let request = get_users::GetUsersRequest::builder()
-//!     .id(vec!["1234".into()])
-//!     .login(vec!["justintvfan".into()])
+//!     .id(&user_ids)
+//!     .login(&usernames)
 //!     .build();
 //! let response: Vec<get_users::User> = client.req_get(request, &token).await?.data;
 //! # Ok(())
@@ -43,15 +45,15 @@ use helix::RequestGet;
 /// Query Parameters for [Get Users](super::get_users)
 ///
 /// [`get-users`](https://dev.twitch.tv/docs/api/reference#get-users)
-#[derive(PartialEq, typed_builder::TypedBuilder, Deserialize, Serialize, Clone, Debug)]
+#[derive(PartialEq, typed_builder::TypedBuilder, Serialize, Clone, Debug)]
 #[non_exhaustive]
-pub struct GetUsersRequest {
+pub struct GetUsersRequest<'a> {
     /// User ID. Multiple user IDs can be specified. Limit: 100.
     #[builder(default)]
-    pub id: Vec<types::UserId>,
+    pub id: &'a [&'a types::UserIdRef],
     /// User login name. Multiple login names can be specified. Limit: 100.
     #[builder(default)]
-    pub login: Vec<types::UserName>,
+    pub login: &'a [&'a types::UserNameRef],
 }
 
 /// Return Values for [Get Users](super::get_users)
@@ -91,7 +93,7 @@ pub struct User {
     pub view_count: usize,
 }
 
-impl Request for GetUsersRequest {
+impl<'a> Request for GetUsersRequest<'a> {
     type Response = Vec<User>;
 
     #[cfg(feature = "twitch_oauth2")]
@@ -101,15 +103,14 @@ impl Request for GetUsersRequest {
     const SCOPE: &'static [twitch_oauth2::Scope] = &[];
 }
 
-impl RequestGet for GetUsersRequest {}
+impl<'a> RequestGet for GetUsersRequest<'a> {}
 
 #[cfg(test)]
 #[test]
 fn test_request() {
     use helix::*;
-    let req = GetUsersRequest::builder()
-        .id(vec!["44322889".into()])
-        .build();
+    let ids = ["44322889".into()];
+    let req = GetUsersRequest::builder().id(&ids).build();
 
     // From twitch docs
     // FIXME: This is not valid anymore. Twitch....

helix/client/client_ext.rs

diff --git a/src/helix/client/client_ext.rs b/src/helix/client/client_ext.rs
index 4d70b442c..4d963b6d1 100644
--- a/src/helix/client/client_ext.rs
+++ b/src/helix/client/client_ext.rs
@@ -12,7 +12,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [User](helix::users::User) from user login
     pub async fn get_user_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<helix::users::User>, ClientError<'a, C>>
     where
@@ -20,7 +20,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::users::GetUsersRequest::builder()
-                .login(vec![login.into()])
+                .login(&[login.as_ref()])
                 .build(),
             token,
         )
@@ -31,7 +31,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [User](helix::users::User) from user id
     pub async fn get_user_from_id<T>(
         &'a self,
-        id: impl Into<types::UserId>,
+        id: impl AsRef<types::UserIdRef>,
         token: &T,
     ) -> Result<Option<helix::users::User>, ClientError<'a, C>>
     where
@@ -39,7 +39,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::users::GetUsersRequest::builder()
-                .id(vec![id.into()])
+                .id(&[id.as_ref()])
                 .build(),
             token,
         )
@@ -50,7 +50,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [ChannelInformation](helix::channels::ChannelInformation) from a broadcasters login
     pub async fn get_channel_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<helix::channels::ChannelInformation>, ClientError<'a, C>>
     where
@@ -66,7 +66,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get [ChannelInformation](helix::channels::ChannelInformation) from a broadcasters id
     pub async fn get_channel_from_id<T>(
         &'a self,
-        id: impl Into<types::UserId>,
+        id: impl AsRef<types::UserIdRef>,
         token: &T,
     ) -> Result<Option<helix::channels::ChannelInformation>, ClientError<'a, C>>
     where
@@ -74,7 +74,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     {
         self.req_get(
             helix::channels::GetChannelInformationRequest::builder()
-                .broadcaster_id(id.into())
+                .broadcaster_id(id.as_ref())
                 .build(),
             token,
         )
@@ -361,7 +361,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get a users, with login, follow count
     pub async fn get_total_followers_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<i64>, ClientError<'a, C>>
     where
@@ -547,7 +547,7 @@ impl<'a, C: crate::HttpClient<'a> + Sync> HelixClient<'a, C> {
     /// Get channel emotes in channel with user login
     pub async fn get_channel_emotes_from_login<T>(
         &'a self,
-        login: impl Into<types::UserName>,
+        login: impl AsRef<types::UserNameRef>,
         token: &T,
     ) -> Result<Option<Vec<helix::chat::ChannelEmote>>, ClientError<'a, C>>
     where

Emilgardis · 2022-08-04T06:39:35Z

this looks promising for the deserialization: yoke and zerofrom

Emilgardis · 2022-11-22T11:22:40Z

Should investigate if there is a better surface for storing multiple values than

foos: Cow<'a, [&'a FooRef]>,

the issue is that requiring FooRef means that there is a situation where your collection is Vec<Foo> and to actually pass it to the request you'd need to do let v: Vec<&'a FooRef> = vec.iter().map(Deref::deref).collect() which is another allocation.

It'd be neat to abstract this so that all variants works. i.e passing &[Foo], [Foo; _], Vec<Foo>, Vec<&FooRef>, &[&FooRef].

The trait that unifies these should be IntoIterator<Item = Into<&FooRef>>, however it's not very nice. This is what I came up with

use std::borrow::Cow;
use twitch_types as types;

fn main() {
    let ids: Vec<types::UserId> = vec!["123".into(), "456".into(), "789".into()];
    println!("{:p}", &*ids);

    let req = Request {
        id: Cow::from(&ids),
        _pd: <_>::default(),
    };
    println!("{:p}", &*req.id);
}

pub struct Request<'a, 'i, IT, D: 'a>
where
    &'i IT: IntoIterator<Item = D>,
    IT: ToOwned + ?Sized,
    D: ?Sized + Into<&'a types::UserIdRef>,
{
    pub id: Cow<'i, IT>,
    _pd: std::marker::PhantomData<&'a ()>,
}

    Finished dev [unoptimized + debuginfo] target(s) in 0.36s
     Running `target/debug/twitch_api_ref_coll`
0x1516069e0
0x1516069e0

Emilgardis · 2022-11-29T11:57:14Z

Maybe an enum

enum Cowtainer<'a, T> where T: ToOwned + ?Sized {
    One(Cow<'a, T>),
    Multiple(Cow<'a, [&'a T]>),
}

Emilgardis · 2022-12-14T07:50:36Z

Lessons from ICU4X (with great talk https://www.youtube.com/watch?v=DM2DI3ZI_BQ)

VarZeroVec basically implements Cow<'a, [&'a T]> but nicer (depending on how you see it)

I'm still not satisfied though, I'll experiment with this

laundmo · 2023-05-30T17:00:06Z

as a new user of this crate, the usage of braid and Cow in many places is not something i have encountered in any other crate and leads to quite verbose code. for example, going from a Option<Cursor> to a Option<Cow<CursorRef>> requires this monstrosity: input.as_ref().map(|c| Cow::Borrowed(c.as_ref()))

that is simply not a nice API. i hope it can be improved in some way.

personally i would be perfectly fine with just using String everywhere - the few extra allocations are a price i would happily pay to be able to simplify my code a lot.

laundmo · 2023-05-30T17:07:04Z

if there is no willingness to drop braid, i imaging a From implementation could be used:

impl From<Option<Cursor>> for Option<CursorRef> { /*...*/}

Emilgardis · 2023-05-30T21:23:52Z

@laundmo Happy to have you here! Thank you for the feedback, there is definitely a trade-off we need to take into consideration with this.

As for the Cursor problem the following should work fine and is much smaller: cursor.map(Into::into). Using into should be enough thanks to this impl

fn cow<'a>(cursor: Option<Cursor>) -> Option<std::borrow::Cow<'a, CursorRef>> {
    cursor.map(Into::into)
}

The suggestion wouldn't work with that signature for the same reason there's no impl From<Option<String>> for Option<str>, it'd have to be for Option<&'a str> as Option<T> has an implicit T: Sized bound. There is no valid lifetime to pick here either so we're stuck. Further, this hits the, very unfortunate and annoying, orphan rule.

Option<CursorRef> is not a valid Option:

error[E0277]: the size for values of type `str` cannot be known at compilation time
   --> src/helix/mod.rs:168:39
    |
168 |     fn from(value: Option<Cursor>) -> Option<CursorRef> {
    |                                       ^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: within `helix::CursorRef`, the trait `std::marker::Sized` is not implemented for `str`
note: required because it appears within the type `CursorRef`
   --> src/helix/mod.rs:165:1
    |
165 | #[aliri_braid::braid(serde)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: required by a bound in `std::option::Option`
   --> /Users/emil/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:563:17
    |
563 | pub enum Option<T> {
    |                 ^ required by this bound in `Option`
    = note: this error originates in the attribute macro `aliri_braid::braid` (in Nightly builds, run with -Z macro-backtrace for more info)

and the orphan rule:

impl From<Option<Cursor>> for Option<CursorRef> {
    fn from(value: Option<Cursor>) -> Self {
        value.map(Into::into)
    }
}

fails with

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   --> src/helix/mod.rs:168:1
    |
168 | impl From<Option<Cursor>> for Option<CursorRef> {
    | ^^^^^--------------------^^^^^-----------------
    | |    |                        |
    | |    |                        `std::option::Option` is not defined in the current crate
    | |    `std::option::Option` is not defined in the current crate
    | impl doesn't use only types from inside the current crate
    |
    = note: define and implement a trait or new type instead

this also hinders a impl From<Option<Cursor>> for Option<Cow<'static, CursorRef>>, std would have to implement it

regarding simplifying the code, if you have some examples I'd love to have a look to see if there's anything we can do in this codebase or clarify how to consume the api in a convenient way. feel free to drop them in a issue or discussion here or create a thread over at our channel #twitch-rs in Twitch API Discord (or just reach out to me directly)

laundmo · 2023-05-30T22:15:22Z

thank you, that .map(Into::into) worked and i so much nicer (so nice in fact, that i dont think a custom (to avoid orphan rules) conversion trait would be useful)

i'll probably send you a message on discord in a bit with some smaller things tho after looking through the issues, you seem to be aware of most of them (like the slices are already discussed herem, and the builders not being ideal).

Emilgardis mentioned this issue Jul 27, 2021

feat(pubsub): implement Clone and Hash for topics #181

Merged

Emilgardis mentioned this issue Jul 31, 2022

Use References in Requests Where Possible #261

Closed

Emilgardis linked a pull request Aug 1, 2022 that will close this issue

references #194

Draft

Emilgardis mentioned this issue Oct 15, 2022

Trait to turn paginated APIs into async streams #271

Open

Emilgardis mentioned this issue Oct 27, 2022

refactor: Use (mostly) References in Requests #280

Merged

Emilgardis mentioned this issue Jan 26, 2023

Make helix::client::custom first-class #322

Open

2 tasks

Emilgardis added the enhancement New feature or request label May 8, 2023

Emilgardis mentioned this issue May 30, 2023

Clarify how or make it easier to pass in collections that are T: Iterator<Item=D> in builder functions for helix requests #374

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

References on all the things #114

References on all the things #114

Emilgardis commented Apr 25, 2021 •

edited

Emilgardis commented May 14, 2021 •

edited

Nerixyz commented Aug 1, 2022

Emilgardis commented Aug 4, 2022 •

edited

Emilgardis commented Nov 22, 2022 •

edited

Emilgardis commented Nov 29, 2022

Emilgardis commented Dec 14, 2022 •

edited

laundmo commented May 30, 2023 •

edited

laundmo commented May 30, 2023 •

edited

Emilgardis commented May 30, 2023 •

edited

laundmo commented May 30, 2023 •

edited

References on all the things #114

References on all the things #114

Comments

Emilgardis commented Apr 25, 2021 • edited

Emilgardis commented May 14, 2021 • edited

Nerixyz commented Aug 1, 2022

Emilgardis commented Aug 4, 2022 • edited

Emilgardis commented Nov 22, 2022 • edited

Emilgardis commented Nov 29, 2022

Emilgardis commented Dec 14, 2022 • edited

laundmo commented May 30, 2023 • edited

laundmo commented May 30, 2023 • edited

Emilgardis commented May 30, 2023 • edited

laundmo commented May 30, 2023 • edited

Emilgardis commented Apr 25, 2021 •

edited

Emilgardis commented May 14, 2021 •

edited

Emilgardis commented Aug 4, 2022 •

edited

Emilgardis commented Nov 22, 2022 •

edited

Emilgardis commented Dec 14, 2022 •

edited

laundmo commented May 30, 2023 •

edited

laundmo commented May 30, 2023 •

edited

Emilgardis commented May 30, 2023 •

edited

laundmo commented May 30, 2023 •

edited