Add `inner` and `into_inner` methods to iterator adapters

[krtab]

Proposal

Problem statement

There exists in the ecosystem structs that are both:

1. Iterators (they implement std::iter::Iterator)
2. "Entry-point" of some crate specific logic. This logic may even be independent of the Iterator's state of advancement.

Sometimes, one wants to wrap such an iterator in another struct to implement new logic on it via methods. This is all very fine until one wants to also use iterator combinator (say filter). As of now, once an iterator I is wrapped in an adapter A<I>, there is no way to access the I again.

This proposal discusses adding a method inner() and a method into_inner() to some iterator adapters of the standard library.

Motivation, use-cases

The example in my case is xmlparser::Tokenizer. This is an iterator on XML Tokens, but also the entry point of the crate, created with Tokenizer::from(&str). I am using it to parse a format built on top of XML.

In a yet to be published version of the xmlparser crate, Tokenizer has a stream(&self) -> Stream<'a> method, which in turns gives access to the underlying Stream's gen_text_pos_from(&self, pos: usize) which allows to compute a position in line/col from a position in bytes in the text. This is useful to get clear error messages when there are error in the outmost format.

Initially, my FormatParser wrapped a Peekable<Filter<Tokenizer, _>>, because some tokens are irrelevant for this format, and because the architecture required a look ahead of one. My problem was that I could not access the Stream this way.

Solution sketches

Proposed solution

The solution would be to add an inner(&self) -> &I to Adapter<I> in cases where it is possible.

Addition of a into_inner(self) -> I method can be also discussed but is more worthy of attention. Indeed, as pointed out in discussion elsewhere (see links), the state of the inner iterator is not necessarily obvious from the wrapper. Giving back ownership of the iterator could lead to logic error. Only allowing a & getter mitigates these issues, as one cannot iterate on a non mutable reference. (Clonable iterator would however be an issue).

Another issue to discuss is, quoting @the8472 comment:

At least for Zip and possibly other adapters this is unsound because TrustedRandomAccess specializations bring the iterator into a state that is not consistent unless used exclusively through the methods whitelisted in the TrustedRandomAccess documentation.

It would also preclude some future optimizations that would similarly assume that internals are not observable once an iterator has been wrapped into an adapter.

Other examples of the stronger than first thought commitment that such an API would create is given by @scottmcm here

For a bunch of them, though, this being an infallible API is more of a commitment than you might be thinking. For example, Take<I> is currently (usize, I) internally, but that's not really fundamental. It could also be Option<(NonZeroUsize, I)> internally, for example, so that it drops the wrapped iterator once it'll no longer take anything from it. And that would mean that into_inner(self) -> I could no longer be implemented.

Even for Skip, where taking out the inner thing is comparatively reasonable because after the first call it doesn't do anything, the behaviour isn't necessarily obvious. It would be very easy for someone to write it = it.skip(10).into_inner(); and expect that to be equivalent to it.advance_by(10), but it's very much not.

In conclusion, much of the discussion to be had revolves around deciding for which, if any, adapter the proposed methods would make sense. It coudl also be envisionned that once the reasons preventing the implementation of these methods are clearer, a alternative and more constraining API is designed.

Alternative solution

The alternative is to preserve the status quo, which, in my opinion, officialize that iterator adapters are intended to be used as "lightweight and discardable".

Links and related work

• Draft: Add inner and into_inner methods to iterator adapters rust#103294
• Tracking Issue for (into_)inner() methods on iterator adapters rust#103302
• https://rust-lang.zulipchat.com/#narrow/stream/122652-new-members/topic/.5BPre.20RFC.5D.20.28into_.29inner.28.29.20for.20Iterator.20Adapters

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

[the8472]

There exists in the ecosystem structs that are both:

Iterators (they implement std::iter::Iterator)
"Entry-point" of some crate specific logic. This logic may even be independent of the Iterator's state of advancement.

In the std we have often found this kind of structure to be a mistake. E.g. Range<T> should have been IntoIterator<IntoIter=RangeIter> or something like that instead of Iterator<Item=T>, that way we could have implemented Copy for Range, which we currently avoid because implementing Copy on iterators leads to confusing borrowing behavior.

