WIP 1

TODO.md

@@ -0,0 +1,18 @@
+# TODO
+
+* Handle setting `binding.isFrozenName` on `this` better. Better to set it, but just not to delete internal var trails.
+* No point setting `isFrozenName` on external vars passed to serializer? (as it depends entirely on `containsEval` anyway)
+  * Right now, it is pointless, but in future when scope tree is built dynamically, it would allow
+    pulling vars which aren't accessible by `eval()` up to a higher level in the tree so they can be renamed.
+* `serialize/blocks` + `serialize/functions` + `serialize/parseFunction` could have changes stripped out if so.
+* TODO comment in `visitors/function`
+* TODO comment in `visitors/assignee`
+* TODO comment in `serialize/parseFunction`
+* 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
+* 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.

lib/init/eval.js

@@ -143,7 +143,7 @@ function evalDirect(args, filename, blockIdCounter, externalPrefixNum, evalDirec
 			} else {
 				// Whether var is function is not relevant because it will always be in external scope
 				// of the functions it's being recorded on, and value of `isFunction` only has any effect
-				// for internal vars
+				// for internal vars. Ditto `isFrozenName`.
 				createBindingWithoutNameCheck(
 					block, varName, {isConst: !!isConst, isSilentConst: !!isSilentConst, argNames}
 				);

lib/instrument/blocks.js

@@ -101,6 +101,8 @@ function createBlockId(state) {
  * @param {boolean} [props.isVar=false] - `true` if defined with `var` statement or function declaration
  * @param {boolean} [props.isFunction=false] - `true` if is function or class name
  *   (relevant as these canot be renamed)
+ * @param {boolean} [props.isFrozenName=false] - `true` if is var which is not renamed when used
+ *   inside function in which it's bound
  * @param {Array<string>} [props.argNames] - Linked argument names (only relevant for `arguments`)
  * @param {Object} state - State object
  * @returns {Object} - Binding object
@@ -125,6 +127,8 @@ function createBinding(block, varName, props, state) {
  * @param {boolean} [props.isVar=false] - `true` if defined with `var` statement or function declaration
  * @param {boolean} [props.isFunction=false] - `true` if is function or class name
  *   (relevant as these canot be renamed)
+ * @param {boolean} [props.isFrozenName=false] - `true` if is var which is not renamed when used
+ *   inside function in which it's bound
  * @param {Array<string>} [props.argNames] - Linked argument names (only relevant for `arguments`)
  * @returns {Object} - Binding object
  */
@@ -136,8 +140,9 @@ function createBindingWithoutNameCheck(block, varName, props) {
 		isSilentConst: !!props.isSilentConst,
 		isVar: !!props.isVar,
 		isFunction: !!props.isFunction,
+		isFrozenName: !!props.isFrozenName,
 		argNames: props.argNames,
-		trails: [] // Trails of usages within same function as binding, where var not function
+		trails: [] // Trails of usages within same function as binding, where var not function or frozen
 	};
 }
 
@@ -161,7 +166,8 @@ const thisNode = t.thisExpression();
  * @returns {Object} - Binding object
  */
 function createArgumentsBinding(block, isConst, argNames) {
-	return createBindingWithoutNameCheck(block, 'arguments', {isConst, argNames});
+	// `isFrozenName: true` because it cannot be renamed within function in which it's created
+	return createBindingWithoutNameCheck(block, 'arguments', {isConst, isFrozenName: true, argNames});
 }
 
 /**
@@ -201,6 +207,7 @@ function getOrCreateExternalVar(externalVars, block, varName, binding) {
 			binding,
 			isReadFrom: false,
 			isAssignedTo: false,
+			isFrozenName: false,
 			trails: []
 		};
 		blockVars[varName] = externalVar;

lib/instrument/visitors/assignee.js

@@ -178,9 +178,25 @@ function IdentifierVar(node, state) {
 		block = state.currentHoistBlock;
 	let binding = block.bindings[varName];
 	if (!binding) {
-		binding = createBinding(block, varName, {isVar: true}, state);
+		// 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.isFunction) {
+	} else if (binding.isFunction) { // TODO: Should `isFrozenName` be checked here too?
 		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

@@ -723,16 +723,24 @@ function insertTrackerComment(fnId, fnType, commentHolderNode, commentType, stat
  */
 function createFunctionInfoFunction(fn, state) {
 	// Compile internal vars
-	const internalVars = Object.create(null);
-	for (const {name: varName, trails, isFunction} of fn.bindings) {
+	const internalVars = Object.create(null),
+		reservedVarNames = new Set();
+	for (const {name: varName, trails, isFunction, 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.
+		// So skip them here. `parseFunction` adds them to list of reserved var names.
 		if (isFunction) 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);
+		}
 	}
 
+	// TODO: Remove from `reservedVarNames` where internal var with same name
+
 	// Create JSON function info string
 	const {children} = fn;
 	let argNames;
@@ -741,11 +749,13 @@ function createFunctionInfoFunction(fn, state) {
 			blockId: block.id,
 			blockName: block.name,
 			vars: mapValues(vars, (varProps, varName) => {
-				if (varName === 'arguments') argNames = varProps.binding.argNames;
+				const {binding} = varProps;
+				if (varName === 'arguments') argNames = binding.argNames;
 				return {
 					isReadFrom: varProps.isReadFrom || undefined,
 					isAssignedTo: varProps.isAssignedTo || undefined,
-					isFrozenInternalName: varProps.binding.isFunction || undefined,
+					isFrozenName: varProps.isFrozenName || undefined,
+					isFrozenInternalName: binding.isFrozenName || binding.isFunction || undefined,
 					trails: varProps.trails
 				};
 			})
@@ -756,6 +766,7 @@ function createFunctionInfoFunction(fn, state) {
 		containsImport: fn.containsImport || undefined,
 		argNames,
 		internalVars,
+		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()

lib/instrument/visitors/identifier.js

@@ -167,7 +167,7 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign
 
 	// Record if internal var
 	if (block.id >= fn.id) {
-		if (!binding.isFunction && !binding.argNames) binding.trails.push(trail);
+		if (!binding.isFrozenName && !binding.isFunction) binding.trails.push(trail);
 		return;
 	}
 
@@ -209,5 +209,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/serialize/blocks.js

@@ -58,11 +58,15 @@ module.exports = {
 	processBlock(block, getParentCreateScopeRecord, inheritedVars, frozenNames, isRoot) {
 		const returnNodes = [];
 
-		// Init param objects
-		const paramsByName = Object.create(null);
-		const params = [...block.params.keys()].map((name) => {
+		// Init param objects and add frozen param names to `frozenNames`
+		const {containsEval} = block,
+			paramsByName = Object.create(null);
+		let frozenNamesIsCloned = false;
+		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,
@@ -79,6 +83,14 @@ module.exports = {
 				injectionVarNode: null
 			};
 			paramsByName[name] = param;
+			// `super` and `new.target` are not actually frozen
+			if (isFrozenName && !['super', 'new.target'].includes(name)) {
+				if (!frozenNamesIsCloned) {
+					frozenNames = new Set(frozenNames);
+					frozenNamesIsCloned = true;
+				}
+				frozenNames.add(name);
+			}
 			return param;
 		});
 
@@ -93,7 +105,7 @@ module.exports = {
 		// Also count which params are most used in calls to create scope function, so params
 		// which are most often undefined (or circular, and so undefined in initial call to scope function)
 		// go last.
-		const {functions: blockFunctions, scopes: blockScopes, children: childBlocks, containsEval} = block,
+		const {functions: blockFunctions, scopes: blockScopes, children: childBlocks} = block,
 			localFunctions = new Map();
 		for (const fnDef of blockFunctions) {
 			for (const [scope, fnRecord] of fnDef.scopes) {
@@ -342,9 +354,6 @@ module.exports = {
 		const isSingular = !isRoot
 			&& returnNodeIndex + blockFunctions.length - numInternalOnlyFunctions + childBlocks.length === 1;
 
-		// If block contains `eval()`, freeze all param names
-		if (containsEval) frozenNames = new Set([...frozenNames, ...params.map(param => param.name)]);
-
 		// Init vars to track strict/sloppy children
 		const strictFns = [];
 		let someSloppy = false;
@@ -490,40 +499,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
@@ -641,7 +637,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) {
@@ -653,9 +649,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) {
@@ -668,7 +665,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.