Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crashes when editing wide characters spanning 2 `u16`s (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 `u16`s (UTF-16) Nov 2, 2018
Copy link
Member Author

@Xanewok Xanewok left a comment

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 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 Xanewok force-pushed the Xanewok:fix-format-range branch from 84992c2 to b672685 Nov 6, 2018
@Xanewok
Copy link
Member Author

@Xanewok 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

@nrc
nrc approved these changes Nov 11, 2018
Copy link
Member

@nrc nrc left a comment

Looks good, thank you!

@nrc nrc merged commit 0e7432a into rust-lang:master Nov 11, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.