Freeze vars in function containing eval() even if eval is local v…

…ar [fix]

lib/instrument/visitors/eval.js

@@ -47,10 +47,14 @@ function visitEval(node, parent, key, state) {
 	state.fileContainsFunctionsOrEval = true;
 
 	// Queue instrumentation on 2nd pass
-	state.secondPass(
-		instrumentEval, node, isEvalCall, parent, key, state.currentBlock, fn,
-		state.isStrict, canUseSuper, state.currentSuperIsProto, state
-	);
+	if (isEvalCall) {
+		state.secondPass(
+			instrumentEvalCall,
+			parent, state.currentBlock, fn, state.isStrict, canUseSuper, state.currentSuperIsProto, state
+		);
+	} else {
+		state.secondPass(instrumentEvalIdentifier, node, state.currentBlock, parent, key, state);
+	}
 }
 
 /**
@@ -76,34 +80,6 @@ function flagFunctionsAsContainingEval(fn) {
 	}
 }
 
-/**
- * Instrument `eval` - either direct `eval()` or e.g. `x = eval;` / `(0, eval)('...')`.
- * @param {Object} node - `eval` identifier AST node
- * @param {boolean} isEvalCall - `true` if is eval call
- * @param {Object|Array<Object>} parent - Parent AST node/container
- * @param {string|number} key - Node's key on parent AST node/container
- * @param {Object} block - Block object for block `eval` is in
- * @param {Object} [fn] - Function object for function `eval` is in (`undefined` if not in a function)
- * @param {boolean} isStrict - `true` if is strict mode
- * @param {boolean} canUseSuper - `true` if `super()` can be used in `eval()`
- * @param {boolean} superIsProto - `true` if `super` refers to class prototype
- * @param {Object} state - State object
- * @returns {undefined}
- */
-function instrumentEval(
-	node, isEvalCall, parent, key, block, fn, isStrict, canUseSuper, superIsProto, state
-) {
-	// Check `eval` is global
-	if (!isGlobalEval(block)) return;
-
-	// Handle either `eval()` call or `eval` identifier
-	if (isEvalCall) {
-		instrumentEvalCall(parent, block, fn, isStrict, canUseSuper, superIsProto, state);
-	} else {
-		instrumentEvalIdentifier(node, parent, key, state);
-	}
-}
-
 /**
  * Instrument `eval()` call.
  * Convert `eval()` to `livepack_tracker.evalDirect()`.
@@ -128,7 +104,8 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 	// Capture all vars in scopes above
 	const scopeNodes = [],
 		varNamesUsed = new Set();
-	let isExternalToFunction = false;
+	let isExternalToFunction = false,
+		evalIsLocal = false;
 	do {
 		// Check if block is external to function
 		if (!isExternalToFunction && fn && block.id < fn.id) isExternalToFunction = true;
@@ -159,6 +136,8 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 
 			varNamesUsed.add(varName);
 
+			if (varName === 'eval') evalIsLocal = true;
+
 			// Create array for var `[varName, isConst, isSilentConst, argNames, tempVarValue]`.
 			// NB: Assignment to var called `arguments` or `eval` is illegal in strict mode.
 			// This is relevant as it affects whether var is flagged as mutable or not.
@@ -200,6 +179,14 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 		}
 	} while (block = block.parent); // eslint-disable-line no-cond-assign
 
+	// If `eval` is not refering to global, don't replace `eval()`
+	// TODO: This isn't perfect, but problem is local `eval` var may a reference to global `eval`,
+	// but if so it will have been swapped for `livepack_tracker.evalIndirect`, and no way to reliably
+	// restore `eval` in local scope to native `eval` in order to call it if in strict mode.
+	// e.g. `const x = 1; (eval => { 'use strict'; eval('x'); })(eval);`
+	// https://github.com/overlookmotel/livepack/issues/485 would be a solution to this.
+	if (evalIsLocal) return;
+
 	// Replace `eval(x)` with
 	// `livepack_tracker.evalDirect(
 	//   x,
@@ -232,30 +219,24 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
  * Instrument `eval` used standalone (not as an `eval()` call).
  * Replace `eval` with `livepack_tracker.evalIndirect`.
  * @param {Object} node - `eval` identifier AST node
+ * @param {Object} block - Block object for block `eval` is in
  * @param {Object|Array} parent - Parent AST node/container
  * @param {string|number} key - Node's key on parent AST node/container
  * @param {Object} state - State object
  * @returns {undefined}
  */
-function instrumentEvalIdentifier(node, parent, key, state) {
+function instrumentEvalIdentifier(node, block, parent, key, state) {
+	// Don't alter if `eval` doesn't refer to global `eval`
+	do {
+		if (block.bindings.has('eval')) return;
+	} while (block = block.parent); // eslint-disable-line no-cond-assign
+
 	parent[key] = copyLocAndComments(
 		t.memberExpression(state.trackerVarNode, t.identifier('evalIndirect')),
 		node
 	);
 }
 
-/**
- * Determine if `eval` identifier refers to global `eval` (not var called `eval` defined in file).
- * @param {Object} block - Block object that `eval` is in.
- * @returns {boolean} - `true` if `eval` is a global var
- */
-function isGlobalEval(block) {
-	do {
-		if (block.bindings.has('eval')) return false;
-	} while (block = block.parent); // eslint-disable-line no-cond-assign
-	return true;
-}
-
 /**
  * If `super` is usable in this `eval()`, activate `super` block.
  * @param {Object} [fn] - Function object

test/eval.test.js

@@ -1695,6 +1695,45 @@ describe('eval', () => {
 				});
 			});
 
+			describe('freezes vars where `eval` is local var', () => {
+				itSerializes('external vars', {
+					in: `
+						const {eval} = global,
+							extA = 1, extB = 2;
+						module.exports = function(module, exports) {
+							return eval('[extA, extB]');
+						};
+					`,
+					out: `(0,eval)("
+						(eval,extA,extB)=>function(module,exports){return eval(\\"[extA, extB]\\")}
+					")(eval,1,2)`,
+					validate(fn) {
+						expect(fn).toBeFunction();
+						expect(fn()).toEqual([1, 2]);
+					}
+				});
+
+				itSerializes('internal vars', {
+					in: `
+						module.exports = function(module, exports) {
+							const {eval} = global,
+								intA = 1, intB = 2;
+							return eval('[intA, intB]');
+						};
+					`,
+					out: `(0,eval)("
+						(function(module,exports){
+							const{eval}=global,intA=1,intB=2;
+							return eval(\\"[intA, intB]\\")
+						})
+					")`,
+					validate(fn) {
+						expect(fn).toBeFunction();
+						expect(fn()).toEqual([1, 2]);
+					}
+				});
+			});
+
 			describe('does not freeze vars internal to function where not accessible to eval', () => {
 				describe('in nested blocks', () => {
 					itSerializes('where vars differently named from frozen vars', {