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 Vec::contains_ref which is worse for inference but less restrictive than Vec::contains #42671

Open
Istvan91 opened this issue Jun 15, 2017 · 16 comments
Labels
A-collections Area: `std::collection` C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Istvan91
Copy link

Using Vec::contains currently only works if the contains parameter is of the same &Type as the Vector elements. This means the parameter must be converted to the vec-item-type even though it is comparable with the type in the first place.

e.g. following code does not work, even though impl<'a, 'b> PartialEq<str> for String exists:

fn main() {
    let mut v: Vec<String> = Vec::new();
    v.contains("mystring");
}

The Error:

error[E0308]: mismatched types
 --> <anon>:3:16
  |
3 |     v.contains("mystring");
  |                ^^^^^^^^^^ expected struct `std::string::String`, found str
  |
  = note: expected type `&std::string::String`
             found type `&'static str`

I'd be better if Vec::contains had definition like:
fn contains<U>(&self, value: &U) where T: PartialEq<U>, U: ?Sized

@kennytm
Copy link
Member

kennytm commented Jun 15, 2017

You could use v.iter().any(|x| x == "mystring") as a workaround, you don't need to convert the &str to String.

@Istvan91
Copy link
Author

I'm aware of this workaround, but isn't that what "contains" should be in the first place?

@Mark-Simulacrum Mark-Simulacrum added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 23, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 27, 2017
@kennytm
Copy link
Member

kennytm commented Jul 27, 2017

There was an attempt to fix this in #43020 but it has caused some major inference failure.

@briansmith
Copy link
Contributor

Is the alternative mentioned in #43020 (comment) any better with respect to the inference failure?

@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 15, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 15, 2017

Tagging C-feature-accepted to follow up on the approach in #43020 (comment). Please submit a PR with an implementation and tests. We will need to confirm that this causes less breakage than the original Borrow-based approach.

varkor added a commit to varkor/rust that referenced this issue Dec 22, 2017
This allows `contains` to be used on slices to search for elements that
satisfy a partial equality with the slice item type, rather than an
identity. This closes rust-lang#42671. Note that string literals need to be
referenced in this implementation due to `str` not implementing
`Sized`. It’s likely this will have to go through a Crater run to test
for breakage (see rust-lang#43020).
bors added a commit that referenced this issue Dec 27, 2017
Generalise slice::contains over PartialEq

This allows `contains` to be used on slices to search for elements that
satisfy a partial equality with the slice item type, rather than an
identity. This closes #42671. Note that string literals need to be
referenced in this implementation due to `str` not implementing
`Sized`. It’s likely this will have to go through a Crater run to test
for breakage (see #43020).
@varkor
Copy link
Member

varkor commented Jan 18, 2018

This was attempted using the other approach in #46934. Unfortunately, it too causes too much breakage in downstream crates. Even though this API change would be significantly nicer, it doesn't seem plausible unless this breakage is acceptable (maybe for a later epoch).

@varkor
Copy link
Member

varkor commented Jan 30, 2018

Note that the problems mentioned here are the same as those in #20063.

@sigod
Copy link

sigod commented Jun 6, 2019

Shouldn't this issue and a workaround be mentioned in Vec::contains documentation?

@Mark-Simulacrum Mark-Simulacrum added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 6, 2019
@Mark-Simulacrum
Copy link
Member

Relabeling as a documentation issue. contains is probably not fixable as it stands today but documenting the workaround would be good.

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Aug 13, 2019
@leo60228
Copy link
Contributor

leo60228 commented Dec 6, 2019

I'm not sure when it was added, but the docs do mention the workaround. Perhaps this issue should be closed?

@Raniz85
Copy link

Raniz85 commented Jan 23, 2020

How about adding a new method alongside contains?

Something like fn contains_ref<U>(&self, x: &U) -> bool where T: PartialEq<U>

@jonas-schievink jonas-schievink added the A-collections Area: `std::collection` label Mar 6, 2020
@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2020
@poliorcetics
Copy link
Contributor

I am willing to do the work about contains_ref but would it be accepted ? Creating it means ensuring it works forever, but if someone from the relevant team can confirm it is ok for contains_ref to be added, I will tackle it.

@p0lunin
Copy link

p0lunin commented Aug 12, 2020

Can someone review this?

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Aug 12, 2020
@Mark-Simulacrum Mark-Simulacrum changed the title Vec::contains is too restrictive Add Vec::contains_ref which is worse for inference but less restrictive than Vec::contains Aug 12, 2020
@stephanemagnenat
Copy link

stephanemagnenat commented Oct 20, 2022

We just stumbled upon this problem in our project, which is kind of frustrating and makes the code ugly and less readable. Could the Rust edition mechanism (or similar) be used for doing some cleanup in the standard library? I understand that fully supporting programs using two different stdlib editions might requires having two versions of the standard library in memory sharing data together, but in cases of enhancements like this there might be a way for third-party packages to be tested against new versions and declare compatibility with these versions (a program could then be compiled with a newer version only if all its dependencies declare compatibility with that version), progressively allowing the ecosystem to use the better interfaces.

@tkr-sh
Copy link

tkr-sh commented Sep 14, 2024

Any update ?

@kennytm
Copy link
Member

kennytm commented Sep 14, 2024

If someone wants a Vec::contains_ref method in std consider filing an ACP to the libs-team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet