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

Tracking issue for Option::contains and Result::contains #62358

Open
Centril opened this issue Jul 4, 2019 · 143 comments
Open

Tracking issue for Option::contains and Result::contains #62358

Centril opened this issue Jul 4, 2019 · 143 comments

Comments

@Centril
Copy link
Contributor

@Centril Centril commented Jul 4, 2019

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
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jul 4, 2019

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

@Centril
Copy link
Contributor Author

@Centril Centril commented Jul 4, 2019

@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
Copy link
Contributor

@timvermeulen timvermeulen commented Jul 4, 2019

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

@soc
Copy link
Contributor

@soc soc commented Jul 4, 2019

@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
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Jul 4, 2019

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

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jul 6, 2019

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
Copy link
Contributor

@soc soc commented Jul 8, 2019

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

@czipperz
Copy link
Contributor

@czipperz czipperz commented Jul 14, 2019

What does that mean @soc?

@soc
Copy link
Contributor

@soc soc commented Jul 14, 2019

@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
Copy link
Contributor

@czipperz czipperz commented Jul 14, 2019

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

@timvermeulen
Copy link
Contributor

@timvermeulen timvermeulen commented Jul 14, 2019

There is indeed not, see #20063.

ppannuto added a commit to alphan/tock that referenced this issue May 19, 2020
This matches the new `contains` method of Option:
rust-lang/rust#62358
@dtolnay
Copy link
Member

@dtolnay dtolnay commented Jun 17, 2020

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
Copy link
Contributor

@sinistersnare sinistersnare commented Jul 26, 2020

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

@xkr47
Copy link
Contributor

@xkr47 xkr47 commented Aug 19, 2020

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
Copy link
Contributor

@sinistersnare sinistersnare commented Aug 19, 2020

how about contains_in or map_contains?

@soc
Copy link
Contributor

@soc soc commented Aug 19, 2020

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
Copy link
Contributor

@camsteffen camsteffen commented Aug 21, 2020

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.

@avirule
Copy link

@avirule avirule commented Jan 9, 2022

if deadline.contains(|d| d > now) {
    // ...
}

This latter one on the other hand I do feel comfortable reading directly, and I'd read this as "if deadline contains some instant later than now".

Admittedly, one could still outline and name the condition in the second example the same as the first, and the difference would be substantially reduced, but I do think that the former snippet is more likely to encourage this style of separation. My conclusion is that you're absolutely right @m-ou-se that contains would be less verbose, but I'm still not at all convinced that it reduces confusion.

In my opinion, the latter is confusing—at least, in my opinion, .contains() is mostly relevant when working with slices or collections. I've been programming for several years, and I can honestly say I've never desired .contains() to take a closure rather than a direct equatable type. This has been consistent through the several languages I've become somewhat familiar with: C++, C#, and Rust.

Which isn't to say my experience means this is a proper assertion, but rather that my experience with these languages has somewhat imparted the idea that .contains(other) semantically equates to:

for obj in collection {
  if obj.eq(&obj_other) {
    return true;
  }
}

Effectively, a direct equality comparison.

So, to my mind, taking a closure in .contains() is a move that would distance Rust's Option<> API from what a common programmer would immediately assume of it. I do believe we should—where possible—strive maintain a consistency with the established conventions present in existing languages.

I, for one, am a fan of some_eq or some_is semantics. This convention would closely follow existing APIs (i.e. map_ok, map_err, is_some) in their deference to the unwrappable state of the type, while additionally following relevant semantics regarding equality comparison naming conventions.

@chris-zen
Copy link

@chris-zen chris-zen commented Jan 13, 2022

I suggest looking at how other functional languages solve this same problem. In the case of Scala, the interfaces between Option, Either and other collections are quite consistent and they provide the following operation:

def exists(predicate: (B) => Boolean): Boolean

I personally read maybe_value.exists(|value| value > 5) as exists a value such that it is greater than 5.

Not willing to introduce yet another name, but to remark the importance for it to help the reader understand something like exists a value such that <predicate>.

