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

Produce better size_hints from Flatten & FlatMap #48544

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@scottmcm
Copy link
Member

scottmcm commented Feb 26, 2018

Adds const SIZE_HINT to IntoIterator, allowing a good estimate before the actual iterators are available.

These two adapters typically produced largely-unhelpful size_hints, as they had no idea what was going to come out of the inner iterators. This change lets flat_map do as well as filter_map with Options, and allows flatten to get its size_hint exactly right in cases like U = &[T; N].

Produce better size_hints from Flatten & FlatMap
Adds a const SIZE_HINT to IntoIterator, allowing a good estimate before the actual iterators are available.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 26, 2018

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

/// assert_eq!(<Option<String> as IntoIterator>::SIZE_HINT, (0, Some(1)));
/// ```
#[unstable(feature = "typelevel_size_hint", issue = "7777777")]
const SIZE_HINT: (usize, Option<usize>) = (0, None);

This comment has been minimized.

@BurntSushi

BurntSushi Feb 26, 2018

Member

A style nit, but why are you declaring the const SIZE_HINTs beneath all of the method definitions? They kind of feel like they should be before method definitions?

This comment has been minimized.

@scottmcm

scottmcm Feb 26, 2018

Member

I had it up there originally, but then it felt like it was pretending to be too important, and that the things one actually needs to implement should be first. I'm happy to use either.

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Feb 26, 2018

cc @rust-lang/libs This PR is adding a new (but unstable) const SIZE_HINT to the IntoIterator trait, and I'm not sure if we're OK with that. Other than that, this change in principle seems fine?

@Centril Centril added the T-libs label Feb 26, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 26, 2018

Hm I'm sort of hesitant about this in the sense that it seems like there's no end to tiny tweaks to iterators and associated traits and items. Are we eventually going to grow a suite of associated items for all the various properties of iterators? Will we one day need a hint like this on the trait itself rather than the into trait?

While I also understand the use case here I think it seems a little odd to be an associated constant because size hints are otherwise always associated with an actual instance, so having a difference here seems slightly odd

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Mar 2, 2018

How should this PR be handled? Do we wish to promote this to a team discussion?

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 3, 2018

@alexcrichton I can't disagree with anything you said 🙂

Are we eventually going to grow a suite of associated items for all the various properties of iterators?

It doesn't surprise me that we keep finding new things, as we have a bunch of things -- like size_hint -- that help alot but where we can't just open Stepanov to get a list of everything to do.

I'm sure there are more to come -- we don't have anything for random-access, for example -- but probably also many things we won't bother with -- as a strawman guess, things like whether the length is even. I don't know in which category this PR falls, but I figured it was worth a PR to have the discussion since flatten and flat_map are so often stuck with (0, None) without it.

Will we one day need a hint like this on the trait itself rather than the into trait?

Given that every Iterator is IntoIterator, not necessarily. I would expect that some form of specialization or intersections or something would let an iterator that really cared customize it. Allowing that on stable today would need a const on Iterator for the blanket impl to pick up, but I don't think it's urgent.

It's particularly not-urgent since it's rarer for an iterator to be able to say something useful anyway. Since iterators need to shrink, the only possible SIZE_HINTs they could offer would be (0, _) or (usize::MAX, None), whereas IntoIterator can offer (N, Some(N)) in realistic cases. Notably, const generics will let .flat_map(|x| [foo(x), bar(x)]) be legal and produce an exact hint. Certainly some iterators can offer usable upper bounds statically (like option::Iter or Once), but that's both rarer and usually easier to phrase with IntoIter anyway: one can my_option.filter() instead of my_option.iter().filter(), one can just .map(|x| f(x)) instead of .flat_map(|x| iter::once(f(x))), etc.

it seems a little odd to be an associated constant because size hints are otherwise always associated with an actual instance

I don't disagree, but I do think it provides a distinct value not otherwise possible. And having it on IntoIterator I think helps, to some degree, since it's the "I don't actually have an iterator yet" class, and thus getting a prediction, of sorts, from it is plausible.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Mar 3, 2018

the const can't ever be added to Iterator because its not object safe (and there's no where Self: Sized on consts, yet at least)

Is there evidence of the performance impact of this change? Any benchmarks or anything?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 6, 2018

Ok thanks for the info @scottmcm! I think I'm personally in camp "prefers things to be simple even if slightly deficient" in the sense that I'm not sure the end-all-be-all iterator library where every possible case is optimized as much as possible should be in the standard library. I personally see optimizations like this as great candidates for community crates (so they can be used when necessary), but otherwise I might prefer to approach this from the point of view of "how do we enable this to be done on crates.io" rather than "how to put this in libstd"

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 10, 2018

Ok, that sounds like disposition close to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment