Update io iterators to produce IoResults

[DaGenix]

Most IO related functions return an IoResult so that the caller can handle failure in whatever way is appropriate. However, the lines, bytes, and chars iterators all supress errors. This means that code that needs to handle errors can't use any of these iterators. All three of these iterators were updated to produce IoResults.

Fixes #12368

[DaGenix]

This is part of a discussion on the ML and as part of #12368. At this point, there doesn't appear to be a generally agreed upon consensus regarding a solution or even that there is a problem that needs a solution. So, this should not be merged until that discussion concludes and only if this is the agreed upon solution. However, in the meantime, this is a concrete proposal that can be analyzed. Discussion of the general issue should probably continue on #12368.

[alexcrichton]

This seems like a good compromise between the competing interests to me. Some thoughts:

• This takes an already wordy "read lines from stdin" example and makes it wordier with an extra import and method call. I'm beginning to think we should have io::stdin() and io::stdin_raw() where the first is a buffered reader by default (not relevant to this PR, just thinking out loud)
• It seems unfortunate to require IoResultIterator being imported. I don't think a trait like that quite belongs in the prelude as well. Is there a reason you didn't use methods? It wouldn't be implemented generically for all iterators, but the constructor could work generically for all iterators (you'd just have to remember to call it).
• fail_on_error seems a little wordy, perhaps a better name can work?

All-in-all, I'd like to reduce your example down to:

use std::io;

fn main() {
    for line in io::stdin().lines().fail_on_error() {
        println!("{}", line);
    }
}

[DaGenix]

I don't love the name either, but I didn't have any better ideas.

I didn't implement it as a method on Lines because there is also the Chars iterator which can also use this functionality. Those are currently the only two iterators I'm aware of, but I could imagine there being something like a Words iterator in the future and I thought it would be nice to have something that would just work with any Iterator over IoResults without having to specifically implement it.

I didn't implement it as a function, because I thought that:

for line in io::fail_on_error(io::stdin().lines())

didn't read really well.

[DaGenix]

I update the PR since I had some changes sitting around necessary to make everything compile. I also renamed the extension trait to IteratorIoExt since the original name, IoIterator, seemed a bit misleading since it wasn't actually an Iterator. If it were decided to merge this, I'd like to squash a few of these things first.

[alexcrichton]

Closing due to a lack of activity, but feel free to reopen with a rebase!

[DaGenix]

Rebased against master, passes make check. I'd mostly left this PR alone since I opened it in response to a mailing list / Issue discussion and I was waiting to see if that discussion was going to reach some sort of consensus. I haven't seen any further developments in that conversation, however.

I've made a few changes:

• I renamed the extension trait once again. Now its called IteratorExtensions.
• I updated the Bytes iterator to also return IoResults instead of ignoring errors
• I don't know if anyone was depending upon the previous behavior of ignoring errors. In case they were, I created another extension trait, result::IteratorExtensions, that adds the method stop_on_error() for any Iterator over Results that simply ceases iteration on the first error as opposed to calling fail!(). This is exactly what the Lines, Chars, and Bytes iterators currently do.

With the change to make stdin() buffered and with these changes, the example program that was being discussed now looks like this:

use std::io;
use std::io::util::IteratorExtensions;

fn main() {
    for line in io::stdin().lines().fail_on_error() {
        print!("received: {}", line);
    }
}

So, as it currently stands, it still requires the extra use statement. The wordiness of this is unfortunate, however, it does mean that it works on all iterators over IoResults. Currently there are only 3 direct iterators over IoResults - Lines, Chars, and Bytes. However, those aren't really the only iterators to consider - if you use an Iterator method like chain(), the result is a new iterator type which also operates on IoResults. So, if fail_on_error() were implemented as a method of the Lines iterator and you wanted to iterate over the lines of two files, you would need to write:

for x in file1.lines().fail_on_error().chain(file2.lines().fail_on_error()) { ... }

However, with fail_on_error() implemented as an extension method, you could write the above, or you could write:

for x in file1.lines().chain(file2.lines()).fail_on_error() { ... }

Which I think reads a bit nicer. Also, I think its a bit more user-friendly - fail_on_error() having different ordering requirements than chain() or skip() could be a bit confusing.

I haven't come up with a better name for fail_on_error(), but I'm open to suggestions.

[bstrie]

Reopening at @DaGenix's request.

[alexcrichton]

The iterator extension traits seem difficult to discover, and I'm coming around to think that they may not be necessary at all. We have the unwrap() method and the try! macro, so if we really want to go forward with this, I think we should try without the iterator extensions first.

I'm still a bit wary of whether this is the direction that we want to go in, the whole point is to make lines() an easy "iterate over each line" method, and this is moving it significantly away from that direction. I suppose using .unwrap() isn't so bad though:

use std::io;

fn main() {
    for line in io::stdin().lines() {
        print!("received: {}", line.unwrap());
    }
}

[DaGenix]

I agree that discoverability is an issue. Putting the extension in the prelude would somewhat address the issue, however, then there is a magic method that shows up somewhat out of nowhere.

Anyway, I'm also of the opinion that the extension trait isn't necessary. In my opinion what is important is that all of the APIs in the standard library should be a) correct and b) easy to use, in that order. If an API isn't correct, ease of use becomes a bad thing since it will just spread the problem farther, faster, and with a lower chance that people look up the documentation. Ignoring errors makes the particular 5 line program under discussion look mildly nicer; however, it also makes the API impossible to use in anything non-trivial - if I want to iterate over the lines of a file and I also want to handle errors, where's my lines() method? What I'm getting at is I think we should make these iterators correct and consistent first and then try to figure out if there is some nice sugar to make that particular example prettier.

I've rebased the PR to remove the extension traits and to just change the iterators to produce IoResults.

[DaGenix]

I left in the Fixes #12368, since I think it still addresses the intended use case of that issue. Maybe I'm wrong, but my impression was that the basic idea behind #12368 was to provide a way to handle errors while using the io iterators, which this PR still does.

[DaGenix]

One thing I noticed while making the changes in this PR is that one of the examples in the io::mod.rs documentation doesn't work:

use std::io::BufferedReader;
use std::io::File;

let path = Path::new("message.txt");
let mut file = BufferedReader::new(File::open(&path));
for line in file.lines() {
    print!("{}", line);
}

The issue is that messages.txt doesn't exist. Since lines() swallows errors, the result is that it also swallows the error about the file not existing. Rust's #[must_use] annotation doesn't help. Forcing the caller to unwrap the IoResult, reveals the issue, however.

[alexcrichton]

We've been trying to prevent imports of the prelude throughout libstd, it often leads to obscure resolve errors.

[alexcrichton]

Just two minor nits, then I think this is good for merging.

[DaGenix]

Rebased to address your comments