Fix formatting the end of a selection (#46875)

* Fix formatting edits at end of range * Adjust test * Revert API baseline changes
microsoft · Nov 29, 2021 · 68bf5a5 · 68bf5a5
1 parent 04f831d
commit 68bf5a5
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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;
     }
 
@@ -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)) {
+                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;

diff --git 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
@@ -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;`
+);
diff --git 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<T = NewType> {
     a: boolean

diff --git 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<T = string> {
     a: NewType