Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FromIntoRef and TryFromIntoRef #618

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

oblique
Copy link
Contributor

@oblique oblique commented Jul 16, 2023

Conversions from references are rare, however a user who controls the implementation of the type, can implement Into<T> from &'a MyType and use this to avoid cloning.

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #618 (babfaeb) into master (e096f0b) will increase coverage by 0.58%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master     #618      +/-   ##
==========================================
+ Coverage   63.63%   64.22%   +0.58%     
==========================================
  Files          35       36       +1     
  Lines        2090     2127      +37     
==========================================
+ Hits         1330     1366      +36     
- Misses        760      761       +1     
Impacted Files Coverage Δ
serde_with/src/de/impls.rs 58.66% <80.00%> (+0.21%) ⬆️
serde_with/src/ser/impls.rs 91.66% <100.00%> (+0.36%) ⬆️
serde_with/tests/serde_as/fromintoref.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jonasbb
Copy link
Owner

jonasbb commented Jul 16, 2023

Thanks for the contribution. I think I see the utility. Can you please confirm: the new types are almost identical to FromInto and TryFromInto, except that the SerializeAs trait bound does not require Clone by instead including the reference in the Into/TryInto bounds?
Do you have a use case for which you need this?

However, the “borrow” name part seems wrong here. In the serde ecosystem, borrow usually refers to borrowing during deserialization. The new types do not enable borrowing during serialization either, they only remove the Clone requirement. The reason is that for<'a> &'a T: Into<U> does not allow U to borrow from T. What the for<'a> means is any lifetime, no matter how short. Therefore, for any lifetime of U you can find a smaller 'a, thus U cannot borrow from T.
I don't have any name suggestions right now. Do you have any idea?

@oblique
Copy link
Contributor Author

oblique commented Jul 17, 2023

Can you please confirm: the new types are almost identical to FromInto and TryFromInto, except that the SerializeAs trait bound does not require Clone by instead including the reference in the Into/TryInto bounds?

Yes I confirm the above.

Do you have a use case for which you need this?

I'm working on a WASM project that has many serializations of data that resides in heap and I wanted to minimize cloning whenever I can.

However, the “borrow” name part seems wrong here.
.....
I don't have any name suggestions right now. Do you have any idea?

I agree borrow doesn't feel right. I didn't know what name to choose. These are some ideas:

  1. FromRefInto/TryFromRefInto
  2. RefFromInto/RefTryFromInto
  3. FromIntoRef/TryFromIntoRef
  4. FromIntoNonCloning/TryFromIntoNonCloning

@jonasbb
Copy link
Owner

jonasbb commented Jul 17, 2023

Thanks for the info. I like FromIntoRef/TryFromIntoRef the most. Could you please update the name? The rest looks good. You have tests and documentation written. The only thing missing is a changelog entry, but I can create that when releasing the crate.

@oblique oblique force-pushed the feat/borrow-from-into branch from 94b0b7f to babfaeb Compare July 17, 2023 12:34
@oblique oblique changed the title Add BorrowFromInto and BorrowTryFromInto Add FromIntoRef and TryFromIntoRef Jul 17, 2023
@oblique
Copy link
Contributor Author

oblique commented Jul 17, 2023

I changed the names. I also changed the first line of the doc comments. Let me know if you prefer other wording there.

@jonasbb
Copy link
Owner

jonasbb commented Jul 17, 2023

Thank you for the changes. This looks good now.

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 17, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit e530987 into jonasbb:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants