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 Default impls for iterators and adapters #77

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
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