(Proposal) Parse option to make sourcepos column char-based

[Martin005]

As discussed in #762, comrak uses amount of UTF-8 bytes for the value in LineColumn::column.

But if you want to use comrak to pinpoint the exact location in the source file, the user would expect the column to point at the readable character, not at the position of a UTF-8 byte.

When you have a Markdown file that contains only the 好 character (U+597D - 0xE5 0xA5 0xBD in UTF-8 bytes), comrak puts 1:1-1:3 as the sourcepos.

Contrary to that, if you open the same Markdown file in a text editor (e.g. Notepad++), the end column is at 2, and the position is 4. comrak treats the range as end-inclusive, so it puts 1:1-1:3 instead of 1:1-1:4 as the sourcepos.

My proposal is to create a parse option (disabled by default) which, when enabled, would make the column value in LineColumn character-based, not based on UTF-8 bytes.

That would make the Markdown example above have sourcepose 1:1-1:1 (as the end-inclusivity would be maintained).

[kivikakk]

Hey @Martin005! I like the sound of this option, but a few notes:

• The way they are now is because the parser itself is largely byte-oriented. This is mostly a performance thing; it’s fast and easy to iterate over byte slices. Dealing with Unicode strings and iterating in both directions is slower for no real gain for us, and a lot more awkward in the code.
• It would necessitate a rewrite of the core to actually do this at parse-time, and it wouldn’t be an option (unless we put checks for the option everywhere). Many of the positions are calculated using other positions as a basis, so it’d be all or nothing.
• It’d be most straightforward to do it as a pass on the finished AST at the very end.

It could therefore be done completely outside of the parse: an operation that takes the input string and result AST, and modifies the AST in place to be character-oriented instead of byte-oriented. This would fail if the byte-oriented sourcepos was incorrect.

It is important to be precise about what one means by “character”. See https://doc.rust-lang.org/std/primitive.char.html for notes here and what the easiest version would be for a Rust program.

[Martin005]

@kivikakk Thank you for your notes, I really appreciate it ❤️

Totally agree with you - the best option would be to handle this after the parsing step is finished and the final AST is constructed. That way, we don't need to add a bunch of checks all over the code that would be unnecessary for the majority of users.

As this would be done outside of parsing itself, do you think it should be moved into render options instead? Or rather, it should be a separate method on Node (or somewhere completely else)?

And as for the definition of "character", the Unicode scalar value definition fits perfectly 🙂

I already have a PoC ready – could you please assign me this issue? I will create a PR shortly.

[kivikakk]

As this would be done outside of parsing itself, do you think it should be moved into render options instead? Or rather, it should be a separate method on Node (or somewhere completely else)?

Good question! I don't mind it being a parse option; render option seems wrong since it doesn't require a render to see the effect. It could be a separate method to transform the created AST once, too, on Node (or somewhere). I don't mind, to be honest, I'll let you choose :D

I already have a PoC ready – could you please assign me this issue? I will create a PR shortly.

Sweet! Done!