Skip to content

Change core::iter::Fuse's Default impl to do what its docs say it does #140985

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

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

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented May 14, 2025

The docs on impl<I: Default> Default for core::iter::Fuse<I> 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 iterator1, 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 (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 #140961

impl<I: Default> Default for Fuse<I> was added in 1.70.0 (#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).

Footnotes

  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 https://github.com/rust-lang/libs-team/issues/77#issuecomment-1194681709 , 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.

@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 May 14, 2025
@zachs18 zachs18 changed the title Change core::iter::Fuse's Default impl to do what it's docs say it does Change core::iter::Fuse's Default impl to do what its docs say it does May 14, 2025
@hanna-kruppe
Copy link
Contributor

Either way (fix the impl or fix the docs) this should probably gain a test since it’s extremely non-obvious from the code alone.

@scottmcm
Copy link
Member

And maybe change the code to either Option::default() just None or to I::default() to help emphasize, too.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants