Error structs for librsvg - request for comments

help
#1

In librsvg we are -><- this close to being able to publish the library as a crate on crates.io. One of the last tasks is to make the public error types follow Rust best practices, and to allow for some changes in the future without API breaks. Let me explain.

Librsvg had two public error types more or less like this:

// Errors which can only happen during the loading stage
pub enum LoadingError {
    // we use libxml2, and don't want to expose its error type publically, hence a String
    XmlParseError(String),

    // yeah yeah if you can't allocate how can you even String
    OutOfMemory(String),

    // public API takes `url: &str` and this is what you get if it's malformed
    BadUrl,

    // for when the public API lets the caller pass some CSS chunk
    BadCss,

    // Toplevel element is not a `<svg>`
    NoSvgRoot,

    // Implementation detail, see below
    Io(glib::Error),

    // An implementation limit (mitigation for malicious files) was hit
    LimitExceeded(ImplementationLimit),

    // Catch-all for other errors during loading
    Other(String),
}

// Errors which can only happen during the rendering stage
pub enum RenderingError {
    // Something inside Cairo exploded
    Cairo(cairo::Status),
    
    // Similar, mitigation for malicious SVGs
    LimitExceeded(ImplementationLimit),

    // when the API lets you pass an element ID to render
    IdNotFound,
    InvalidId,

    OutOfMemory(String),
}

I don't think the public error types should expose implementation details like "librsvg uses libxml2 internally" or "the Cairo rendering backend had a problem". In particular, librsvg is going to change to Rust-only code for those tasks at some point in the future, and I would like to prevent an API break due to changing the error types. Should we move to Box<dyn Error + ...> for those variants?

For historical/practical reasons, librsvg uses Glib's GIO for reading files and such. The Rust API can also take just an AsRef<Path> and just use GIO internally without you ever knowing, so exposing a glib::Error (the Rust binding for GError from C) doesn't seem right. Would it be appropriate to type-erase this with Box as well?

It seems we do need a catch-all case for obscure corners in the code that cause things to fail. I wonder how other libraries do this.

As much as I would like to have actual types for each error variant, I think this plays badly with the following:

  • Changes in the internals in the future - we'll switch to Rust code for things like XML and rendering; in principle GIO could be hidden behind a feature flag.
  • For librsvg it is important to have localized error messages (it doesn't, right now, but that got lost in the port to Rust), so e.g. an image viewer can tell you "could not load SVG because of $blahblah" in your language. I haven't seen discussion of impl Display for MyError returning localized messages, but for user-visible things this sounds about right.

I think there's a vibe of "use concrete types for library errors, and type-erased ones for application errors" but librsvg seems to sit kind of in the middle?

2 Likes
#2

You can newtype the implementation details, like:

#[non_exhaustive] // future proofing
pub enum LoadingError {
    Io(IoError)
    XmlParse(XmlParseError),
    //...
}
pub struct IoError(glib::Error);
pub struct XmlParseError(Libxml2ErrorType);

Another strategy is to keep the enum private as the kind field of a pub struct MyError, then provide is_io_error(), is_xml_error(), etc.

#3

If you want to expose underlying errors while technically not breaking the API, you can use source() to return &glib::Error, etc. That may work well with the opaque newtypes.

#4

The "in the middle" solution for this cases is the error kind pattern, which is discussed in

I am not familiar with i18n in Rust, but I would probably avoid exposing all details about the errors just for localization.

Like, what should happen if xml parser says that "on line 92, column 62, there's missing <"? I think it would be unreasonable (and not really helpful) to localize each and every parser error. As a user, I would probably expect to see "invalid SVG file" / "некорректный SVG файл" with an optional "more details" link which gives me (developer oriented) stacktrace and specific error message in English. As a user, I can understand "svg file is somehow corrupted". I can't really understand "parse error" -- is this a problem with my file, with network, with display?

So I'd probably made LoadingError into a opaque struct with a field-less enum LoadingErrorKind, and localized the kind (or rather, made it localizible by consumers). For devs, I'd use Display/Debug of the error. If it is somehow possible to react to Cairo/glib errors, I'd add fn cairo_error(&self) -> Option<&cairo::Status> accessors.

I would remove BadUrl url variant by pushing it onto consumers.

fn frobnicate(url: &str) -> Result<Whatever, Error> // needs to handle BadUrl

->

fn validate_url(url: &str) -> Result<Url, BadUrlError>;
fn frobnicate(url: &Url) -> Result<Whatever, Error> // can't fail with bad url anymore!

I would avoid exposing Oups(String) variants. What if you decide that tracking position of XmlParseError is important? You can't really add it to String.