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 experimental Iterator::contains #135018

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leb-kuchen
Copy link

tracking issue
This is the continuation of the abandoned PR.

@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joboet (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 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 Jan 2, 2025
@rust-log-analyzer

This comment has been minimized.

@leb-kuchen leb-kuchen force-pushed the iter-contains branch 4 times, most recently from e1b2669 to 35193cb Compare January 2, 2025 11:06
@rust-log-analyzer

This comment has been minimized.

@@ -289,6 +289,16 @@ macro_rules! iterator {
false
}

fn contains<Q: ?Sized>(&mut self, item: Q) -> bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this reimplemented when it uses the same code as the default impl?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to use SliceContains as a optimization later, but the trait has different trait bounds, so this requires further changes to the trait.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't add an empty specialization like this, either actually specialize it or leave it out for now.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -2770,6 +2770,39 @@ pub trait Iterator {
self.try_fold((), check(f)) == ControlFlow::Break(())
}

/// Returns `true` if the iterator contains a value.
Copy link
Member

@Noratrieb Noratrieb Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with other methods

Suggested change
/// Returns `true` if the iterator contains a value.
/// Tests whether a value is contained in the iterator.

@rust-log-analyzer

This comment has been minimized.

@leb-kuchen
Copy link
Author

There is the itertools breakage with rust analyzer and the UI tests have still been failing.

@rust-log-analyzer
Copy link
Collaborator

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)
    Checking load-cargo v0.0.0 (/checkout/src/tools/rust-analyzer/crates/load-cargo)
error: a method with this name may be added to the standard library in the future
   --> src/tools/rust-analyzer/crates/ide-assists/src/handlers/bool_to_enum.rs:384:48
    |
384 |     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`
   --> src/tools/rust-analyzer/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
   --> src/tools/rust-analyzer/crates/ide-assists/src/handlers/bool_to_enum.rs:450:41
    |
    |
450 |     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`
   --> src/tools/rust-analyzer/crates/ide-assists/src/lib.rs:63:1
    |
63  + #![feature(iter_contains)]

error: could not compile `ide-assists` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `ide-assists` (lib test) due to 2 previous errors

Comment on lines +2780 to +2781
/// on collections that have a `.contains()` or `.contains_key()` method (such as
/// `HashMap` or `BtreeSet`, using those methods directly will be faster.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// on collections that have a `.contains()` or `.contains_key()` method (such as
/// `HashMap` or `BtreeSet`, using those methods directly will be faster.
/// on collections that have a `.contains()` or `.contains_key()` method (such as
/// `HashMap` or `BtreeSet`), using those methods directly will be faster.

@joboet
Copy link
Member

joboet commented Jan 3, 2025

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 3, 2025
@rustbot rustbot assigned Amanieu and unassigned joboet Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants