WIP 1

TODO.md

@@ -0,0 +1,30 @@
+# TODO
+
+* Tests
+* Deal with duplicate bindings:
+  * Hoisted sloppy functions - actually no need, because `isFunction` means both are frozen anyway
+  * `for (let x of (() => x, [])) eval('x');` - done, but write test
+  * `function (x, _ = eval('x')) { var x; return x; }` - done (apart from `arguments`), but write test
+* Tests for binding in higher scope which is shadowed still not being renamed
+  e.g. `() => { let x = 1; { let x; eval('typeof a !== "undefined" && a = 2'); } return x; }` - make sure param `x` not renamed to `a`
+  * Make sure this includes vars which are illegal to access inside `eval()` if `eval()` is strict mode.
+* Test for const violations with frozen vars when internal to function:
+
+```js
+module.exports = function(module, exports) {
+	const x = 1;
+	eval('x');
+	return () => { x = 2; };
+};
+```
+
+Also where another binding which isn't frozen:
+
+```js
+module.exports = function(module, exports) {
+	const x = 1;
+	eval('x');
+	{ const x = 2; }
+	return () => { x = 2; };
+};
+```

lib/instrument/blocks.js

@@ -210,6 +210,7 @@ function getOrCreateExternalVar(externalVars, block, varName, binding) {
 			binding,
 			isReadFrom: false,
 			isAssignedTo: false,
+			isFrozenName: false,
 			trails: []
 		};
 		blockVars[varName] = externalVar;

lib/instrument/visitors/assignee.js

