Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/services/refactors/convertExport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ namespace ts.refactor {
return { error: getLocaleSpecificMessage(Diagnostics.Could_not_find_export_statement) };
}

const exportingModuleSymbol = isSourceFile(exportNode.parent) ? exportNode.parent.symbol : exportNode.parent.parent.symbol;

const checker = program.getTypeChecker();
const exportingModuleSymbol = getExportingModuleSymbol(exportNode, checker);
const flags = getSyntacticModifierFlags(exportNode) || ((isExportAssignment(exportNode) && !exportNode.isExportEquals) ? ModifierFlags.ExportDefault : ModifierFlags.None);

const wasDefault = !!(flags & ModifierFlags.Default);
Expand All @@ -75,7 +75,6 @@ namespace ts.refactor {
return { error: getLocaleSpecificMessage(Diagnostics.This_file_already_has_a_default_export) };
}

const checker = program.getTypeChecker();
const noSymbolError = (id: Node) =>
(isIdentifier(id) && checker.getSymbolAtLocation(id)) ? undefined
: { error: getLocaleSpecificMessage(Diagnostics.Can_only_convert_named_export) };
Expand Down Expand Up @@ -165,6 +164,7 @@ namespace ts.refactor {
const checker = program.getTypeChecker();
const exportSymbol = Debug.checkDefined(checker.getSymbolAtLocation(exportName), "Export name should resolve to a symbol");
FindAllReferences.Core.eachExportReference(program.getSourceFiles(), checker, cancellationToken, exportSymbol, exportingModuleSymbol, exportName.text, wasDefault, ref => {
if (exportName === ref) return;
const importingSourceFile = ref.getSourceFile();
if (wasDefault) {
changeDefaultToNamedImport(importingSourceFile, ref, changes, exportName.text);
Expand Down Expand Up @@ -258,4 +258,16 @@ namespace ts.refactor {
function makeExportSpecifier(propertyName: string, name: string): ExportSpecifier {
return factory.createExportSpecifier(/*isTypeOnly*/ false, propertyName === name ? undefined : factory.createIdentifier(propertyName), factory.createIdentifier(name));
}

function getExportingModuleSymbol(node: Node, checker: TypeChecker) {
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;
Comment on lines +263 to +271
Copy link
Member

@DanielRosenwasser DanielRosenwasser Apr 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my previous comment also applies to default imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@a-tarasyuk a-tarasyuk May 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
26 changes: 26 additions & 0 deletions tests/cases/fourslash/refactorConvertExport_namedToDefaultInModuleAugmentation1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/// <reference path='fourslash.ts' />

// @Filename: /node_modules/@types/foo/index.d.ts
////export {};
////declare module "foo" {
//// /*a*/export function foo(): void;/*b*/
////}

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

goTo.select("a", "b");
edit.applyRefactor({
refactorName: "Convert export",
actionName: "Convert named export to default export",
actionDescription: "Convert named export to default export",
newContent: {
"/node_modules/@types/foo/index.d.ts":
`export {};
declare module "foo" {
export default function foo(): void;
}`,
"/b.ts":
`import foo from "foo";`
}
});
18 changes: 18 additions & 0 deletions tests/cases/fourslash/refactorConvertExport_namedToDefaultInModuleAugmentation2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path='fourslash.ts' />

////export {};
////declare module "foo" {
//// /*a*/export function func(): void;/*b*/
////}

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