LanguageTag::parse should allow to retrieve T from the Err variant

[dodomorandi]

When LanguageTag is used with an owned inner type like String, there is a problem in case of error: it is not possible to get back the String, which is useful in order to communicate back which kind of language tag was invalid.

I see two possible solutions for this, and both are breaking changes:

1. Change the signature in the following way:

   pub fn parse(tag: T) -> Result<Self, (LanguageTagParseError, T)>

   This is... debatable. I am not a fan of this approach.
2. Make LanguageTagParseError generic over T and make it stateful. Then it could gain an into_inner function in order to take back the original value. This obviously has the downside of making the error type generic, which is slightly worse in ergonomics.

Let me know your point of view and if you have other solutions in mind. I would be happy to contribute with a PR if you want.

In any case, thank you for your efforts!

[Tpt]

Hi! That's a good point! I also had this problem. I agree that LanguageTagParseError contain the String definitely make sense and your option 2 is the best.

A question I am asking myself: if we include the language tag in the error, should it be printed alongside of the error message or should we keep the error Display implementation unchanged? I am slightly more in favor of the first option but I am still a bit unsure.

[dodomorandi]

Thank you @Tpt for the fast reply!
Good point for the impl Display of LanguageTagParseError. I do not have a strong opinion for one solution or another, but I am inclined toward your point because the current errors are just related to subtags and extlangs, the "original" whole value does not seem to be extremely relevant.

In any case, feel free to take your time to make a decision (in the end, it would be a breaking change) and then let me know if you want me to send a PR. ☺️

[Tpt]

I have an other concern: Let's say that the user wants to parse a &'a str with 'a a local lifetime. If we have a LanguageTagParseError<&'a str> then the error struct will be limited to this local lifetime and the caller won't be able to easily return it like before. I am not sure how to properly fix that...

[dodomorandi]

That's a very good point indeed. It is pretty common to bubble up errors, even to main, therefore it's something that should be handled.

I can propose you to have two error types, a stateful one and a stateless. Obviously we already have the stateless error, what would change is that LanguageTag::parse would return the stateful error instead, which can be eventually be converted into the stateless.

Let me write a possible (rough) implementation, just to be sure we are on the same page:

impl<T: Deref<Target = str>> LanguageTag<T> {
    pub fn parse(tag: T) -> Result<Self, LanguageTagStatefulParseError<T>> {
        // Cannot use chaining (because `tag` is otherwise moved). `let-else` could make things
        // a bit cleaner.
        let positions = match parse_language_tag(&tag, &mut VoidOutputBuffer::default()) {
            Ok(positions) => positions,
            Err(error) => {
                return Err(LanguageTagStatefulParseError { error, value: tag });
            }
        };
        Ok(Self { tag, positions })
    }
}

pub struct LanguageTagStatefulParseError<T> {
    error: LanguageTagParseError,
    value: T,
}

impl<T> LanguageTagStatefulParseError<T> {
    #[inline]
    pub fn into_stateless(self) -> LanguageTagParseError {
        self.error
    }

    #[inline]
    pub fn into_value(self) -> T {
        self.value
    }

    #[inline]
    pub fn into_stateless_and_value(self) -> (LanguageTagParseError, T) {
        (self.error, self.value)
    }
}

// This is unideal, but I think that we would need specialization for a better solution...
impl<T> fmt::Debug for LanguageTagStatefulParseError<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("LanguageTagStatefulParseError")
            .field("error", &self.error)
            .field("value", &"[original value]")
            .finish()
    }
}

impl<T> fmt::Display for LanguageTagStatefulParseError<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        self.error.fmt(f)
    }
}

impl<T> Error for LanguageTagStatefulParseError<T> {}

Obviously, this is just a proposal and it is also something that should be evaluated thoughtfully. For instance, I opted for making LanguageTagStatefulParseError fields private and use converting functions, but could we just make them public? And in this case, should we use #[non_exhaustive]? It is a balance between keeping the implementation separated from the API and provide a better ergonomics.

Nevertheless, I already wrote too much. What do you think? Do you have alternative ideas?

[Tpt]

Thank you for this proposal. Having two error types sounds a bit cumbersome and missleading.
The more I think about it the more I like the option 1 in your proposal i.e. pub fn parse(tag: T) -> Result<Self, (LanguageTagParseError, T)>.
This is not very nice but keep things clear and simple in my opinion. What do you think about it?

[dodomorandi]

From one point of view, it is easy to see what's going on from the signature, no needs for two error types and helper functions. From another perspective however, the error variant is not an actual error (just because the tuple does not implement Display + Error), and the user will always need to perform a map_err in order to bubble up the real error.

Don't get me wrong, I am playing devil's advocate because I am doubtful about both the solutions as well. In any case, if both solutions have pros and cons, at least using a tuple introduces less new things. I can imagine that it could be better to have a future breaking change in which the stateful error is added, than introducing it now and removing it in a future release.

If you are in contact with other developers using `oxilangtag', maybe it could be an idea just to ask them what they would like.