That's also why most collections have one or more related Iter structs.

That said, having a Tokenizer and a borrowing TokenizerIter wouldn't really change all that much as far as lifetimes go. It would be similar to using by_ref().

Initially, my FormatParser wrapped a Peekable<Filter<Tokenizer, _>>, because some tokens are irrelevant for this format, and because the architecture required a look ahead of one. My problem was that I could not access the Stream this way.

If you don't need the peeked element to persist across method calls you could construct that internally on the fly via self.tokenizer.by_ref().filter().peekable() and then throw it away at the end of the method. Alternatively you could implement the behavior of Peekable manually and persist the lookahead item in your parser.

But your proposed Peekable::into_inner also loses that state. So... just constructing it on the fly would seem appropriate for your needs?

Sometimes it's simpler to achieve complex control flow by not trying to get iterators to play along.

[the8472]

That said, having a Tokenizer and a borrowing TokenizerIter wouldn't really change all that much as far as lifetimes go. It would be similar to using by_ref().

Actually, since it's Tokenizer<'a> (lifetime of the parsed data) one can construct a TokenizerIter<'a>, meaning the iterator doesn't need to borrow the tokenizer and both can be used at the same time. Alternatively one can just construct 2 tokenizers from the same &str.

[krtab]

In the std we have often found this kind of structure to be a mistake. E.g. Range<T> should have been IntoIterator<IntoIter=RangeIter> or something like that instead of Iterator<Item=T>, that way we could have implemented Copy for Range, which we currently avoid because implementing Copy on iterators leads to confusing borrowing behavior.

That's also why most collections have one or more related Iter structs.

I think I do wholeheartedly agree. I would like to re-state that to me, a possible issue of this proposal is re-enforcing and "officializing" the position that iterators (and even more so their adapters) should be made "as discardable as possible" and that the struct implementing the iterator specification (the range for example) should not be the same as implementing the "iteration machinery". To the best of my current knowledge, this is not currently part of say the API guidelines.

That said, having a Tokenizer and a borrowing TokenizerIter wouldn't really change all that much as far as lifetimes go. It would be similar to using by_ref().

Indeed. I do agree with you that my specific issue could be solved by reworking xmlparser's API to better separate what requires mutable access to a parsing state and what is meta information about the text being parsed. However as a user of the library, this is not exactly in my hands.

Initially, my FormatParser wrapped a Peekable<Filter<Tokenizer, _>>, because some tokens are irrelevant for this format, and because the architecture required a look ahead of one. My problem was that I could not access the Stream this way.

If you don't need the peeked element to persist across method calls you could construct that internally on the fly via self.tokenizer.by_ref().filter().peekable() and then throw it away at the end of the method. Alternatively you could implement the behavior of Peekable manually and persist the lookahead item in your parser.

But your proposed Peekable::into_inner also loses that state. So... just constructing it on the fly would seem appropriate for your needs?

Sometimes it's simpler to achieve complex control flow by not trying to get iterators to play along.

Indeed, and in the end I rewrote my parser to a) not look ahead and b) "inline" filtering and so I removed both adapters, but at least ergonomically having inner() would have helped.

I also agree with you that into_inner() is trickier than inner() because of the amount of assumptions implied on the state of the wrapped iterator.

[dtolnay]

We discussed this proposal in the most recent T-libs-api meeting. We are not on board with adding inner (with or without into_inner) as boilerplate API across a broad range of standard library iterator adapters as done in rust-lang/rust#103294. If there are individual iterators for which having inner available is uniquely impactful, we are on board with considering those in PRs with a dedicated justification for each. They would truly have to be strongly justified though: overall for the use cases described so far in this issue, our preference is that you write your own dedicated iterator combining the functionality and API that you need, or refactor in a way that does not need to involve iterators, as it sounds like you ended up doing.

Thanks anyway for the API proposal and draft PR!

[krtab]

Thanks for the review !