From 0345c1bc1ded6af8d66f8605e6fdbeeb9b70c5b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Tue, 26 Feb 2019 20:09:02 +0100 Subject: [PATCH] Use `for..of Object.keys` instead of `for..in` (#9518) In https://github.com/babel/babel/issues/9511 (and #9495 is another symptom), @PavelKastornyy reported a node crash becaue the JavaScript heap run out of memory. The problem was that their code was adding enumerable properties to `Object.prototype`: it is something that shouldn't be done, but Babel shouldn't make node crash if someone adds them. I reduced down the problem to `for...in` loops in `@babel/traverse` that grew the memory consumption exponentially because of that unexpected properties. --- .eslintrc.json | 3 ++- packages/babel-helper-define-map/src/index.js | 2 +- .../babel-helper-hoist-variables/src/index.js | 2 +- packages/babel-parser/src/options.js | 2 +- .../test/helpers/runFixtureTests.js | 4 ++-- .../src/index.js | 2 +- .../src/index.js | 20 +++++++++---------- .../src/tdz.js | 2 +- .../src/index.js | 2 +- .../src/index.js | 8 +++++--- packages/babel-traverse/src/path/context.js | 2 +- packages/babel-traverse/src/path/index.js | 2 +- .../babel-traverse/src/path/lib/hoister.js | 6 +++--- .../babel-traverse/src/path/replacement.js | 2 +- packages/babel-traverse/src/scope/index.js | 13 ++++++------ packages/babel-traverse/src/visitors.js | 20 +++++++++---------- packages/babel-types/src/builders/builder.js | 2 +- .../converters/gatherSequenceExpressions.js | 2 +- .../babel-types/src/converters/valueToNode.js | 2 +- packages/babel-types/src/definitions/utils.js | 2 +- .../flow/removeTypeDuplicates.js | 4 ++-- .../babel-types/src/modifications/inherits.js | 2 +- .../src/modifications/removeProperties.js | 2 +- .../src/validators/isNodesEquivalent.js | 7 ++++++- 24 files changed, 60 insertions(+), 55 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index 8a8366b60b..a59dc0d9e2 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -14,7 +14,8 @@ "rules": { "@babel/development/no-undefined-identifier": "error", "@babel/development/no-deprecated-clone": "error", - "import/no-extraneous-dependencies": "error" + "import/no-extraneous-dependencies": "error", + "guard-for-in": "error" } }, { diff --git a/packages/babel-helper-define-map/src/index.js b/packages/babel-helper-define-map/src/index.js index ff588c1ce3..da81b3a43b 100644 --- a/packages/babel-helper-define-map/src/index.js +++ b/packages/babel-helper-define-map/src/index.js @@ -98,7 +98,7 @@ export function push( } export function hasComputed(mutatorMap: Object): boolean { - for (const key in mutatorMap) { + for (const key of Object.keys(mutatorMap)) { if (mutatorMap[key]._computed) { return true; } diff --git a/packages/babel-helper-hoist-variables/src/index.js b/packages/babel-helper-hoist-variables/src/index.js index 4f9c6d938b..6b8bb4cf3b 100644 --- a/packages/babel-helper-hoist-variables/src/index.js +++ b/packages/babel-helper-hoist-variables/src/index.js @@ -28,7 +28,7 @@ const visitor = { ); } - for (const name in declar.getBindingIdentifiers()) { + for (const name of Object.keys(declar.getBindingIdentifiers())) { state.emit(t.identifier(name), name, declar.node.init !== null); } } diff --git a/packages/babel-parser/src/options.js b/packages/babel-parser/src/options.js index 3bab7c04a2..c11dd55cca 100755 --- a/packages/babel-parser/src/options.js +++ b/packages/babel-parser/src/options.js @@ -61,7 +61,7 @@ export const defaultOptions: Options = { export function getOptions(opts: ?Options): Options { const options: any = {}; - for (const key in defaultOptions) { + for (const key of Object.keys(defaultOptions)) { options[key] = opts && opts[key] != null ? opts[key] : defaultOptions[key]; } return options; diff --git a/packages/babel-parser/test/helpers/runFixtureTests.js b/packages/babel-parser/test/helpers/runFixtureTests.js index be42014fd7..a3915587f4 100644 --- a/packages/babel-parser/test/helpers/runFixtureTests.js +++ b/packages/babel-parser/test/helpers/runFixtureTests.js @@ -195,12 +195,12 @@ function misMatch(exp, act) { return ppJSON(exp) + " !== " + ppJSON(act); } } else { - for (const prop in exp) { + for (const prop of Object.keys(exp)) { const mis = misMatch(exp[prop], act[prop]); if (mis) return addPath(mis, prop); } - for (const prop in act) { + for (const prop of Object.keys(act)) { if (typeof act[prop] === "function") { continue; } diff --git a/packages/babel-plugin-proposal-object-rest-spread/src/index.js b/packages/babel-plugin-proposal-object-rest-spread/src/index.js index 8506c21db7..c4738f0c67 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/src/index.js +++ b/packages/babel-plugin-proposal-object-rest-spread/src/index.js @@ -302,7 +302,7 @@ export default declare((api, opts) => { const specifiers = []; - for (const name in path.getOuterBindingIdentifiers(path)) { + for (const name of Object.keys(path.getOuterBindingIdentifiers(path))) { specifiers.push( t.exportSpecifier(t.identifier(name), t.identifier(name)), ); diff --git a/packages/babel-plugin-transform-block-scoping/src/index.js b/packages/babel-plugin-transform-block-scoping/src/index.js index ef91aa55b9..cc63c554dd 100644 --- a/packages/babel-plugin-transform-block-scoping/src/index.js +++ b/packages/babel-plugin-transform-block-scoping/src/index.js @@ -153,8 +153,7 @@ function convertBlockScopedToVar( // Move bindings from current block scope to function scope. if (moveBindingsToParent) { const parentScope = scope.getFunctionParent() || scope.getProgramParent(); - const ids = path.getBindingIdentifiers(); - for (const name in ids) { + for (const name of Object.keys(path.getBindingIdentifiers())) { const binding = scope.getOwnBinding(name); if (binding) binding.kind = "var"; scope.moveBindingTo(name, parentScope); @@ -245,8 +244,7 @@ const loopLabelVisitor = { const continuationVisitor = { enter(path, state) { if (path.isAssignmentExpression() || path.isUpdateExpression()) { - const bindings = path.getBindingIdentifiers(); - for (const name in bindings) { + for (const name of Object.keys(path.getBindingIdentifiers())) { if ( state.outsideReferences[name] !== path.scope.getBindingIdentifier(name) @@ -410,7 +408,7 @@ class BlockScoping { const scope = this.scope; const state = this.state; - for (const name in scope.bindings) { + for (const name of Object.keys(scope.bindings)) { const binding = scope.bindings[name]; if (binding.kind !== "const") continue; @@ -444,7 +442,7 @@ class BlockScoping { const parentScope = scope.getFunctionParent() || scope.getProgramParent(); const letRefs = this.letReferences; - for (const key in letRefs) { + for (const key of Object.keys(letRefs)) { const ref = letRefs[key]; const binding = scope.getBinding(ref.name); if (!binding) continue; @@ -471,7 +469,7 @@ class BlockScoping { // those in upper scopes and then if they do, generate a uid // for them and replace all references with it - for (const key in letRefs) { + for (const key of Object.keys(letRefs)) { // just an Identifier node we collected in `getLetReferences` // this is the defining identifier of a declaration const ref = letRefs[key]; @@ -491,7 +489,7 @@ class BlockScoping { } } - for (const key in outsideLetRefs) { + for (const key of Object.keys(outsideLetRefs)) { const ref = letRefs[key]; // check for collisions with a for loop's init variable and the enclosing scope's bindings // https://github.com/babel/babel/issues/8498 @@ -514,7 +512,7 @@ class BlockScoping { // remap loop heads with colliding variables if (this.loop) { - for (const name in outsideRefs) { + for (const name of Object.keys(outsideRefs)) { const id = outsideRefs[name]; if ( @@ -814,7 +812,7 @@ class BlockScoping { pushDeclar(node: { type: "VariableDeclaration" }): Array { const declars = []; const names = t.getBindingIdentifiers(node); - for (const name in names) { + for (const name of Object.keys(names)) { declars.push(t.variableDeclarator(names[name])); } @@ -852,7 +850,7 @@ class BlockScoping { } if (has.hasBreakContinue) { - for (const key in has.map) { + for (const key of Object.keys(has.map)) { cases.push(t.switchCase(t.stringLiteral(key), [has.map[key]])); } diff --git a/packages/babel-plugin-transform-block-scoping/src/tdz.js b/packages/babel-plugin-transform-block-scoping/src/tdz.js index e142351348..9612b8deb2 100644 --- a/packages/babel-plugin-transform-block-scoping/src/tdz.js +++ b/packages/babel-plugin-transform-block-scoping/src/tdz.js @@ -83,7 +83,7 @@ export const visitor = { const nodes = []; const ids = path.getBindingIdentifiers(); - for (const name in ids) { + for (const name of Object.keys(ids)) { const id = ids[name]; if (isReference(id, path.scope, state)) { diff --git a/packages/babel-plugin-transform-destructuring/src/index.js b/packages/babel-plugin-transform-destructuring/src/index.js index 52a76928e1..de2b9ff4ba 100644 --- a/packages/babel-plugin-transform-destructuring/src/index.js +++ b/packages/babel-plugin-transform-destructuring/src/index.js @@ -421,7 +421,7 @@ export default declare((api, options) => { const specifiers = []; - for (const name in path.getOuterBindingIdentifiers(path)) { + for (const name of Object.keys(path.getOuterBindingIdentifiers(path))) { specifiers.push( t.exportSpecifier(t.identifier(name), t.identifier(name)), ); diff --git a/packages/babel-plugin-transform-modules-systemjs/src/index.js b/packages/babel-plugin-transform-modules-systemjs/src/index.js index f43fc7a34f..0159e2257f 100644 --- a/packages/babel-plugin-transform-modules-systemjs/src/index.js +++ b/packages/babel-plugin-transform-modules-systemjs/src/index.js @@ -113,7 +113,7 @@ export default declare((api, options) => { if (arg.isObjectPattern() || arg.isArrayPattern()) { const exprs = [path.node]; - for (const name in arg.getBindingIdentifiers()) { + for (const name of Object.keys(arg.getBindingIdentifiers())) { if (this.scope.getBinding(name) !== path.scope.getBinding(name)) { return; } @@ -266,7 +266,7 @@ export default declare((api, options) => { } else if (path.isImportDeclaration()) { const source = path.node.source.value; pushModule(source, "imports", path.node.specifiers); - for (const name in path.getBindingIdentifiers()) { + for (const name of Object.keys(path.getBindingIdentifiers())) { path.scope.removeBinding(name); variableIds.push(t.identifier(name)); } @@ -320,7 +320,9 @@ export default declare((api, options) => { addExportName(name, name); path.insertAfter([buildExportCall(name, t.identifier(name))]); } else { - for (const name in declar.getBindingIdentifiers()) { + for (const name of Object.keys( + declar.getBindingIdentifiers(), + )) { addExportName(name, name); } } diff --git a/packages/babel-traverse/src/path/context.js b/packages/babel-traverse/src/path/context.js index a97045e10f..0057b1694d 100644 --- a/packages/babel-traverse/src/path/context.js +++ b/packages/babel-traverse/src/path/context.js @@ -169,7 +169,7 @@ export function _resyncKey() { } } } else { - for (const key in this.container) { + for (const key of Object.keys(this.container)) { if (this.container[key] === this.node) { return this.setKey(key); } diff --git a/packages/babel-traverse/src/path/index.js b/packages/babel-traverse/src/path/index.js index 506a25e95d..f76ba1ec5e 100644 --- a/packages/babel-traverse/src/path/index.js +++ b/packages/babel-traverse/src/path/index.js @@ -183,7 +183,7 @@ for (const type of (t.TYPES: Array)) { }; } -for (const type in virtualTypes) { +for (const type of Object.keys(virtualTypes)) { if (type[0] === "_") continue; if (t.TYPES.indexOf(type) < 0) t.TYPES.push(type); diff --git a/packages/babel-traverse/src/path/lib/hoister.js b/packages/babel-traverse/src/path/lib/hoister.js index edd2738510..2fd8339eaa 100644 --- a/packages/babel-traverse/src/path/lib/hoister.js +++ b/packages/babel-traverse/src/path/lib/hoister.js @@ -58,7 +58,7 @@ export default class PathHoister { // A scope is compatible if all required bindings are reachable. isCompatibleScope(scope) { - for (const key in this.bindings) { + for (const key of Object.keys(this.bindings)) { const binding = this.bindings[key]; if (!scope.bindingIdentifierEquals(key, binding.identifier)) { return false; @@ -98,7 +98,7 @@ export default class PathHoister { // avoid hoisting to a scope that contains bindings that are executed after our attachment path if (targetScope.path.isProgram() || targetScope.path.isFunction()) { - for (const name in this.bindings) { + for (const name of Object.keys(this.bindings)) { // check binding is a direct child of this paths scope if (!targetScope.hasOwnBinding(name)) continue; @@ -182,7 +182,7 @@ export default class PathHoister { // Returns true if a scope has param bindings. hasOwnParamBindings(scope) { - for (const name in this.bindings) { + for (const name of Object.keys(this.bindings)) { if (!scope.hasOwnBinding(name)) continue; const binding = this.bindings[name]; diff --git a/packages/babel-traverse/src/path/replacement.js b/packages/babel-traverse/src/path/replacement.js index 0769185334..0c411cb401 100644 --- a/packages/babel-traverse/src/path/replacement.js +++ b/packages/babel-traverse/src/path/replacement.js @@ -15,7 +15,7 @@ const hoistVariablesVisitor = { if (path.node.kind !== "var") return; const bindings = path.getBindingIdentifiers(); - for (const key in bindings) { + for (const key of Object.keys(bindings)) { path.scope.push({ id: bindings[key] }); } diff --git a/packages/babel-traverse/src/scope/index.js b/packages/babel-traverse/src/scope/index.js index 88383fa66c..2976cff173 100644 --- a/packages/babel-traverse/src/scope/index.js +++ b/packages/babel-traverse/src/scope/index.js @@ -97,8 +97,7 @@ const collectorVisitor = { if (binding) binding.reference(path); } else if (t.isVariableDeclaration(declar)) { for (const decl of (declar.declarations: Array)) { - const ids = t.getBindingIdentifiers(decl); - for (const name in ids) { + for (const name of Object.keys(t.getBindingIdentifiers(decl))) { const binding = scope.getBinding(name); if (binding) binding.reference(path); } @@ -388,7 +387,7 @@ export default class Scope { let scope = this; do { console.log("#", scope.block.type); - for (const name in scope.bindings) { + for (const name of Object.keys(scope.bindings)) { const binding = scope.bindings[name]; console.log(" -", name, { constant: binding.constant, @@ -501,7 +500,7 @@ export default class Scope { registerConstantViolation(path: NodePath) { const ids = path.getBindingIdentifiers(); - for (const name in ids) { + for (const name of Object.keys(ids)) { const binding = this.getBinding(name); if (binding) binding.reassign(path); } @@ -521,7 +520,7 @@ export default class Scope { const parent = this.getProgramParent(); const ids = path.getBindingIdentifiers(true); - for (const name in ids) { + for (const name of Object.keys(ids)) { for (const id of (ids[name]: Array)) { const local = this.getOwnBinding(name); @@ -746,7 +745,7 @@ export default class Scope { // register undeclared bindings as globals const ids = path.getBindingIdentifiers(); let programParent; - for (const name in ids) { + for (const name of Object.keys(ids)) { if (path.scope.getBinding(name)) continue; programParent = programParent || path.scope.getProgramParent(); @@ -886,7 +885,7 @@ export default class Scope { for (const kind of (arguments: Array)) { let scope = this; do { - for (const name in scope.bindings) { + for (const name of Object.keys(scope.bindings)) { const binding = scope.bindings[name]; if (binding.kind === kind) ids[name] = binding; } diff --git a/packages/babel-traverse/src/visitors.js b/packages/babel-traverse/src/visitors.js index fd8a8ece29..77c1488d5d 100644 --- a/packages/babel-traverse/src/visitors.js +++ b/packages/babel-traverse/src/visitors.js @@ -23,7 +23,7 @@ export function explode(visitor) { visitor._exploded = true; // normalise pipes - for (const nodeType in visitor) { + for (const nodeType of Object.keys(visitor)) { if (shouldIgnoreKey(nodeType)) continue; const parts: Array = nodeType.split("|"); @@ -59,7 +59,7 @@ export function explode(visitor) { // wrap all the functions const fns = visitor[nodeType]; - for (const type in fns) { + for (const type of Object.keys(fns)) { fns[type] = wrapCheck(wrapper, fns[type]); } @@ -81,7 +81,7 @@ export function explode(visitor) { } // add aliases - for (const nodeType in visitor) { + for (const nodeType of Object.keys(visitor)) { if (shouldIgnoreKey(nodeType)) continue; const fns = visitor[nodeType]; @@ -111,7 +111,7 @@ export function explode(visitor) { } } - for (const nodeType in visitor) { + for (const nodeType of Object.keys(visitor)) { if (shouldIgnoreKey(nodeType)) continue; ensureCallbackArrays(visitor[nodeType]); @@ -130,7 +130,7 @@ export function verify(visitor) { ); } - for (const nodeType in visitor) { + for (const nodeType of Object.keys(visitor)) { if (nodeType === "enter" || nodeType === "exit") { validateVisitorMethods(nodeType, visitor[nodeType]); } @@ -145,7 +145,7 @@ export function verify(visitor) { const visitors = visitor[nodeType]; if (typeof visitors === "object") { - for (const visitorKey in visitors) { + for (const visitorKey of Object.keys(visitors)) { if (visitorKey === "enter" || visitorKey === "exit") { // verify that it just contains functions validateVisitorMethods( @@ -189,7 +189,7 @@ export function merge( explode(visitor); - for (const type in visitor) { + for (const type of Object.keys(visitor)) { let visitorType = visitor[type]; // if we have state or wrapper then overload the callbacks to take it @@ -208,7 +208,7 @@ export function merge( function wrapWithStateOrWrapper(oldVisitor, state, wrapper: ?Function) { const newVisitor = {}; - for (const key in oldVisitor) { + for (const key of Object.keys(oldVisitor)) { let fns = oldVisitor[key]; // not an enter/exit array of callbacks @@ -237,7 +237,7 @@ function wrapWithStateOrWrapper(oldVisitor, state, wrapper: ?Function) { } function ensureEntranceObjects(obj) { - for (const key in obj) { + for (const key of Object.keys(obj)) { if (shouldIgnoreKey(key)) continue; const fns = obj[key]; @@ -278,7 +278,7 @@ function shouldIgnoreKey(key) { } function mergePair(dest, src) { - for (const key in src) { + for (const key of Object.keys(src)) { dest[key] = [].concat(dest[key] || [], src[key]); } } diff --git a/packages/babel-types/src/builders/builder.js b/packages/babel-types/src/builders/builder.js index 7458eb1c92..e01cf9fd97 100644 --- a/packages/babel-types/src/builders/builder.js +++ b/packages/babel-types/src/builders/builder.js @@ -28,7 +28,7 @@ export default function builder(type: string, ...args: Array): Object { i++; }); - for (const key in node) { + for (const key of Object.keys(node)) { validate(node, key, node[key]); } diff --git a/packages/babel-types/src/converters/gatherSequenceExpressions.js b/packages/babel-types/src/converters/gatherSequenceExpressions.js index 4b526767e2..e9808e6ed3 100644 --- a/packages/babel-types/src/converters/gatherSequenceExpressions.js +++ b/packages/babel-types/src/converters/gatherSequenceExpressions.js @@ -36,7 +36,7 @@ export default function gatherSequenceExpressions( for (const declar of (node.declarations: Array)) { const bindings = getBindingIdentifiers(declar); - for (const key in bindings) { + for (const key of Object.keys(bindings)) { declars.push({ kind: node.kind, id: cloneNode(bindings[key]), diff --git a/packages/babel-types/src/converters/valueToNode.js b/packages/babel-types/src/converters/valueToNode.js index a4f195765c..56bb7ff06b 100644 --- a/packages/babel-types/src/converters/valueToNode.js +++ b/packages/babel-types/src/converters/valueToNode.js @@ -77,7 +77,7 @@ export default function valueToNode(value: any): Object { // object if (isPlainObject(value)) { const props = []; - for (const key in value) { + for (const key of Object.keys(value)) { let nodeKey; if (isValidIdentifier(key)) { nodeKey = identifier(key); diff --git a/packages/babel-types/src/definitions/utils.js b/packages/babel-types/src/definitions/utils.js index 45cf72b297..c1d2ce762d 100644 --- a/packages/babel-types/src/definitions/utils.js +++ b/packages/babel-types/src/definitions/utils.js @@ -201,7 +201,7 @@ export default function defineType( fields[key] = fields[key] || {}; } - for (const key in fields) { + for (const key of Object.keys(fields)) { const field = fields[key]; if (builder.indexOf(key) === -1) { diff --git a/packages/babel-types/src/modifications/flow/removeTypeDuplicates.js b/packages/babel-types/src/modifications/flow/removeTypeDuplicates.js index d27a8d2571..629c265ff5 100644 --- a/packages/babel-types/src/modifications/flow/removeTypeDuplicates.js +++ b/packages/babel-types/src/modifications/flow/removeTypeDuplicates.js @@ -73,12 +73,12 @@ export default function removeTypeDuplicates( } // add back in bases - for (const type in bases) { + for (const type of Object.keys(bases)) { types.push(bases[type]); } // add back in generics - for (const name in generics) { + for (const name of Object.keys(generics)) { types.push(generics[name]); } diff --git a/packages/babel-types/src/modifications/inherits.js b/packages/babel-types/src/modifications/inherits.js index dba679c050..a802182859 100644 --- a/packages/babel-types/src/modifications/inherits.js +++ b/packages/babel-types/src/modifications/inherits.js @@ -16,7 +16,7 @@ export default function inherits(child: T, parent: Object): T { } // force inherit "private" properties - for (const key in parent) { + for (const key of Object.keys(parent)) { if (key[0] === "_" && key !== "__clone") child[key] = parent[key]; } diff --git a/packages/babel-types/src/modifications/removeProperties.js b/packages/babel-types/src/modifications/removeProperties.js index 2f81d9cd7e..e0f2af6c1a 100644 --- a/packages/babel-types/src/modifications/removeProperties.js +++ b/packages/babel-types/src/modifications/removeProperties.js @@ -20,7 +20,7 @@ export default function removeProperties( if (node[key] != null) node[key] = undefined; } - for (const key in node) { + for (const key of Object.keys(node)) { if (key[0] === "_" && node[key] != null) node[key] = undefined; } diff --git a/packages/babel-types/src/validators/isNodesEquivalent.js b/packages/babel-types/src/validators/isNodesEquivalent.js index 68672ff964..26d61a122f 100644 --- a/packages/babel-types/src/validators/isNodesEquivalent.js +++ b/packages/babel-types/src/validators/isNodesEquivalent.js @@ -25,6 +25,11 @@ export default function isNodesEquivalent(a: any, b: any): boolean { if (typeof a[field] !== typeof b[field]) { return false; } + if (a[field] == null && b[field] == null) { + continue; + } else if (a[field] == null || b[field] == null) { + return false; + } if (Array.isArray(a[field])) { if (!Array.isArray(b[field])) { @@ -46,7 +51,7 @@ export default function isNodesEquivalent(a: any, b: any): boolean { typeof a[field] === "object" && (!visitorKeys || !visitorKeys.includes(field)) ) { - for (const key in a[field]) { + for (const key of Object.keys(a[field])) { if (a[field][key] !== b[field][key]) { return false; }