Inconsistent error handling

[JohnHeitmann]

If my bundle has the message:

foo = { bar }

Then format('foo') returns ('___', []). That alone was surprising. It seems like it's common behavior for most "stuff is missing" problems. Either None or ('___', [ warnings]) feel better to me. Middle layers should be able to learn and log about message glitches.

If my message was instead:

foo =
    -attr= something

Then format('foo') returns ('foo', [error]). Those two kinds of errors feel like they should be handled the same way.

To get the best of graceful degradation, clear error signaling, and consistent error handling, a good return type might be (string, [warnings]). If you want to let higher layers skip a pointer chase for the NotFound case, then maybe instead of [warnings] you can use something like:

enum Warning {
    None,
    MessageNotFound,
    IncompleteMessage([details])
}

[zbraniecki]

That's a good point! We'll want to remodel error handling soon, after we release the major revision to move us to zero-copy parsing and update the syntax to 0.8 (I have a branch that does it that I'm planning to stabilize soon!)

[zbraniecki]

I landed a major rewrite that I think helps unify the error handling, but I'm not sure if we're done yet.

At the moment, we have four major "sources" of potential errors, and we handle them slightly differently:

• fluent_syntax::parse - returns Result<ast::Resource, (ast::Resource, Vec<ParserError>)>
• fluent_bundle::resource::FluentResource::try_new - returns Result<Self, (Self, Vec<ParserError>)>
• fluent_bundle::FluentBundle::add_resource returns Result<(), Vec<FluentError>>
• fluent_bundle::FluentBundle::format returns Option<(Cow<'bundle, str>, Vec<FluentError>)>

The way my thinking goes here is the following:

Fluent parser is lenient and attempts to recover as much as possible from the source. In result, it will always return something, but it may also return errors. I use Result here to indicate whether there are any errors, but I might as well just return a tuple and you can check if the second argument is empty for the same result.

Creating FluentResource using try_new is basically the same. Both return ParserErrors.

add_resource is a bit different, because it only operates on the AST of FluentResource so basically whatever got rejected and ended up as errors from parser is ignored at this point (should be handled earlier). Now we just try to add entries to the FluentBundle and if that fails, because an entry slot is already occupied and you're trying to add a duplicated value, we report FluentErrors, but we continue adding all other entries.

format (and compound) on FluentBundle attempt to retrieve and format a message or its value. If the message cannot be retrieved we'll return None. If it can be, we'll return the appropriate formatted value but there may be errors accumulated during the resolution. In that case we'll accumulate ResolverErrors in the result.

In theory, to stay consistent with the parser, we should probably return Option<Result<T, (T, Vec<ResolverError>)>> or in the parser we should switch to (AST, Vec<ParserError>) and skip the result.

I'm not sure what's the right course of action, and if and how this inconsistency should be best resolved.

@manishearth, @JohnHeitmann , @stasm - what do you think?

[Manishearth]

Probably should make a concrete error type for each case, and then have From impls that tree up into a FluentError enum for ?-ability, at least. That lends at least some level of consistency, and you can document each error type individually.

I think including the partial result is somewhat useful, so we shouldn't get rid of that capability.

[zbraniecki]

A backpointer to a conversation on discourse where I asked about errors - https://users.rust-lang.org/t/l10n-library-api-question/19967/6

[zbraniecki]

@JohnHeitmann - could you take a look at master and let me know how the errors look like now? I'm likely going to push this to 0.8, but would like to verify that the current API is not fully confusing.

[zbraniecki]

A lot of small improvements happened to the resolver since this issue was reported.

I reviewed the examples provided by @JohnHeitmann in master branch:

1) Missing MessageReference

foo = { bar }

Returns: {bar} Errors:

[
    ResolverError(
        Reference(
            "Unknown message: bar",
        ),
    ),
]

2) Missing AttributeReference

foo =
    -attr= something

Returns: -attr= something

but I assume you meant:

foo =
    .attr = something

so, a message with no value, and with an attribute.

If you try that now, you'll get an error when unpacking the value:

    let msg = bundle
        .get_message("foo")
        .expect("Message doesn't exist.");
    let mut errors = vec![];
    let pattern = msg.value.expect("Message has no value.");
    let value = bundle.format_pattern(&pattern, None, &mut errors);

will fail with Message has no value.

I hope this is more consistent and logical.

I'm going to close this issue since it documents a different behavior, but if you feel that the current behavior is still not logical, please reopen with examples!