-
Notifications
You must be signed in to change notification settings - Fork 23
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
Simple fix for index out of bounds #567
Conversation
51d382a
to
ee4bfb7
Compare
@@ -158,7 +158,8 @@ class CodyAutocompleteManager { | |||
val originalText = editor.document.getText(TextRange(offset - caretPositionInLine, offset)) | |||
|
|||
val prefixStartPosition = maxOf(originalText.lastIndexOf("."), originalText.lastIndexOf(" "), 0) | |||
if (!lookupString.isNullOrEmpty() && | |||
if (prefixStartPosition < originalText.length && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the original stack trace, this happens with empty strings
@ "".subSequence(1, 0)
java.lang.StringIndexOutOfBoundsException: begin 1, end 0, length 0
if (prefixStartPosition < originalText.length && | |
if (prefixStartPosition + 1 < originalText.length && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was crashing on this line originalText.subSequence(prefixStartPosition + 1, originalText.length))
when originalText.length = 0
and prefixStartPosition + 1 = 1
(so prefixStartPosition = 0
).
Thus I think condition I added is valid and sufficient to prevent crash.
That said it can still result in producing empty subSequence and in such case startsWith
will be always try no matter what lookupString
is.
I would say let's double check and merge on Monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree the current diff fixes the case when originalText.length === 0
. Still, the logic reads like the bug below
if (index < text.length) {
text.charAt(index + 1)
}
My brain is just wired to not allow that kind of code.
Either way, a much cleaner fix is to use the startsWith()
overload from the Kotlin stdlib that accepts a startIndex
parameter, and remove .subSequence()
. The original code was written in Java where that overload doesn't exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great idea, but it Returns true if a substring of this string starting at the specified offset startIndex starts with the specified prefix.
So startIndex
would affect position in lookupString
while we want it to affect a prefix (originalText
).
Truth to be told I see at least one more bug there and I do not fully understand what that code is supposed to do exactly, so maybe let's discuss offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would merge but it's 9pm Friday here so don't want to make any rushed decision
LGTM 👍🏻 |
ee4bfb7
to
ac2e148
Compare
ac2e148
to
a54aeb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #565
Test plan
Truth to be told I'm totally unable to reproduce this issue artifically.
Hopefully fix is simple enough to speak for itself.