Fix formatting the end of a selection #46875
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
| break; | ||
| } | ||
| consumeTokenAndAdvanceScanner(tokenInfo, node, nodeDynamicIndentation, node); | ||
| } | ||
|
|
||
| if (!node.parent && formattingScanner.isOnEOF()) { |
There was a problem hiding this comment.
This moved up a level in the call stack, outside the recursive processNode function. It was checking !node.parent as a way to execute only at the top of that recursive chain, but it’s simpler to just move it out of the recursion altogether. Also, in #46832, Gabriela is calling into this with synthetic nodes that don't have parent pointers set. We can and maybe should set them, but this was a bit of a hack and it's much cleaner and safer to move it.
| if (previousRange! && formattingScanner.getStartPos() >= originalRange.end) { | ||
| const token = | ||
| formattingScanner.isOnEOF() ? formattingScanner.readEOFTokenRange() : | ||
| formattingScanner.isOnToken() ? formattingScanner.readTokenInfo(enclosingNode).token : | ||
| undefined; |
There was a problem hiding this comment.
Here’s where that block of code moved to, with some minor changes. We previously handled edits at the end of a file in this block by getting a token representation of the EOF token and passing it to processPair. But for the same to work with formatting a selection, we’ll probably be dealing with a regular token instead of an EOF.
| @@ -717,9 +721,12 @@ namespace ts.formatting { | |||
| return inheritedIndentation; | |||
| } | |||
|
|
|||
| while (formattingScanner.isOnToken()) { | |||
| while (formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) { | |||
There was a problem hiding this comment.
isOnToken() used to return false if the scanner moved past the end of the original range. Since we need to let it move past that range to apply the last edit, I removed that condition and inlined it where it was called previously.
| previousRangeStartLine!, | ||
| previousParent!, | ||
| enclosingNode, | ||
| /*dynamicIndentation*/ undefined); |
There was a problem hiding this comment.
I honestly have no idea what the right value for this is and if it ever matters, but it didn’t matter for any existing tests, so I made it optional and skipped line/indentation edits in the implementation if it’s not passed.
| @@ -344,7 +344,7 @@ namespace ts.formatting { | |||
| } | |||
|
|
|||
| export function formatNodeGivenIndentation(node: Node, sourceFileLike: SourceFileLike, languageVariant: LanguageVariant, initialIndentation: number, delta: number, formatContext: FormatContext): TextChange[] { | |||
| const range = { pos: 0, end: sourceFileLike.text.length }; | |||
| const range = { pos: node.pos, end: node.end }; | |||
There was a problem hiding this comment.
can't believe the fix here was so simple 🤦♀️ thanks!
| @@ -265,8 +267,7 @@ namespace ts.formatting { | |||
|
|
|||
| function isOnToken(): boolean { | |||
There was a problem hiding this comment.
just for my own understanding, what is isOnToken supposed to do and also why were we checking if startPos < endPos before and are not checking it now?
I guess a pre-requisite question for that: what are startPos and endPos and the several poss supposed to be?
There was a problem hiding this comment.
you answered the first part of my question already in another comment, didn't see the comment when I was reviewing
| @@ -438,6 +438,25 @@ namespace ts.formatting { | |||
| } | |||
| } | |||
|
|
|||
| if (previousRange! && formattingScanner.getStartPos() >= originalRange.end) { | |||
There was a problem hiding this comment.
why do we need the bang in previousRange! here?
There was a problem hiding this comment.
TypeScript is complaining that I’m using it before initialization, but it gets initialized in child functions. It’s complaining because the type annotation doesn’t include undefined, so it wants to prevent me from using it where it could be undefined. I think it actually could be undefined since I don’t think the child functions are guaranteed to run. Probably I should just annotate it with | undefined, but then I need to add bangs in multiple other places.
| withSemicolon++; | ||
| } | ||
| else { | ||
| withoutSemicolon++; | ||
| } | ||
| } | ||
| else if (syntaxRequiresTrailingCommaOrSemicolonOrASI(node.kind)) { |
There was a problem hiding this comment.
This change is tangentially related; it just allows us to infer a preference for semicolonless style from property and method signatures, as long as they don’t end in a comma or are in a single-line object type kinda thing where it’s very common to omit punctuation altogether, even among semicolon users.
* Fix formatting edits at end of range * Adjust test * Revert API baseline changes
I thought I filed a bug for this in the past, but I can't find it. Formatting works by advancing the scanner token by token and deciding if any rules apply for a pair of consecutive tokens. When you format a specific range, we used to stop the scanner as soon as it advanced outside that range. But that means we usually failed to apply the last edit inside the selection, because we sometimes need to compare the last token within the selection with the first token after the selection to come up with that edit. This should unblock #46832.