rust-itertools / itertools

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

About fallible iterators #844

Open Philippe-Cholet opened 8 months ago

Philippe-Cholet commented 8 months ago

I just created the label "fallible-iterator" and labelled (only opened) related issues and pull requests where the iterator item is fallible: results mostly, options too. It's one recurring topic, and a proper discussion might be useful.


To start, I think our adaptors of iterators of results should be _ok-suffixed. try_-prefixed should be reserved to methods tied to the currently unstable Try trait.

jswrenn commented 7 months ago

I'd like to take the approach described in https://github.com/rust-itertools/itertools/pull/814#discussion_r1439725414; i.e.:

With this approach, we can give users a cutting-edge experience without increasing our MSRV. And, by using the maybe_ prefix, we can also freely introduce new methods without worrying of conflicting with future additions to the standard library.

Philippe-Cholet commented 7 months ago

From https://github.com/rust-itertools/itertools/pull/814#discussion_r1439867108

In libcore, fold is usually specialized using try_fold specializations alongside with NeverShortCircuit which surely won't be public forever so I thought that we would copy it at some point.

so I think it's natural to copy part (here Try related parts) of the std as we simply want to expand it.

I would largely prefer try_-prefixed names over maybe_ but I reckon that avoid conflict with libcore is more important, with RFC2845 implemented or not.

jswrenn commented 7 months ago

Yes, avoiding conflict with libcore is important, and it's why I'm recommending using the maybe_ prefix. Without RFC2845 implemented, the conflict would cause a compile error. With RFC2845 implemented, our implementations would shadow the standard library's, but almost certainly be sub-optimal in implementation (e.g., for any try_ methods stabilized before the Try trait, we can't delegate to the stdlib's try_ methods in our own implementations, since we couldn't replicate the stdlib's Try bound).

Philippe-Cholet commented 7 months ago

@phimuemue What do you think on this matter?


What would be the inconvenients of using Rust nightly (for one feature only)? As we would be able to use the Try trait and specialize try_fold. (We would still have to copy some parts such as NeverShortCircuit but not much). It would require to have the nightly toolchain installed (I currently don't), which might "intimidate" new contributors. Copy things would not limit us about our MSRV (as we could copy more recent things). I'm not very familiar with nightly (and history on why itertools went to stable in the first place), what am I missing?

phimuemue commented 7 months ago

I think replicating Try in itertools is fine.

I’m not an expert on how we implement it in a waterproof way that does not leak the details (the „pub-in-priv“ thing), but I think it is possible (at least to a certain extent).

Maybe I misunderstand you, @Philippe-Cholet, but I do not see why nightly would be required (especially for the average contributor).

Regarding ok vs maybe, I have no preference, but I’d like to be consistent.

Philippe-Cholet commented 7 months ago

Instead of copy/paste, if we were to use the nightly unstable feature related to Try (just wondering) then "what would be the inconvenients?" was my question.

phimuemue commented 7 months ago

Instead of copy/paste, if we were to use the nightly unstable feature related to Try (just wondering) then "what would be the inconvenients?" was my question.

Ah, I see. I am reluctant to use nightly (don’t want to do conditional compilation if not strictly necessary).

Philippe-Cholet commented 7 months ago

So we agree on jswrenn proposition. Then

cuviper commented 7 months ago

FWIW, rayon also uses a private Try for its public methods like try_fold. You're welcome to copy it: https://github.com/rayon-rs/rayon/blob/5876c80e1208c0aae60092ddfa690a71709e326d/src/iter/mod.rs#L3388

(and rayon doesn't have the same constraints for wanting to avoid std method names like Itertools does.)