Tracking issue for `Option::contains` and `Result::contains`

[Centril]

This is a tracking issue for Option::contains and Result::contains.

The implementations are as follows:

    pub fn contains(&self, x: &T) -> bool where T: PartialEq {
        match self {
            Some(y) => y == x,
            None => false,
        }
    }

    pub fn contains(&self, x: &T) -> bool where T: PartialEq {
        match self {
            Ok(y) => y == x,
            Err(_) => false,
        }
    }
``

[Mark-Simulacrum]

Should we update the signatures to fn contains<U>(&self, x: &U) -> bool where T: PartialEq<U>?

[Centril]

@Mark-Simulacrum That seems like a good idea -- type inference should be driven by x in any case. Let's do that in the PR.

[timvermeulen]

Looks like slice/Vec/VecDeque/LinkedList and possibly others have a contains method that could benefit in the same way.

[soc]

@Mark-Simulacrum I didn't do that because I checked the collections first, and they were all invariant. Shall I change this for Option and Result only?

[Mark-Simulacrum]

We usually can't change preexisting APIs due to inference breakage, but since this is new, we should be able to be general.

[frewsxcv]

Are these functionally equivalent?

    pub fn contains(&self, x: &T) -> bool where T: PartialEq {
        match self {
            Some(y) => y == x,
            None => false,
        }
    }

    pub fn contains(&self, x: &T) -> bool where T: PartialEq {
        self.as_ref() == Some(x)
    }

[soc]

@frewsxcv Not quite, due to the requested variance changes.

[czipperz]

What does that mean @soc?

[soc]

@czipperz The implementation is:

// Option
pub fn contains<U>(&self, x: &U) -> bool where U: PartialEq<T> {
    match self {
        Some(y) => x == y,
        None => false,
    }
}

// Result
pub fn contains<U>(&self, x: &U) -> bool where U: PartialEq<T> {
    match self {
        Ok(y) => x == y,
        Err(_) => false
    }
}

pub fn contains_err<F>(&self, f: &F) -> bool where F: PartialEq<E> {
    match self {
        Ok(_) => false,
        Err(e) => f == e
    }
}

The argument doesn't have to be of the same type as the value inside option/result, only that it knows how to compare itself to it. I. e. the operation is not invariant on the contained type, it allows an arbitrary type.

[czipperz]

So there's no implementation of PartialEq<Option<U>> for Option<T> where U: PartialEq<T>?

[timvermeulen]

There is indeed not, see #20063.

[dtolnay]

These methods are way too high up in the docs on both the Option and Result rustdoc. Would someone be able to send a PR to move these a lot further down? For example stuff like ok and map and and_then should all be featured higher than contains.

[sinistersnare]

What is the status on this? I would love to use these in stable.

[xkr47]

Assume you have a function getFoo() returning Option<String> then it might be misleading if you do getFoo().contains("bar") since it looks like it checks for a substring when it in fact checks for an exact match. Would some other name perhaps be more clear?

[sinistersnare]

how about contains_in or map_contains?

[soc]

option.contains_in("foo") reads weird. You could of course pick something else entirely different, like option.has("foo") or option.includes("foo").

But in the end, Rust is a typed language, so we should not be scared that much about confusion, because the types tell us which contains method is used.

[camsteffen]

Naming bikeshed. Sorry if this is the wrong place.

How about the names eq_some, eq_ok and eq_err? Or maybe only using those names for Result? I get that contains is consistent with containers, but that analogy weakens especially with Result.

[selendym]

I'm quite surprised bikeshedders have not suggested wraps yet, so I guess I'll be that person. ;) This would be consistent with both Option and Result, and also unwrap.

[frewsxcv]

if unwrap is going to be renamed in the long-term, why not rename a theoretical Option#wraps at the same time?

[genusistimelord]

you could write it as is().

then it would read as getFoo().is("bar")

BUT... in a sense the contains here is if the Some() contains something that is equal to that or if it doesn't contain anything. now if we where to take the string out of the Some and use contains on it directly then I would agree with @xkr47

