Skip to content

Fix error on duplicate commonjs exports #40545

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
sandersn merged 2 commits into from
Sep 14, 2020
Merged

Fix error on duplicate commonjs exports #40545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e39732
Fix error on duplicate commonjs exports
sandersn Sep 14, 2020
a24cc31
Switch to excludes=None, add test case
sandersn Sep 14, 2020
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
1 change: 1 addition & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2811,6 +2811,7 @@ namespace ts {
const isAlias = isAliasableExpression(node.right) && (isExportsIdentifier(node.left.expression) || isModuleExportsAccessExpression(node.left.expression));
const flags = isAlias ? SymbolFlags.Alias : SymbolFlags.Property | SymbolFlags.ExportValue;
const excludeFlags = isAlias ? SymbolFlags.AliasExcludes : SymbolFlags.None;
setParent(node.left, node);
declareSymbol(symbol.exports!, symbol, node.left, flags, excludeFlags);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6828,6 +6828,7 @@ namespace ts {
break;
case SyntaxKind.BinaryExpression:
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
// Could be best encoded as though an export specifier or as though an export assignment
// If name is default or export=, do an export assignment
// Otherwise do an export specifier
Expand Down
12 changes: 12 additions & 0 deletions tests/baselines/reference/moduleExportAliasElementAccessExpression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [moduleExportAliasElementAccessExpression.js]
function D () { }
exports["D"] = D;


//// [moduleExportAliasElementAccessExpression.js]
function D() { }
exports["D"] = D;

@weswigham weswigham Sep 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And non-identifier names, like "A B", emit as...what? Probably needs a test.

@sandersn sandersn Sep 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

_A_B. I added a test the in the bug, but I guess I can add it here.



//// [moduleExportAliasElementAccessExpression.d.ts]
export function D(): void;
16 changes: 16 additions & 0 deletions tests/baselines/reference/moduleExportDuplicateAlias.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js(1,9): error TS2300: Duplicate identifier 'apply'.
tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js(3,1): error TS2322: Type '() => void' is not assignable to type 'undefined'.
tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js(3,9): error TS2300: Duplicate identifier 'apply'.


==== tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js (3 errors) ====
exports.apply = undefined;
~~~~~
!!! error TS2300: Duplicate identifier 'apply'.
function a() { }
exports.apply = a;
~~~~~~~~~~~~~
!!! error TS2322: Type '() => void' is not assignable to type 'undefined'.
~~~~~
!!! error TS2300: Duplicate identifier 'apply'.

@weswigham weswigham Sep 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To get rid of this error, we can defer detection of this case in the binder by removing the offending case from AliasExcludes (like we did for class/function merges) and then look for it in the checker, where we can filter certain declarations (probably undefined assignments) and except it, while retaining the error more generally.

I think this pattern is actually super common, considering it's our current emit for cjs, so we should probably invest the effort into making it work smoothly.

@sandersn sandersn Sep 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Per our discussion, I switched this to None for the beta. It turns out that the checker already issues an error in this case, but we can add an exclusion for undefined-as-initialiser later.


14 changes: 14 additions & 0 deletions tests/baselines/reference/moduleExportDuplicateAlias.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//// [moduleExportAliasDuplicateAlias.js]
exports.apply = undefined;
function a() { }
exports.apply = a;


//// [moduleExportAliasDuplicateAlias.js]
exports.apply = undefined;
function a() { }
exports.apply = a;


//// [moduleExportAliasDuplicateAlias.d.ts]
export { undefined as apply };
16 changes: 16 additions & 0 deletions tests/baselines/reference/moduleExportDuplicateAlias.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
=== tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js ===
exports.apply = undefined;
>exports.apply : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 0, 0))
>exports : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 0, 0))
>apply : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 0, 0))
>undefined : Symbol(apply)

function a() { }
>a : Symbol(a, Decl(moduleExportAliasDuplicateAlias.js, 0, 26))

exports.apply = a;
>exports.apply : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 0, 0))
>exports : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 1, 16))
>apply : Symbol(apply, Decl(moduleExportAliasDuplicateAlias.js, 1, 16))
>a : Symbol(a, Decl(moduleExportAliasDuplicateAlias.js, 0, 26))

18 changes: 18 additions & 0 deletions tests/baselines/reference/moduleExportDuplicateAlias.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
=== tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias.js ===
exports.apply = undefined;
>exports.apply = undefined : undefined
>exports.apply : undefined
>exports : typeof import("tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias")
>apply : undefined
>undefined : undefined

function a() { }
>a : () => void

exports.apply = a;
>exports.apply = a : () => void
>exports.apply : undefined
>exports : typeof import("tests/cases/conformance/salsa/moduleExportAliasDuplicateAlias")
>apply : undefined
>a : () => void

3 changes: 2 additions & 1 deletion tests/cases/conformance/salsa/moduleExportAliasElementAccessExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @noEmit: true
// @declaration: true
// @checkJs: true
// @filename: moduleExportAliasElementAccessExpression.js
// @outdir: out

function D () { }
exports["D"] = D;
7 changes: 7 additions & 0 deletions tests/cases/conformance/salsa/moduleExportDuplicateAlias.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// @checkJs: true
// @declaration: true
// @filename: moduleExportAliasDuplicateAlias.js
// @outdir: out
exports.apply = undefined;
function a() { }
exports.apply = a;