Documentation for `is_done()` may be overly specific

[justinlovinger]

The documentation for is_done() says, "Checks if get() will return None". However, is_done() can be implemented to return true even when get() returns Some. Is the documentation overly specific or is it undefined behavior to implement is_done() such that it can return true when get() returns Some?

P.S. this is relevant to me because I have a use-case where I need to get the final value from a streaming iterator. With the current streaming-iterator API, this requires get() to return Some when is_done() returns true.

[cuviper]

is it undefined behavior to implement is_done() such that it can return true when get() returns Some?

I think the other direction is more interesting, because someone might be tempted to write:

if !iter.is_done() {
    // maybe do some other prep work, then:
    unsafe { iter.get().unwrap_unchecked() }
}

However, this is unsound for a generic StreamingIterator, because that is a safe trait that might be implemented contrary to its documented requirements. So that's indeed UB, but the fault lies in the unsafe code.

There are similar issues that could arise if you make assumptions about size_hint, and in the standard library Iterator::size_hint has documented a warning about this:

size_hint() is primarily intended to be used for optimizations such as reserving space for the elements of the iterator, but must not be trusted to e.g., omit bounds checks in unsafe code. An incorrect implementation of size_hint() should not lead to memory safety violations.

For a known type instance of StreamingIterator, unsafe code can reasonably choose whether to trust that implementation. It's hard to do much of anything without trusting some safe code to be correct. But for generic implementations, unsafe code must be defensive.

P.S. this is relevant to me because I have a use-case where I need to get the final value from a streaming iterator. With the current streaming-iterator API, this requires get() to return Some when is_done() returns true.

As above, I think the more problematic case is is_done() -> false followed by get() -> None -- unsafe code must not assume that will never happen. However it might well unwrap() that None and panic, so as an implementor you shouldn't create that kind of situation on purpose.

The main point of is_done() -> true is to avoid calling another get() at all, so it probably won't harm anything if you actually provide Some anyway. It might be more robust to provide a separate method to get that final value though. That way the documented behavior of StreamingIterator will be consistent, and you can do your thing on the side.

[justinlovinger]

It might be more robust to provide a separate method to get that final value though.

The problem with providing a separate method to get the final value is it does not play nicely with methods like .inspect(...).

[cuviper]

Inspect would also be within its right to defer to the default is_done(), which calls get().is_none(), so it wouldn't notice your different behavior. It's an optimization to pass through to the inner is_done(), but not a requirement.

[justinlovinger]

It sounds like get() returning Some when is_done() returns true is undefined behavior, even if it happens to work right now.

More importantly, is that to say streaming-iterator has no intention of supporting a last() method, now or in the future? I have ideas for supporting last(), but it would require extensive API changes, and may be out of scope for this issue.

[cuviper]

It sounds like get() returning Some when is_done() returns true is undefined behavior, even if it happens to work right now.

No, not unless you're using a looser meaning of "undefined behavior" than the accepted term of art. A major principle of Rust is that safe code (e.g. an impl StreamingIterator) must not to lead to UB. If an API uses unsafe internally that allows safe misuse to cause UB, then that API is unsound.

It can lead to other misbehavior, like logic errors or panics.

More importantly, is that to say streaming-iterator has no intention of supporting a last() method, now or in the future? I have ideas for supporting last(), but it would require extensive API changes, and may be out of scope for this issue.

That's a tough one. Off-hand, I don't see how it could be done, especially with things like filter where you can't know if there are any more until you evaluate the predicate, and then you don't have access to the prior item anymore.

[justinlovinger]

No, not unless you're using a looser meaning of "undefined behavior" than the accepted term of art.

I should have been more specific. By "undefined behavior", I mean to say, the behavior is not defined by the streaming-iterator API and may not be relied upon for future compatibility.

That's a tough one. Off-hand, I don't see how it could be done, especially with things like filter where you can't know if there are any more until you evaluate the predicate, and then you don't have access to the prior item anymore.

filter is a tricky one. My idea involved get() returning &Self::Item, instead of Option<&Self::Item>, and a required state() method returning a State enum, with variants like Last and Done. However, that would not work with filter. There may need to be a separate trait for iterators capable of calling last(), not unlike DoubleEndedStreamingIterator, which the struct returned by filter would not implement.

[cuviper]

Yes, a new trait could do it!