From 630f1717f0072f8df8dc2da6f2a450a013b81953 Mon Sep 17 00:00:00 2001 From: Sebastian McKenzie Date: Tue, 3 Feb 2015 13:20:52 +1100 Subject: [PATCH] clean up scope collision tracking and constants transformer - fixes #331 --- lib/6to5/file.js | 2 +- .../transformers/es6/constants.js | 2 +- lib/6to5/traverse/scope.js | 80 ++++++++++--------- lib/6to5/types/index.js | 9 ++- .../es6-constants/no-classes/options.json | 2 +- .../es6-constants/no-declaration/options.json | 2 +- .../es6-constants/no-functions/options.json | 2 +- .../es6-constants/update-expression/actual.js | 2 + .../update-expression/options.json | 3 + 9 files changed, 59 insertions(+), 45 deletions(-) create mode 100644 test/fixtures/transformation/es6-constants/update-expression/actual.js create mode 100644 test/fixtures/transformation/es6-constants/update-expression/options.json diff --git a/lib/6to5/file.js b/lib/6to5/file.js index 3486f41a2f..cbb26b5218 100644 --- a/lib/6to5/file.js +++ b/lib/6to5/file.js @@ -412,7 +412,7 @@ File.prototype.generateUid = function (name, scope) { File.prototype.generateUidIdentifier = function (name, scope) { scope = scope || this.scope; var id = t.identifier(this.generateUid(name, scope)); - scope.add(id); + scope.addDeclarationToFunctionScope("var", id); return id; }; diff --git a/lib/6to5/transformation/transformers/es6/constants.js b/lib/6to5/transformation/transformers/es6/constants.js index 394bf7a49d..ce245c63b0 100644 --- a/lib/6to5/transformation/transformers/es6/constants.js +++ b/lib/6to5/transformation/transformers/es6/constants.js @@ -5,7 +5,7 @@ var t = require("../../../types"); var visitor = { enter: function (node, parent, scope, context, state) { - if (t.isDeclaration(node) || t.isAssignmentExpression(node)) { + if (t.isAssignmentExpression(node) || t.isUpdateExpression(node)) { var ids = t.getBindingIdentifiers(node); for (var key in ids) { diff --git a/lib/6to5/traverse/scope.js b/lib/6to5/traverse/scope.js index b3eeb649e4..340ad5c125 100644 --- a/lib/6to5/traverse/scope.js +++ b/lib/6to5/traverse/scope.js @@ -2,17 +2,16 @@ module.exports = Scope; +var contains = require("lodash/collection/contains"); var traverse = require("./index"); +var defaults = require("lodash/object/defaults"); var globals = require("globals"); +var flatten = require("lodash/array/flatten"); +var extend = require("lodash/object/extend"); var object = require("../helpers/object"); -var t = require("../types"); var each = require("lodash/collection/each"); var has = require("lodash/object/has"); -var contains = require("lodash/collection/contains"); -var flatten = require("lodash/array/flatten"); -var defaults = require("lodash/object/defaults"); - -var FOR_KEYS = ["left", "init"]; +var t = require("../types"); /** * This searches the current "scope" and collects all references/declarations @@ -39,22 +38,6 @@ function Scope(block, parentBlock, parent, file) { Scope.defaultDeclarations = flatten([globals.builtin, globals.browser, globals.node].map(Object.keys)); -Scope.prototype._add = function (node, references, throwOnDuplicate) { - if (!node) return; - - var ids = t.getBindingIdentifiers(node); - - for (var key in ids) { - var id = ids[key]; - - if (throwOnDuplicate && references[key]) { - throw this.file.errorWithNode(id, "Duplicate declaration", TypeError); - } - - references[key] = id; - } -}; - /** * Description * @@ -145,7 +128,7 @@ Scope.prototype.generateTempBasedOnNode = function (node) { var functionVariableVisitor = { enter: function (node, parent, scope, context, state) { if (t.isFor(node)) { - each(FOR_KEYS, function (key) { + each(t.FOR_INIT_KEYS, function (key) { var declar = node[key]; if (t.isVar(declar)) state.add(declar); }); @@ -158,11 +141,14 @@ var functionVariableVisitor = { // function identifier doesn't belong to this scope if (state.blockId && node === state.blockId) return; + // delegate block scope handling to the `blockVariableVisitor` + if (t.isBlockScoped(node)) return; + + // this will be hit again once we traverse into it after this iteration + if (t.isExportDeclaration(node) && t.isDeclaration(node.declaration)) return; + // we've ran into a declaration! - // we'll let the BlockStatement scope deal with `let` declarations unless - if (t.isDeclaration(node) && !t.isBlockScoped(node)) { - state.add(node); - } + if (t.isDeclaration(node)) state.add(node); } }; @@ -177,7 +163,7 @@ var programReferenceVisitor = { var blockVariableVisitor = { enter: function (node, parent, scope, context, add) { if (t.isBlockScoped(node)) { - add(node, false, t.isLet(node)); + add(node, false); } else if (t.isScope(node)) { context.skip(); } @@ -193,14 +179,28 @@ Scope.prototype.getInfo = function () { var info = block._scopeInfo = {}; var references = info.references = object(); var declarations = info.declarations = object(); - var declarationKinds = info.declarationKinds = {}; + var declarationKinds = info.declarationKinds = { + "var": object(), + "let": object(), + "const": object() + }; - var add = function (node, reference, throwOnDuplicate) { - self._add(node, references); + var add = function (node, reference) { + var ids = t.getBindingIdentifiers(node); + + extend(references, ids); if (!reference) { - self._add(node, declarations, throwOnDuplicate); - self._add(node, declarationKinds[node.kind] = declarationKinds[node.kind] || object()); + for (var key in ids) { + if (declarationKinds["let"][key] || declarationKinds["const"][key]) { + throw self.file.errorWithNode(ids[key], "Duplicate declaration " + key, TypeError); + } + } + + extend(declarations, ids); + + var kinds = declarationKinds[node.kind]; + if (kinds) extend(kinds, ids); } }; @@ -212,7 +212,7 @@ Scope.prototype.getInfo = function () { // ForStatement - left, init if (t.isFor(block)) { - each(FOR_KEYS, function (key) { + each(t.FOR_INIT_KEYS, function (key) { var node = block[key]; if (t.isBlockScoped(node)) add(node, false, true); }); @@ -304,12 +304,20 @@ Scope.prototype.push = function (opts) { * Walk up the scope tree until we hit a `Function` and then * push our `node` to it's references. * + * @param {String} kind * @param {Object} node */ -Scope.prototype.add = function (node) { +Scope.prototype.addDeclarationToFunctionScope = function (kind, node) { var scope = this.getFunctionParent(); - scope._add(node, scope.references); + var ids = t.getBindingIdentifiers(node); + + extend(scope.declarations, ids); + extend(scope.references, ids); + + // this ignores the duplicate declaration logic specified in `getInfo` + // but it doesn't really matter + extend(scope.declarationKinds[kind], ids); }; /** diff --git a/lib/6to5/types/index.js b/lib/6to5/types/index.js index 878b8f5424..d874496bdd 100644 --- a/lib/6to5/types/index.js +++ b/lib/6to5/types/index.js @@ -34,11 +34,11 @@ function registerType(type, skipAliasCheck) { } t.STATEMENT_OR_BLOCK_KEYS = ["consequent", "body"]; -t.NATIVE_TYPE_NAMES = ["Array", "Object", "Number", "Boolean", "Date", "Array", "String"]; +t.NATIVE_TYPE_NAMES = ["Array", "Object", "Number", "Boolean", "Date", "Array", "String"]; +t.FOR_INIT_KEYS = ["left", "init"]; t.VISITOR_KEYS = require("./visitor-keys"); - -t.ALIAS_KEYS = require("./alias-keys"); +t.ALIAS_KEYS = require("./alias-keys"); t.FLIPPED_ALIAS_KEYS = {}; @@ -605,6 +605,7 @@ t.getBindingIdentifiers.keys = { MemeberExpression: ["object"], SpreadElement: ["argument"], RestElement: ["argument"], + UpdateExpression: ["argument"], Property: ["value"], ComprehensionBlock: ["left"], AssignmentPattern: ["left"], @@ -635,7 +636,7 @@ t.isLet = function (node) { */ t.isBlockScoped = function (node) { - return t.isFunctionDeclaration(node) || t.isLet(node); + return t.isFunctionDeclaration(node) || t.isClassDeclaration(node) || t.isLet(node); }; /** diff --git a/test/fixtures/transformation/es6-constants/no-classes/options.json b/test/fixtures/transformation/es6-constants/no-classes/options.json index 9b96d41ac9..d51ef9b5ce 100644 --- a/test/fixtures/transformation/es6-constants/no-classes/options.json +++ b/test/fixtures/transformation/es6-constants/no-classes/options.json @@ -1,3 +1,3 @@ { - "throws": "MULTIPLIER is read-only" + "throws": "Duplicate declaration MULTIPLIER" } diff --git a/test/fixtures/transformation/es6-constants/no-declaration/options.json b/test/fixtures/transformation/es6-constants/no-declaration/options.json index 9b96d41ac9..d51ef9b5ce 100644 --- a/test/fixtures/transformation/es6-constants/no-declaration/options.json +++ b/test/fixtures/transformation/es6-constants/no-declaration/options.json @@ -1,3 +1,3 @@ { - "throws": "MULTIPLIER is read-only" + "throws": "Duplicate declaration MULTIPLIER" } diff --git a/test/fixtures/transformation/es6-constants/no-functions/options.json b/test/fixtures/transformation/es6-constants/no-functions/options.json index 9b96d41ac9..d51ef9b5ce 100644 --- a/test/fixtures/transformation/es6-constants/no-functions/options.json +++ b/test/fixtures/transformation/es6-constants/no-functions/options.json @@ -1,3 +1,3 @@ { - "throws": "MULTIPLIER is read-only" + "throws": "Duplicate declaration MULTIPLIER" } diff --git a/test/fixtures/transformation/es6-constants/update-expression/actual.js b/test/fixtures/transformation/es6-constants/update-expression/actual.js new file mode 100644 index 0000000000..0ea22ef9f3 --- /dev/null +++ b/test/fixtures/transformation/es6-constants/update-expression/actual.js @@ -0,0 +1,2 @@ +const foo = 1; +foo++; diff --git a/test/fixtures/transformation/es6-constants/update-expression/options.json b/test/fixtures/transformation/es6-constants/update-expression/options.json new file mode 100644 index 0000000000..e3a5a52096 --- /dev/null +++ b/test/fixtures/transformation/es6-constants/update-expression/options.json @@ -0,0 +1,3 @@ +{ + "throws": "foo is read-only" +}