Error collector

[ijackson]

I noticed the poor ergonomics of Error::multiple. I see there's an issue for this already, #115.

Here is my idea of what a nice API woudl be like. It handles not only the awkward if errors.is_empty() at the end, but also collection of errors as we go. This is IMO a better alternative to #155.

[ijackson]

I experimented with this in derive_builder on top of my RFC colin-kiegel/rust-derive-builder#243 and the result I came up with looks like this (using the fact that Option can be an iterator to pass it to extend; other approaches, eg like the one I put here in this MR in the doc comment, are possible):

    let mut errors = darling::error::Collector::new();

    for attr in input.drain(..) {
        let unnest = outputs
            .iter_mut()
            .find(|(ptattr, _)| attr.path.is_ident(ptattr));

        if let Some((_, unnest)) = unnest {
            let unnested = errors.ok_of(unnest_from_one_attribute(attr));
            unnest.extend(unnested);
        } else {
            for (_, output) in outputs.iter_mut() {
                output.push(attr.clone());
            }
        }
    }

    errors.conclude(())

[TedDriggs]

I think having a Collector type is reasonable, but I would do the API a bit differently.

let mut errors = darling::Error::collector();

for attr in input.drain(..) {
    let unnest = outputs
        .iter_mut()
        .find(|(ptattr, _)| attr.path.is_ident(ptattr));

    if let Some((_, unnest)) = unnest {
        match unnest_from_one_attribute(attr) {
            Ok(attr) => unnest.push(attr),
            Err(e) => errors.push(e)
        }
    } else {
        for (_, output) in outputs.iter_mut() {
            output.push(attr.clone());
        }
    }
}

errors.checkpoint()

Key differences:

• Have the collector be created off darling::Error so someone doesn't need to import more things
• Keep the imperative errors.push approach, primarily for drop-in compat with existing code, and because I think Option having an IntoIter is so clever it's easy to make a mistake with.
• Add fn checkpoint(&mut self) -> darling::Result<()>; this could be in addition to finish.

My main concern with this is the lack of prior art; I haven't had time to look for other examples of crates that accumulate/collect errors to see how they do it.

I wonder if this is a Context or something? That naming might be better.

[ijackson]

Key differences:

* Have the collector be created off `darling::Error` so someone doesn't need to import more things

Makes sense to me. It should probably still impl Default in case that's helpful to someone.

* Keep the imperative `errors.push` approach, primarily for drop-in compat with existing code, and because I think `Option` having an `IntoIter` is so clever it's easy to make a mistake with.

I think whether using extend is nice is a question about the code in derive_builder really :-). I do think it's really worth supporting is some convenience approach that can be used to wrap error-generating code. What did you think of this bit in my docs example (edited lightly to remove a pedagogic type decatation) ?

for thing in inputs {
    errors.run(||{
        let validateed = thing.validate()?;
        outputs.push(validateed);
        Ok(())
    });
}

* Add `fn checkpoint(&mut self) -> darling::Result<()>`; this could be in addition to `finish`.

That could be useful.

My main concern with this is the lack of prior art; I haven't had time to look for other examples of crates that accumulate/collect errors to see how they do it.

I did look for some prior art. I'm sure I've seen something like this somewhere but wasn't able to find it. I don't think there's any in std.

I wonder if this is a Context or something? That naming might be better.

I find Context is a very common word which people use for everything. OK, it will be called darling::error::Context, but I still think that's vague. I think a more precise word is probably better.

[ijackson]

I did another search for prior art and found these:

