Make it possible for combine::primitives::ParseError to be std::error::Error for more range types

[ncalexan]

Over in https://github.com/mozilla/mentat, we're using combine to parse &'a [edn::Value] slices. Here, edn::Value is a generic value type conceptually similar to JSON. What is happening here is not specific to edn::Value, so I'll use T: we're using combine to parse &'a [T] streams.

The error type of such a Parser is combine::primitives::ParseError<&'a [T]>. Now, around

combine/src/primitives.rs

Line 449 in 6f2cec6

impl<S> StdError for ParseError<S>

, you'll see that such an error will be a std::error::Error only when the underlying Stream::Range is std::fmt::Display. However, no slice &'a [T] can ever satisfy this trait bound due to rust-lang/rust#39328.

I can think of the following ways to work around this issue:

• wait for the Rust specialization feature to land in stable, so that we can specialize the implementation of std::error::Error for &'a [edn::Value].
  Even with specialization, this may not allow to solve the problem, depending on the restrictions on third-party types.
• wrap our &'a [T] streams in a newtype implementing Stream, and wrap the Stream::Range type as well.
  I have done this locally; it's not hard, but I expect it to have a non-trivial runtime cost, since all ranges need to be wrapped and unwrapped. I'd be surprised if the compiler could make this zero-cost, but I'd love to be amazed.
• add a new function to the Range trait specifically for wrapping or otherwise helping format Range instances.
  I doubt this would be difficult, but it would be a breaking API change: existing consumers with non-default Range implementations would have to add the new method, even if they didn't care about this issue (which they can't have been using without significant effort).
• add a new RangeDisplay trait in combine, define it for the Range types in combine, and expect that in the std::error::Error implementation.
  This is the least intrusive solution, but ties all Range implementations to a single display format. I think this is okay, though -- it's already the case that &str ranges have a single display format.

@Marwes, have you thought about this problem? Is there an alternate approach you can suggest? Would you accept a patch for one of the final two proposed solutions?

[ncalexan]

Oh, I forgot a wrinkle: we want std::error::Error so that we can use https://github.com/brson/error-chain. However, error-chain doesn't support error types that contain references and lifetimes (although I can't find the issue where I learned this right now). That means it might not be enough to solve this problem for us; we might need additional support for converting range types to owned types in errors.

Maybe what I want is to require ToOwned when constructing the Info objects in errors? It looks like one type can have multiple ToOwned implementations, so this still might not be specific enough.

[Marwes]

That means it might not be enough to solve this problem for us; we might need additional support for converting range types to owned types in errors.

This I think is the first problem we should solve. For the errors actually created by the parser I don't see any way around the the errors having lifetimes in them. I think the best way to work around that is to make it easy to transform a ParseError with lifetimes into one without them. If ParseError were defined as a type with three parameters (ParsError<Position, Item, Range>) instead of the current which only takes Input https://docs.rs/combine/2.2.2/combine/struct.ParseError.html, then it would be possible to provide a map function.

fn map(self, f: impl FnMut(Position) -> Position2, g: impl FnMut(Item) -> Item2, h: impl FnMut(Range) -> Range2) -> ParseError<Position2, Item2, Range2>

I always wanted to this change but I have held out waiting for rust-lang/rust#21903. If that issue were fixed it should be possible to do

type ParseError<S: StreamOnce> = ParseErrorEx<S::Position, S::Item, S::Range>;

struct ParseErrorEx<P, I, R> { .. }

[ncalexan]

Ah, interesting, that would be nice. I didn't want to generalize ParseError into having three types because it churns a lot of combine, and it churns external consumers.

Locally, I have elected to post-process ParseError<&'a [T]> into a simpler error with owned ranges. To do that, I implemented a map-like function map_err_range for ParseError, Error, and Info. I'll post a patch, since it could live in combine; but I think everything's pub, so this mapping could happen in our consuming code as well.