rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.64k stars 299 forks source link

Support for TryFrom::try_from on the items of iterator #913

Open schneiderfelipe opened 3 months ago

schneiderfelipe commented 3 months ago

It would be nice to have a map_try_into similar to map_into already implemented (in #288, requested in #280). The signature would be something like

fn map_try_into<T>(self) -> impl Iterator<Item = Result<T, <Self::Item as TryInto<T>>::Error>>
where
    Self::Item: TryInto<T>,
{
    std::iter::from_fn(|| todo!())
}
Philippe-Cholet commented 3 months ago

No doubt it's easily doable by copying the code we have for MapInto and merely update it.

The alternative is .map(TryInto::try_into). "Is such addition worth it?" is the main question. And where do stop with map_-prefixed methods? Because I can imagine a bunch of those, like .map_expect(msg) for .map(|result| result.expect(msg)). I kinda wonder what's the difference of generated codes for .map_into() vs .map(Into::into). Does it optimize similarily well?

scottmcm commented 3 months ago

In general, I'd expect custom map iterators to actually optimize worse, because they can't take advantage of all the specializations in the standard library.

For example, std::iter::Map is https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/std/iter/trait.TrustedLen.html if the original iterator is TrustedLen, but iterators in itertools can't be.

So I would say that itertools shouldn't have any of the .map(thing) iterators. It's just not that hard to write .map(Into::into) or whatever.

Philippe-Cholet commented 3 months ago

Apart from not making map_try_into... @jswrenn @phimuemue Should we consider deprecate .map_into() in favor of .map(Into::into)?

jswrenn commented 3 months ago

Agree with @scottmcm that we shouldn't be adding more trivial shorthands that don't optimize well. We should revisit this decision when type_alias_impl_trait is stabilized and RPITIT is usable on our MSRV: those will let us easily define short-hands that optimize as well as if they had been in the standard library.

In the meantime, I don't think we need to go so far as deprecating map_into; a documentation note on performance might suffice.

scottmcm commented 3 months ago

RPITIT is a great call-out here. If these could be written as just -> std::iter::Map<impl Fn<…> + Copy> or whatever, things could well be different. (Though even then I'm not convinced that .map_try_into() is necessarily better than .map(TryInto::try_into) or .map(|x| x.try_into()), because there's cost to remembering all the custom things too.)