WIP 1

TODO.md

@@ -0,0 +1,31 @@
+# TODO
+
+* Add functions to `reservedVarNames` in instrumentation, instead of in `parseFunction`
+* 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

@@ -137,7 +137,7 @@ function createBindingWithoutNameCheck(block, varName, props) {
 		isVar: !!props.isVar,
 		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 frozen
 	};
 }
 
@@ -147,8 +147,13 @@ function createBindingWithoutNameCheck(block, varName, props) {
  * @returns {Object} - Binding object
  */
 function createThisBinding(block) {
-	// eslint-disable-next-line no-use-before-define
-	return createBindingWithoutNameCheck(block, 'this', {varNode: thisNode, isConst: true});
+	// `isFrozenName: true` because it cannot be renamed within function in which it's created.
+	// If `this` has to be substituted for a temp var for transpiled `super()`,
+	// `isFrozenName` will be set to false.
+	return createBindingWithoutNameCheck(
+		// eslint-disable-next-line no-use-before-define
+		block, 'this', {varNode: thisNode, isConst: true, isFrozenName: true}
+	);
 }
 
 const thisNode = t.thisExpression();
@@ -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});
 }
 
 /**
@@ -170,9 +176,12 @@ function createArgumentsBinding(block, isConst, argNames) {
  * @returns {Object} - Binding object
  */
 function createNewTargetBinding(block) {
+	// `isFrozenName: true` because it cannot be renamed within function in which it's created
 	/* eslint-disable no-use-before-define */
 	newTargetNode ||= t.metaProperty(t.identifier('new'), t.identifier('target'));
-	return createBindingWithoutNameCheck(block, 'new.target', {varNode: newTargetNode, isConst: true});
+	return createBindingWithoutNameCheck(
+		block, 'new.target', {varNode: newTargetNode, isConst: true, isFrozenName: true}
+	);
 	/* eslint-enable no-use-before-define */
 }
 
@@ -201,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,7 +178,24 @@ 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.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
@@ -442,7 +443,6 @@ function createFunction(id, node, isStrict, state) {
 		hasSuperClass: false,
 		firstSuperStatementIndex: undefined,
 		returnsSuper: false,
-		hasThisBindingForSuper: false,
 		firstComplexParamIndex: undefined
 	};
 
@@ -722,15 +722,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 = new Set();
 	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
@@ -745,6 +759,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
 				};
@@ -756,10 +771,9 @@ 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()
-			: undefined,
+		amendments,
 		hasSuperClass: fn.hasSuperClass || undefined,
 		firstSuperStatementIndex: fn.firstSuperStatementIndex,
 		returnsSuper: fn.returnsSuper || undefined,

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.isFrozenName && !binding.argNames) binding.trails.push(trail);
+		if (!binding.isFrozenName) binding.trails.push(trail);
 		return;
 	}
 
@@ -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

@@ -132,10 +132,13 @@ function activateSuperBinding(superBlock, state) {
 	activateBlock(superBlock, state);
 	let binding = superBlock.bindings.super;
 	if (!binding) {
-		// NB: No need to add this binding to function, as `super` is always an external var
+		// No need to add this binding to function, as `super` is always an external var.
+		// `isFrozenName: true` because it shouldn't be added to internal vars if a function
+		// containing the class/object which creates `super` is serialized.
 		binding = createBindingWithoutNameCheck(superBlock, 'super', {
 			varNode: createBlockTempVar(superBlock, state),
-			isConst: true
+			isConst: true,
+			isFrozenName: true
 		});
 	}
 	return binding;
@@ -148,9 +151,10 @@ function activateSuperBinding(superBlock, state) {
  * @returns {undefined}
  */
 function createInternalVarForThis(fn, state) {
-	if (!fn.hasThisBindingForSuper) {
-		fn.bindings.push(state.currentThisBlock.bindings.this);
-		fn.hasThisBindingForSuper = true;
+	const binding = state.currentThisBlock.bindings.this;
+	if (binding.isFrozenName) {
+		binding.isFrozenName = false;
+		fn.bindings.push(binding);
 	}
 }
 
@@ -203,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
 	});
 }