Skip to content

Add Default impls for iterators and adapters #77

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

Closed
the8472 opened this issue Jul 25, 2022 · 7 comments
Closed

Add Default impls for iterators and adapters #77

the8472 opened this issue Jul 25, 2022 · 7 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@the8472
Copy link
Member

the8472 commented Jul 25, 2022

Proposal

Problem statement

Currently many iterators don't implement Default which means one can't derive(Default) structs containing them, mem::take() or Cell::take them.

Motivation, use-cases

https://github.com/rust-lang/rust/blob/dc2d232c7485c60dd856f8b9aee83426492d4661/library/alloc/src/vec/drain.rs#L129

        let iter = mem::replace(&mut self.iter, (&mut []).iter());

could be replaced with

        let iter = mem::take(&mut self.iter);

Solution sketches

Iterator sources should implement Default when it is unambiguous that the default should be an empty iterator. Examples:

  • slice::Iter{Mut}
  • vec::IntoIter
  • iter::Empty (already implements it)
  • btree_map::Iter
  • etc.

Adapters should implement it when the inner iterator does and when they don't have any closure that would have to be materialized

  • Skip<I> where I: Default
  • Flatten<I> where I: Default
  • Chain<A, B> where A: Default, B: Default

Examples where it should not be implemented

  • array::IntoIter since one could expect [T; N]::default().into_iter() where T: Default
  • iter::Once since one could expect iter::once(Default::default())
  • adapters::Map since it would require summoning an FnMut ex nihilo

Links and related work

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@the8472 the8472 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 25, 2022
@scottmcm
Copy link
Member

Iterator sources should [...]

That sounds to me like this should perhaps be an RFC, since it substantially effects ecosystem crates too. (For example, https://docs.rs/ndarray/latest/ndarray/iter/struct.ExactChunksIterMut.html and https://docs.rs/itertools/latest/itertools/structs/struct.TupleWindows.html don't implement Default today, so accepting this is a de-facto "please send PRs adding Default to all these crates".)

But we'll see what libs-api says.

@the8472
Copy link
Member Author

the8472 commented Jul 25, 2022

so accepting this is a de-facto "please send PRs adding Default to all these crates".

Eh. The "should" is more a recommendation to improve ergonomics, not as a requirement. Just as one "should" implement a size_hint better than the default if possible, but the TupleWindows you linked does not for example. Not every crate has to meet std's QoI standards, the "should" was meant for std.

@yaahc
Copy link
Member

yaahc commented Jul 26, 2022

This one's insta-stable (right?) so it will need an FCP but my preliminary reaction is that this seems like a good idea.

Seconded

@jdahlstrom
Copy link

I think the general rule of thumb should be that if an iterator type has a reasonable invariant of yielding at least a single element, its Default, if any, must not be empty. Ie. any array::IntoIter<_; N> instance must yield exactly N items; Repeat must never yield None, and transitively for adapters.

@the8472
Copy link
Member Author

the8472 commented Jul 30, 2022

Imo that's just one possible interpretation what Default would mean for array::IntoIter. Another possible meaning is to return an empty one. Due to that ambiguity I wouldn't implement it on those.

@jdahlstrom
Copy link

jdahlstrom commented Jul 30, 2022

Yes, I agree that it's better to leave it unimplemented if there's any ambiguity. I just meant that IMHO it's better to not impl Default at all, rather than to impl an empty Default, for any iterator type whose "normal" constructor has an (explicit or implicit) postcondition on how many items it yields before None.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 25, 2023
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
github-actions bot pushed a commit to eggyal/copse that referenced this issue Apr 2, 2023
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
oli-obk pushed a commit to oli-obk/miri that referenced this issue Apr 4, 2023
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
@joshtriplett joshtriplett added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label May 17, 2023
@the8472
Copy link
Member Author

the8472 commented May 17, 2023

Has been implemented

@the8472 the8472 closed this as completed May 17, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jun 1, 2023
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Implement Default for some alloc/core iterators

Add `Default` impls to the following collection iterators:

* slice::{Iter, IterMut}
* binary_heap::IntoIter
* btree::map::{Iter, IterMut, Keys, Values, Range, IntoIter, IntoKeys, IntoValues}
* btree::set::{Iter, IntoIter, Range}
* linked_list::IntoIter
* vec::IntoIter

and these adapters:

* adapters::{Chain, Cloned, Copied, Rev, Enumerate, Flatten, Fuse, Rev}

For iterators which are generic over allocators it only implements it for the global allocator because we can't conjure an allocator from nothing or would have to turn the allocator field into an `Option` just for this change.

These changes will be insta-stable.

ACP: rust-lang/libs-team#77
bors added a commit to rust-lang-ci/rust that referenced this issue May 15, 2025
Change `core::iter::Fuse`'s `Default` impl to do what its docs say it does

The [docs on `impl<I: Default> Default for core::iter::Fuse<I>`](https://doc.rust-lang.org/nightly/std/iter/struct.Fuse.html#impl-Default-for-Fuse%3CI%3E) say (as the `I: Default` bound implies) that `Fuse::<I>::default` "Creates a `Fuse` iterator from the default value of `I`". However, the implementation creates a `Fuse` with `Fuse { iter: Default::default() }`, and since the `iter` field is an `Option<I>`, this is actually `Fuse { iter: None }`, not `Fuse { iter: Some(I::default()) }`, so `Fuse::<I>::default()` always returns an empty iterator, even if `I::default()` would not be empty.

This PR changes `Fuse`'s `Default` implementation to match the documentation. This will be a behavior change for anyone currently using `Fuse::<I>::default()` where `I::default()` is not an empty iterator[^1], as `Fuse::<I>::default()` will now also not be an empty iterator.

(Alternately, the docs could be updated to reflect what the current implementation actually does, i.e. returns an always-exhausted iterator that never yields any items (even if `I::default()` would have yielded items). With this option, the `I: Default` bound could also be removed to reflect that no `I` is ever created.)

[Current behavior example](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=a1e0adc4badca3dc11bfb70a99213249) (maybe an example like this should be added to the docs either way?)

This PR changes publicly observable behavior, so I think requires at least a T-libs-api FCP?

r? libs-api

cc rust-lang#140961

`impl<I: Default> Default for Fuse<I>` was added in 1.70.0 (rust-lang#99929), and it's docs and behavior do not appear to have changed since (`Fuse`'s `iter` field has been an `Option` since before the impl was added).

[^1]: IIUC it is a "de facto" guideline for the stdlib that an iterator type's `default()` should be empty (and for iterators where that would not make sense, they should not implement `Default`): cc rust-lang/libs-team#77 (comment) , so for stdlib iterators, I don't think this would change anything. However, if a user has a custom `Iterator` type `I`, *and* they are using `Fuse<I>`, *and* they call `Fuse::<I>::default()`, this may change the behavior of their code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants