From 68bf5a519a17f5ae15bcd7b7ae71c20b40ddd8c5 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 29 Nov 2021 09:27:21 -0800 Subject: [PATCH] Fix formatting the end of a selection (#46875) * Fix formatting edits at end of range * Adjust test * Revert API baseline changes --- src/services/formatting/formatting.ts | 91 ++++++++++--------- src/services/formatting/formattingScanner.ts | 5 +- src/services/utilities.ts | 26 +++++- .../fourslash/completionsOverridingMethod5.ts | 2 +- .../formatSelectionEditAtEndOfRange.ts | 12 +++ .../cases/fourslash/refactorExtractType12.ts | 2 +- .../cases/fourslash/refactorExtractType13.ts | 2 +- 7 files changed, 89 insertions(+), 51 deletions(-) create mode 100644 tests/cases/fourslash/formatSelectionEditAtEndOfRange.ts diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts index 1a8542741ea46..3b68b44063e62 100644 --- a/src/services/formatting/formatting.ts +++ b/src/services/formatting/formatting.ts @@ -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 }; return getFormattingScanner(sourceFileLike.text, languageVariant, range.pos, range.end, scanner => formatSpanWorker( range, node, @@ -438,6 +438,25 @@ namespace ts.formatting { } } + if (previousRange! && formattingScanner.getStartPos() >= originalRange.end) { + const token = + formattingScanner.isOnEOF() ? formattingScanner.readEOFTokenRange() : + formattingScanner.isOnToken() ? formattingScanner.readTokenInfo(enclosingNode).token : + undefined; + + if (token) { + processPair( + token, + sourceFile.getLineAndCharacterOfPosition(token.pos).line, + enclosingNode, + previousRange, + previousRangeStartLine!, + previousParent!, + enclosingNode, + /*dynamicIndentation*/ undefined); + } + } + return edits; // local functions @@ -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()) { - 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, @@ -717,9 +721,12 @@ namespace ts.formatting { return inheritedIndentation; } - while (formattingScanner.isOnToken()) { + while (formattingScanner.isOnToken() && formattingScanner.getStartPos() < originalRange.end) { // 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); @@ -731,7 +738,7 @@ namespace ts.formatting { consumeTokenAndAdvanceScanner(tokenInfo, node, parentDynamicIndentation, node); } - if (!formattingScanner.isOnToken()) { + if (!formattingScanner.isOnToken() || formattingScanner.getStartPos() >= originalRange.end) { return inheritedIndentation; } @@ -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 @@ -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; @@ -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); @@ -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 diff --git a/src/services/formatting/formattingScanner.ts b/src/services/formatting/formattingScanner.ts index b619f98eb2b85..05bbc34041600 100644 --- a/src/services/formatting/formattingScanner.ts +++ b/src/services/formatting/formattingScanner.ts @@ -5,6 +5,7 @@ namespace ts.formatting { export interface FormattingScanner { advance(): void; + getStartPos(): number; isOnToken(): boolean; isOnEOF(): boolean; readTokenInfo(n: Node): TokenInfo; @@ -49,6 +50,7 @@ namespace ts.formatting { lastTrailingTriviaWasNewLine: () => wasNewLine, skipToEndOf, skipToStartOf, + getStartPos: () => lastTokenInfo?.token.pos ?? scanner.getTokenPos(), }); lastTokenInfo = undefined; @@ -265,8 +267,7 @@ namespace ts.formatting { function isOnToken(): boolean { 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 { diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 30552aa9b9d3b..89979d4cef46a 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -2726,7 +2726,7 @@ 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 @@ -2734,7 +2734,7 @@ namespace ts { || kind === SyntaxKind.MethodSignature; } - export function syntaxRequiresTrailingFunctionBlockOrSemicolonOrASI(kind: SyntaxKind) { + function syntaxRequiresTrailingFunctionBlockOrSemicolonOrASI(kind: SyntaxKind) { return kind === SyntaxKind.FunctionDeclaration || kind === SyntaxKind.Constructor || kind === SyntaxKind.MethodDeclaration @@ -2742,7 +2742,7 @@ namespace ts { || kind === SyntaxKind.SetAccessor; } - export function syntaxRequiresTrailingModuleBlockOrSemicolonOrASI(kind: SyntaxKind) { + function syntaxRequiresTrailingModuleBlockOrSemicolonOrASI(kind: SyntaxKind) { return kind === SyntaxKind.ModuleDeclaration; } @@ -2831,13 +2831,29 @@ 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)) { + 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; } @@ -2845,7 +2861,7 @@ namespace ts { 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; diff --git a/tests/cases/fourslash/completionsOverridingMethod5.ts b/tests/cases/fourslash/completionsOverridingMethod5.ts index 58c03962e47ba..6fa1564180fe7 100644 --- a/tests/cases/fourslash/completionsOverridingMethod5.ts +++ b/tests/cases/fourslash/completionsOverridingMethod5.ts @@ -6,7 +6,7 @@ ////abstract class Ab { //// abstract met(n: string): void; //// met2(n: number): void { -//// +//// return; //// } ////} //// diff --git a/tests/cases/fourslash/formatSelectionEditAtEndOfRange.ts b/tests/cases/fourslash/formatSelectionEditAtEndOfRange.ts new file mode 100644 index 0000000000000..9e60b686ab0b5 --- /dev/null +++ b/tests/cases/fourslash/formatSelectionEditAtEndOfRange.ts @@ -0,0 +1,12 @@ +/// + + +//// /*1*/var x = 1;/*2*/ +//// void 0; + +format.setOption("semicolons", "remove"); +format.selection("1", "2"); +verify.currentFileContentIs( +`var x = 1 +void 0;` +); diff --git a/tests/cases/fourslash/refactorExtractType12.ts b/tests/cases/fourslash/refactorExtractType12.ts index 8c4a9e316e4aa..2ae1d914f5be3 100644 --- a/tests/cases/fourslash/refactorExtractType12.ts +++ b/tests/cases/fourslash/refactorExtractType12.ts @@ -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 { a: boolean diff --git a/tests/cases/fourslash/refactorExtractType13.ts b/tests/cases/fourslash/refactorExtractType13.ts index e72eadb2dd136..cc6ba779a9149 100644 --- a/tests/cases/fourslash/refactorExtractType13.ts +++ b/tests/cases/fourslash/refactorExtractType13.ts @@ -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 { a: NewType