I'd also like to stress that beyond the name that it is chosen, this feature will be really useful, and has been missing for a log time 😉

@ShadowJonathan
Copy link

@ShadowJonathan ShadowJonathan commented Jan 13, 2022

exists has not been suggested before in this thread, I'm heavily in favour of it, it alludes to a predicate much better than contains does. 👍

@avirule
Copy link

@avirule avirule commented Jan 13, 2022

I suggest looking at how other functional languages solve this same problem. In the case of Scala, the interfaces between Option, Either and other collections are quite consistent and they provide the following operation:

def exists(predicate: (B) => Boolean): Boolean

I personally read maybe_value.exists(|value| value > 5) as exists a value such that it is greater than 5.

Not willing to introduce yet another name, but to remark the importance for it to help the reader understand something like exists a value such that <predicate>.

I believe, for the same reasons .eq() doesn't take a closure, it would be unwise to do so for this API. At its heart (at least, in the spirit of the functionality I've grasped), it is just a shorthand for the specific case of a stateful equality comparison (i.e. equality reliant on the state of the container).

(Which makes me think, .is_{some/ok/err}_eq() would be the most accurate & semantic explanation of the functionality—it isn't pretty, but it's accurate).

The bikeshedding here seems quite endless, and I think it would be wise to take some of the well-liked suggestions here and perform a general poll or something. The lack of consensus on this after 2 years indicates perhaps intervention by staff is required to reach a final decision.

@soc
Copy link
Contributor

@soc soc commented Jan 13, 2022

As there seems to exist some severe confusion about the method in question, here is the signature again for everyone's convenience:

pub fn contains<U>(&self, x: &U) -> bool where U: PartialEq<T>

In the case of Scala [...] they provide the following operation:

@chris-zen Scala's exists is Rust's any (and Scala's forall is Rust's all). Scala's contains is Rust's contains.


perform a general poll or something

@avirule This tracking issue hast 69 upvotes.

@avirule
Copy link

@avirule avirule commented Jan 16, 2022

@avirule This tracking issue hast 69 upvotes.

It also has 120 comments, a large portion of which are related to bikeshedding the naming. Admittedly, I don't spend much time in issues or PRs, but it seems to me that the community agrees .contains() wouldn't align with the semantics of Option<> and Result<,>. I'm inclined to agree, given the aforementioned methods are generally used in the contexts of collections, which the two types definitely are not.

I believe, for example, it's widely seen as a mistake to have shared so many functions between Option and Iterator, as it leads to confusion (or entirely misrepresents) what happens to the type behind-the-scenes. And so, it's with that in mind that I've tried to move this issue's bikeshedding along, rather than suppress it—to me, there is a clear need to disassociate Option<> and Result<,> from collection types, such as Iter, Vec, slices, etc.

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

It also has 120 comments

The 69 upvotes each stand for a distinct person, and they all seem to generally approve of this PR.

The 120 comments on the other hand are produced by a minority that can't even agree with itself whether the problem with contains is that it is too consistent and should be made more inconsistent – or that it is too inconsistent and should be made more consistent. And that's not even taking the value-vs.-lambda bikeshedding into account.

it's widely seen as a mistake to have shared so many functions between Option and Iterator

Iterator does not provide a contains method. And even if it was, I'm not sure on what "widely seen as a mistake" is based on.

there is a clear need to disassociate Option<> and Result<,> from collection types, such as Iter, Vec, slices, etc.

I don't see that "clear need", and I can back that up with 10 years of Scala shipping with Option::contains. In that time, how many people came up to me and reported an issue with this design (not just some conjecture of what might happen)?

Not a single one.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

I came up with reported issues very early on in this discussion if people care to go back and read

@glittershark
Copy link
Contributor

@glittershark glittershark commented Jan 16, 2022

[Option and Result] definitely are not [collections]

Can you expand on why you think this is the case? Every criterion that I can imagine that qualifies a type as a collection applies to both Option and Result

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

@Rustinante And I dismissed them, because making assumptions about what might happen is not equal to a decade of actual evidence to the contrary.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

They were actual bugs, not assumptions

