Skip to content

ColumnRanges: unicode support - #26

Merged
brandonchinn178 merged 5 commits into
brandonchinn178:mainfrom
dcastro:diogo/utf-16
Jul 2, 2026
Merged

ColumnRanges: unicode support#26
brandonchinn178 merged 5 commits into
brandonchinn178:mainfrom
dcastro:diogo/utf-16

Conversation

@dcastro

@dcastro dcastro commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

When I added ColumnRange in #17, I failed to give unicode proper thought!

It seems that ColumnRange (as of the switch to bytestring parser) is now counting columns as bytes.

Since ColumnRange is meant to feed into the LSP server, and the LSP protocol counts columns as UTF-16 code units by default, I wrote this PR to fix how columns are counted.

I'm aware you recently made performance optimizations, so I ran the benchmarks before + after, and the results don't seem to have changed, but please do feel free to double-check them to be safe.

@brandonchinn178 brandonchinn178 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main hesitation here is implementing utf-16 logic here, since I'm not a utf expert and cant commit to maintaining it. Is there a third party library we could use here? Or a simpler function? AI suggests:

import qualified Data.Text.Lazy as TL
import Data.Text.Lazy.Encoding (decodeUtf8)
import Data.Char (ord)

utf16CodeUnits :: BL.ByteString -> Int
utf16CodeUnits bs =
  TL.foldl' (\acc c -> acc + if ord c >= 0x10000 then 2 else 1) 0
             (decodeUtf8 bs)

@dcastro

dcastro commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

My main hesitation here is implementing utf-16 logic here, since I'm not a utf expert and cant commit to maintaining it.

Understandable! For what it's worth, I don't think it'll require much/any maintenance. I added a property test to ensure it matches encodeUtf16BE from the text package.

Is there a third party library we could use here?

Unfortunately, I couldn't find any! The text package, version 1.x, used to have a function that returns the utf16 length, but the function was removed in v2.

Or a simpler function? AI suggests:

Yup, that would work too.
Codepoints up to U+FFFF are encoded with 1 unit, and U+10000 and up with 2 units.

The main difference would be in performance, since the AI suggestion needs to convert from bytestring to text first. Not sure how big the perf hit would be in practice though.

Comment thread test/XReferee/SearchResultSpec.hs Outdated
@dcastro

dcastro commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Just read your blog post where you mentioned using hyperfine, so I ran that as well:

$ hyperfine --ignore-failure --warmup 3 xreferee-old xreferee-new
Benchmark 1: xreferee-old
  Time (mean ± σ):     767.6 ms ±  29.1 ms    [User: 587.5 ms, System: 935.4 ms]
  Range (min … max):   731.6 ms … 817.7 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: xreferee-new
  Time (mean ± σ):     881.0 ms ±  32.6 ms    [User: 679.0 ms, System: 955.3 ms]
  Range (min … max):   845.4 ms … 949.6 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  xreferee-old ran
    1.15 ± 0.06 times faster than xreferee-new

@brandonchinn178 brandonchinn178 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just some minor tweaks, but otherwise looks great!

Comment thread test/XReferee/SearchResultSpec.hs Outdated
Comment thread test/XReferee/SearchResultSpec.hs Outdated
@brandonchinn178
brandonchinn178 merged commit ab5f3c1 into brandonchinn178:main Jul 2, 2026
3 checks passed
@brandonchinn178

Copy link
Copy Markdown
Owner

Thanks!

@brandonchinn178

Copy link
Copy Markdown
Owner

Do you need a release, or is the code being in main sufficient?

@dcastro

dcastro commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thank you! 🙇 No need for a release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants