fix(47821): Assertion failure on "convert to default export" in module augmentation

[a-tarasyuk]

Fixes #47821

[amcasey]

A few questions.

[amcasey]

The change looks correct to me and, in practice, there's only one caller, so whether the check is pushed upstream is mostly a hygiene thing. I think it's worth investigating, but I think the change is also acceptable as-is.

[sandersn]

It sounds like this is ready to go except (probably) unconditionally calling getMergedSymbol as @DanielRosenwasser pointed out.

At least, I'm pretty sure that @weswigham is right in his concern, and also that there's no penalty for calling getMergedSymbol too much.

[DanielRosenwasser]

I think you just always want the merged symbol. Otherwise you won't correctly catch when an existing module augmentation adds a default export (which admittedly is very weird).

// @Filename: foo.ts
/////*a*/export function bar() {}/*b*/;

// @Filename: augmenter.ts
////import "./foo";
////declare module "./foo" {
////    export default function foo(): void;
////}

// @Filename: /b.ts
////import foo from "./foo";

So maybe just

Suggested change

	const parent = node.parent;
	if (isSourceFile(parent)) {
	return parent.symbol;
	}
	const symbol = parent.parent.symbol;
	if (symbol.valueDeclaration && isExternalModuleAugmentation(symbol.valueDeclaration)) {
	return checker.getMergedSymbol(symbol);
	}
	return symbol;
	const parent = node.parent;
	const symbol = checker.getMergedSymbol(isSourceFile(parent) ? parent.symbol : parent.parent.symbol);
	return symbol;

[a-tarasyuk]

Import in /b.ts will not be changed. Maybe need to use two symbols, something like this

const parent = node.parent;
const symbol = isSourceFile(parent) ? parent.symbol : parent.parent.symbol;
const mergedSymbol = checker.getMergedSymbol(symbol);
return symbol === mergedSymbol ? [symbol] : [symbol, mergedSymbol];

or handle merged symbols directly in importTracker

[DanielRosenwasser]

(edited my last comment to actually use a default import)

- import { foo } from "./foo";
+ import foo from "./foo";

[a-tarasyuk]

I think my previous comment also applies to default imports.

[andrewbranch]

I’m sorry but I can’t actually tell what issue is being discussed here but it seems to be the one that’s preventing this from being merged. Is there a concrete failing test we can discuss?

[a-tarasyuk]

Changes should be made to the b.ts and c.ts files - @DanielRosenwasser am I right?

// @Filename: /a.ts
/////*a*/export function a() {}/*b*/;

// @Filename: /b.ts
////import "./a";
////declare module "./a" {
////    export function a(): void;
////}

// @Filename: /c.ts
////import { a } from "./a";

goTo.select("a", "b");
edit.applyRefactor({
    refactorName: "Convert export",
    actionName: "Convert named export to default export",
    actionDescription: "Convert named export to default export",
    newContent: {
        "/a.ts":
`export default function a() {};`,
        "/b.ts":
`import "./a";
declare module "./a" {
    export default function a(): void;
}`,
        "/c.ts":
`import a from "./a";`
    }
});

If we use only the merged symbol, changes will only be made in the /b.ts file. I see two ways how we can fix it -

1. fix(47821): Assertion failure on "convert to default export" in module augmentation #47829 (comment),

2. TypeScript/src/services/refactors/convertExport.ts

   Line 167 in 95731f0

   FindAllReferences.Core.eachExportReference(program.getSourceFiles(), checker, cancellationToken, exportSymbol, exportingModuleSymbol, exportName.text, wasDefault, ref => {

   handle merged symbols in the import tracker, I'm not sure how safe it is

[andrewbranch]

Changes should be made to the b.ts and c.ts files

@a-tarasyuk that test shows no changes to b.ts 🤔

[a-tarasyuk]

@andrewbranch default is useless in /b.ts. I've changed that

[andrewbranch]

It’s actually not 100% clear to me what the expected behavior should be. This is a very weird edge case, and I think it could be unexpected to generate changes to a merged declaration of any export. Personally I would be fine with the refactor being unavailable here.

[andrewbranch]

What did the behavior of this hypothetical test end up being?

[sandersn]

@andrewbranch @amcasey I got lost reading the PR history. I think this is ready to go, and that the last outstanding request from @andrewbranch -- that the refactor not apply in the weird case given by @DanielRosenwasser -- is already true. Did I read that right?

[amcasey]

@sandersn Fine by me