@ShadowJonathan
Copy link

@ShadowJonathan ShadowJonathan commented Jan 16, 2022

In that time, how many people came up to me and reported an issue with this design (not just some conjecture of what might happen)?

Not a single one.

That is not as decisive as a data point as you might think, language design is subtle, for anyone who might not be distinctly aware of it's intricacies, or any alternatives, they'd either wouldn't know which grass would be greener, or would simply have a vague feeling of "wrong", from all the small language decisions that accumulated into that sense, based on their experience with the language.

Rust especially prides itself with being correct and "understandable", this is one such instance, i think that @avirule put it right with that "we feel this is wrong", i personally am just trying to find an alternative to contains for this discussion, as, while i think it works plenty well for a "y is a box, is x is inside of it?" scenario, it doesn't feel right with a predicate.


And I dismissed them, because making assumptions about what might happen is not equal to a decade of actual evidence to the contrary.

Please keep in mind that, while your experience in Scala may be very good with applying Scala ideology to Scala problems, Rust might not fit in the same corner. Superficially, the types may look and work the same, but i think that "experience" here doesn't translate as distinctively and decisively as you make it out to be, while you may have an opinion, i don't think you have an absolute authority in this decision, and especially not one to ignore experiences from other users.

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

They were actual bugs, not assumptions

@Rustinante Could you share the links to the (at least two) bug reports we should clearly all be aware of?

it doesn't feel right with a predicate

contains does not take a predicate.

while you may have an opinion, i don't think you have an absolute authority in this decision, and especially not one to ignore experiences from other users.

@ShadowJonathan The "opinion" is backed up by a standard library shipping this design to its users for 10 years. I don't have any authority to make this decision, but I think it's fair to argue for the design the overwhelming majority agrees with. Just because the 69 people in this PR follow etiquette (by voting, instead of writing "me too" comments) shouldn't mean their position can be disregarded.

I'd love to learn more about these "experiences", but at this point people had plenty of time to come up with anything that would support it, but so far nothing has materialized in that regard. I have high hopes of @Rustinante coming through, though.

@ShadowJonathan
Copy link

@ShadowJonathan ShadowJonathan commented Jan 16, 2022

contains does not take a predicate.

A large part of the conversation has been to change fn contains(&T) to a fn contains(f: impl FnOnce(&T) -> bool), i was talking about this part.

The "opinion" is backed up by facts of a standard library shipping this design to its users for 10 years.

In Scala, this is Rust.

but I think it's fair to argue for the design the overwhelming majority seems to be perfectly fine with.

That is not what is happening here, the bikeshedding has shifted away from "contains with value" to "contains with predicate", this is not reflected in the initial post, and so i believe that (contains(&T)) is what most people are upvoting.

I'm fine with contains(&T), im not fine with contains(fn(&T) -> bool).


I'd like to see how scala handles this (Option::contains) though, but then also have some idea of how scala handles iterators and Option/Result(-esc) sum types in general, so we can get a better picture.

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

In Scala, this is Rust.

If you have more applicable experience of actual usage of such a method in the wild, then please share it.

this is not reflected in the initial post, and so i believe that is what most people are upvoting

Why would people upvote a PR about contains(<value>) if they actually wanted contains(<lambda>)?

I'm fine with contains(&T), im not fine with contains(fn(&T) -> bool).

Great! Then let's get this over with, and the people who want to have a method that takes a lambda can have their own PR and all the bike sheds they want in it.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

As a user of Rust, why should I have to think about whether what I pass to contains should include the Option and the value inside the Option, or just the value inside the option? Why don't we just use another name so that this mental gymnastics never has to be done??? This mental gymnastics is even worse with the predicate.
Can we use our common sense instead of saying Scala is without flaws?

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

As a user of Rust, why should I have to think about whether what I pass to contains should include the Option and the value inside the Option, or just the thing inside the option?

As thousands of users of Scala have shown, you don't. The problem does not exist.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

As thousands of users of Scala have shown, Rust does not need to exist.

@ShadowJonathan
Copy link