@@ -178,8 +178,24 @@ function IdentifierVar(node, state) {
 		block = state.currentHoistBlock;
 	let binding = block.bindings[varName];
 	if (!binding) {
-		binding = createBinding(block, varName, {isVar: true}, state);
-		if (fn) fn.bindings.push(binding);
+		// If function has a param with same name, create a separate binding on the hoist block,
+		// but reuse the same binding object. This is so that if the param is frozen,
+		// this var is too, so that the var continues to inherit its initial value from the param.
+		// e.g. `function(x, y = eval('foo')) { var x; return x; }`
+		// TODO: This doesn't work for `arguments` because that binding is created after visiting
+		// function body. Could fix that by visiting function params first, then create `arguments` binding,
+		// and then visiting function body.
+		// https://github.com/overlookmotel/livepack/issues/547
+		binding = block.parent.bindings[varName];
+		if (binding) {
+			// Param binding with same name as var.
+			// Flag as `isVar` to allow function declarations to reuse same binding.
+			block.bindings[varName] = binding;
+			binding.isVar = true;
+		} else {
+			binding = createBinding(block, varName, {isVar: true}, state);
+			if (fn) fn.bindings.push(binding);
+		}
 	} else if (binding.isFrozenName) {
 		return;
 	}

lib/instrument/visitors/eval.js

@@ -115,6 +115,20 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 
 		const varDefsNodes = [];
 		for (const [varName, binding] of Object.entries(block.bindings)) {
+			// Freeze binding to prevent var being renamed when used internally in a function.
+			// All vars in scope need to be frozen, even if not accessible to `eval()`
+			// because if they're renamed, they would become accessible to `eval()` when they weren't before.
+			// `this` should not be frozen internally, as it can be replaced with a variable
+			// in a class constructor when `super()` is transpiled.
+			// For `super` and `new.target`, the var name can't actually be frozen, but flagged here
+			// as frozen anyway, to indicate that replacement var name should be prefixed with `_`.
+			// Frozen `new.target` is not currently supported.
+			// https://github.com/overlookmotel/livepack/issues/448
+			if (varName !== 'this') {
+				binding.isFrozenName = true;
+				binding.trails.length = 0;
+			}
+
 			// Skip var if it's shadowed by var with same name in higher scope
 			if (varNamesUsed.has(varName)) continue;
 
@@ -151,11 +165,20 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 			// If var is external to function, record function as using this var.
 			// Ignore `new.target` and `super` as they're not possible to recreate.
 			// https://github.com/overlookmotel/livepack/issues/448
-			if (externalVars && varName !== 'new.target' && varName !== 'super') {
-				activateBinding(binding, varName);
-				const externalVar = getOrCreateExternalVar(externalVars, block, varName, binding);
-				externalVar.isReadFrom = true;
-				if (!isConst) externalVar.isAssignedTo = true;
+			if (externalVars) {
+				if (varName === 'new.target' || varName === 'super') {
+					const externalVar = externalVars.get(block)?.[varName];
+					if (externalVar) externalVar.isFrozenName = true;
+				} else {
+					activateBinding(binding, varName);
+					const externalVar = getOrCreateExternalVar(externalVars, block, varName, binding);
+					externalVar.isReadFrom = true;
+					if (!isConst) externalVar.isAssignedTo = true;
+					if (!externalVar.isFrozenName) {
+						externalVar.isFrozenName = true;
+						externalVar.trails.length = 0;
+					}
+				}
 			}
 		}
 

lib/instrument/visitors/function.js

@@ -43,7 +43,8 @@ const Statement = require('./statement.js'),
 	{combineArraysWithDedup} = require('../../shared/functions.js'),
 	{
 		FN_TYPE_FUNCTION, FN_TYPE_ASYNC_FUNCTION, FN_TYPE_GENERATOR_FUNCTION,
-		FN_TYPE_ASYNC_GENERATOR_FUNCTION, TRACKER_COMMENT_PREFIX
+		FN_TYPE_ASYNC_GENERATOR_FUNCTION, TRACKER_COMMENT_PREFIX,
+		CONST_VIOLATION_NEEDS_VAR, CONST_VIOLATION_NEEDS_NO_VAR
 	} = require('../../shared/constants.js');
 
 // Exports
@@ -724,15 +725,29 @@ function insertTrackerComment(fnId, fnType, commentHolderNode, commentType, stat
  * @returns {Object} - AST node for function info function
  */
 function createFunctionInfoFunction(fn, state) {
-	// Compile internal vars
-	const internalVars = Object.create(null);
+	// Compile internal vars and reserved var names
+	const internalVars = Object.create(null),
+		{reservedVarNames} = fn;
 	for (const {name: varName, trails, isFrozenName} of fn.bindings) {
-		// Function name bindings are not usually added to `fn.bindings`,
-		// but can be included if binding is initially a `var` statement e.g. `var x; function x() {}`.
-		// So skip them here.
-		if (isFrozenName) continue;
-		const internalVar = internalVars[varName] || (internalVars[varName] = []);
-		internalVar.push(...trails);
+		if (isFrozenName) {
+			reservedVarNames.add(varName);
+		} else {
+			const internalVar = internalVars[varName] || (internalVars[varName] = []);
+			internalVar.push(...trails);
+		}
+	}
+
+	// Compile amendments.
+	// Reverse order so deepest are first.
+	// Remove the need for an internal var if binding has frozen name.
+	let amendments;
+	if (fn.amendments.length > 0) {
+		amendments = fn.amendments.map(({type, blockId, trail, binding}) => {
+			if (type === CONST_VIOLATION_NEEDS_VAR && binding.isFrozenName) {
+				type = CONST_VIOLATION_NEEDS_NO_VAR;
+			}
+			return [type, blockId, ...trail];
+		}).reverse();
 	}
 
 	// Create JSON function info string
@@ -747,6 +762,7 @@ function createFunctionInfoFunction(fn, state) {
 				return {
 					isReadFrom: varProps.isReadFrom || undefined,
 					isAssignedTo: varProps.isAssignedTo || undefined,
+					isFrozenName: varProps.isFrozenName || undefined,
 					isFrozenInternalName: varProps.binding.isFrozenName || undefined,
 					trails: varProps.trails
 				};
@@ -758,11 +774,9 @@ function createFunctionInfoFunction(fn, state) {
 		containsImport: fn.containsImport || undefined,
 		argNames,
 		internalVars,
-		reservedVarNames: fn.reservedVarNames.size !== 0 ? [...fn.reservedVarNames] : undefined,
+		reservedVarNames: reservedVarNames.size !== 0 ? [...reservedVarNames] : undefined,
 		globalVarNames: fn.globalVarNames.size !== 0 ? [...fn.globalVarNames] : undefined,
-		amendments: fn.amendments.length !== 0
-			? fn.amendments.map(({type, blockId, trail}) => [type, blockId, ...trail]).reverse()
-			: undefined,
+		amendments,
 		hasSuperClass: fn.hasSuperClass || undefined,
 		firstSuperStatementIndex: fn.firstSuperStatementIndex,
 		returnsSuper: fn.returnsSuper || undefined,

lib/instrument/visitors/identifier.js

@@ -180,14 +180,18 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign
 		// This is not the case if var is being read from, as an external var is created below,
 		// which will be converted to an internal var.
 		// It's also not needed if binding is frozen, as they're not treated as internal vars.
+		// Not always possible to know at this point if binding is frozen (it could be frozen
+		// by an `eval()` later in the function). So store the binding, and type will be changed
+		// to `CONST_VIOLATION_NEEDS_NO_VAR` later on if the binding is frozen.
 		fn.amendments.push({
 			type: binding.isSilentConst && !isStrict
 				? CONST_VIOLATION_SILENT
 				: isReadFrom || binding.isFrozenName
 					? CONST_VIOLATION_NEEDS_NO_VAR
 					: CONST_VIOLATION_NEEDS_VAR,
 			blockId: block.id,
-			trail
+			trail,
+			binding
 		});
 
 		if (!isReadFrom) return;
@@ -215,5 +219,5 @@ function recordExternalVar(binding, block, varName, fn, trail, isReadFrom, isAss
 	const externalVar = getOrCreateExternalVar(fn.externalVars, block, varName, binding);
 	if (isReadFrom) externalVar.isReadFrom = true;
 	if (isAssignedTo) externalVar.isAssignedTo = true;
-	externalVar.trails.push(trail);
+	if (!externalVar.isFrozenName) externalVar.trails.push(trail);
 }

lib/instrument/visitors/loop.js

@@ -22,7 +22,7 @@ const Statement = require('./statement.js'),
 	Expression = require('./expression.js'),
 	VariableDeclaration = require('./variableDeclaration.js'),
 	{AssigneeAssignOnly} = require('./assignee.js'),
-	{createBlock, createAndEnterBlock, createBindingWithoutNameCheck} = require('../blocks.js'),
+	{createBlock, createAndEnterBlock} = require('../blocks.js'),
 	{insertBlockVarsIntoBlockStatement} = require('../tracking.js'),
 	{visitKey, visitKeyMaybe, visitKeyContainer} = require('../visit.js');
 
@@ -98,15 +98,15 @@ function ForXStatement(node, state) {
 	// with all same bindings as init block. Accessing these vars from within right-hand side
 	// is temporal dead zone violation.
 	// https://github.com/overlookmotel/livepack/issues/323
+	// These bindings are in a different block, but just copy the binding objects themselves.
+	// This ensures that if any binding is frozen by `eval()`, the corresponding binding is too.
+	// e.g. `function() { let f; for (let x of (f = () => typeof x, [])) { eval('x'); } }`
+	// Without this, serializing outer function would allow `x` in `typeof x` to be mangled so it wouldn't
+	// be a TDZ violation any more.
 	state.currentBlock = parentBlock;
-	const initBindingNames = Object.keys(initBlock.bindings);
-	if (initBindingNames.length !== 0) {
-		const rightBlock = createAndEnterBlock('for', false, state),
-			fn = state.currentFunction;
-		for (const varName of initBindingNames) {
-			const binding = createBindingWithoutNameCheck(rightBlock, varName, {isConst: true});
-			if (fn) fn.bindings.push(binding);
-		}
+	if (Object.keys(initBlock.bindings).length !== 0) {
+		const rightBlock = createAndEnterBlock('for', false, state);
+		rightBlock.bindings = initBlock.bindings;
 	}
 
 	// Visit right-hand side

lib/instrument/visitors/super.js

@@ -207,7 +207,8 @@ function recordAmendmentForSuper(node, superBlock, isSuperCall, fn, trail) {
 	fn.amendments.push({
 		type: isSuperCall ? SUPER_CALL : SUPER_EXPRESSION,
 		blockId: superBlock.id,
-		trail
+		trail,
+		binding: undefined // Not used, but keeps same object shape for all amendments
 	});
 }
 

lib/serialize/blocks.js

@@ -62,9 +62,11 @@ module.exports = {
 		const {containsEval} = block,
 			paramsByName = Object.create(null);
 		let frozenNamesIsCloned = false;
-		const params = [...block.params.keys()].map((name) => {
+		const params = [...block.params].map(([name, {isFrozenName}]) => {
+			if (containsEval) isFrozenName = true;
 			const param = {
 				name,
+				isFrozenName,
 				// Identifier nodes referring to this param
 				localVarNodes: [],
 				// If param always contains another function defined in this block,
@@ -83,7 +85,7 @@ module.exports = {
 			paramsByName[name] = param;
 
 			// `super` and `new.target` are not actually frozen
-			if (containsEval && !['super', 'new.target'].includes(name)) {
+			if (isFrozenName && !['super', 'new.target'].includes(name)) {
 				if (!frozenNamesIsCloned) {
 					frozenNames = new Set(frozenNames);
 					frozenNamesIsCloned = true;
@@ -499,40 +501,27 @@ module.exports = {
 				fnReservedVarNames.add(varName);
 			}
 
-			// Add function names to reserved var names.
-			// Function names treated differently from internal vars as not renaming them,
-			// but still need to make sure other vars don't clash with function names.
+			// Add var names which are frozen internal to function to reserved names
+			// to prevent other vars clashing with them
 			for (const varName of blockFunction.reservedVarNames) {
 				reservedVarNames.add(varName);
 				fnReservedVarNames.add(varName);
 			}
 
 			// Rename internal vars.
 			// Names avoid clashing with internal and globals vars used within this function.
-			if (fnContainsEval) {
-				// Rename `this` if it's a temp var used in transpiled `super()`
-				const internalVarNames = Object.keys(internalVars),
-					thisVarNodes = internalVars.this;
-				if (thisVarNodes) {
-					const transformVarName = this.createVarNameTransform(fnReservedVarNames),
-						newName = transformVarName('this', true);
-					for (const varNode of thisVarNodes) {
+			// Function names and vars which are frozen by `eval()` aren't included in `internalVars`.
+			const transformVarName = this.createVarNameTransform(fnReservedVarNames);
+			for (const varName in internalVars) {
+				// `this` is only included in `internalVars` if it's the faux `this` created for transpiled
+				// `super()`. If function contains `eval()`, make sure it's prefixed with "_".
+				const newName = transformVarName(varName, fnContainsEval && varName === 'this');
+				if (newName !== varName) {
+					for (const varNode of internalVars[varName]) {
 						varNode.name = newName;
 					}
-					internalVarNames[internalVarNames.indexOf('this')] = newName;
-				}
-				setAddFrom(reservedVarNames, internalVarNames);
-			} else {
-				const transformVarName = this.createVarNameTransform(fnReservedVarNames);
-				for (const varName in internalVars) {
-					const newName = transformVarName(varName);
-					if (newName !== varName) {
-						for (const varNode of internalVars[varName]) {
-							varNode.name = newName;
-						}
-					}
-					reservedVarNames.add(newName);
 				}
+				reservedVarNames.add(newName);
 			}
 
 			// Record if strict or sloppy
@@ -650,7 +639,7 @@ module.exports = {
 
 			if (paramName === 'new.target') {
 				// `new.target` is always renamed as it can't be provided for `eval()` anyway
-				newName = transformVarName('newTarget', containsEval);
+				newName = transformVarName('newTarget', param.isFrozenName);
 
 				// Convert `MetaProperty` nodes into `Identifier`s with new name
 				for (const varNode of param.localVarNodes) {
@@ -662,9 +651,10 @@ module.exports = {
 
 				// Rename injection node
 				renameInjectionVarNode();
-			} else if (!containsEval || paramName === 'super') {
-				// NB: `super` var is always renamed
-				newName = transformVarName(paramName, containsEval);
+			} else if (!param.isFrozenName || paramName === 'super') {
+				// Param can be renamed.
+				// NB: `super` param is always renamed.
+				newName = transformVarName(paramName, param.isFrozenName);
 				if (newName !== paramName) {
 					// Rename all nodes
 					for (const varNode of param.localVarNodes) {
@@ -677,7 +667,7 @@ module.exports = {
 			} else {
 				// Frozen var name (potentially used in `eval()`)
 				// eslint-disable-next-line no-lonely-if
-				if (paramName === 'this' || (paramName === 'arguments' && paramsByName.this)) {
+				if (paramName === 'this' || (paramName === 'arguments' && paramsByName.this?.isFrozenName)) {
 					// `this` or `arguments` captured from function.
 					// `arguments` is only injected with a function wrapper if `this` is frozen too.
 					// Otherwise, can just make `arguments` a normal param.

lib/serialize/functions.js

@@ -150,12 +150,13 @@ module.exports = {
 				}
 
 				// Add var names to block
-				for (const [varName, {isAssignedTo}] of Object.entries(vars)) {
+				for (const [varName, {isAssignedTo, isFrozenName}] of Object.entries(vars)) {
 					const param = params.get(varName);
 					if (!param) {
-						params.set(varName, {isMutable: isAssignedTo || argNames?.has(varName)});
-					} else if (isAssignedTo) {
-						param.isMutable = true;
+						params.set(varName, {isMutable: isAssignedTo || argNames?.has(varName), isFrozenName});
+					} else {
+						if (isAssignedTo) param.isMutable = true;
+						if (isFrozenName) param.isFrozenName = true;
 					}
 				}