Adding the has_item function to the iterator trait. #127604

yonikremer · 2024-07-11T09:10:34Z

Refrences

the tracking issue and the feature proposal.

Here is my implementation, including the doc string and tests

…Self::Item

…in a &str iterator

… trait

…here Self::Item implement Self::Item == arg

rustbot · 2024-07-11T09:10:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-07-11T09:10:43Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Veykril · 2024-07-11T09:23:08Z

src/tools/rust-analyzer/crates/ide-db/src/generated/lints.rs

+"##,
+    },
+    Lint {
+        label: "contains",
+        description: r##"# `contains`
+
+The tracking issue for this feature is: [#94047]
+
+[#94047]: https://github.com/rust-lang/rust/issues/94047
+
+


This file is autogenerated so no need for adding this (its also adding it one slot too late)

@yonikremer see this comment too, you can revert this file

tgross35 · 2024-07-11T09:55:09Z

library/core/Cargo.toml

+# Adds a function to check if an element is contained in an iterator
+contains = []


Library features don't need to be listed here, this is just for Cargo features that get exposed during build-std

tgross35 · 2024-07-11T09:56:15Z

library/core/src/iter/traits/iterator.rs


-    /// Advances the iterator and returns an array containing the next `N` values.
+    /// Advances the iterator and returns an array has_iteming the next `N` values.
    ///
    /// If there are not enough elements to fill the array then `Err` is returned
-    /// containing an iterator over the remaining elements.
+    /// has_iteming an iterator over the remaining elements.


Looks like maybe you did find+replace on contain? Change looks accidental (more of these below, please double check the diff)

tgross35 · 2024-07-11T09:56:41Z

library/core/src/iter/traits/iterator.rs

    /// The returned iterator is a prefix of length `n` if the original iterator
-    /// contains at least `n` elements, otherwise it contains all of the
+    /// has_items at least `n` elements, otherwise it has_items all of the


tgross35 · 2024-07-11T09:56:47Z

library/core/src/iter/traits/iterator.rs


-    /// Converts an iterator of pairs into a pair of containers.
+    /// Converts an iterator of pairs into a pair of has_itemers.


Lol, this one is kind of funny

tgross35 · 2024-07-11T09:58:41Z

library/core/src/iter/traits/iterator.rs

+    /// Example:
+    /// ```
+    /// #![feature(iter_has_item)]
+    /// assert!(![1i32, 2i32, 3i32].iter().has_item(&4i32));
+    /// assert!([Some(2i32), Option::<i32>::None].iter().has_item(&None));
+    /// assert!([Some(2i32), Option::<i32>::None].iter().has_item(&Some(2i32)));
+    /// assert!(!Vec::<i32>::new().iter().has_item(&1i32));
+    /// assert!([1i32, 2i32, 2i32, 3i32].iter().has_item(&2i32));
+    /// #[derive(PartialEq)]
+    /// struct Item {
+    ///     value: i32,
+    /// }
+    /// assert!([Item { value: 1i32 }, Item { value: 2i32 }].iter().has_item(&Item { value: 2i32 }));
+    /// assert!(["a", "b", "c"].iter().has_item(&"b".to_owned()));
+    /// assert!(!["a", "b", "c"].iter().has_item(&"d".to_owned()));
+    /// assert!(["a", "b", "c"].iter().has_item(&"b"));
+    /// assert!(!["a", "b", "c"].iter().has_item(&"d"));
+    /// assert!(["a".to_owned(), "b".to_owned(), "c".to_owned()].iter().has_item(&"b"));
+    /// assert!(!["a".to_owned(), "b".to_owned(), "c".to_owned()].iter().has_item(&"d"));
+    /// assert!((1..1000).has_item(500i32));
+    /// ```


As mentioned at the ACP, make the doc example something small and easy to understand. More complete testing only needs to live in unit/integration tests.

tgross35 · 2024-07-11T10:01:41Z

library/core/src/iter/traits/iterator.rs

+    /// Checks if the Iterator has a value.
+    /// 'has_items' is short-circuiting; in other words, it will stop processing
+    /// as soon as the function finds the item in the Iterator.


Suggested change

/// Checks if the Iterator has a value.

/// 'has_items' is short-circuiting; in other words, it will stop processing

/// as soon as the function finds the item in the Iterator.

/// Returns `true` if the iterator contains a value.

///

/// 'has_items' is short-circuiting; in other words, it will stop processing

/// as soon as the function finds the item in the `Iterator`.

Made the summary line more consistent with slice::contains/HashSet::contains. Also remember that a line break is needed between the summary and the content (otherwise the whole thing gets treated as a summary line).

tgross35 · 2024-07-11T10:02:52Z

library/core/src/iter/traits/iterator.rs

+    #[unstable(feature = "iter_has_item", reason = "new API", issue = "127494")]
+    fn has_item<Q: ?Sized>(&mut self, item: Q) -> bool


From #127494, this can be renamed back to contains

…to use the function contains of the Iterator trait

yonikremer · 2024-07-11T12:56:29Z

@tgross35 I have updated the pull request. Now it breaks external libraries like ide-assist.
It should be fixed by Supertrait item shadowing. Is there a way to activate both features and see if everything works as intended?

yonikremer · 2024-07-11T13:51:51Z

There is a pull request for Super trait shadowing here. It just needs someone to review it.

juntyr · 2024-07-11T13:51:14Z

library/core/src/iter/traits/iterator.rs

@@ -1327,7 +1327,7 @@ pub trait Iterator {
    /// `take(n)` yields elements until `n` elements are yielded or the end of
    /// the iterator is reached (whichever happens first).
    /// The returned iterator is a prefix of length `n` if the original iterator
-    /// contains at least `n` elements, otherwise it contains all of the
+    /// contains at least `n` elements, otherwise it containss all of the


Suggested change

/// contains at least `n` elements, otherwise it containss all of the

/// contains at least `n` elements, otherwise it contains all of the

library/core/src/iter/traits/iterator.rs

Co-authored-by: Juniper Tyree <[email protected]>

tgross35 · 2024-07-11T16:42:47Z

src/tools/rust-analyzer/crates/ide-db/src/generated/lints.rs

+"##,
+    },
+    Lint {
+        label: "contains",
+        description: r##"# `contains`
+
+The tracking issue for this feature is: [#94047]
+
+[#94047]: https://github.com/rust-lang/rust/issues/94047
+
+


@yonikremer see this comment too, you can revert this file

tgross35 · 2024-07-11T17:33:54Z

library/core/src/iter/traits/iterator.rs

+    /// Performance:
+    /// This method checks the whole iterator, which takes O(n) time.
+    /// If the iterator is sorted, or a hash map please use the appropriate method instead.


Suggested change

/// Performance:

/// This method checks the whole iterator, which takes O(n) time.

/// If the iterator is sorted, or a hash map please use the appropriate method instead.

/// This method checks the whole iterator, which is O(n). If the iterator is a sorted

/// slice, [`binary_search`](slice::binary_search) may be faster. If this is an iterator

/// on collections that have a `.contains()` or `.contains_key()` method (such as

/// [`HashMap`] or [`BTreeSet`]), using those methods directly will be faster.

We can be more specific about what the appropriate methods are here. No section heading needed, I think rustdoc would have just merged Performance: into the next paragraph anyway.

You can check the rendering with ./x doc library/core --open

tgross35 · 2024-07-11T17:36:43Z

library/core/tests/iter/traits/iterator.rs

+#[test]
+fn test_happy_path_item_not_in_iterator() {
+    assert!(![1i32, 2i32, 3i32].iter().contains(&4i32));
+}
+
+#[test]
+fn test_edge_case_handling_none_values() {
+    assert!([Some(2i32), Option::<i32>::None].iter().contains(&None));
+    assert!([Some(2i32), Option::<i32>::None].iter().contains(&Some(2i32)));
+}
+
+#[test]
+fn test_edge_case_handling_empty_iterator() {
+    assert!(!Vec::<i32>::new().iter().contains(&1i32));
+}
+
+#[test]
+fn test_edge_case_handling_iterator_with_duplicates() {
+    assert!([1i32, 2i32, 2i32, 3i32].iter().contains(&2i32));
+}
+
+#[test]
+/// Tests that short-circuiting works correctly when using `contains`
+/// When you run the function, it should move the iterator forward after the first appearance of the item
+fn test_short_circuiting() {
+    let vector: Vec<i32> = vec![1i32, 2i32, 3i32, 1i32, 1i32];
+    let mut iterator = vector.into_iter();
+    assert!(iterator.contains(1i32));
+    assert_eq!(iterator.next(), Some(2));
+    assert!(!iterator.contains(4i32));
+    assert_eq!(iterator.next(), None);
+}
+
+#[test]
+fn test_edge_case_handling_iterator_with_custom_struct() {


Nit: since these tests only check .contains(), they should have test_contains in the name

tgross35 · 2024-07-11T17:39:46Z

library/core/src/iter/traits/iterator.rs

+    /// assert!([1, 2, 3].iter().contains(&1));
+    /// assert!(![1, 2, 3].iter().contains(&4));
+    /// // You can check if a String is in a string slice iterator.
+    /// assert!(["a", "b", "c"].iter().contains(&"b".to_owned()));
+    /// // You can also check if a String iterator contains a string slice.
+    /// assert!(["a".to_owned(), "b".to_owned(), "c".to_owned()].iter().contains(&"b"));


Maybe make it an example that you couldn't just use slice::contains(...) on, like [1, 2, 3].iter().map(|x| x * 3).contains(6)

tgross35 · 2024-07-11T17:42:15Z

tests/ui/suggestions/deref-path-method.rs

 fn main() {
    let vec = Vec::new();
    Vec::contains(&vec, &0);
-    //~^ ERROR no function or associated item named `contains` found for struct `Vec<_, _>` in the current scope
+    //~^ ERROR `Vec<_, _>` is not an iterator [E0599]
    //~| HELP the function `contains` is implemented on `[_]`
 }


Bit of an unfortunate suggestion change for an unstable method. This was added in #100302, cc @compiler-errors since this will no longer test what it was intended to.

pls file a bug, we probably can filter by stability or sth idk

compiler-errors · 2024-07-11T17:55:14Z

also please make sure this commit history gets squashed before approval -- 27 commits is a bit excessive, especially when a lot of the commits are removing/renaming and don't add much valuable information to the commit history lol

tgross35 · 2024-07-11T17:56:30Z

@tgross35 I have updated the pull request. Now it breaks external libraries like ide-assist. It should be fixed by Supertrait item shadowing. Is there a way to activate both features and see if everything works as intended?

That PR is a draft so it is not ready yet. That feature will fix the conflicts with itertools on stable features, there shouldn't be any conflict for unstable features. But it will raise the unstable_name_collisions, as seen in the RA build failures (you can fix these either by using the suggested fully qualified syntax, or with #[allow(unstable_name_collisions)]).

You can try it locally to see the interactions. After you build you can do rustup toolchain link stage1 ... (see https://rustc-dev-guide.rust-lang.org/building/quickstart.html), and then cargo +stage1 ... in a test crate to use it.

rustbot · 2024-07-12T12:57:14Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

d8fc920

rust-log-analyzer · 2024-07-12T13:10:14Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#12 [ 5/11] RUN npm install [email protected] [email protected] -g
#12 5.037 npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
#12 5.103 npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
#12 5.106 npm WARN deprecated [email protected]: Rimraf versions prior to v4 are no longer supported
#12 5.166 npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/config-array instead
#12 5.190 npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported
#12 5.207 npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/object-schema instead
#12 6.367 
#12 6.367 added 205 packages in 6s
#12 6.367 
#12 6.368 20 packages are looking for funding
---
#16 3.568 Building wheels for collected packages: reuse
#16 3.569   Building wheel for reuse (pyproject.toml): started
#16 3.915   Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 3.916   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#16 3.916   Stored in directory: /tmp/pip-ephem-wheel-cache-fnplqin3/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#16 3.919 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#16 3.942   Attempting uninstall: setuptools
#16 3.942     Found existing installation: setuptools 59.6.0
#16 3.943     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
    Checking ide-assists v0.0.0 (/checkout/src/tools/rust-analyzer/crates/ide-assists)
error: a method with this name may be added to the standard library in the future
   --> crates/ide-assists/src/handlers/bool_to_enum.rs:372:48
    |
372 |     if !bin_expr.lhs()?.syntax().descendants().contains(name.syntax()) {
    |
    = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
    = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
    = help: call with fully qualified syntax `itertools::Itertools::contains(...)` to keep using the current method
    = help: call with fully qualified syntax `itertools::Itertools::contains(...)` to keep using the current method
    = note: `-D unstable-name-collisions` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unstable_name_collisions)]`
help: add `#![feature(iter_contains)]` to the crate attributes to enable `std::iter::Iterator::contains`
   --> crates/ide-assists/src/lib.rs:63:1
    |
63  + #![feature(iter_contains)]

error: a method with this name may be added to the standard library in the future
   --> crates/ide-assists/src/handlers/bool_to_enum.rs:438:41
    |
    |
438 |     if !receiver.syntax().descendants().contains(name.syntax()) {
    |
    = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
    = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
    = help: call with fully qualified syntax `itertools::Itertools::contains(...)` to keep using the current method
    = help: call with fully qualified syntax `itertools::Itertools::contains(...)` to keep using the current method
help: add `#![feature(iter_contains)]` to the crate attributes to enable `std::iter::Iterator::contains`
   --> crates/ide-assists/src/lib.rs:63:1
    |
63  + #![feature(iter_contains)]

    Checking ide-diagnostics v0.0.0 (/checkout/src/tools/rust-analyzer/crates/ide-diagnostics)
    Checking load-cargo v0.0.0 (/checkout/src/tools/rust-analyzer/crates/load-cargo)
error: could not compile `ide-assists` (lib) due to 2 previous errors

bors · 2024-07-28T21:17:53Z

☔ The latest upstream changes (presumably #128313) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2024-08-21T06:35:58Z

@yonikremer this still has a merge confict that needs to be removed and pr rebased over in order for this to proceed. If you need help doing this let us know

yonikremer added 18 commits July 9, 2024 00:19

implemented the contains feature

c068fdc

the function contains can now get every type that can be borrowed as …

c143b42

…Self::Item

added tests for contain

7931998

formatted the code

fd40ecb

fixed failing tests

5372e4b

using the feature contains

82d481d

added the tests for the "contains" feature

15d6c9f

added a test for checking that the function can check if a String is …

48616ce

…in a &str iterator

moved tests for the contains feature to the test file of the Iterator…

7ba6c5e

… trait

changed function signature according to the rust library team request

b69db01

removed tests from function doc string

3751b5d

getting arguments that implement arg == Self::Item and not argument w…

7ebf719

…here Self::Item implement Self::Item == arg

rust formatting: deleted 2 empty lines

ac8d1e3

removed unneeded feature specification

cd839b8

renamed contain to has_item

66c55b2

renamed feature from has_item to iter_has_item

b67ee44

added a test for the short-circuiting behaviour

1ba1ca7

fixed test_short_circuiting

934a194

rustbot assigned Amanieu Jul 11, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 11, 2024

This comment has been minimized.

Sign in to view

Veykril reviewed Jul 11, 2024

View reviewed changes

tgross35 reviewed Jul 11, 2024

View reviewed changes

yonikremer added 4 commits July 11, 2024 13:28

fixed test_short_circuiting

3e3ccd8

renamed feature and function back to contains

bc20afd

changed excepted compiler suggestion. Now the compiler is suggesting …

8a7c12b

…to use the function contains of the Iterator trait

deleted a test that uses the function contains of a range

dac6cd7

yonikremer added 4 commits July 11, 2024 14:18

deleted a test that uses the function contains of a range

2a32a8c

removed the contains feature from library/core/Cargo.toml

ba5b980

made the doc tests small and simple

e70effd

made the docs more consistent with similar methods of other types

82b6b10

This comment has been minimized.

Sign in to view

Amanieu mentioned this pull request Jul 11, 2024

Supertrait item shadowing v2 rust-lang/rfcs#3624

Open

juntyr reviewed Jul 11, 2024

View reviewed changes

fixed typos

2f72670

Co-authored-by: Juniper Tyree <[email protected]>

This comment has been minimized.

Sign in to view

tgross35 reviewed Jul 11, 2024

View reviewed changes

yonikremer added 2 commits July 12, 2024 15:55

improved doc example

fb98975

Merge remote-tracking branch 'origin/master'

d8fc920

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the has_item function to the iterator trait. #127604

Adding the has_item function to the iterator trait. #127604

yonikremer commented Jul 11, 2024

rustbot commented Jul 11, 2024

rustbot commented Jul 11, 2024

This comment has been minimized.

Veykril Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024 •

edited

Loading

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024 •

edited

Loading

This comment has been minimized.

yonikremer commented Jul 11, 2024

yonikremer commented Jul 11, 2024

juntyr Jul 11, 2024

This comment has been minimized.

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

tgross35 Jul 11, 2024

compiler-errors Jul 11, 2024

compiler-errors commented Jul 11, 2024

tgross35 commented Jul 11, 2024

rustbot commented Jul 12, 2024

rust-log-analyzer commented Jul 12, 2024

bors commented Jul 28, 2024

Dylan-DPC commented Aug 21, 2024

		# Adds a function to check if an element is contained in an iterator
		contains = []


		/// Converts an iterator of pairs into a pair of containers.
		/// Converts an iterator of pairs into a pair of has_itemers.

		#[unstable(feature = "iter_has_item", reason = "new API", issue = "127494")]
		fn has_item<Q: ?Sized>(&mut self, item: Q) -> bool

	/// contains at least `n` elements, otherwise it containss all of the
	/// contains at least `n` elements, otherwise it contains all of the

-    /// Performance:
-    /// This method checks the whole iterator, which takes O(n) time.
-    /// If the iterator is sorted, or a hash map please use the appropriate method instead.
+    /// This method checks the whole iterator, which is O(n). If the iterator is a sorted
+    /// slice, [`binary_search`](slice::binary_search) may be faster. If this is an iterator
+    /// on collections that have a `.contains()` or `.contains_key()` method (such as
+    /// [`HashMap`] or [`BTreeSet`]), using those methods directly will be faster.

Adding the has_item function to the iterator trait. #127604

Are you sure you want to change the base?

Adding the has_item function to the iterator trait. #127604

Conversation

yonikremer commented Jul 11, 2024

Refrences

rustbot commented Jul 11, 2024

rustbot commented Jul 11, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

yonikremer commented Jul 11, 2024

yonikremer commented Jul 11, 2024

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Jul 11, 2024

tgross35 commented Jul 11, 2024

rustbot commented Jul 12, 2024

rust-log-analyzer commented Jul 12, 2024

bors commented Jul 28, 2024

Dylan-DPC commented Aug 21, 2024

tgross35 Jul 11, 2024 •

edited

Loading

tgross35 Jul 11, 2024 •

edited

Loading