@ShadowJonathan ShadowJonathan commented Jan 16, 2022

Why would people upvote a PR about contains() if they actually wanted contains()?

Apologies, i meant that people upvote for value, not lambda, i had some confusing wording, i fixed it.

Great! Then let's get this over with, and the people who want to have a method that takes a lambda can have their own PR and all the bike sheds they want in it.

Its not that easy, @m-ou-se expressed an opinion to change it from value to lambda directly, so i don't think the current API will stabilise as-is (as easily).

As thousands of users of Scala have shown, you don't.

Since when have Scala users started thinking exactly like Rust users? And since when do they have the exact same library, community, history and process as Rust does?

Wit aside, i think that you are placing too much weight on your experience with Scala specifically to dictate other's opinions. I would like to see some examples of how Scala handles all of this, as i asked in a previous comment.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

If you just want to carbon copy the ideology of Scala into Rust, and are not interested in improving on the language, I suggest you go back and work on Scala

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

@Rustinante Please calm down, this is not a good look.

@avirule
Copy link

@avirule avirule commented Jan 16, 2022

[Option and Result] definitely are not [collections]

Can you expand on why you think this is the case? Every criterion that I can imagine that qualifies a type as a collection applies to both Option and Result

You cannot iterate Option<>s or Result<,>s, even if they can be coerced into an iterator (this is likely a convenience measure, rather than a purely design-focused choice). Additionally, the types cannot be indexed, as every collection is expected to be able to do (in some form or another).

At first glance, what's proposed here makes sense. Option<> can be thought of as a container, with either something or nothing in it. So, to say option.contains(stuff_that_goes_in_a_container) or does this container contain this value? makes sense. I'll outline, hopefully more clearly, the issues I see with this reasoning:

  • Option<> isn't a container. It's a wrapper (.unwrap()..), around a single object. Where you can put as many items as you want (or can fit) into a container, an Option<> will only ever hold 1 or None objects.
  • Similarly, given the association .contains() has with lists, this would further suggest, semantically, that Option<> could somehow hold multiple values.
  • Option<> cannot be iterated, and this is where my issues with it sharing so many methods with Iterator comes from—relevant to this discussion, however, is the desire for Option<> to not stray even further towards being treated as a collection.
  • The semantics of Option<> don't support this kind of operation, with this kind of naming. To actually use the value within an option, you're expected to somehow unwrap it, or acknowledge that it's Some() or None, i.e. .is_some(), .expect(), .map(), let Some(_) = option. These force you to implicitly understand the option is wrapping something, and what you're doing is unwrapping it.

I don't see that "clear need", and I can back that up with 10 years of Scala shipping with Option::contains. In that time, how many people came up to me and reported an issue with this design (not just some conjecture of what might happen)?

Not a single one.

I don't know if you regret writing this or not, but this is an awful argument. Now, I've never used Scala—but Rust isn't Scala. One of the core tenets of Rust is to do things right, and not make the same mistakes other languages have made.

So, to finalize: Does this, in the grand scheme of things, add up to much? Certainly, no. One method won't make or break a language. But as we've all experienced with countless languages, it's these little inconsistencies that can add up to what makes a confusing, difficult-to-grok developer experience.

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

Wit aside, i think that you are placing too much weight on your experience with Scala specifically to dictate other's opinions.

@ShadowJonathan I hope you see the utter hypocrisy in that?

I'm offering evidence to have a fact-based discussion and get criticized for it, but at the same time "opinions" need to be treated as gospel? Sorry, opinions and facts don't have the same merit.

If you don't like the evidence I provide, please bring your own evidence to the table instead of complaining that you don't like mine.

I don't know if you regret writing this or not, but this is an awful argument. Now, I've never used Scala—but Rust isn't Scala.

@avirule It's not an awful argument, it's a very good one.

It is based on learning from other languages. Pretending that Scala and Rust have nothing in common and that it's impossible to apply the lessons learned from the former to the latter is pretty anti-intellectual in my opinion.