• pahs::error_accumulator::ErrorAccumulator. It has a plain add_err method for "manual" handling, but also add_progress which seems to do for a pahs::Progress roughly what my of_ok does for Result.
• frunk::validated::Validated. The interface it offers for accumulation (assuming you don't just open-code it) is an impl of Add which ... well, I'm sure it's very nice for parsing results which are all Add but I think it is a bit on the abstruse side.

I am definitely unsure about the right names for run and of_ok. I also considered that maybe there should be of_ok_or_default etc., which would be a convenience method equivelant to errors.of_ok(something).unwrap_or_default(). But ultimately it seemed too much API surface, when Option already has all the methods.

Other name/bikeshed suggestions:

• Follow pahs: add or add_result for of_ok and add_err for push. I think push is better.
• handle or push_result for of_ok ?

I quite like handle. We could write this:

        if let Some((_, destination)) = unnest {
            if let Some(unnested) = errors.handle(unnest_from_one_attribute(attr)) {
                destination.push(unnested);
            }

Of course run is just a convenience method: you can always use an IEFE. But run saves on a set of (...)() and makes code using it much clearer IMO.

[ijackson]

Hmm, mem::take is too new. I will fix it.

[TedDriggs]

Does it make sense for Collector to have a drop-bomb, so that will panic if it's dropped while not empty? It seems like in most cases, it's a mistake to forget to call checkpoint or conclude, and a panic seems more useful than errors silently disappearing (and possibly leading to bad code generation).

[ijackson]

Does it make sense for Collector to have a drop-bomb, so that will panic if it's dropped while not empty? It seems like in most cases, it's a mistake to forget to call checkpoint or conclude, and a panic seems more useful than errors silently disappearing (and possibly leading to bad code generation).

Interesting idea. I think I like it. But it ought to happen on any plain drop, so that ytou can't ship code that triggers it by mistake, even if you only test the success case. This is slightly fiddly inside Accumulator since it has to distinguish "consumed by finish" from "simply empty", but I think it's worth it.

I'll revise this MR tomorrow.

[TedDriggs]

That's true, though checkpoint does need to defuse it so it's also not as simple as defusing only once.

[ijackson]

That's true, though checkpoint does need to defuse it so it's also not as simple as defusing only once.

With the drop bomb, checkpoint wants to consume the Accumulator, and return Result<Accumulator>. Otherwise if checkpoint returns Err the caller gets to keep the (necessarily defused) Accumulator, which ought to panic if it were used, and we want to avoid panics on unlikely error paths.

[TedDriggs]

We're getting very close here.

I think we can do better than run and handle for the two method names. Maybe take_err for handle and take_err_from for run?

Part of my gripe with those two names is that we encourage this to be called errors when it's in scope. errors.run(...) doesn't inspire much confidence when I'm reading it.

[ijackson]

To my mind take_err would return an err, not squirrel errors away and return the success value. Cf Option::take (which extracts data from its receiver - almost the opposite of handle); TcpStream::take_error (which returns an Error), take_pidfd, etc.

I see your problem with run. I think there's no problem with errors.handle(something).

How about renaming run to handle_from ?

    errors.handle_from(||{
        let validated = thing.validate()?;
        outputs.push(validated);
        Ok(())
    });

noun.transitive_verb(wombat) often means "ask noun to transitive_verb this wombat". handle fits that well. "Ask the errors to handle this result".

[TedDriggs]

I'm officially in love with the use of wombat for placeholder variable names.

---

You're right about take - I was thinking about its effect on the residual, e.g. "I took the error, so now what's left is the Ok value or a None" but that isn't quite what the std lib ones do.

---

I think I prefer handle_in or handle_with over handle_from, mostly because of the strong conversion connotation that from has in Rust. Then it's errors.handle_in(|| { ... }).

Is it bad that I actually quite like errors.deal_with<T>(v: Result<T>) -> Option<T>? It's probably too colloquial, but it does read quite clearly in American English. The closure version would be errors.deal_with_in or errors.deal_with_from.

[ijackson]

"Deal with" is fine in British English too. It is very slightly less formal, I think. My only concern with it is whether non-native speakers of English would find it confusing (especially because of the way Rust uses with).

I looked at Wiktionary's entries for deal with and handle. "Deal with" is less ambiguous if you know what it means at all (since it can't mean "get a handle"), but as I say I wonder if it's too confusing if you don't. I did a search of some random local rustdocs here "handle" as in "deal with" is used a fair bit in web frameworks as a thing you do to requests or, indeed, errors.

Of the things listed in Wiktionary's synonyms sections for these words (I tried an online thesaurus but it was no use) I think "manage" is a possibility. Also maybe "consider"

[ijackson]

So, having slept on it I think best would be handle and handle_in. (handle_with is definitely wrong because that implies that errors go into the closure, rather than out of it.)

[TedDriggs]

I'm supportive of handle and handle_in.

[TedDriggs]

This looks great; really appreciate all the work you've put into it, and think this is a great addition to darling.

[TedDriggs]

A final note on this that makes me happy: To merge two accumulators is as easy as acc1.handle(acc2.finish()).

[ijackson]

This looks great; really appreciate all the work you've put into it, and think this is a great addition to darling.

Thanks for your kind words :-). You're very welcome.