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

@@ -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"
       }
     },
     {

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;
     }

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);
       }
     }

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;

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;
       }

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)),
           );

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 of Object.keys(path.getBindingIdentifiers())) {
-    for (const name in ids) {
       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 of Object.keys(path.getBindingIdentifiers())) {
-      for (const name in bindings) {
         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<Object> {
     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]]));
       }
 

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)) {

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)),
           );

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);
                   }
                 }

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);
       }

packages/babel-traverse/src/path/index.js

@@ -183,7 +183,7 @@ for (const type of (t.TYPES: Array<string>)) {
   };
 }
 
-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);
 

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];

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] });
     }
 

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<Object>)) {
-          const ids = t.getBindingIdentifiers(decl);
+          for (const name of Object.keys(t.getBindingIdentifiers(decl))) {
-          for (const name in ids) {
             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<Object>)) {
         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;
         }

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<string> = 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]);
   }
 }

packages/babel-types/src/builders/builder.js

@@ -28,7 +28,7 @@ export default function builder(type: string, ...args: Array<any>): Object {
     i++;
   });
 
-  for (const key in node) {
+  for (const key of Object.keys(node)) {
     validate(node, key, node[key]);
   }
 

packages/babel-types/src/converters/gatherSequenceExpressions.js

@@ -36,7 +36,7 @@ export default function gatherSequenceExpressions(
 
       for (const declar of (node.declarations: Array<any>)) {
         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]),

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);

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) {

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]);
   }
 

packages/babel-types/src/modifications/inherits.js

@@ -16,7 +16,7 @@ export default function inherits<T: Object>(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];
   }
 

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;
   }
 

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;
         }