Open clarfonthey opened 4 years ago
I'm no expert on how backwards compatibility is handled. But Cow
implements Deref
, which means these new methods wouldn't be backwards compatible as they'd conflict with methods of the same name on the inner object. To fix this you'll have to change them to take cow: &Self
, similar to what most of the methods on Box
/Rc
etc. take.
Procedural: I’ve edited the title and labels to reflect that this is not implemented.
My opinion: I’m not sure that these method carry their weight, since if let Cow::Borrowed(_) = foo
or matches!(foo, Cow::Borrowed(_))
can be used. Neither of these existed when Option::is_some
was added, and Option
is much more common than Cow
.
@petertodd That's fair; that said, though, I think that having methods named is_borrowed
or is_owned
on a type which implements ToOwned
would be extremely unusual, because to_owned
would not affect said methods.
@SimonSapin I agree that Option
is much more common than Cow
, but I feel that these are still useful enough to carry their own weight. This is partially because I was writing code and was honestly surprised they didn't exist, so, I'm already pre-biased to assume that they should exist.
Mostly, I figured that adding these methods would be worthwhile because they're unlikely to clash with existing ones, and can incubate in nightly for a while to see if they do carry their weight. If they don't, they can be removed.
It appears that this is now merged.
@SimonSapin it would make sense to revert the changes to the title and tags, or discuss here whether the change should be reverted.
Done.
Sorry for the somewhat-pointless churn, I’d gotten here from https://github.com/rust-lang/rust/issues?q=is%3Aopen+is%3Aissue+label%3AC-tracking-issue+label%3AT-libs and hadn’t realized there was an implementation PR already.
All good! Thanks for updating things :)
Is anything blocking this from being stabilized?
Six month check-in: I don't see any issues involving these on the Issue Tracker. Can we move to the next stage?
Quite sad it's not stabilized yet. Are there any issues? 🥲 I find the method would be pretty convenient when cloning of the content under Cow
is bad and has to be double-checked for being Owned
in critical places.
Example:
if my_expensive_thing.is_borrowed() {
return Err(MyError::AttemptToCloneExpensiveThing);
} else {
do_something_heavy(my_expensive_thing.to_mut());
}
At this point I feel this should be fine to stabilize, but until that happens you can match a pattern: https://github.com/rust-lang/rust/issues/65143#issuecomment-543955294
Going through some old issues. Is there anything stopping this from going into FCP? I found myself using one of these methods again recently.
I can't see a strong case for the inherent methods over match cow {}
or matches!(cow, Cow::Owned(_))
nor can I think of any compelling uses for passing a Cow::is_owned
fn pointer.
Let-else being stabilized surely plays into this decision, too.
I would agree, but there exist other methods like is_some
, is_ok
, and is_err
which are also useful but could be replaced by similar patterns.
EDIT: Note, below may be wrong in most cases:
Another important thing to consider is that the method ensures that you're always borrowing the argument, whereas matches!(cow, Cow::Owned(_))
would consume cow
by-value and require adding &cow
.
matches!(cow, Cow::Owned(_))
would consume cow by-value and require adding&cow
That's not the case. See this playground.
You'd have to name the inner pattern for there to be an issue. E.g., matches!(cow, Cow::Owned(_cow))
.
Hmm, I guess you're right then. Still, I question whether such usages are always-applicable (e.g. if there's some usage that might break it), and the original point about other methods existing still stands. IMHO, unless those other methods are deprecated (which seems unlikely), this method should be considered for inclusion.
One use case for which I wanted these: I'm writing unit tests in which I want to differentiate between a borrowed and owned Cow
. I have a helper function that takes a expected: Cow<Foo>
as well as some inputs, then calls the method-under-test on the inputs to get an actual Cow
:
fn check(expected: Cow<Foo>, input: &str) {
let actual = method_under_test(input);
assert_eq!(expected, actual); // does not differentiate between borrowed or owned
assert_eq!(expected.is_owned(), actual.is_owned());
}
It's not hard to work around the absence of this method, and I don't know if it's worth adding the method just for that use case; but I figured I'd mention it.
(I'm also a self-taught Rust newbie, so it's possible there's a better way to do this that I just don't know!)
For the assertion in that unit test, you can assert_eq two matches!
calls, which is only marginally longer.
More broadly, I don’t think it is reasonable for all enums to provide is_
methods. Pattern matching has become substantially more ergonomic over time, and is likely to be improved further with e.g. if-let chaining. IMO if suitable language features exist they should be used, rather than providing more functions which bloat the API for little benefit.
I believe this proposal actually predates matches!
, huh.
Yeah, I guess it's fair to say that pattern-matching is the best way to do this. We'll see what people prefer.
On Sun, Aug 25, 2024 at 05:03:41AM -0700, Clar Fon wrote:
I believe this proposal actually predates
matches!
, huh.Yeah, I guess it's fair to say that pattern-matching is the best way to do this. We'll see what people prefer.
IMO matches!
is sufficient. Having all these little methods clutters up the
documentation. I also recently found out that a friend of mine had someone
missed the fact that matches!
exists... they might not have if it was used
more often.
The only case where matches!
might not be desired (only via preference) is when passing a function reference (e.g., .filter(Cow::is_owned)
) though certainly this is a less useful method than other types in std when it comes to filter/filter_map etc.
While matches!
is a working solution, is_borrowed/is_owned
would be much less cognitively demanding one, so to speak. More like natural language (if cow is owned, then). And IDE completion friendly, easy to find: cow.<TAB COMPLETION>
. While know and remember matches!(cow, Cow::Owned(_))
you must know and remember.
We already write option.is_some()
, result.is_ok()
, str.is_empty()
etc right?
All the is_*
methods to complete the picture:
https://doc.rust-lang.org/std/?search=is_
That's why I'd like to see this stabilized one day, even if it's just a syntax sugar for some.
I just tried to use these in my own code and was kind of shocked they didn't exist.
Justification: this seems like a common Rust pattern. We have
is_some
andis_none
forOption
,is_ok
andis_err
forResult
, etc., so, it seems pretty fair to haveis_borrowed
andis_owned
forCow
.Having
as_borrowed
andas_owned
wouldn't really make much sense, as a simple&
and&mut/to_mut
cover those use cases. But, these check functions are pretty useful on their own.