Closed Spu7Nix closed 4 months ago
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.
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
let len = line.chars().fold(0, |mut acc, i| {acc += i.len_utf8(); acc });
Isn't that simply line.len()
?
haha TIL String::len
gives the length in bytes! yes you are correct
I'm interested in working on this. @zesterer would you be open to switching entirely to byte indices? Or should both ways be supported?
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.
Is there a branch which already solves this?
Not yet, no. I don't currently have the time to work on it, although I wish I did.
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.
This has been suggested elsewhere, yes. That's probably something to add to the list for an eventual refactor.
I'm getting started in a PR to implement this, which approach would you prefer @zesterer ?
fn is_byte_span(&self) -> bool
, returning false by default not to break existing code, then add a ByteSpan impl Span
, orReport/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
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.
Any chance that this feature could get into a patch release soon? I'm fine with putting
ariadne = { git = "https://github.com/zesterer/ariadne.git", rev = "a061071" }
in my Cargo.toml
for now, but it would be much nicer if this could be version 0.4.1
instead.
EDIT:
Actually, while playing around with this feature, I discovered that the column number of the offset of a report is printed incorrectly when using byte indices. I created a PR to fix this: #113.
I've just released 0.4.1
, which includes these changes. Thanks @VonTum and @Zollerboy1 for the contributions!
Right now, the error drawer seems to assume that
span.start()
andspan.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:
The error gets offset because of some Unicode characters before the error position:
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 likefn uses_bytes() -> bool
, where the default implementation just returnsfalse
. Another way is to add the option to theConfig
struct.