search

rust-dev-tools / rls-vfs

Virtual File System for the RLS
17 stars 11 forks source link

Handle UTF-16 code unit offsets in file changes API #24

Closed Xanewok closed 5 years ago

Xanewok commented 5 years ago

This adds the ability to change text with ranges defined in UTF-16 code units (per LSP spec, needed for the RLS).

Since we embed index base in Span type, we could also embed the 'text atom' there as well. Should we do that? The tricky bit is that conversion between those requires a source buffer - to change from 0-based to 1-based indexing we only need to add/subtract 1 but to translate column offset we need actual string of characters for which the range is defined. Since we need the source buffer, I decided that it'd be best to implement that API on VFS directly, because it operates on the buffer directly.

Should I come up with more tests? Previously crashing UTF-16 test case now works and RLS using this patched vfs version doesn't crash and reformats succesfully the test case from https://github.com/rust-lang-nursery/rls/issues/1104.

Xanewok commented 5 years ago

Done! Similar to Indexed trait in rls-span, I introduced Split trait implemented for UnicodeScalarValue and Utf16CodePoint void structs.

However, this seems like ergonomics loss - while this binds range length and span under a single type, this introduces more type-level machinery and makes the actual types more verbose; also we can't process now series of changes with different Split markers, because we have &[Change<S>] with a single type and Change::AddFile also has to carry this marker trait, although it's unrelated.

Maybe it'd make more sense to implement an enum ChangeText with those two USV/UTF-16 variants instead? This wouldn't infect outer types and corresponding function signatures with another generic parameter. @nrc what do you think?

nrc commented 5 years ago

Maybe it'd make more sense to implement an enum ChangeText with those two USV/UTF-16 variants instead? This wouldn't infect outer types and corresponding function signatures with another generic parameter. @nrc what do you think?

This sounds like a good compromise to me

Xanewok commented 5 years ago

Pushed a commit with second approach. Does this look good now?

EDIT: For comparison, previous commit with type parameter is at https://github.com/Xanewok/rls-vfs/commit/62ee1b90b968bd3e24ae60db9fd6ed995fccb086.

nrc commented 5 years ago

Thanks!