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 Borrow and BorrowMut derives #145

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

Slabity
Copy link

@Slabity Slabity commented Nov 3, 2020

Had to switch from using AsRef to using Borrow in a project of mine for certain reasons and I'd really like to continue using this crate for that purpose. Borrow is identical in functionality to AsRef, but is used in different contexts and implemented for both T and &T.

This literally duplicates the functionality for AsRef, except the duplicated instances are replaced with Borrow. Same with AsMut and BorrowMut, which can be used like so:

#[derive(Borrow)]
struct MyStruct {
    #[borrow]
    data: u32
}

Let me know if there are any issues with this and I'll fix them. I think I got everything in the documentation included that is necessary, but I'm not 100% sure.

@Slabity
Copy link
Author

Slabity commented Nov 3, 2020

This could also be seen as a solution to #123 as AsRef<T> is not really suppose to be implemented for T or &T, but Borrow<T> is already blanket-implemented for T and &T (again, different contexts despite same behavior).

@Slabity
Copy link
Author

Slabity commented Nov 25, 2020

Any particular issues with this? Anything that would need to be changed to allow it to be merged in?

Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this! I read up a bit on the difference between Borrow and AsRef, these two quotes from the rust docs seem the most important:

In particular Eq, Ord and Hash must be equivalent for borrowed and owned values: x.borrow() == y.borrow() should give the same result as x == y.

Borrow also requires that Hash, Eq and Ord for borrowed value are equivalent to those of the owned value. For this reason, if you want to borrow only a single field of a struct you can implement AsRef, but not Borrow.

This makes it seem like maybe Borrow should only be allowed to be derived on single field structs (i.e. newtypes). What do you think of that? Does it still cover your usecase then?

Other than that the PR is missing some tests. You can probably copy most of the as_ref/as_mut tests: https://github.com/JelteF/derive_more/blob/master/tests/as_ref.rs

There's also the generics test, that you should probably add it to: https://github.com/JelteF/derive_more/blob/master/tests/generics.rs

I'm pretty busy at the moment, but I'll try to make some time for a full review of this PR.


Generates:

```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```
```rust

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Slabity
Copy link
Author

Slabity commented Mar 22, 2021

In particular Eq, Ord and Hash must be equivalent for borrowed and owned values: x.borrow() == y.borrow() should give the same result as x == y.

Borrow also requires that Hash, Eq and Ord for borrowed value are equivalent to those of the owned value. For this reason, if you want to borrow only a single field of a struct you can implement AsRef, but not Borrow.

This makes it seem like maybe Borrow should only be allowed to be derived on single field structs (i.e. newtypes). What do you think of that? Does it still cover your usecase then?

My usecase has actually somewhat changed and I am taking a different approach by using Deref instead. I can definitely say I was not ensuring Hash, Eq, or Ord were implemented properly though. I'm not sure if only allowing it for newtypes is the correct solution as the user still needs to ensure the other traits are implemented themselves.

Other than that the PR is missing some tests. You can probably copy most of the as_ref/as_mut tests: https://github.com/JelteF/derive_more/blob/master/tests/as_ref.rs

There's also the generics test, that you should probably add it to: https://github.com/JelteF/derive_more/blob/master/tests/generics.rs

I'm pretty busy at the moment, but I'll try to make some time for a full review of this PR.

I've added tests for Borrow and BorrowMut. I have not added anything to tests/generics.rs though as I do not see tests for AsRef or AsMut in that either.

I'm not sure if there's anyone else interested in this, but I've resolved the merge conflicts just in case.

@Skirmisher
Copy link

I can say I'd appreciate a derive(Borrow), precisely in the single-field-struct case where it makes sense that a reference to the newtype "is" just a reference to the inner data. That said, a newtype may have additional fields that are ultimately secondary to the single "main" field, and don't compromise Hash/Eq/Ord semantics. So, I thought a bit about how to approach this situation.

Regarding the semantics of Borrow vs AsRef, I understand the concepts as such:

  • impl Borrow<Borrowed> for Type
    • Represents a reference to Type as a whole
    • Operations on Borrowed are semantically equivalent to those on Type
    • Impl is "canonical", i.e. there is at most* one impl for Type besides the blanket impl
      • *unless you make a very compelling argument otherwise, maybe
  • impl AsRef<T> for Type
    • Represents a "view" of all or part of Type
    • T may be semantically distinct from Type; examples:
      • T is a more generic view of Type's underlying data (slices)
      • T is a distinct (possibly specialized) concept related to Type (OsStr, Path, ...)
      • T is the same as Borrowed (usability with generic code)
    • Variety of impls for any T that can trivially represent Type's data
      • Generic code takes advantage of this, making interop easier overall

I established these principles from viewing the implementors of Borrow and AsRef in std, which I found quite helpful for more thoroughly understanding the traits' purposes. (Also, it's worth noting that Borrow's documentation states "A type is free to borrow as several different types." It's not really borne out by std's usage, but it's certainly possible to do idiomatically... and such idiomatic usage seems very unlikely to come up in practice. How many crates actually impl Borrow in the first place?)

I think a reasonable compromise, to better adhere to the spirit of Borrow, would be to limit the derived impl to precisely one field of the struct, rather than allowing one impl per unique Borrowed type, as is the case for derive(AsRef). This would allow "newtypes" with associated data to derive(Borrow), while limiting the possible damage of careless use of the macro.

With all the above said, restricting the derive to single-field structs is definitely the safest option; that's all I need personally (I don't have any examples of multi-field newtypes using Borrow), and some more boilerplate for more complex types is a reasonable cost. I don't think derive(Borrow) should be as lax as derive(AsRef), because of the risk of unsound semantics (both in terms of Eq etc. and because it doesn't make sense for a struct to borrow as multiple types which reference disjoint data within the struct). The comparative rarity of impl Borrow makes this a much more compelling tradeoff, in my opinion.

@tyranron tyranron linked an issue May 22, 2023 that may be closed by this pull request
@JelteF
Copy link
Owner

JelteF commented Dec 18, 2023

going over old PRs

With all the above said, restricting the derive to single-field structs is definitely the safest option; that's all I need personally (I don't have any examples of multi-field newtypes using Borrow), and some more boilerplate for more complex types is a reasonable cost. I don't think derive(Borrow) should be as lax as derive(AsRef), because of the risk of unsound semantics (both in terms of Eq etc. and because it doesn't make sense for a struct to borrow as multiple types which reference disjoint data within the struct). The comparative rarity of impl Borrow makes this a much more compelling tradeoff, in my opinion.

I totally agree with this. I think if someone changes this PR to add the single-field restriction then it can probably be merged quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for deriveing Borrow
3 participants