[xkr47]

or has()? :)

[genusistimelord]

or has()? :)

Yeah Has would have the same meaning if it was a String comparison as you mentioned before. Though contains works great for this since we are looking for what is in the Some() and not looking at what is contained within the Variable contained within the Some() =3.

[jakoschiko]

Or

getFoo().is_some_and("bar")
tryFoo().is_ok_and("bar")
tryFoo().is_err_and(FooErr)

• The names are similar to is_some, is_ok, and is_err
• There are already functions that end with _or, e.g. map_or

[Rustinante]

A lot of programming mistakes would be made due to the potential type confusion caused by this. Sometimes people will forget they're dealing with an Option<HashSet<...>> and call the .contains method on the option, thinking that it's actually a HashSet.

If some feature of this kind is really needed, I suggest calling it something like is_some_eq, where the is_some_ prefix signifies that first of all it must be is_some() and then unwraps and compares under PartialEq under the hood.

[Rustinante]

@Centril please consider renaming this, the name collision will likely cause a lot of confusions and bugs down the road.

[jplatte]

I wouldn't mind it keeping the current name but since many are opposed to that, here's another idea: .contains_some() (and .contains_ok() for Result; then there could be .contains_err() as well).

[Rustinante]

The word “some” has to come before "contains" due to the meaning of "contains", because first of all it must be a some, then a comparison is made, which is why at the minimum we should have something like some_eq. If we call it contains_some, it can be interpreted as saying the Option contains a nested Option value that is Some.

How about we rename this to eq_some? This way it mirrors the existing is_some, the difference being that it takes an argument to be eq compared with the wrapped value. Furthermore, things like eq_some(3) also reads nicely as “equals Some value that is 3”.

Similarly, we can have eq_err for Result, mirroring is_err.

[camsteffen]

x.eq_some(y) reads like x == Some(y)

[ximon18]

I don’t mind x.contains(y) as conceptually I think it matches the goal well, and the clash with HashMap is (to me at least) not different from the fact that any type can have any fn and thus you have to know the type being operated on to understand the effect of the fn, but to differentiate it further from sounding like a collection test one could perhaps use:

x.contained_is(y) and x.contained_err_is(y)

or:

x.inside_is(y) and x.inside_err_is(y)

?

[Rustinante]

@ximon18 do you think eq_some is not good?

[ximon18]

@Rustinante: my concern with eq_some() is that while it might sound right for the Option use case, to me it sounds too much like it deals with an Option to be right for the Result use case.

[Rustinante]

@ximon18 for the Result there's the eq_err() suggestion above.

[ximon18]

@Rustinante: True, but both eq_some and eq_err read (to me, edit: I’m not at all sure about this “concern”) like the value inside is Some(y) or Err(y) rather than actually being just y. Additionally by using some and err you duplicate something about the type, Option or Result, in the fn name which doesn’t feel right to me.

[Rustinante]

That objection would also apply to the existing functions is_some() and is_err(), do those existing functions feel right?

[ximon18]

@Rustinante: I see your point, and I’m not convinced that I’m right, but to me is_some and is_err don’t have the concern I mentioned because they don’t seem to be talking about the inside value while eq_some(y) and eq_err(y) are trying to say something about the inside value.

At any rate, I was trying to offer another possibility for consideration, not to comment directly on the value or otherwise of eq_some and eq_err as I’m not sure what is best or even if the proposal is actually necessary, especially with the new matches! syntax covering this use case (I think, if somewhat more verbosely).

[frewsxcv]

My take on eq is that it's misleading because we're using PartialEq in this method, not Eq

[Rustinante]

Then why do people still use == for both Eq and PartialEq comparisons? Isn't that misleading as well? And why can we still do x == Some(y)? I'm under the impression that this whole feature is to provide a shorter way for writing x == Some(y).

[frewsxcv]

Then why do people still use == for both Eq and PartialEq comparisons? Isn't that misleading as well? And why can we still do x == Some(y)? I'm under the impression that this whole feature is to provide a shorter way for writing x == Some(y).

