Add option for a Span to be based on bytes rather than characters

[Spu7Nix]

Right now, the error drawer seems to assume that span.start() and span.end() are character indices rather than byte indices. This might be useful for manually choosing the error position, but most lexers use byte positions instead of character positions.

This makes a difference when you use Unicode characters in the source. This is a language that is lexed using logos, which uses byte indices:

Error: Index 5 is out of range of array (length 3)
   ╭─[test/test.spwn:3:6]
   │
 3 │ ╭─▶ arr[5] = 1
 4 │ ├─▶ // this should not be in the error
   · │
   · ╰──────────────────────────────────────── Index 5 is out of range of array (length 3)
───╯

The error gets offset because of some Unicode characters before the error position:

// øæåææ
let arr = [1, 2, 3]
arr[5] = 1
// this should not be in the error

I am not sure what the most idiomatic API for this is, but one way could be to have an optional function in the Span trait that looks something like fn uses_bytes() -> bool, where the default implementation just returns false. Another way is to add the option to the Config struct.

[danaugrs]

I was using spans with byte-based indexing and came across this issue so I changed to character-based indexing. It actually turned out to be a bit more efficient as well.

[brockelmore]

fwiw I created a fork that does this (but removes char based version entirely) here

All it does is switch this line: https://github.com/zesterer/ariadne/blob/ccd465160129d2546d67f7ee78727a4098a64330/src/source.rs#L94

to:

let len = line.chars().fold(0, |mut acc, i| {acc += i.len_utf8(); acc });

I am happy to open a PR that adds a function from_with_byte_offsets(s: S) -> Self where S: AsRef<str> to Source if that is desirable

[Johan-Mi]

let len = line.chars().fold(0, |mut acc, i| {acc += i.len_utf8(); acc });

Isn't that simply line.len()?

[brockelmore]

haha TIL String::len gives the length in bytes! yes you are correct

[goto-bus-stop]

I'm interested in working on this. @zesterer would you be open to switching entirely to byte indices? Or should both ways be supported?

[zesterer]

I'm interested in working on this. @zesterer would you be open to switching entirely to byte indices? Or should both ways be supported?

Definitely interested! In my view, spans should use byte indices by default, with some built-in way to look up character indices. I'm happy to accept a temporary solution for now: long-term, I think the crate needs more substantial changes anyway.

[VonTum]

Is there a branch which already solves this?

[zesterer]

Not yet, no. I don't currently have the time to work on it, although I wish I did.

[VonTum]

I see in the code it's possible to provide your own impl Cache<>, perhaps turning Source into a trait, (or of course for backward compatibility, have Source implement a new SourceTrait), which can have differing implementations such that one can also avoid the String allocations for each line, and allow byte indexing.

[zesterer]

This has been suggested elsewhere, yes. That's probably something to add to the list for an eventual refactor.

[VonTum]

I'm getting started in a PR to implement this, which approach would you prefer @zesterer ?

• Extend the Span trait with a fn is_byte_span(&self) -> bool, returning false by default not to break existing code, then add a ByteSpan impl Span, or
• Add a field or template parameter to Report/ReportBuilder that sais if the given spans are byte spans.

The first is of course more general, even allowing users to combine Char and Byte spans, but requires two new structs for Span with and without SourceId.

The second is less general, requiring the user to specify it for the whole report. Which is more ergonomic, but limits freedom

[VonTum]

Well, I've finally come to the point where I need to switch it over. I'll implement it as a boolean flag to ReportBuilder.