Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add is_empty() to Iterator #90676

Closed

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Nov 7, 2021

This is a draft PR as an alternative to #35428.

The original feature was blocked because we felt that it shouldn't be specific to ExactSizeIterator, as some iterators can know whether they are empty or not but do not have an exact size e.g. Filter.

I didn't see any proposal looking exactly like this. @scottmcm created an experiment that separated it to its own trait, but the implementation seemed too restrictive to me, and adding another trait might not be worth it for a single method: functions using Iterators should not require the iterator to know whether it is empty or not, it should already be able to tell using the size hint.

cc @SimonSapin

EDIT: Let's discuss this change before me fixing errors. I could also just remove ExactSizeIterator::is_empty() in this PR.

@fee1-dead fee1-dead added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-iterators Area: Iterators labels Nov 7, 2021
@rust-highfive
Copy link
Contributor

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2021
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling memchr v2.4.1
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.49
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0541]: unknown meta item 'feaure'
    |
    |
186 |     #[unstable(feaure = "iter_is_empty", reason = "recently added", issue = "0")]
    |                ^^^^^^^^^^^^^^^^^^^^^^^^ expected one of `feature`, `reason`, `issue`, `soft`
error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/cloned.rs:112:17
    |
    |
112 |         self.it.is_empty()
    |                 ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
112 |         ExactSizeIterator::is_empty(self.it)
help: disambiguate the associated function for candidate #2
    |
    |
112 |         Iterator::is_empty(self.it)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/copied.rs:138:17
    |
    |
138 |         self.it.is_empty()
    |                 ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
138 |         ExactSizeIterator::is_empty(self.it)
help: disambiguate the associated function for candidate #2
    |
    |
138 |         Iterator::is_empty(self.it)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/enumerate.rs:228:19
    |
    |
228 |         self.iter.is_empty()
    |                   ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
228 |         ExactSizeIterator::is_empty(self.iter)
help: disambiguate the associated function for candidate #2
    |
    |
228 |         Iterator::is_empty(self.iter)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/fuse.rs:206:36
    |
    |
206 |             Some(ref iter) => iter.is_empty(),
    |                                    ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
206 |             Some(ref iter) => ExactSizeIterator::is_empty(&iter),
help: disambiguate the associated function for candidate #2
    |
    |
206 |             Some(ref iter) => Iterator::is_empty(&iter),

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/inspect.rs:144:19
    |
    |
144 |         self.iter.is_empty()
    |                   ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
144 |         ExactSizeIterator::is_empty(self.iter)
help: disambiguate the associated function for candidate #2
    |
    |
144 |         Iterator::is_empty(self.iter)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/map.rs:175:19
    |
    |
175 |         self.iter.is_empty()
    |                   ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
175 |         ExactSizeIterator::is_empty(self.iter)
help: disambiguate the associated function for candidate #2
    |
    |
175 |         Iterator::is_empty(self.iter)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/adapters/rev.rs:129:19
    |
    |
129 |         self.iter.is_empty()
    |                   ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
129 |         ExactSizeIterator::is_empty(self.iter)
help: disambiguate the associated function for candidate #2
    |
    |
129 |         Iterator::is_empty(self.iter)

error[E0034]: multiple applicable items in scope
   --> library/core/src/iter/traits/exact_size.rs:145:18
    |
    |
145 |         (**self).is_empty()
    |                  ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in the trait `ExactSizeIterator`
    |
    |
134 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `Iterator`
    |
    |
187 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
    |
145 |         ExactSizeIterator::is_empty((**self))
help: disambiguate the associated function for candidate #2
    |
    |
145 |         Iterator::is_empty((**self))

error[E0034]: multiple applicable items in scope
    --> library/core/src/slice/iter.rs:2225:19
     |
     |
2225 |         self.iter.is_empty()
     |                   ^^^^^^^^ multiple `is_empty` found
     |
note: candidate #1 is defined in an impl of the trait `ExactSizeIterator` for the type `slice::iter::Iter<'_, T>`
     |
41   |  / macro_rules! iterator {
42   |  |     (
42   |  |     (
43   |  |         struct $name:ident -> $ptr:ty,
44   |  |         $elem:ty,
...     |
124  |  |             fn is_empty(&self) -> bool {
...     |
397  |  |     }
398  |  | }
     |  |_- in this expansion of `iterator!`
     |  |_- in this expansion of `iterator!`
     |
    ::: library/core/src/slice/iter.rs:134:1
     |
134  | /  iterator! {struct Iter -> *const T, &'a T, const, {/* no mut */}, {
135  | |      fn is_sorted_by<F>(self, mut compare: F) -> bool
136  | |      where
137  | |          Self: Sized,
143  | |      }
144  | |  }}
     | |___- in this macro invocation
     | |___- in this macro invocation
note: candidate #2 is defined in an impl of the trait `Iterator` for the type `slice::iter::Iter<'a, T>`
     |
     |
187  |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
     |
     |
2225 |         ExactSizeIterator::is_empty(&self.iter)
help: disambiguate the associated function for candidate #2
     |
     |
2225 |         Iterator::is_empty(&self.iter)

error[E0034]: multiple applicable items in scope
    --> library/core/src/slice/iter.rs:2343:19
     |
     |