Like it or not, Rust's Option took more inspiration from Scala's Option than from any other language (including OCaml's Option), which is why comparing it with the former makes a lot of sense.

One of the core tenets of Rust is to do things right, and not make the same mistakes other languages have made.

I could probably write a book about things-wrong-in-Scala, but having consistent naming is certainly not one of its chapters.

But as we've all experienced with countless languages, it's these little inconsistencies that can add up to what makes a confusing, difficult-to-grok developer experience.

contains as proposed is consistent with Rust's existing naming and design choices.

@Rustinante
Copy link

@Rustinante Rustinante commented Jan 16, 2022

Evidence can be either empirical, or a priori, Analytical reasoning is not the same as opinions.
I want to point out one thing about the “container” for those who still have common sense left in them. It is the “Some” entity that can be said to “contain” something (even then wraps would be a better word), not the Option entity. Calling the “contains” on the Option entity would be to skip one level of abstraction, and ignoring the None variant, which is extremely confusing.

@soc
Copy link
Contributor

@soc soc commented Jan 16, 2022

for those who still have common sense left in them

@Rustinante not sure writing things like this is helping your argument.

What would certainly help your point of view though – is providing links to the actual bugs you encountered, such that other people are also able to use their own analytical reasoning to decide how much merit your opinion holds.

Calling the “contains” on the Option entity would be to skip one level of abstraction, and ignoring the None variant, which is extremely confusing.

Perhaps your confusion comes from applying an ... unorthodox mental model? (I'm not saying it's wrong, but the way to describe it makes very little sense to me.)

@camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Jan 17, 2022

There is a lot of bickering and one-upmanship around things that have little to no consequence with regard to the goals of this thread. Please consider that sometimes the best contribution you have to offer is to allow someone else to have the last word, even if (especially if) they are "wrong".

Please don't use the laugh reaction to laugh at people when they are not being humorous. That's just inflammatory and passive-aggressive.

@jakoschiko
Copy link
Contributor

@jakoschiko jakoschiko commented Jan 17, 2022

I've learned Scala for my current job. At the beginning I was surprised that Option was treated as a container, but I got used to it very quickly. My conclusion after using it for several years now: It's convenient when writing code, but I would agree that it hurts readability. For example, I prefer this

// Rust
if let Some(result) = calculate() {
  ... // This code will be executed at most once
}

over this

// Scala
calculate().foreach { result =>
  ... // How often will this code be executed?
}

Treating Option as a container has pros and cons. If Rust has a focus on readability, it's totally valid to avoid function names used by container types.

@avirule
Copy link

@avirule avirule commented Jan 17, 2022

I don't know if you regret writing this or not, but this is an awful argument. Now, I've never used Scala—but Rust isn't Scala.

@avirule It's not an awful argument, it's a very good one.

It is based on learning from other languages. Pretending that Scala and Rust have nothing in common and that it's impossible to apply the lessons learned from the former to the latter is pretty anti-intellectual in my opinion.

Like it or not, Rust's Option took more inspiration from Scala's Option than from any other language (including OCaml's Option), which is why comparing it with the former makes a lot of sense.

One of the core tenets of Rust is to do things right, and not make the same mistakes other languages have made.

I could probably write a book about things-wrong-in-Scala, but having consistent naming is certainly not one of its chapters.

But as we've all experienced with countless languages, it's these little inconsistencies that can add up to what makes a confusing, difficult-to-grok developer experience.

contains as proposed is consistent with Rust's existing naming and design choices.

I've tried to outline my reasoning on this topic to the best of my ability, so at this point I believe anything more I have to say will just be reiteration. I'm still of the mind you've failed to address my reasoning with any substantial arguments, but I don't control this issue, and if your mind can't be changed—well, there's not much to be done about it. I support the essence of this issue (of what it seeks to achieve), even if I don't support the semantics.

For what it's worth, I wouldn't suggest throwing around your experience working on Scala as much as you do. Our pasts can inform our present and future, but they do not define them—if your ideas are to have merit, they must be able to stand on more than themselves. They will appeal to logic and reason, rather than to your own authority of the subject.

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

Successfully merging a pull request may close this issue.

None yet