Skip to content

Fix StringIndexOutOfBoundsException in FileCache.didChange() for piecemeal didChange events (#112) #113

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

Merged
merged 3 commits into from
May 1, 2025

Conversation

chenyenchung
Copy link
Contributor

Description

This PR fixes a bug in FileCache.didChange() that causes a StringIndexOutOfBoundsException when processing multiple incremental text changes in a single didChange event (#112). The issue occurs when LSP clients send character-by-character insertions as separate changes within the same event.

Problem

The current implementation:

  1. Sorts changes by start position
  2. Applies all changes using positions relative to the original text
  3. Uses a previousEnd pointer to track progress

This approach fails when changes modify text length because subsequent change positions become invalid.

Solution

This patch:

  • Removes the sorting logic (unnecessary for sequential application)
  • Applies each change to the current text state
  • Updates the text after each change to maintain correct positions

Testing

This proposed fix seems to:

Please let me know if you have any questions! Happy to further improve the edit.

@bentsherman
Copy link
Member

Thanks for looking into this. I think I have encountered this error in VS Code as well, mysterious out-of-range errors that I could never reproduce

I guess I was trying to be clever, but I see now that applying the changes all at once clearly isn't robust

Although it raises a question I hadn't considered before -- why does it work with sequential application? If I receive a didChange with changes 1, 2, and 3 in that order, is change 2 aware of the shift caused by change 1? Surely it must be

@bentsherman
Copy link
Member

In any case, I will try to merge this by next week. Things are just busy right now getting ready for the 25.04 release

@chenyenchung
Copy link
Contributor Author

Thanks for looking into this. I think I have encountered this error in VS Code as well, mysterious out-of-range errors that I could never reproduce.

No problem! I don't seem to encounter this a lot in VSCode, but I somehow did not find a way to log at debug level, so most of the debugging was in neovim.

I guess I was trying to be clever, but I see now that applying the changes all at once clearly isn't robust
Although it raises a question I hadn't considered before -- why does it work with sequential application? If I receive a didChange with changes 1, 2, and 3 in that order, is change 2 aware of the shift caused by change 1? Surely it must be

The specification seems to set the expectation that change 2 would be described on the basis of change 1, so I guess different clients have their own implementation to do so.

In any case, I will try to merge this by next week. Things are just busy right now getting ready for the 25.04 release.

That's awesome. Looking forward to the new version!

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman linked an issue May 1, 2025 that may be closed by this pull request
@bentsherman
Copy link
Member

Failing tests are false positives caused by 7fdcac1, aside from that everything looks good

@bentsherman bentsherman merged commit e470f4f into nextflow-io:main May 1, 2025
1 check failed
bentsherman added a commit that referenced this pull request May 2, 2025
---------

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Co-authored-by: Ben Sherman <bentshermann@gmail.com>
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.

FileCache.didChange() crashes with character-by-character insertions
2 participants