Cherry-pick PR #38579 into release-3.9 (#38666)

TypeScript Bot · andrewbranch · web-flow · commit 5b29b141fa0d · 2020-05-19T14:40:23.000-07:00
Component commits: 0a696c9 Ensure formatter can always get a newline character ab09d67 Make FormatContext.host optional since it’s not necessary if format options are all applied 90923e2 Make FormattingHost required again Co-authored-by: Andrew Branch <andrew@wheream.io>
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
@@ -1480,7 +1480,7 @@ namespace ts {
                 position,
                 { name, source },
                 host,
-                (formattingOptions && formatting.getFormatContext(formattingOptions))!, // TODO: GH#18217
+                (formattingOptions && formatting.getFormatContext(formattingOptions, host))!, // TODO: GH#18217
                 preferences,
                 cancellationToken,
             );
@@ -1818,16 +1818,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) {
@@ -1849,7 +1849,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();
@@ -1861,7 +1861,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 });
         }
@@ -1870,13 +1870,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>;
@@ -2119,7 +2119,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
@@ -201,6 +201,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;
+}`);

Original file line number	Diff line number	Diff line change
`@@ -515,6 +515,12 @@ namespace FourSlash {`
`515`	`515`	`}`
`516`	`516`	`}`
`517`	`517`
	`518`	`+ public verifyOrganizeImports(newContent: string) {`
	`519`	`+ const changes = this.languageService.organizeImports({ fileName: this.activeFile.fileName, type: "file" }, this.formatCodeSettings, ts.emptyOptions);`
	`520`	`+ this.applyChanges(changes);`
	`521`	`+ this.verifyFileContent(this.activeFile.fileName, newContent);`
	`522`	`+ }`
	`523`	`+`
`518`	`524`	`private raiseError(message: string): never {`
`519`	`525`	`throw new Error(this.messageAtLastKnownMarker(message));`
`520`	`526`	`}`
Original file line number	Diff line number	Diff line change
`@@ -560,6 +560,10 @@ namespace FourSlashInterface {`
`560`	`560`	`public noMoveToNewFile(): void {`
`561`	`561`	`this.state.noMoveToNewFile();`
`562`	`562`	`}`
	`563`	`+`
	`564`	`+ public organizeImports(newContent: string) {`
	`565`	`+ this.state.verifyOrganizeImports(newContent);`
	`566`	`+ }`
`563`	`567`	`}`
`564`	`568`
`565`	`569`	`export class Edit {`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`/* @internal */`
`2`	`2`	`namespace ts.formatting {`
`3`		`- export function getFormatContext(options: FormatCodeSettings): FormatContext {`
`4`		`- return { options, getRules: getRulesMap() };`
	`3`	`+ export function getFormatContext(options: FormatCodeSettings, host: FormattingHost): FormatContext {`
	`4`	`+ return { options, getRules: getRulesMap(), host };`
`5`	`5`	`}`
`6`	`6`
`7`	`7`	`let rulesMapCache: RulesMap \| undefined;`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ namespace ts {`
`19`	`19`	`const newLineCharacter = getNewLineCharacter(printerOptions);`
`20`	`20`
`21`	`21`	`function getRuleProvider(placeOpenBraceOnNewLineForFunctions: boolean): formatting.FormatContext {`
`22`		`- return formatting.getFormatContext(placeOpenBraceOnNewLineForFunctions ? { ...testFormatSettings, placeOpenBraceOnNewLineForFunctions: true } : testFormatSettings);`
	`22`	`+ return formatting.getFormatContext(placeOpenBraceOnNewLineForFunctions ? { ...testFormatSettings, placeOpenBraceOnNewLineForFunctions: true } : testFormatSettings, notImplementedHost);`
`23`	`23`	`}`
`24`	`24`
`25`	`25`	`// validate that positions that were recovered from the printed text actually match positions that will be created if the same text is parsed.`