Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
XAMPPRocky opened this issue Apr 28, 2020 · 7 comments
Open

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

XAMPPRocky opened this issue Apr 28, 2020 · 7 comments
Labels
crate:fluent-bundle Issues related to fluent-bundle crate design-decision Issues pending design decision, usually Rust specific enhancement good first issue Want to help? Those are great bugs to start with! help wanted We need help making decisions or writing PRs for this. question

Comments

@XAMPPRocky
Copy link

XAMPPRocky commented Apr 28, 2020

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
Copy link
Collaborator

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 :)

@zbraniecki zbraniecki added design-decision Issues pending design decision, usually Rust specific enhancement crate:fluent-bundle Issues related to fluent-bundle crate good first issue Want to help? Those are great bugs to start with! help wanted We need help making decisions or writing PRs for this. question labels Jun 16, 2020
@XAMPPRocky
Copy link
Author

XAMPPRocky commented Jun 16, 2020

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
Copy link
Collaborator

@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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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!

@zbraniecki zbraniecki added this to the 0.16 milestone Feb 9, 2021
@alerque alerque removed this from the 0.16 milestone May 6, 2024
@alerque
Copy link
Collaborator

alerque commented May 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:fluent-bundle Issues related to fluent-bundle crate design-decision Issues pending design decision, usually Rust specific enhancement good first issue Want to help? Those are great bugs to start with! help wanted We need help making decisions or writing PRs for this. question
Projects
None yet
Development

No branches or pull requests

3 participants