WIP: Add byte offset to TextPos.

[Jayonas]

The goal here is to provide a byte position/offset in errors in addition to a row/col position. The byte position is helpful for callers that want to do more sophisticated error reporting, such as:

1. showing source annotations using libraries like annotate_snippets, which naturally expect annotation positions as byte offsets,
2. using a more complex row/col calculation algorithm, such as accounting for single glyphs that use multiple chars / code points, or doing tab expansion.

The changes are pretty straight forward, and I only ran into one problem that I need feedback to resolve: there's a test that requires size_of::<Error> <= 64, but the new data in TextPos increases it to 72. For now I commented that test out just to make sure all the other tests run and pass. Any thoughts about this? Can we just allow Error to get bigger? Can we remove and/or shrink something else to compensate? Can we potentially do the byte to row/col calculation lazily (e.g. as a method on Error) instead of storing row/col data in the error (I'm not sure that the necessary original data string would still be available, plus that could be a pretty break-y change)? Can we hide this behind a feature flag? Or does this error size limit potentially sink this PR/idea?

I also left a TODO in the Display impl of TextPos because I'm not sure if/how you might want to incorporate the byte position into that.

Sort of unrelatedly -- I added a new TextPos::start constructor for the not uncommon case where an error has no meaningful position and so just indicates the start of the data. Seemed like a nice refactor, but it's obviously optional for my goal and I'm happy to change and/or revert that as desired.

[adamreichold]

The changes are pretty straight forward, and I only ran into one problem that I need feedback to resolve: there's a test that requires size_of:: <= 64, but the new data in TextPos increases it to 72. For now I commented that test out just to make sure all the other tests run and pass. Any thoughts about this? Can we just allow Error to get bigger? Can we remove and/or shrink something else to compensate? Can we potentially do the byte to row/col calculation lazily (e.g. as a method on Error) instead of storing row/col data in the error (I'm not sure that the necessary original data string would still be available, plus that could be a pretty break-y change)? Can we hide this behind a feature flag? Or does this error size limit potentially sink this PR/idea?

Due to the pervasiveness of Result<(), Error> in the tokenizer and parser code, the size of that type is highly relevant for the overall performance. Especially since we pay for the error case even when the majority of processed documents are valid.

Personally, I think this should be resolved by more aggressively pessimizing the error case and thereby improve the success case by boxing the error. We do depend on alloc even in no_std context so changing all relevant types to Result<T, Box<Error>> should not reduce the usefulness of the library while actually improving performance in the common case and making the size of Error itself largely irrelevant.

[RazrFalcon]

We should either return a byte offset for error or text position. Not both.
I suggest returning just a byte offset and have a nice method to convert to "text position".
But that's would be a breaking change.

[adamreichold]

I suggest returning just a byte offset and have a nice method to convert to "text position".

The problem I see is that this requires access on the original text, i.e. that method would effectively end up on Document, not on Error.

[RazrFalcon]

We can have just a free function, like roxmltree::text_position(string: &str, offset: usize) -> TextPos.
Error reporting would be as nice out of the box, but it's better than having both values in the Error.

[adamreichold]

Having both values there does not cost us anything, if we box the error.

[RazrFalcon]

Having Box is an unnecessary complexity.

[adamreichold]

It is also a performance optimization as it ensures size_of::<Result<(), Box<Error>>() == size_of::<usize>() thereby improving performance throughout call chains in the parser and tokenizer. This is the same reason why anyhow works so hard to avoid trait objects (and thereby fat pointers) to ensure size_of::<anyhow::Error>() == size_of::<usize>() using unsafe and raw vtables.

Admittedly, there is trade-off between added complexity and performance gain, but in my experience, I mostly interact with errors for forwarding and logging when calling into roxmltree and there, the Box does not make a difference at all.

[RazrFalcon]

Oh, you're suggesting to put the whole Error into a Box, not just TextPos. Sure, it might help a bit, but I doubt we would see any performance differences.

But I still against storing byte offset in TextPos. We should store error position either as bytes or TextPos.

[Jayonas]

Thank you both for the feedback.

Purely from an API design standpoint, I personally think that errors should only store the byte offset, since that's the more fundamental data, i.e. row/col is a function of the byte position (and data string). Strictly speaking, I don't think that the actual byte -> row/col calculation is really core functionality for this crate, although I still think it makes good sense to include a basic algorithm like this because it's easy and it gives callers an almost trivial way to greatly improve error reports. So I think an ideal API design would be that callers can get the byte offset from the error and then they could optionally pass that to a function to turn it into a row/col.

The difficulties would all be in how we get from here to there. It would obviously be a breaking change from the current API. And as discussed, the calculation requires the string, which complicates things a little. The functionality already exists as Document::text_pos_at, but that assumes that you have a Document, which you don't if you called Document::parse and received an Error instead. That said, at that point you should still have the string that you passed to Document::parse, so you have all the data that you need, the question would mainly be about ergonomics. If you're using ? on Document::parse and expecting to automatically have a row/col at a higher level of the call stack where you don't have that string available, then you'd have to do some refactoring.

My ideas for a byte -> row/col API are:

• A free function, as @RazrFalcon suggested.
• A method of Error that you pass the string to.
• A TextPos constructor method (basically a free function by a different name, not sure how the discoverability compares).
• Maybe several of the above, where they all call down to the same thing (might improve discoverability).

All that said, I'm not opposed to the idea of allowing Error to grow in size without unwanted perf costs by always boxing it, or to any other potential solutions. I'll let @RazrFalcon (and/or any other maintainers) decide what they think is best for their crate. I really just want to somehow have access to the original byte position of an error, so I'm probably willing to try coding up whatever solution they'd be willing to merge. Otherwise the only options I could come up with were to use a private fork of this crate, or to write my own code to reverse the byte -> row/col algorithm. Neither option is good. I did the latter, and it works, but it feels a little absurd (and fragile).

[RazrFalcon]

I think we should start by replacing TextPos with usize. And maybe after that box errors, as a separate PR.

As for the API, just a free function should be fine. No need for a TextPos constructor and stuff.
And yes, we should remove Document::text_pos_at, since it will only confuse people.

Effectively, the new error handling approach should look like:

if let Err(error) = roxmltree::Document::parse(src_str) {
    let text_pos = roxmltree::text_pos(src_str, error.pos());
    eprintln!("Error: {} at {}", error, text_pos); // assuming we have changed `Display` for `roxmltree::Error` as well.
}

[Jayonas]

Closing this in favor of #141, where I took a stab at @RazrFalcon's preferred approach.