-
Notifications
You must be signed in to change notification settings - Fork 13.1k
fix58438: do not delete comments/code following unterminated string completion #58742
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
fix58438: do not delete comments/code following unterminated string completion #58742
Conversation
Co-authored-by: Daniel Rosenwasser <[email protected]>
src/services/utilities.ts
Outdated
| export function createTextSpanFromStringLiteralLikeContent(node: StringLiteralLike, position?: number) { | ||
| let replacementEnd = node.getEnd() - 1; | ||
| if (node.isUnterminated) { | ||
| // we return no replacement range only if unterminated string is empty | ||
| if (node.getStart() === replacementEnd) return undefined; | ||
| replacementEnd = node.getEnd(); | ||
| replacementEnd = position !== undefined && position <= node.getEnd() ? position : node.getEnd(); |
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.
There are only two callers of createTextSpanFromStringLiteralLikeContent, and neither of them omit the position argument, so just make it required (and document what position is supposed to mean)
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.
Also, do you have any tests for when position is strictly greater than the end of the node? I don't think I'm seeing any.
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.
Ah, that was old code, thanks for catching it! (I tried to make position optional, but it ended up being needed in more places)
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.
It shouldn't be possible in a completion for position > node end but I will add that check incase createTextSpanFromStringLiteralLikeContent will be used in a non-completion context in the future.
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.
The only thing I can think of is possibly an unterminated string literal on the previous line
let s: "string" = "s
/*$*/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.
Completions won't be triggered on new lines -- so
type LongString = `asdf
string
line`
let s: LongString = `asdf
/**/
^ no suggestions
let r: LongString = `asdf/**/
^ completes to `asdf\nstring\nline`|
@typescript-bot cherry-pick to release-5.5 |
|
Hey, @DanielRosenwasser! I've created #58762 for you. |
…e-5.5 (#58762) Co-authored-by: Isabel Duan <[email protected]>
fixes #58438
The above issue is a regression bug that happened because of #57839, which was included with 5.5 beta. This PR updates the
replacementRangeto be more inline with 5.4 behavior, namely completions on unterminated strings will not remove anything to the right of the cursor. Closed strings will replace everything within the quotes.Examples:
becomes
after completions are triggered at all three locations.