3.9 Regression: organizeImports prepends/appends "undefined" when there are 2+ named imports

[mathieumg]

TypeScript Version: 3.9.2

Search Terms: organizeImports, imports, undefined

Code

import {
  stat,
  statSync,
} from "fs";

export function fakeFn() {
  stat;
  statSync;
}

Run through ts.organizeImports.

Expected behavior:

import { stat, statSync } from "fs";

export function fakeFn() {
  stat;
  statSync;
}

Actual behavior:

import { undefinedstat, statSyncundefined } from "fs";

export function fakeFn() {
  stat;
  statSync;
}

Doesn't happen if there's only one import:

import { stat } from "fs";

export function fakeFn() {
  stat;
}

Only happens at the start and end of the block:

import {
  lstatSync,
  stat,
  statSync,
} from "fs";

export function fakeFn() {
  lstatSync;
  stat;
  statSync;
}

will yield:

import { undefinedlstatSync, stat, statSyncundefined } from "fs";

export function fakeFn() {
  lstatSync;
  stat;
  statSync;
}

Playground Link: https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=9&pc=1#code/JYWwDg9gTgLgBAbwFBzgZxgQxgGherGAZQE8A7AYzwF84AzKCEOAIjrRYG4kkBTAD0ix6AV0oxgEMvUwBrXgDEyACgCUifBmzdUW4uQrdqSIA

Don't think it can run the programmatic API.

https://ts-ast-viewer.com/#code/JYWwDg9gTgLgBAbwFBzgZxgQxgGherGAZQE8A7AYzwF84AzKCEOAIjrRYG4kkBTAD0ix6AV0oxgEMvUwBrXgDEyACgCUifBmzdUW4uQrdqSIA

Doesn't have TS 3.9 yet.

I setup a demo repo with both TS 3.8.7 and TS 3.9.2 here:

https://github.com/mathieumg/organize-imports/blob/v3.8.3/index.js yields:

TypeScript 3.8.3
Text Changes [
  {
    span: { start: 0, length: 55 },
    newText: 'import { lstatSync,stat,statSync } from "fs";\r\n'
  }
]
Failure? NO

and https://github.com/mathieumg/organize-imports/blob/v3.9.2/index.js yields:

TypeScript 3.9.2
Text Changes [
  {
    span: { start: 0, length: 55 },
    newText: 'import {undefinedlstatSync,\r\nstat,\r\nstatSyncundefined} from "fs";\r\n'
  }
]
Failure? YES

It's a simplified version of https://github.com/simonhaenisch/prettier-plugin-organize-imports/blob/f590b06c382aeeeeb3dadd9191c31aab9284d68c/index.js (which my use case emanates from), so it's not impossible they could be misusing the Language Service API. (but I wouldn't know) I imagine it could also be there was an undocumented breaking change there.

Possibly Related Issues: #38507

[simonhaenisch]

I've been digging into this...

import { foo,
	bar, baz } from "foobar";

import {
	foo,
	bar, baz } from "foobar";

import {
	foo,
	bar, baz
} from "foobar";

These all work, but as soon as there's a new line between bar, and baz it breaks.

---

I could track it down to

TypeScript/src/services/textChanges.ts

Line 973 in c1f676d

const changes = formatting.formatNodeGivenIndentation(node, file, sourceFile.languageVariant, initialIndentation, delta, { ...formatContext, options: formatOptions });

returning an invalid set of changes, like

{ changes:
   [ { span: { start: 8, length: 1 }, newText: undefined },
     { span: { start: 14, length: 4 }, newText: '' },
     { span: { start: 22, length: 1 }, newText: '' },
     { span: { start: 26, length: 1 }, newText: undefined } ] }

I can confirm that those are the undefineds that end up in the formatted text. I also found out that if newLineCharacter: '\n' is explicitly set in the formatting options of the organizeImports call, this issue doesn't occur, and the set of changes is correct:

{ changes:
   [ { span: { start: 8, length: 1 }, newText: '\n' },
     { span: { start: 14, length: 4 }, newText: '' },
     { span: { start: 22, length: 1 }, newText: '' },
     { span: { start: 26, length: 1 }, newText: '\n' } ] }

(The formatting options are passed into the formatNodeGivenIndentation call as formatContext).

However I'm not able to track it past this... I think the error happens somewhere in formatSpanWorker but that function is a beast.

That file has last been edited by @andrewbranch and the commit message (237ea52) sounds like it might be related, which is why I'm tagging you here.

[andrewbranch]

Thanks for tracking down the formatOptions.newLineCharacter thing @simonhaenisch! I wasn’t able to repro otherwise since AFAIK editors always send that option.