projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.04k stars 96 forks source link

Replace `Vec<ParserError>` with opaque Error type. #176

Open XAMPPRocky opened 4 years ago

XAMPPRocky commented 4 years ago

Currently in a few places fluent error type is vector of errors that allows you to display multiple syntax errors, but Vec<T> wasn't designed as an error type and as such when you're building a library on-top of fluent, where you just want to propagate the error using an error handling library or ?, you have to write quite a bit of boilerplate to correctly handle it.

zbraniecki commented 4 years ago

Sure, See #125 and #74 for open issues about more consistent error handling.

I don't have a lot of experience of prior art around returning list of errors rather than a single error. I'd appreciate feedback and/or PRs :)

XAMPPRocky commented 4 years ago

Well I don't know about the specific requirements for your error type but I would say creating a newtype that contains the list of errors, and itself also implements Error, would be enough. E.g.

pub struct ParserErrors(Vec<ParserError>);

impl std::error::Error for ParserErrors {
  // impl body
}
zbraniecki commented 3 years ago

@XAMPPRocky I am polishing fluent-syntax for an upcoming 1.0 release and started looking into this. My hope was that by now we'll have more refined understanding of the future design of the standard Rust error model, but it seems like thiserror is the closest thing and it doesn't really handle lists of errors.

I started thinking about your example, and I feel like a model of encapsulating list of errors just to implement Error on that list is awkward. Have you seen such model anywhere else? Can you describe more the awkwardness of handling a vector of Errors that such wrapper would alleviate?

I'm open to try to fix that, but I'm trying to see if there's a more natural solution.

XAMPPRocky commented 3 years ago

My hope was that by now we'll have more refined understanding of the future design of the standard Rust error model, but it seems like thiserror is the closest thing and it doesn't really handle lists of errors.

FWIW, my preferred error handling libraries are snafu for libraries, and eyre for applications.

I started thinking about your example, and I feel like a model of encapsulating list of errors just to implement Error on that list is awkward. Have you seen such model anywhere else? Can you describe more the awkwardness of handling a vector of Errors that such wrapper would alleviate?

I mean, in general I've never seen an API return Vec<Error>, so fluent is more the outlier here. Returning multiple error is also pretty uncommon, but as a point of comparison rustc has out variable called sess where all the errors are collected, and then the actual method just returns a marker struct to indicate that it found errors.

As I mentioned above, you can't use the Try/? operator with Vec<T>, which makes it awkward to use in common situations such as returning Box<Error>, you (as a user) have to either unwrap or create your own wrapper to use with .map_err. To put it in an example, i would expect the following to compile.

fn main() -> Result<(), Box<dyn std::error::Error>> {
    // Returns Result<Vec<FluentResource>, std::io::Error>
    let result = read_from_dir("path")?;

    let mut bundle = FluentBundle::new(vec![unic_langid::langid!("en-US")]);

    for resource in &result {
        bundle.add_resource(resource)?;
    }
    Ok(())
}

What awkwardness to foresee in using an opaque wrapper over Vec<Error>? You could just as easily also provide trait implementations that allow the opaque error type behave like Vec<Error> with IntoIterator, and Deref. You could also now provide helper inherent methods that you couldn't when it was just Vec.

zbraniecki commented 3 years ago

What awkwardness to foresee in using an opaque wrapper over Vec?

Error trait in Rust is meant to be just that, an Error. An Error has a description, source, backtrace, and all other artifacts that are singular. A list of errors is something else. A list of errors does not have any of such traits in a singular form, because they differ between each of the errors in the list.

It feels a bit as if I had a Vec<Person> and had to implement a trait that returns a name() out of it.

In my ideal world, we'd start handling lists of errors alongside single errors and Result<_, Vec<Error>> would become an actual part of Rust API design, but I don't think we're there yet :)

zbraniecki commented 3 years ago

I opened #219 to discuss the API signature of the parser. @XAMPPRocky I'd love your feedback there and if you know other people who may be able to help make a decision there, please, send them the link to that issue!

alerque commented 1 month ago

Also for cross reference, #323 has discussion of several different issues with &mut Vec<FluentError> (concurrency, allocation, ergonomics, etc.).