Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Fix error causing tree-sitter highlighting to get out-of-sync when editing rapidly #18375

Merged
merged 2 commits into from
Nov 3, 2018

Conversation

maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented Oct 31, 2018

Identify the Bug

When using Tree-sitter, it was previously possible to get the buffer and the syntax tree into an inconsistent state by making many rapid changes to the buffer.

See also:

Description of the Change

Whenever you make a change to a buffer that's using Tree-sitter, we start incrementally re-parsing the buffer. If that parse does not complete quickly, we allow it to finish parsing in the background. Then, if you make additional edits while that parse is still going, we need to store those edits so that we can apply them after-the-fact to the new syntax tree.

We store the edits in a Patch in order to group the changes efficiently. Unfortunately, the code that was mutating that patch was passing the wrong values to Patch.splice. I'm not sure how this huge logic error didn't cause more problems than it did!

Verification Process

The easiest way I found to reproduce the original bug was to open a large JavaScript file, place the cursor somewhere near the top of the file, and hold down command-x (Editor: Delete Line). Previously, you would periodically get broken syntax highlighting.

Now, with this change, I'm not seeing this problem.

Release Notes

  • Fixed an issue where making many rapid edits could cause incorrect syntax highlighting to be rendered.

@maxbrunsfeld
Copy link
Contributor Author

/cc @Ben3eeE - I think this may be contributing to #18342, though there might be other problems causing that.

@maxbrunsfeld
Copy link
Contributor Author

/cc @as-cii - I think this is the bug that you mentioned to me the other day. Gonna take your suggestion of using a randomized test to cover this.

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Nov 1, 2018

I was still able to reproduce the issue with syntax highlighting breaking and Atom freezing that I saw after building Atom on this commit.

I saw syntax highlighting break when pasting a 42 lines long erb file holding ctrl-v until it pasted to a couple of thousand lines.
I got atom to freeze by pasting a 7000 line erb file and undoing rapidly.

I send the files I used on Slack.

@maxbrunsfeld
Copy link
Contributor Author

I was still able to reproduce the issue with syntax highlighting breaking and Atom freezing that I saw after building Atom on this commit.

Ok, thanks for testing that. I think that there's a separate issue relating to ERB specifically. I'll do a little more testing to make sure that it is actually ERB-specific.

@as-cii
Copy link
Contributor

as-cii commented Nov 2, 2018

I think this is the bug that you mentioned to me the other day.

Yes, it seems quite possible.

Gonna take your suggestion of using a randomized test to cover this.

Nice! ✨

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Nov 2, 2018

@maxbrunsfeld Just wanna say I was able to reproduce the issue that this PR fixes to confirm the fix.

I could reproduce it by just typing fast at the top of text-editor-component. On this branch it did not reproduce again.

ERB files seem like a different issue 👍

@maxbrunsfeld maxbrunsfeld force-pushed the mb-highlighting-on-fast-edits branch from 8014e81 to 815cd2b Compare November 3, 2018 00:07
@maxbrunsfeld maxbrunsfeld merged commit e0ace16 into master Nov 3, 2018
@maxbrunsfeld maxbrunsfeld deleted the mb-highlighting-on-fast-edits branch November 3, 2018 06:26
maxbrunsfeld pushed a commit that referenced this pull request Nov 6, 2018
Fix error causing tree-sitter highlighting to get out-of-sync when editing rapidly
maxbrunsfeld pushed a commit that referenced this pull request Nov 6, 2018
Fix error causing tree-sitter highlighting to get out-of-sync when editing rapidly
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.

3 participants