Skip to content

Fix formatting the end of a selection #46875

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 29, 2021
Merged

Fix formatting the end of a selection #46875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31f507f
Fix formatting edits at end of range
andrewbranch Nov 19, 2021
cdde69e
Adjust test
andrewbranch Nov 19, 2021
8bd3da4
Revert API baseline changes
andrewbranch Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 50 additions & 41 deletions src/services/formatting/formatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't believe the fix here was so simple 🤦‍♀️ thanks!

return getFormattingScanner(sourceFileLike.text, languageVariant, range.pos, range.end, scanner => formatSpanWorker(
range,
node,
Expand Down Expand Up @@ -438,6 +438,25 @@ namespace ts.formatting {
}
}

if (previousRange! && formattingScanner.getStartPos() >= originalRange.end) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need the bang in previousRange! here?

Copy link
Member Author

@andrewbranch andrewbranch Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for clarifying

const token =
formattingScanner.isOnEOF() ? formattingScanner.readEOFTokenRange() :
formattingScanner.isOnToken() ? formattingScanner.readTokenInfo(enclosingNode).token :
undefined;
Comment on lines +441 to +445
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (token) {
processPair(
token,
sourceFile.getLineAndCharacterOfPosition(token.pos).line,
enclosingNode,
previousRange,
previousRangeStartLine!,
previousParent!,
enclosingNode,
/*dynamicIndentation*/ undefined);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

return edits;

// local functions
Expand Down Expand Up @@ -653,29 +672,14 @@ namespace ts.formatting {
});

// proceed any tokens in the node that are located after child nodes
while (formattingScanner.isOnToken()) {
while (formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) {
const tokenInfo = formattingScanner.readTokenInfo(node);
if (tokenInfo.token.end > node.end) {
if (tokenInfo.token.end > Math.min(node.end, originalRange.end)) {
break;
}
consumeTokenAndAdvanceScanner(tokenInfo, node, nodeDynamicIndentation, node);
}

if (!node.parent && formattingScanner.isOnEOF()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const token = formattingScanner.readEOFTokenRange();
if (token.end <= node.end && previousRange) {
processPair(
token,
sourceFile.getLineAndCharacterOfPosition(token.pos).line,
node,
previousRange,
previousRangeStartLine,
previousParent,
contextNode,
nodeDynamicIndentation);
}
}

function processChildNode(
child: Node,
inheritedIndentation: number,
Expand Down Expand Up @@ -717,9 +721,12 @@ namespace ts.formatting {
return inheritedIndentation;
}

while (formattingScanner.isOnToken()) {
while (formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// proceed any parent tokens that are located prior to child.getStart()
const tokenInfo = formattingScanner.readTokenInfo(node);
if (tokenInfo.token.end > originalRange.end) {
return inheritedIndentation;
}
if (tokenInfo.token.end > childStartPos) {
if (tokenInfo.token.pos > childStartPos) {
formattingScanner.skipToStartOf(child);
Expand All @@ -731,7 +738,7 @@ namespace ts.formatting {
consumeTokenAndAdvanceScanner(tokenInfo, node, parentDynamicIndentation, node);
}

if (!formattingScanner.isOnToken()) {
if (!formattingScanner.isOnToken() || formattingScanner.getStartPos() >= originalRange.end) {
return inheritedIndentation;
}

Expand Down Expand Up @@ -773,7 +780,7 @@ namespace ts.formatting {

if (listStartToken !== SyntaxKind.Unknown) {
// introduce a new indentation scope for lists (including list start and end tokens)
while (formattingScanner.isOnToken()) {
while (formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) {
const tokenInfo = formattingScanner.readTokenInfo(parent);
if (tokenInfo.token.end > nodes.pos) {
// stop when formatting scanner moves past the beginning of node list
Expand Down Expand Up @@ -814,7 +821,7 @@ namespace ts.formatting {
}

const listEndToken = getCloseTokenForOpenToken(listStartToken);
if (listEndToken !== SyntaxKind.Unknown && formattingScanner.isOnToken()) {
if (listEndToken !== SyntaxKind.Unknown && formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) {
let tokenInfo: TokenInfo | undefined = formattingScanner.readTokenInfo(parent);
if (tokenInfo.token.kind === SyntaxKind.CommaToken && isCallLikeExpression(parent)) {
const commaTokenLine = sourceFile.getLineAndCharacterOfPosition(tokenInfo.token.pos).line;
Expand Down Expand Up @@ -969,7 +976,7 @@ namespace ts.formatting {
previousStartLine: number,
previousParent: Node,
contextNode: Node,
dynamicIndentation: DynamicIndentation): LineAction {
dynamicIndentation: DynamicIndentation | undefined): LineAction {

formattingContext.updateContext(previousItem, previousParent, currentItem, currentParent, contextNode);

Expand All @@ -982,24 +989,26 @@ namespace ts.formatting {
// win in a conflict with lower priority rules.
forEachRight(rules, rule => {
lineAction = applyRuleEdits(rule, previousItem, previousStartLine, currentItem, currentStartLine);
switch (lineAction) {
case LineAction.LineRemoved:
// Handle the case where the next line is moved to be the end of this line.
// In this case we don't indent the next line in the next pass.
if (currentParent.getStart(sourceFile) === currentItem.pos) {
dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ false, contextNode);
}
break;
case LineAction.LineAdded:
// Handle the case where token2 is moved to the new line.
// In this case we indent token2 in the next pass but we set
// sameLineIndent flag to notify the indenter that the indentation is within the line.
if (currentParent.getStart(sourceFile) === currentItem.pos) {
dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ true, contextNode);
}
break;
default:
Debug.assert(lineAction === LineAction.None);
if (dynamicIndentation) {
switch (lineAction) {
case LineAction.LineRemoved:
// Handle the case where the next line is moved to be the end of this line.
// In this case we don't indent the next line in the next pass.
if (currentParent.getStart(sourceFile) === currentItem.pos) {
dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ false, contextNode);
}
break;
case LineAction.LineAdded:
// Handle the case where token2 is moved to the new line.
// In this case we indent token2 in the next pass but we set
// sameLineIndent flag to notify the indenter that the indentation is within the line.
if (currentParent.getStart(sourceFile) === currentItem.pos) {
dynamicIndentation.recomputeIndentation(/*lineAddedByFormatting*/ true, contextNode);
}
break;
default:
Debug.assert(lineAction === LineAction.None);
}
}

// We need to trim trailing whitespace between the tokens if they were on different lines, and no rule was applied to put them on the same line
Expand Down
5 changes: 3 additions & 2 deletions src/services/formatting/formattingScanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace ts.formatting {

export interface FormattingScanner {
advance(): void;
getStartPos(): number;
isOnToken(): boolean;
isOnEOF(): boolean;
readTokenInfo(n: Node): TokenInfo;
Expand Down Expand Up @@ -49,6 +50,7 @@ namespace ts.formatting {
lastTrailingTriviaWasNewLine: () => wasNewLine,
skipToEndOf,
skipToStartOf,
getStartPos: () => lastTokenInfo?.token.pos ?? scanner.getTokenPos(),
});

lastTokenInfo = undefined;
Expand Down Expand Up @@ -265,8 +267,7 @@ namespace ts.formatting {

function isOnToken(): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@gabritto gabritto Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you answered the first part of my question already in another comment, didn't see the comment when I was reviewing

const current = lastTokenInfo ? lastTokenInfo.token.kind : scanner.getToken();
const startPos = lastTokenInfo ? lastTokenInfo.token.pos : scanner.getStartPos();
return startPos < endPos && current !== SyntaxKind.EndOfFileToken && !isTrivia(current);
return current !== SyntaxKind.EndOfFileToken && !isTrivia(current);
}

function isOnEOF(): boolean {
Expand Down
26 changes: 21 additions & 5 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2726,23 +2726,23 @@ namespace ts {
return typeIsAccessible ? res : undefined;
}

export function syntaxRequiresTrailingCommaOrSemicolonOrASI(kind: SyntaxKind) {
function syntaxRequiresTrailingCommaOrSemicolonOrASI(kind: SyntaxKind) {
return kind === SyntaxKind.CallSignature
|| kind === SyntaxKind.ConstructSignature
|| kind === SyntaxKind.IndexSignature
|| kind === SyntaxKind.PropertySignature
|| kind === SyntaxKind.MethodSignature;
}

export function syntaxRequiresTrailingFunctionBlockOrSemicolonOrASI(kind: SyntaxKind) {
function syntaxRequiresTrailingFunctionBlockOrSemicolonOrASI(kind: SyntaxKind) {
return kind === SyntaxKind.FunctionDeclaration
|| kind === SyntaxKind.Constructor
|| kind === SyntaxKind.MethodDeclaration
|| kind === SyntaxKind.GetAccessor
|| kind === SyntaxKind.SetAccessor;
}

export function syntaxRequiresTrailingModuleBlockOrSemicolonOrASI(kind: SyntaxKind) {
function syntaxRequiresTrailingModuleBlockOrSemicolonOrASI(kind: SyntaxKind) {
return kind === SyntaxKind.ModuleDeclaration;
}

Expand Down Expand Up @@ -2831,21 +2831,37 @@ namespace ts {
forEachChild(sourceFile, function visit(node): boolean | undefined {
if (syntaxRequiresTrailingSemicolonOrASI(node.kind)) {
const lastToken = node.getLastToken(sourceFile);
if (lastToken && lastToken.kind === SyntaxKind.SemicolonToken) {
if (lastToken?.kind === SyntaxKind.SemicolonToken) {
withSemicolon++;
}
else {
withoutSemicolon++;
}
}
else if (syntaxRequiresTrailingCommaOrSemicolonOrASI(node.kind)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const lastToken = node.getLastToken(sourceFile);
if (lastToken?.kind === SyntaxKind.SemicolonToken) {
withSemicolon++;
}
else if (lastToken && lastToken.kind !== SyntaxKind.CommaToken) {
const lastTokenLine = getLineAndCharacterOfPosition(sourceFile, lastToken.getStart(sourceFile)).line;
const nextTokenLine = getLineAndCharacterOfPosition(sourceFile, getSpanOfTokenAtPosition(sourceFile, lastToken.end).start).line;
// Avoid counting missing semicolon in single-line objects:
// `function f(p: { x: string /*no semicolon here is insignificant*/ }) {`
if (lastTokenLine !== nextTokenLine) {
withoutSemicolon++;
}
}
}

if (withSemicolon + withoutSemicolon >= nStatementsToObserve) {
return true;
}

return forEachChild(node, visit);
});

// One statement missing a semicolon isn’t sufficient evidence to say the user
// One statement missing a semicolon isn't sufficient evidence to say the user
// doesn’t want semicolons, because they may not even be done writing that statement.
if (withSemicolon === 0 && withoutSemicolon <= 1) {
return true;
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsOverridingMethod5.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
////abstract class Ab {
//// abstract met(n: string): void;
//// met2(n: number): void {
////
//// return;
//// }
////}
////
Expand Down
12 changes: 12 additions & 0 deletions tests/cases/fourslash/formatSelectionEditAtEndOfRange.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// <reference path="fourslash.ts" />


//// /*1*/var x = 1;/*2*/
//// void 0;

format.setOption("semicolons", "remove");
format.selection("1", "2");
verify.currentFileContentIs(
`var x = 1
void 0;`
);
2 changes: 1 addition & 1 deletion tests/cases/fourslash/refactorExtractType12.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ edit.applyRefactor({
refactorName: "Extract type",
actionName: "Extract to type alias",
actionDescription: "Extract to type alias",
newContent: `type /*RENAME*/NewType = string;
newContent: `type /*RENAME*/NewType = string

interface A<T = NewType> {
a: boolean
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/refactorExtractType13.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ edit.applyRefactor({
refactorName: "Extract type",
actionName: "Extract to type alias",
actionDescription: "Extract to type alias",
newContent: `type /*RENAME*/NewType = boolean;
newContent: `type /*RENAME*/NewType = boolean

interface A<T = string> {
a: NewType
Expand Down