Skip to content

Give a better message for multiple default exported functions of differing names #5198

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
85 changes: 72 additions & 13 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11061,7 +11061,7 @@ namespace ts {
let someHaveQuestionToken = false;
let allHaveQuestionToken = true;
let hasOverloads = false;
let bodyDeclaration: FunctionLikeDeclaration;
let implementationDeclaration: FunctionLikeDeclaration; // should be the first declaration with a body
let lastSeenNonAmbientDeclaration: FunctionLikeDeclaration;
let previousDeclaration: FunctionLikeDeclaration;

Expand Down Expand Up @@ -11143,21 +11143,26 @@ namespace ts {
someHaveQuestionToken = someHaveQuestionToken || hasQuestionToken(node);
allHaveQuestionToken = allHaveQuestionToken && hasQuestionToken(node);

if (nodeIsPresent(node.body) && bodyDeclaration) {
if (nodeIsPresent(node.body) && implementationDeclaration) {
if (isConstructor) {
multipleConstructorImplementation = true;
}
else {
duplicateFunctionDeclaration = true;

// We expect both implementations to agree in defaultness for later on.
if ((node.flags & NodeFlags.Default) !== (implementationDeclaration.flags & NodeFlags.Default)) {
Debug.fail("Expected first and current implementations of and current to agree in export default flag.");
}
}
}
else if (!isExportSymbolInsideModule && previousDeclaration && previousDeclaration.parent === node.parent && previousDeclaration.end !== node.pos) {
reportImplementationExpectedError(previousDeclaration);
}

if (nodeIsPresent(node.body)) {
if (!bodyDeclaration) {
bodyDeclaration = node;
if (!implementationDeclaration) {
implementationDeclaration = node;
}
}
else {
Expand All @@ -11173,15 +11178,19 @@ namespace ts {
}

if (multipleConstructorImplementation) {
forEach(declarations, declaration => {
for (let declaration of declarations) {
error(declaration, Diagnostics.Multiple_constructor_implementations_are_not_allowed);
});
}
}

if (duplicateFunctionDeclaration) {
forEach(declarations, declaration => {
error(declaration.name, Diagnostics.Duplicate_function_implementation);
});
let message = implementationDeclaration.flags & NodeFlags.Default && areAllFunctionDeclarationsWhereSomeNamesDiffer(declarations) ?
Diagnostics.A_module_cannot_have_multiple_default_exports :
Diagnostics.Duplicate_function_implementation;

for (let declaration of declarations) {
error(declaration.name || declaration, message);
}
}

// Abstract methods can't have an implementation -- in particular, they don't need one.
Expand All @@ -11191,12 +11200,12 @@ namespace ts {
}

if (hasOverloads) {
checkFlagAgreementBetweenOverloads(declarations, bodyDeclaration, flagsToCheck, someNodeFlags, allNodeFlags);
checkQuestionTokenAgreementBetweenOverloads(declarations, bodyDeclaration, someHaveQuestionToken, allHaveQuestionToken);
checkFlagAgreementBetweenOverloads(declarations, implementationDeclaration, flagsToCheck, someNodeFlags, allNodeFlags);
checkQuestionTokenAgreementBetweenOverloads(declarations, implementationDeclaration, someHaveQuestionToken, allHaveQuestionToken);

if (bodyDeclaration) {
if (implementationDeclaration) {
let signatures = getSignaturesOfSymbol(symbol);
let bodySignature = getSignatureFromDeclaration(bodyDeclaration);
let bodySignature = getSignatureFromDeclaration(implementationDeclaration);
// If the implementation signature has string literals, we will have reported an error in
// checkSpecializedSignatureDeclaration
if (!bodySignature.hasStringLiterals) {
Expand Down Expand Up @@ -11225,6 +11234,56 @@ namespace ts {
}
}

/**
* Determines if all declarations in the given list are function declarations, and that
* all the declarations have the same name. This is useful for how we report errors.
*
* For instance, if a user has
*
* export default function foo() { }
* export default function bar() { }
*
* We want to tell the user that there are multiple default exports. However, if a user has
*
* export default function foo() { }
* export default function foo() { }
*
* It is probably more accurate to say that there are duplicate function implementations.
*
* @param declarations
Copy link
Member

Choose a reason for hiding this comment

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

I don't know about our JSDoc style, but in the past I have skipped parameters that had no associated documentation. Is that allowed in JSDoc? Alternatively, instead of deleting this line, it would be nice to have some text here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I'll remove it

*/
function areAllFunctionDeclarationsWhereSomeNamesDiffer(declarations: Declaration[]) {
let numDeclarations = declarations.length;
Debug.assert(numDeclarations > 0, "Expected at least one declaration.");

let firstDeclaration = declarations[0];
if (firstDeclaration.kind !== SyntaxKind.FunctionDeclaration) {
return false;
}

let firstName = (firstDeclaration as FunctionDeclaration).name;
for (let i = 1; i < numDeclarations; i++) {
let currentDeclaration = declarations[i];
if (currentDeclaration.kind !== SyntaxKind.FunctionDeclaration) {
return false;
}

let currentName = (currentDeclaration as FunctionDeclaration).name;
if (currentName) {
if (!firstName || currentName.text !== firstName.text) {
// Either 'firstName' must be undefined or the text must differ.
return true;
}
}
else if (firstName) {
// 'currentName' is not defined but 'firstName' is.
return true;
}
}

return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

Checking both properties in this loop is hard to read. Why not use every and some, since the function name already expresses those concepts? I propose this rewrite:

    function areAllFunctionDeclarationsWhereSomeNamesDiffer(declarations: Declaration[]) {
        Debug.assert(declarations.length > 0, "Expected at least one declaration.");
        let firstName = (declarations[0] as FunctionDeclaration).name;
        return declarations.every(d => d.kind === SyntaxKind.FunctionDeclaration) &&
            declarations.some(d => {
                let currentName = (d as FunctionDeclaration).name;
                if (currentName && firstName) {
                    // Either 'firstName' must be undefined or the text must differ.
                    return currentName.text !== firstName.text;
                }
                else if (currentName || firstName) {
                    // one undefined, the other defined
                    return true;
                }
                else {
                    // both undefined
                    return false;
                }
            });
    }

This also makes the conditionals easier to read by avoiding nesting (mostly).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Since we're reporting an error here, perf isn't exactly a concern anyway, and your version is a lot cleaner.

Also, as a heads up, ES5 array functions such as some and every aren't permitted in the compiler because we sort of support running on ES3 runtimes.

function checkExportsOnMergedDeclarations(node: Node): void {
if (!produceDiagnostics) {
return;
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/multipleDefaultExports02.errors.txt
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
tests/cases/conformance/es6/modules/m1.ts(2,25): error TS2393: Duplicate function implementation.
tests/cases/conformance/es6/modules/m1.ts(6,25): error TS2393: Duplicate function implementation.
tests/cases/conformance/es6/modules/m1.ts(2,25): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m1.ts(6,25): error TS2528: A module cannot have multiple default exports.


==== tests/cases/conformance/es6/modules/m1.ts (2 errors) ====

export default function foo() {
~~~
!!! error TS2393: Duplicate function implementation.
!!! error TS2528: A module cannot have multiple default exports.

}

export default function bar() {
~~~
!!! error TS2393: Duplicate function implementation.
!!! error TS2528: A module cannot have multiple default exports.

}

Expand Down
62 changes: 62 additions & 0 deletions tests/baselines/reference/multipleDefaultExports05.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
tests/cases/conformance/es6/modules/m1.ts(2,1): error TS2393: Duplicate function implementation.
tests/cases/conformance/es6/modules/m1.ts(6,1): error TS2393: Duplicate function implementation.
tests/cases/conformance/es6/modules/m2.ts(1,1): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m2.ts(5,1): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m3.ts(1,1): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m3.ts(5,22): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m4.ts(1,25): error TS2528: A module cannot have multiple default exports.
tests/cases/conformance/es6/modules/m4.ts(5,1): error TS2528: A module cannot have multiple default exports.


==== tests/cases/conformance/es6/modules/m1.ts (2 errors) ====

export default function () {
~~~~~~
!!! error TS2393: Duplicate function implementation.

}

export default function () {
~~~~~~
!!! error TS2393: Duplicate function implementation.

}

==== tests/cases/conformance/es6/modules/m2.ts (2 errors) ====
export default function () {
~~~~~~
!!! error TS2528: A module cannot have multiple default exports.

}

export default class {
~~~~~~
!!! error TS2528: A module cannot have multiple default exports.

}

==== tests/cases/conformance/es6/modules/m3.ts (2 errors) ====
export default function () {
~~~~~~
!!! error TS2528: A module cannot have multiple default exports.

}

export default class C {
~
!!! error TS2528: A module cannot have multiple default exports.

}

==== tests/cases/conformance/es6/modules/m4.ts (2 errors) ====
export default function f() {
~
!!! error TS2528: A module cannot have multiple default exports.

}

export default class {
~~~~~~
!!! error TS2528: A module cannot have multiple default exports.

}
84 changes: 84 additions & 0 deletions tests/baselines/reference/multipleDefaultExports05.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
//// [tests/cases/conformance/es6/modules/multipleDefaultExports05.ts] ////

//// [m1.ts]

export default function () {

}

export default function () {

}

//// [m2.ts]
export default function () {

}

export default class {

}

//// [m3.ts]
export default function () {

}

export default class C {

}

//// [m4.ts]
export default function f() {

}

export default class {

}

//// [m1.js]
function default_1() {
}
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_1;
function default_2() {
}
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_2;
//// [m2.js]
function default_1() {
}
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_1;
var default_2 = (function () {
function default_2() {
}
return default_2;
})();
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_2;
//// [m3.js]
function default_1() {
}
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_1;
var C = (function () {
function C() {
}
return C;
})();
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = C;
//// [m4.js]
function f() {
}
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = f;
var default_1 = (function () {
function default_1() {
}
return default_1;
})();
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = default_1;
38 changes: 38 additions & 0 deletions tests/cases/conformance/es6/modules/multipleDefaultExports05.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// @module: commonjs
// @target: ES5

// @filename: m1.ts
export default function () {

}

export default function () {

}

// @filename: m2.ts
export default function () {

}

export default class {

}

// @filename: m3.ts
export default function () {

}

export default class C {

}

// @filename: m4.ts
export default function f() {

}

export default class {

}