2343 |         self.iter.is_empty()
     |                   ^^^^^^^^ multiple `is_empty` found
     |
note: candidate #1 is defined in an impl of the trait `ExactSizeIterator` for the type `slice::iter::IterMut<'_, T>`
     |
41   | / macro_rules! iterator {
42   | |     (
42   | |     (
43   | |         struct $name:ident -> $ptr:ty,
44   | |         $elem:ty,
...    |
124  | |             fn is_empty(&self) -> bool {
...    |
397  | |     }
398  | | }
     | |_- in this expansion of `iterator!`
     | |_- in this expansion of `iterator!`
     |
    ::: library/core/src/slice/iter.rs:316:1
     |
316  |   iterator! {struct IterMut -> *mut T, &'a mut T, mut, {mut}, {}}
     |   --------------------------------------------------------------- in this macro invocation
note: candidate #2 is defined in an impl of the trait `Iterator` for the type `slice::iter::IterMut<'a, T>`
     |
     |
187  |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
     |
     |
2343 |         ExactSizeIterator::is_empty(&self.iter)
help: disambiguate the associated function for candidate #2
     |
     |
2343 |         Iterator::is_empty(&self.iter)

error[E0034]: multiple applicable items in scope
   --> library/core/src/str/iter.rs:342:16
    |
    |
342 |         self.0.is_empty()
    |                ^^^^^^^^ multiple `is_empty` found
    |
note: candidate #1 is defined in an impl of the trait `Iterator` for the type `Copied<I>`
    |
    |
187 |     fn is_empty(&self) -> bool {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #2 is defined in an impl of the trait `ExactSizeIterator` for the type `Copied<I>`
   --> library/core/src/iter/adapters/copied.rs:137:5
    |
137 |     fn is_empty(&self) -> bool {
help: disambiguate the associated function for candidate #1
    |
342 |         Iterator::is_empty(&self.0)
    |

#[inline]
#[unstable(feaure = "iter_is_empty", reason = "recently added", issue = "0")]
fn is_empty(&self) -> bool {
(0, Some(0)) == self.size_hint()
Copy link
Member

@scottmcm scottmcm Nov 7, 2021

Choose a reason for hiding this comment

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

There's no requirement that the size_hint be this precise (unless it's ExactSizeIterator or TrustedLen), so I don't think this implementation is ok. For example, this won't work with from_fn: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0fce510cb4a02171b289d6d3bd0e8808

This would either need to be -> Option<bool> (so it can return None if the size_hint is inconclusive), or be on a different trait (as you mention).

Copy link
Member Author

@fee1-dead fee1-dead Nov 8, 2021

Choose a reason for hiding this comment

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

from_fn obviously doesn't work, but I don't see why this is bad. This method is not always correct when it returns false (return value of false does not mean the iterator has items), but it should be correct when it returns true.

Copy link
Member

Choose a reason for hiding this comment

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

That's not what I'd expect it to do, at least. I'd expect it, since Rust has all the type system machinery it needs to do this, to only compile when it works -- same as .len() isn't "well, here's what it might be".

Someone who wants unreliable checks can always just do .size_hint().0 > 0 or .size_hint().1 == Some(0), depending on which direction they want; I don't think convenience methods for that are appropriate.

Copy link
Member Author

@fee1-dead fee1-dead Nov 8, 2021

Choose a reason for hiding this comment

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

ESI is a safe trait, and to me len() is also "here's what it might be" since we can't say not reporting accurate size is UB. is_empty doesn't have to reside in another trait to me, but we could add a trait for users to require iterators to produce accurate is_empty results, like FusedIterator.

Copy link
Member

Choose a reason for hiding this comment

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

ESI is a safe trait, and to me len() is also "here's what it might be" since we can't say not reporting accurate size is UB.

UB is not the only kind of forbidden behavior. Safe traits can also make promises where breaking these promises is a bug that can cascade into further downstream misbehavior such as infinite loops, panics, leaks and other API contracts being broken which easily can result in data loss or miscomputations. Sure, it's not a buffer overflow or double-free but that's little consolation when your blob storage containing petabytes of critical data suddenly become inaccessible, your account balance gets miscomputed or an airliner has an intersection event with a mountain.

So from the perspective of safe code ExactSizeIterator::len is guaranteed to be correct and it is free to operate on that assumption for all T. B is correct if A is correct.

is_empty here does not meet this level of conditional correctness and would have to be weakened to the level of uselessness because it would have to explicitly mention something like

For some iterator types this function may always return false even when they are empty.

Which renders it useless as a trait method, because then people would have to reason about concrete types rather than the trait.

@the8472
Copy link
Member

the8472 commented Nov 18, 2021

it should already be able to tell using the size hint.

The size hint is not sufficient since returning (0, None) is a valid and in fact the default implementation for it. If we get generator-based iterators in the future than computing a better size hint than that may be equivalent to solving the halting problem.
So for a vast class of iterators this method would always return false even when they are empty.

@fee1-dead fee1-dead closed this Nov 18, 2021
@fee1-dead fee1-dead deleted the add-iter_is_empty branch November 18, 2021 03:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-iterators Area: Iterators S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

6 participants