Ensure formatter can always get a newline character (#38579)

* Ensure formatter can always get a newline character * Make FormatContext.host optional since it’s not necessary if format options are all applied * Make FormattingHost required again
microsoft · May 19, 2020 · 707e977 · 707e977
1 parent 611dd22
commit 707e977
Show file tree

Hide file tree

Showing 12 changed files with 61 additions and 21 deletions.
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -515,6 +515,12 @@ namespace FourSlash {
             }
         }
 
+        public verifyOrganizeImports(newContent: string) {
+            const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file" }, this.formatCodeSettings, ts.emptyOptions);
+            this.applyChanges(changes);
+            this.verifyFileContent(this.activeFile.fileName, newContent);
+        }
+
         private raiseError(message: string): never {
             throw new Error(this.messageAtLastKnownMarker(message));
         }

diff --git a/src/harness/fourslashInterfaceImpl.ts b/src/harness/fourslashInterfaceImpl.ts
@@ -560,6 +560,10 @@ namespace FourSlashInterface {
         public noMoveToNewFile(): void {
             this.state.noMoveToNewFile();
         }
+
+        public organizeImports(newContent: string) {
+            this.state.verifyOrganizeImports(newContent);
+        }
     }
 
     export class Edit {

diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts
@@ -3,6 +3,7 @@ namespace ts.formatting {
     export interface FormatContext {
         readonly options: FormatCodeSettings;
         readonly getRules: RulesMap;
+        readonly host: FormattingHost;
     }
 
     export interface TextRangeWithKind<T extends SyntaxKind = SyntaxKind> extends TextRange {
@@ -394,7 +395,7 @@ namespace ts.formatting {
         initialIndentation: number,
         delta: number,
         formattingScanner: FormattingScanner,
-        { options, getRules }: FormatContext,
+        { options, getRules, host }: FormatContext,
         requestKind: FormattingRequestKind,
         rangeContainsError: (r: TextRange) => boolean,
         sourceFile: SourceFileLike): TextChange[] {
@@ -1193,7 +1194,7 @@ namespace ts.formatting {
             previousRange: TextRangeWithKind,
             previousStartLine: number,
             currentRange: TextRangeWithKind,
-            currentStartLine: number,
+            currentStartLine: number
         ): LineAction {
             const onLaterLine = currentStartLine !== previousStartLine;
             switch (rule.action) {
@@ -1221,7 +1222,7 @@ namespace ts.formatting {
                     // edit should not be applied if we have one line feed between elements
                     const lineDelta = currentStartLine - previousStartLine;
                     if (lineDelta !== 1) {
-                        recordReplace(previousRange.end, currentRange.pos - previousRange.end, options.newLineCharacter!);
+                        recordReplace(previousRange.end, currentRange.pos - previousRange.end, getNewLineOrDefaultFromHost(host, options));
                         return onLaterLine ? LineAction.None : LineAction.LineAdded;
                     }
                     break;

diff --git a/src/services/formatting/rulesMap.ts b/src/services/formatting/rulesMap.ts
@@ -1,7 +1,7 @@
 /* @internal */
 namespace ts.formatting {
-    export function getFormatContext(options: FormatCodeSettings): FormatContext {
-        return { options, getRules: getRulesMap() };
+    export function getFormatContext(options: FormatCodeSettings, host: FormattingHost): FormatContext {
+        return { options, getRules: getRulesMap(), host };
     }
 
     let rulesMapCache: RulesMap | undefined;

diff --git a/src/services/services.ts b/src/services/services.ts
@@ -1502,7 +1502,7 @@ namespace ts {
                 position,
                 { name, source },
                 host,
-                (formattingOptions && formatting.getFormatContext(formattingOptions))!, // TODO: GH#18217
+                (formattingOptions && formatting.getFormatContext(formattingOptions, host))!, // TODO: GH#18217
                 preferences,
                 cancellationToken,
             );
@@ -1840,16 +1840,16 @@ namespace ts {
 
         function getFormattingEditsForRange(fileName: string, start: number, end: number, options: FormatCodeOptions | FormatCodeSettings): TextChange[] {
             const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName);
-            return formatting.formatSelection(start, end, sourceFile, formatting.getFormatContext(toEditorSettings(options)));
+            return formatting.formatSelection(start, end, sourceFile, formatting.getFormatContext(toEditorSettings(options), host));
         }
 
         function getFormattingEditsForDocument(fileName: string, options: FormatCodeOptions | FormatCodeSettings): TextChange[] {
-            return formatting.formatDocument(syntaxTreeCache.getCurrentSourceFile(fileName), formatting.getFormatContext(toEditorSettings(options)));
+            return formatting.formatDocument(syntaxTreeCache.getCurrentSourceFile(fileName), formatting.getFormatContext(toEditorSettings(options), host));
         }
 
         function getFormattingEditsAfterKeystroke(fileName: string, position: number, key: string, options: FormatCodeOptions | FormatCodeSettings): TextChange[] {
             const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName);
-            const formatContext = formatting.getFormatContext(toEditorSettings(options));
+            const formatContext = formatting.getFormatContext(toEditorSettings(options), host);
 
             if (!isInComment(sourceFile, position)) {
                 switch (key) {
@@ -1871,7 +1871,7 @@ namespace ts {
             synchronizeHostData();
             const sourceFile = getValidSourceFile(fileName);
             const span = createTextSpanFromBounds(start, end);
-            const formatContext = formatting.getFormatContext(formatOptions);
+            const formatContext = formatting.getFormatContext(formatOptions, host);
 
             return flatMap(deduplicate<number>(errorCodes, equateValues, compareValues), errorCode => {
                 cancellationToken.throwIfCancellationRequested();
@@ -1883,7 +1883,7 @@ namespace ts {
             synchronizeHostData();
             Debug.assert(scope.type === "file");
             const sourceFile = getValidSourceFile(scope.fileName);
-            const formatContext = formatting.getFormatContext(formatOptions);
+            const formatContext = formatting.getFormatContext(formatOptions, host);
 
             return codefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext, preferences });
         }
@@ -1892,13 +1892,13 @@ namespace ts {
             synchronizeHostData();
             Debug.assert(scope.type === "file");
             const sourceFile = getValidSourceFile(scope.fileName);
-            const formatContext = formatting.getFormatContext(formatOptions);
+            const formatContext = formatting.getFormatContext(formatOptions, host);
 
             return OrganizeImports.organizeImports(sourceFile, formatContext, host, program, preferences);
         }
 
         function getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences = emptyOptions): readonly FileTextChanges[] {
-            return ts.getEditsForFileRename(getProgram()!, oldFilePath, newFilePath, host, formatting.getFormatContext(formatOptions), preferences, sourceMapper);
+            return ts.getEditsForFileRename(getProgram()!, oldFilePath, newFilePath, host, formatting.getFormatContext(formatOptions, host), preferences, sourceMapper);
         }
 
         function applyCodeActionCommand(action: CodeActionCommand, formatSettings?: FormatCodeSettings): Promise<ApplyCodeActionCommandResult>;
@@ -2141,7 +2141,7 @@ namespace ts {
                 endPosition,
                 program: getProgram()!,
                 host,
-                formatContext: formatting.getFormatContext(formatOptions!), // TODO: GH#18217
+                formatContext: formatting.getFormatContext(formatOptions!, host), // TODO: GH#18217
                 cancellationToken,
                 preferences,
             };

diff --git a/src/services/types.ts b/src/services/types.ts
@@ -203,6 +203,11 @@ namespace ts {
         has(dependencyName: string, inGroups?: PackageJsonDependencyGroup): boolean;
     }
 
+    /** @internal */
+    export interface FormattingHost {
+        getNewLine?(): string;
+    }
+
     //
     // Public interface of the host of a language service instance.
     //

diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -2085,9 +2085,9 @@ namespace ts {
     /**
      * The default is CRLF.
      */
-    export function getNewLineOrDefaultFromHost(host: LanguageServiceHost | LanguageServiceShimHost, formatSettings?: FormatCodeSettings) {
-        return (formatSettings && formatSettings.newLineCharacter) ||
-            (host.getNewLine && host.getNewLine()) ||
+    export function getNewLineOrDefaultFromHost(host: FormattingHost, formatSettings?: FormatCodeSettings) {
+        return formatSettings?.newLineCharacter ||
+            host.getNewLine?.() ||
             carriageReturnLineFeed;
     }
 

diff --git a/src/testRunner/unittests/services/convertToAsyncFunction.ts b/src/testRunner/unittests/services/convertToAsyncFunction.ts
@@ -306,7 +306,7 @@ interface Array<T> {}`
                 cancellationToken: { throwIfCancellationRequested: noop, isCancellationRequested: returnFalse },
                 preferences: emptyOptions,
                 host: notImplementedHost,
-                formatContext: formatting.getFormatContext(testFormatSettings)
+                formatContext: formatting.getFormatContext(testFormatSettings, notImplementedHost)
             };
 
             const diagnostics = languageService.getSuggestionDiagnostics(f.path);

diff --git a/src/testRunner/unittests/services/extract/helpers.ts b/src/testRunner/unittests/services/extract/helpers.ts
@@ -102,7 +102,7 @@ namespace ts {
                 startPosition: selectionRange.pos,
                 endPosition: selectionRange.end,
                 host: notImplementedHost,
-                formatContext: formatting.getFormatContext(testFormatSettings),
+                formatContext: formatting.getFormatContext(testFormatSettings, notImplementedHost),
                 preferences: emptyOptions,
             };
             const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromRange(selectionRange));
@@ -164,7 +164,7 @@ namespace ts {
                 startPosition: selectionRange.pos,
                 endPosition: selectionRange.end,
                 host: notImplementedHost,
-                formatContext: formatting.getFormatContext(testFormatSettings),
+                formatContext: formatting.getFormatContext(testFormatSettings, notImplementedHost),
                 preferences: emptyOptions,
             };
             const rangeToExtract = refactor.extractSymbol.getRangeToExtract(sourceFile, createTextSpanFromRange(selectionRange));

diff --git a/src/testRunner/unittests/services/textChanges.ts b/src/testRunner/unittests/services/textChanges.ts
@@ -19,7 +19,7 @@ namespace ts {
         const newLineCharacter = getNewLineCharacter(printerOptions);
 
         function getRuleProvider(placeOpenBraceOnNewLineForFunctions: boolean): formatting.FormatContext {
-            return formatting.getFormatContext(placeOpenBraceOnNewLineForFunctions ? { ...testFormatSettings, placeOpenBraceOnNewLineForFunctions: true } : testFormatSettings);
+            return formatting.getFormatContext(placeOpenBraceOnNewLineForFunctions ? { ...testFormatSettings, placeOpenBraceOnNewLineForFunctions: true } : testFormatSettings, notImplementedHost);
         }
 
         // validate that positions that were recovered from the printed text actually match positions that will be created if the same text is parsed.

diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -394,6 +394,8 @@ declare namespace FourSlashInterface {
         noMoveToNewFile(): void;
 
         generateTypes(...options: GenerateTypesOptions[]): void;
+
+        organizeImports(newContent: string): void;
     }
     class edit {
         backspace(count?: number): void;

diff --git a/tests/cases/fourslash/organizeImportsNoFormatOptions.ts b/tests/cases/fourslash/organizeImportsNoFormatOptions.ts
@@ -0,0 +1,22 @@
+/// <reference path="fourslash.ts" />
+
+// #38548
+
+////import {
+////  stat,
+////  statSync,
+////} from "fs";
+////export function fakeFn() {
+////  stat;
+////  statSync;
+////}
+
+format.setFormatOptions({});
+
+// Default newline is carriage return.
+// See `getNewLineOrDefaultFromHost()` in services/utilities.ts.
+verify.organizeImports(
+`import {\r\nstat,\r\nstatSync\r\n} from "fs";\r\nexport function fakeFn() {
+  stat;
+  statSync;
+}`);