This repository was archived by the owner on Aug 30, 2025. It is now read-only.
Always treat ranges as exclusive in Range.contains?/2 - #763
Closed
zachallaun wants to merge 2 commits into
Closed
Conversation
This was referenced Jun 1, 2024
Collaborator
Author
|
Closing in favor of #768 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Range.contains?/2was treating the end-position of ranges as inclusive for single-line ranges, but exclusive for multi-line ranges. Our ranges should always be exclusive of the end character, however, so this was a bug. There were two other bugs/compensations that were counteracting it (at least in some cases), however:There were a lot of tests with cursor positions like this:
However, this cursor is actually "pointing" to the space after
foo, not to the last character offoo:Fixing
Range.contains?/2caused all of the tests with those kinds of cursor positions to break, but I believe those tests were incorrect in the first place. I changed those cursor positions to point to the first character of the entity instead (|foo).When detecting in strings, a number of
fetch_rangecalls used a-1end offset. This counteracted the inclusiveness of the end character inRange.contains?, but once that was fixed, string ranges became incorrect (short one character).