Repository navigation
Fix trailing formatting edit when range ends mid-token #50082
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -403,6 +403,7 @@ namespace ts.formatting { | |||||||||||
|
|
||||||||||||
| // formatting context is used by rules provider | ||||||||||||
| const formattingContext = new FormattingContext(sourceFile, requestKind, options); | ||||||||||||
| let previousRangeTriviaEnd: number; | ||||||||||||
| let previousRange: TextRangeWithKind; | ||||||||||||
| let previousParent: Node; | ||||||||||||
| let previousRangeStartLine: number; | ||||||||||||
|
|
@@ -439,12 +440,32 @@ namespace ts.formatting { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (previousRange! && formattingScanner.getStartPos() >= originalRange.end) { | ||||||||||||
| // Formatting edits happen by looking at pairs of contiguous tokens (see `processPair`), | ||||||||||||
| // typically inserting or deleting whitespace between them. The recursive `processNode` | ||||||||||||
| // logic above bails out as soon as it encounters a token that is beyond the end of the | ||||||||||||
| // range we're supposed to format (or if we reach the end of the file). But this potentially | ||||||||||||
| // leaves out an edit that would occur *inside* the requested range but cannot be discovered | ||||||||||||
| // without looking at one token *beyond* the end of the range: consider the line `x = { }` | ||||||||||||
| // with a selection from the beginning of the line to the space inside the curly braces, | ||||||||||||
| // inclusive. We would expect a format-selection would delete the space (if rules apply), | ||||||||||||
| // but in order to do that, we need to process the pair ["{", "}"], but we stopped processing | ||||||||||||
| // just before getting there. This block handles this trailing edit. | ||||||||||||
|
Comment on lines
+451
to
+452
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| const tokenInfo = | ||||||||||||
| formattingScanner.isOnEOF() ? formattingScanner.readEOFTokenRange() : | ||||||||||||
| formattingScanner.isOnToken() ? formattingScanner.readTokenInfo(enclosingNode).token : | ||||||||||||
| undefined; | ||||||||||||
|
|
||||||||||||
| if (tokenInfo) { | ||||||||||||
| if (tokenInfo && tokenInfo.pos === previousRangeTriviaEnd!) { | ||||||||||||
| // We need to check that tokenInfo and previousRange are contiguous: the `originalRange` | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| // may have ended in the middle of a token, which means we will have stopped formatting | ||||||||||||
| // on that token, leaving `previousRange` pointing to the token before it, but already | ||||||||||||
| // having moved the formatting scanner (where we just got `tokenInfo`) to the next token. | ||||||||||||
| // If this happens, our supposed pair [previousRange, tokenInfo] actually straddles the | ||||||||||||
| // token that intersects the end of the range we're supposed to format, so the pair will | ||||||||||||
| // produce bogus edits if we try to `processPair`. Recall that the point of this logic is | ||||||||||||
| // to perform a trailing edit at the end of the selection range: but there can be no valid | ||||||||||||
| // edit in the middle of a token where the range ended, so if we have a non-contiguous | ||||||||||||
| // pair here, we're already done and we can ignore it. | ||||||||||||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment describes the fix for this bug. |
||||||||||||
| const parent = findPrecedingToken(tokenInfo.end, sourceFile, enclosingNode)?.parent || previousParent!; | ||||||||||||
| processPair( | ||||||||||||
| tokenInfo, | ||||||||||||
|
|
@@ -888,6 +909,7 @@ namespace ts.formatting { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (currentTokenInfo.trailingTrivia) { | ||||||||||||
| previousRangeTriviaEnd = last(currentTokenInfo.trailingTrivia).end; | ||||||||||||
| processTrivia(currentTokenInfo.trailingTrivia, parent, childContextNode, dynamicIndentation); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
|
@@ -976,6 +998,7 @@ namespace ts.formatting { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| previousRange = range; | ||||||||||||
| previousRangeTriviaEnd = range.end; | ||||||||||||
| previousParent = parent; | ||||||||||||
| previousRangeStartLine = rangeStart.line; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| /// <reference path="fourslash.ts" /> | ||
|
|
||
| //// const x = `${0}/*0*/abc/*1*/`; | ||
|
|
||
| format.selection("0", "1"); | ||
| verify.currentFileContentIs("const x = `${0}abc`;"); |
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.
This comment should have been included with #46875.