Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Fix crashes when editing wide characters spanning 2 u16s (UTF-16) #1112

Merged
merged 7 commits into from
Nov 11, 2018

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Nov 2, 2018

Blocked on rust-dev-tools/rls-vfs#24.
Fixes #1104.

r? @nrc

@Xanewok Xanewok changed the title Fix format range Fix crashes when editing wide characters spanning 2 u16s (UTF-16) Nov 2, 2018
Copy link
Member Author

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

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

Just noticed one caveat, will need to fix that before it lands.

src/lsp_data.rs Show resolved Hide resolved
@Xanewok
Copy link
Member Author

Xanewok commented Nov 2, 2018

With rust-lang/rfcs#2457 merged we additionally now have to be careful around Unicode identifiers and their, currently wrong, Range calculation...
(also VSCode parser seems to choke on non-ASCII identifiers?)

(ノ ゜Д゜)ノ ︵ ┻━┻

utf16

UTF-16 is a can of worms. While this PR fixes an unrecoverable crash and possible text buffer corruption, the remaining UTF-16 Range thing is mostly displaying diagnostics, so I'd treat it with a lower priority (but still something that we need to fix!).

@Xanewok
Copy link
Member Author

Xanewok commented Nov 9, 2018

CI looks good but some tests keep failing for now (#1118). It'd be good to also adjust remaining LSP ranges to send UTF-16 code unit offset (#1113) but I think we can send a follow-up PR for that (only affects visual squiggle length).

r? @nrc

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@nrc nrc merged commit 0e7432a into rust-lang:master Nov 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants