From 2b763fc4526e531a9d3856ff8356ac78101240bd Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 13 Nov 2013 14:53:21 -0500 Subject: [PATCH 1/2] Rewrite vendor/constants.js to use require("ast-types").traverse. Most notably, this new style of transformation gives us access to this.parent.node, which allows us to avoid replacing identifiers that are not actually free variables, such as member expression properties. Closes #496. --- package.json | 2 +- vendor/constants.js | 80 +++++++++++++++++++++++---------------------- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/package.json b/package.json index 670c608cb02..bb21489676b 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "grunt-contrib-jshint": "~0.6.0", "optimist": "~0.6.0", "phantomjs": "~1.9", - "recast": "~0.4.16", + "recast": "~0.4.24", "semver": "~2.1.0", "uglify-js": "~2.4.0", "grunt-contrib-clean": "~0.5.0", diff --git a/vendor/constants.js b/vendor/constants.js index 935b0561c60..ef9b0f3e3b2 100644 --- a/vendor/constants.js +++ b/vendor/constants.js @@ -16,58 +16,60 @@ 'use strict'; var recast = require('recast'); +var types = recast.types; +var namedTypes = types.namedTypes; +var builders = types.builders; +var hasOwn = Object.prototype.hasOwnProperty; -exports.propagate = function(constants, source) { - var ast = recast.parse(source); - ast = new ConstantVisitor(constants).visit(ast); - return recast.print(ast); -}; +function propagate(constants, source) { + return recast.print(transform(recast.parse(source), constants)); +} -var ConstantVisitor = recast.Visitor.extend({ - init: function(constants) { - this.constants = constants || {}; - }, +function transform(ast, constants) { + constants = constants || {}; - visitIdentifier: function(ident) { - if (this.constants.hasOwnProperty(ident.name)) { - return recast.builder.literal(this.constants[ident.name]); - } - }, + return types.traverse(ast, function(node, traverse) { + if (namedTypes.Identifier.check(node)) { + if (namedTypes.MemberExpression.check(this.parent.node) && + this.name === 'property' && + !this.parent.node.computed) { + return false; + } - visitCallExpression: function(call) { - if (!this.constants.__DEV__) { - if (call.callee.type === 'Identifier' && call.callee.name === 'invariant') { - call.arguments.length = 1; + if (hasOwn.call(constants, node.name)) { + this.replace(builders.literal(constants[node.name])); + return false; } - } - this.genericVisit(call); - }, - visitIfStatement: function(stmt) { - // Replaces all identifiers in this.constants with literal values. - this.genericVisit(stmt); + } else if (namedTypes.CallExpression.check(node)) { + if (!constants.__DEV__) { + if (namedTypes.Identifier.check(node.callee) && + node.callee.name === 'invariant') { + node.arguments.length = 1; + } + } - if (stmt.test.type === recast.Syntax.Literal) { - if (stmt.test.value) { - stmt.alternate = null; - } else if (stmt.alternate) { - return stmt.alternate; + } else if (namedTypes.IfStatement.check(node) && + namedTypes.Literal.check(node.test)) { + if (node.test.value) { + node.alternate = null; + } else if (node.alternate) { + this.replace(node.alternate); + return false; } else { - // In case this if statement is an alternate clause for another - // if-statement, replacing that alternate with null will have the - // effect of pruning the unnecessary clause. If this is just a - // free-floating if statement, replacing it with null will have - // the effect of removing it from the enclosing list of - // statements. - return null; + this.replace(); // Remove the if-statement. + return false; } } - } -}); + }); +} if (!module.parent) { var constants = JSON.parse(process.argv[3]); recast.run(function(ast, callback) { - callback(new ConstantVisitor(constants).visit(ast)); + callback(transform(ast, constants)); }); } + +exports.propagate = propagate; +exports.transform = transform; From da717977edc3120ddb5c88766f8564fa09eb7bb0 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 13 Nov 2013 15:28:20 -0500 Subject: [PATCH 2/2] Better comments for vendor/constants.js. --- vendor/constants.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/vendor/constants.js b/vendor/constants.js index ef9b0f3e3b2..3c2b404d21d 100644 --- a/vendor/constants.js +++ b/vendor/constants.js @@ -30,12 +30,17 @@ function transform(ast, constants) { return types.traverse(ast, function(node, traverse) { if (namedTypes.Identifier.check(node)) { + // If the identifier is the property of a member expression + // (e.g. object.property), then it definitely is not a constant + // expression that we want to replace. if (namedTypes.MemberExpression.check(this.parent.node) && this.name === 'property' && !this.parent.node.computed) { return false; } + // There could in principle be a constant called "hasOwnProperty", + // so be careful always to use Object.prototype.hasOwnProperty. if (hasOwn.call(constants, node.name)) { this.replace(builders.literal(constants[node.name])); return false; @@ -45,6 +50,8 @@ function transform(ast, constants) { if (!constants.__DEV__) { if (namedTypes.Identifier.check(node.callee) && node.callee.name === 'invariant') { + // Truncate the arguments of invariant(condition, ...) + // statements to just the condition. node.arguments.length = 1; } } @@ -52,12 +59,21 @@ function transform(ast, constants) { } else if (namedTypes.IfStatement.check(node) && namedTypes.Literal.check(node.test)) { if (node.test.value) { - node.alternate = null; + // If the alternate (then) branch is dead code, remove it. + this.get("alternate").replace(); + + // This is what happens when you replace a node with nothing and + // it can't be removed from a list of statements. + assert.strictEqual(node.alternate, null); + } else if (node.alternate) { + // Replace the whole if-statement with just the alternate clause. this.replace(node.alternate); return false; + } else { - this.replace(); // Remove the if-statement. + // Remove the entire if-statement. + this.replace(); return false; } }