This was covered earlier: https://github.com/rust-lang/rust/issues/62358#issuecomment-508900472

[Rustinante]

I see, but it still doesn't mean we can't use eq, even the trait method for PartialEq is eq https://doc.rust-lang.org/std/cmp/trait.PartialEq.html#tymethod.eq Other proposed names don't even begin to touch on these nuances. We could use peq, but that's ugly and it's not straight-forward to guess its meaning.

[frewsxcv]

Oh, nevermind then. PartialOrd has partial_cmp, but PartialEq has eq. Did not know that.

[tvercruyssen]

What are still the blocking changes keeping this from being stabilized besides possibly a name change?

[tvercruyssen]

These methods are way too high up in the docs on both the Option and Result rustdoc. Would someone be able to send a PR to move these a lot further down? For example stuff like ok and map and and_then should all be featured higher than contains.

@tvercruyssen this perhaps? shrug

If that's what is best, than yea I would be happy to make a PR. But aren't all methods ordered in alphabetical order? This would break that and might confuse people more (when scrolling through the docs looking for a method)?

[jhpratt]

The ordering of methods in documentation should have nothing to do with stabilization, as that can be changed at any time.

[tvercruyssen]

I'm quite surprised bikeshedders have not suggested wraps yet, so I guess I'll be that person. ;) This would be consistent with both Option and Result, and also unwrap.

If we wanted to use this name instead that would have to happen before stabilization. That would move the method lower in the docs as well. If we want this rename to happen, I'm happy to open a PR for it.

[camsteffen]

I would prefer that the "wraps" concept is not extended beyond unwrap. The unwrap function has a "do it the dangerous way" connotation, and so wraps is not really a correlary to unwrap. In fact, it could give a false impression that unwrap is more idiomatic than it actually is.

I still prefer eq_some over contains. One reason is that get_items().contains(x) is ambigious as to whether you are checking a collection type or an Option. The only argument against eq_some is that it is redundant with the type. But that is not true - the method checks that 1) it is the Some variant and 2) that the inner value is equal to the input. eq_some communicates this two-in-one semantic. In my mind, the semantics of this function and Collection::contains are "kind of similar but different", and its better not to conflate them.

[Rustinante]

+1 on @camsteffen's summary. I think we should do eq_some for Option and eq_err for Result based on the foregoing discussions.

[tvercruyssen]

So do we make a PR to rename Option::contains to Option::eq_some, Result::contains to Result::eq_ok and Result::contains_err to Result::eq_err ? So that both methods can possibly(?) be stabilized at the same time.

[selendym]

I would prefer that the "wraps" concept is not extended beyond unwrap. The unwrap function has a "do it the dangerous way" connotation, and so wraps is not really a correlary to unwrap. In fact, it could give a false impression that unwrap is more idiomatic than it actually is.

By that you are implicitly assuming that people are unable to read or comprehend documentation - if that were the case, all hope is already lost and this would be the least of our concerns. :)

Just to defend my bikeshed a bit:

• unwrap describes an action that might fail with panic if it is not appropriate
• wraps describes a proposition that is tested and simply returns the verdict

So there is a clear difference there, no need to underline (un)safeness any more than what the docs would have. If anything, the compiler or linter could flag unwrap as evil, but that's another topic.

wraps is also easier to read, write, and remember than some long or multipart name; for example, consider:

if foo.wraps( bar ) {
    ...
}

This reads quite nicely, no?

In the end, this is just another round of bikeshedding. As long as the name and meaning are easily remembered, anything reasonably sane should do.

[tvercruyssen]

Any sane name will do problem is more like how do we pick the right one. As a name change is seemingly(?) the only thing blocking an otherwise usefull feature from getting stabilized.

[jplatte]

I think .wraps might be a nice name for (Option<T>, impl FnOnce(T) -> bool) -> bool, i.e. the generalized form of .eq_some (assuming that eq_some, eq_ok, eq_err goes through).