libs-team icon indicating copy to clipboard operation
libs-team copied to clipboard

ACP: Extending `Borrow` and `BorrowMut` to owned references

Open clarfonthey opened this issue 11 months ago • 6 comments

Proposal

Problem statement

Code that relies on Borrow and BorrowMut can accept plain references (e.g. &str and &mut str) but they cannot accept owned references (e.g. &String and &mut String).

Motivating examples or use cases

The primary desire here is to be able to have BorrowMut for for owned references like &mut String, since the owned references will permit additional operations that extend buffers in addition to ones that simply mutate the buffer. For cases where a Cow<'_, str> is only mutably borrowed, it can be possible to produce &mut String, but not possible to produce an owned String, which means that generic code using BorrowMut is not possible.

This is currently possible with AsMut due to its transitivity (T: AsRef<U> implies &T: AsRef<U>) but not possible with BorrowMut because these transitive implementations are not available.

Solution sketch

Presumably, there is a historical reason why the Borrow traits weren't given transitive implementations, and it seems likely that adding them now would cause lots of breakage. Instead, the proposal would be to manually implement reference-transitive versions of the existing Borrow implementations in the standard library, such as:

impl<T> Borrow<[T]> for &Vec<T>;
impl<T> Borrow<[T]> for &mut Vec<T>;
impl<T> BorrowMut<[T]> for &mut Vec<T>;

Presumably, this would have to be done via macros, similarly to how arithmetic operations are implemented for references as well.

Alternatives

We could just not do this. That's the main thing.

Links and related work

None so far.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

clarfonthey avatar Jan 27 '25 21:01 clarfonthey

I'm assuming impl<T> BorrowMut<[T]> for &Vec<T> is a mistake since that would allow creating multiple mutable references to the same slice at once in safe code.

programmerjake avatar Jan 28 '25 00:01 programmerjake

Yes, that was 100% a copy error.

clarfonthey avatar Jan 28 '25 04:01 clarfonthey

We discussed this in the libs-api meeting, but had trouble understanding the motivation for this. Could you please include some code examples that demonstrate the problem?

Amanieu avatar Feb 04 '25 19:02 Amanieu

I think it'd probably make more sense to just share the project that motivated me for this: https://codeberg.org/clarfonthey/tagged-string

Essentially, it's a wrapper that adds type-level validations on strings, and initially it used Borrow and BorrowMut as the traits to generalise over string types. (I didn't want to limit people to String, letting them use for example Arc<str> if they wanted.)

The problem came when I wanted to do conversion between the different tags. (Validations, formats, whatever you want to call them. An example might be converting from an internationalised domain name written in Unicode to its ASCII encoding.) For non-mutating conversions, I just coerce everything to the &str variant with Borrow to pass to the validators. For in-place conversions like make_lowercase_ascii, I can use &mut str from BorrowMut. But as you know, most string operations require changing the length of the string, and so, you can't really work with &mut str and will want &mut String instead.

This is when I found out that &mut String was not valid under my type, because it didn't satisfy the Borrow<str> bound. I've since replaced it with my own traits instead.

Basically, I think that Borrow is extremely useful for wrappers that want to be generic over bytes, strings, etc., but especially in the case of strings, you'll often want to work directly with the owned version since the length has to change. And the moment you try and wrap a reference to that, particularly a mutable reference, it fails. (I do this similar to the as_ref and as_mut methods for Option. Wrapping a reference is still important here since it avoids making methods unsafe: since the type enforces that its string is validated, I don't need to require that the caller only passes already-validated strings.)

Not sure how well I got across my intent here, but I think this problem of wrapping types like &mut String under something like a Borrow<str> bound is general enough that it's worth including in the standard library.

clarfonthey avatar Feb 05 '25 02:02 clarfonthey

We discussed this in today's @rust-lang/libs-api meeting.

There was a bunch of shared grumbling about Borrow/AsRef and the annoyance of having both, which happens every time these traits come up. :)

Our consensus was: if these have zero impact on a crater run, and they're a limited set of impls that have concrete motivation rather than a table-completing activity of every impl that might possibly be needed, we'd be willing to consider these.

Please provide a complete list of the impls you're proposing (just String?), and please do a crater run to make sure they don't cause inference failures.

joshtriplett avatar Feb 11 '25 19:02 joshtriplett

Honestly, while I think that the String impls would be useful, I've already resorted to making my own traits for my own project and don't think it would be a good idea to only implement this for strings and none of the other types. I'd personally rather just omit all of them if that's the preference, rather than selectively implementing a few of them.

clarfonthey avatar Feb 12 '25 15:02 clarfonthey

Going to close this as unplanned based upon the above reasoning.

clarfonthey avatar Apr 15 '25 21:04 clarfonthey