Skip to content

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!

@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 <[email protected]>
Co-authored-by: Ben Sherman <[email protected]>
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