Skip to content
This repository was archived by the owner on May 19, 2018. It is now read-only.

Disallow duplicate named exports #107

Merged
merged 1 commit into from
Sep 22, 2016
Merged
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
44 changes: 41 additions & 3 deletions src/parser/statement.js
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,7 @@ pp.parseExport = function (node) {
}
node.declaration = expr;
if (needsSemi) this.semicolon();
this.checkExport(node);
this.checkExport(node, true, true);
Copy link
Member

Choose a reason for hiding this comment

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

just a thought: might be nice if this method wasn't taking in true, true (an object, named, etc) but I know we do that everywhere else ^ this.parseFunction(expr, true, false, false, true); 😄

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, was trying to follow the style I saw elsewhere, but agree. Would you like me to change that to an object?

Copy link
Member

Choose a reason for hiding this comment

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

@hzoo: Do you mean checkExport(node: Node, options: { named: boolean, default: boolean }) or what do you mean with object? Maybe also checkExport(node: Node, type: "named" | "default" | null)?

Copy link
Member

Choose a reason for hiding this comment

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

yeah to make it more readable - dono if it was done before for perf?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed it was done this way for performance reasons

return this.finishNode(node, "ExportDefaultDeclaration");
} else if (this.state.type.keyword || this.shouldParseExportDeclaration()) {
node.specifiers = [];
Expand All @@ -855,7 +855,7 @@ pp.parseExport = function (node) {
node.specifiers = this.parseExportSpecifiers();
this.parseExportFrom(node);
}
this.checkExport(node);
this.checkExport(node, true);
return this.finishNode(node, "ExportNamedDeclaration");
};

Expand Down Expand Up @@ -903,7 +903,31 @@ pp.shouldParseExportDeclaration = function () {
return this.isContextual("async");
};

pp.checkExport = function (node) {
pp.checkExport = function (node, checkNames, isDefault) {
if (checkNames) {
// Check for duplicate exports
if (isDefault) {
// Default exports
this.checkDuplicateExports(node, "default", isDefault);
} else if (node.specifiers && node.specifiers.length) {
// Named exports
for (let specifier of node.specifiers) {
const name = specifier.exported.name;
if (name === "default") isDefault = true;
this.checkDuplicateExports(specifier, name, isDefault);
}
} else if (node.declaration) {
// Exported declarations
if (node.declaration.type === "FunctionDeclaration" || node.declaration.type === "ClassDeclaration") {
this.checkDuplicateExports(node, node.declaration.id.name, isDefault);
} else if (node.declaration.type === "VariableDeclaration") {
for (let declaration of node.declaration.declarations) {
this.checkDuplicateExports(declaration, declaration.id.name, isDefault);
}
}
}
}

if (this.state.decorators.length) {
let isClass = node.declaration && (node.declaration.type === "ClassDeclaration" || node.declaration.type === "ClassExpression");
if (!node.declaration || !isClass) {
Expand All @@ -913,6 +937,20 @@ pp.checkExport = function (node) {
}
};

pp.checkDuplicateExports = function(node, name, isDefault) {
if (this.state.exportedIdentifiers[name]) {
Copy link

Choose a reason for hiding this comment

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

I think we need to manage Object.prototype here.

Something along the lines of this maybe?

if (Object.getOwnPropertyNames(this.state.exportedIdentifiers).indexOf(name) !== -1) {

Copy link
Member

Choose a reason for hiding this comment

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

You are right, I wrap up a fix

Copy link
Member

@hzoo hzoo Sep 22, 2016

Choose a reason for hiding this comment

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

Ah interesting, thanks for the report

cc @kaicataldo

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, that makes sense. @danez Let me know if you need me work on a fix - happy to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Same issue in jscs-dev/node-jscs#1204 😄

this.raiseDuplicateExportError(node, name, isDefault);
}
this.state.exportedIdentifiers[name] = true;
};

pp.raiseDuplicateExportError = function(node, name, isDefault) {
this.raise(node.start, isDefault ?
"Only one default export allowed per module." :
`\`${name}\` has already been exported. Exported identifiers must be unique.`
);
};

// Parses a comma-separated list of module exports.

pp.parseExportSpecifiers = function () {
Expand Down
6 changes: 6 additions & 0 deletions src/tokenizer/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export default class State {
this.containsEsc = this.containsOctal = false;
this.octalPosition = null;

this.exportedIdentifiers = {};

return this;
}

Expand Down Expand Up @@ -119,6 +121,10 @@ export default class State {
containsOctal: boolean;
octalPosition: ?number;

// Names of exports store. `default` is stored as a name for both
// `export default foo;` and `export { foo as default };`.
exportedIdentifiers: {[id:string]: boolean};

curPosition() {
return new Position(this.curLine, this.pos - this.lineStart);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export default function() {};
export { foo as default };
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "Only one default export allowed per module. (2:9)"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export default {};
export default function() {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "Only one default export allowed per module. (2:0)"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { Foo };
export class Foo {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"throws": "`Foo` has already been exported. Exported identifiers must be unique. (2:0)"
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { foo };
export function foo() {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "`foo` has already been exported. Exported identifiers must be unique. (2:0)"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { foo };
export const foo = bar;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "`foo` has already been exported. Exported identifiers must be unique. (2:13)"
}
2 changes: 2 additions & 0 deletions test/fixtures/es2015/modules/duplicate-named-export/actual.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { foo };
export { bar as foo };
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"throws": "`foo` has already been exported. Exported identifiers must be unique. (2:9)"
}
2 changes: 1 addition & 1 deletion test/fixtures/flow/type-exports/interface/actual.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export interface foo { p: number };
export interface foo<T> { p: T };
export interface bar<T> { p: T };
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty new to Flow - let me know if this isn't the correct fix for this test

Copy link
Member

Choose a reason for hiding this comment

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

This is correct

4 changes: 2 additions & 2 deletions test/fixtures/flow/type-exports/interface/expected.json
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@
"column": 20
}
},
"name": "foo"
"name": "bar"
},
"typeParameters": {
"type": "TypeParameterDeclaration",
Expand Down Expand Up @@ -346,4 +346,4 @@
],
"directives": []
}
}
}