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

About fallible iterators #844

Open
Philippe-Cholet opened this issue Jan 12, 2024 · 9 comments
Open

About fallible iterators #844

Philippe-Cholet opened this issue Jan 12, 2024 · 9 comments
Labels
fallible-iterator Iterator of results/options roadmap Big, multi-iterator changes.

Comments

@Philippe-Cholet
Copy link
Member

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.

@Philippe-Cholet Philippe-Cholet added the fallible-iterator Iterator of results/options label Jan 12, 2024
@Philippe-Cholet Philippe-Cholet pinned this issue Jan 12, 2024
@jswrenn
Copy link
Member

jswrenn commented Jan 12, 2024

I'd like to take the approach described in #814 (comment); i.e.:

  • We privately fork the Try trait and implement it for Option and Result.
  • We name our methods with the prefix maybe_.
  • When Try is stabilized, we (hopefully) simply delete our fork of Try and depend on the standard library one.

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.

@jswrenn jswrenn added the roadmap Big, multi-iterator changes. label Jan 12, 2024
@Philippe-Cholet
Copy link
Member Author

From #814 (comment)

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
Copy link
Member

jswrenn commented Jan 16, 2024

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

So we agree on jswrenn proposition. Then

  • I suggest we create a module/folder "maybe" to contain the forked Try and all our new fallible iterators (similar to the "adaptors" module/folder).
  • Deprecate?
    • try_collect for a new maybe_collect? (It would solve the conflict with nightly.)
    • process_results for a new maybe_process (or just maybe)?
    • _ok methods for new maybe_ methods?
  • Which Iterator (and Itertools) methods should have a maybe_ variant? Because there is quite a zoo. For now, only the ones having an _ok variant I guess. We might want to define a clear limit later.

@cuviper
Copy link
Contributor

cuviper commented Feb 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fallible-iterator Iterator of results/options roadmap Big, multi-iterator changes.
Projects
None yet
Development

No branches or pull requests

4 participants