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/init/eval.js

@@ -141,8 +141,8 @@ function evalDirect(args, filename, blockIdCounter, externalPrefixNum, evalDirec
 				state.currentSuperIsProto = superIsProto;
 				tempVars.push({value: tempVarValue, block, varName});
 			} 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
+				// Whether var is frozen is not relevant because it will always be in external scope
+				// of the functions it's being recorded on, and value of `isFrozenName` only has any effect
 				// for internal vars
 				createBindingWithoutNameCheck(
 					block, varName, {isConst: !!isConst, isSilentConst: !!isSilentConst, argNames}

lib/instrument/blocks.js

@@ -99,8 +99,8 @@ function createBlockId(state) {
  * @param {boolean} [props.isSilentConst=false] - `true` if is function expression name,
  *   assignment to which fails silently in sloppy mode
  * @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
@@ -123,8 +123,8 @@ function createBinding(block, varName, props, state) {
  * @param {boolean} [props.isSilentConst=false] - `true` if is function expression name,
  *   assignment to which fails silently in sloppy mode
  * @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
  */
@@ -135,9 +135,9 @@ function createBindingWithoutNameCheck(block, varName, props) {
 		isConst: !!props.isConst,
 		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
 	};
 }
 
@@ -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,9 +178,26 @@ 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.isFrozenName) {
 		return;
 	}
 

lib/instrument/visitors/class.js

@@ -49,7 +49,7 @@ function ClassDeclaration(node, state, parent, key) {
 	assertNoCommonJsVarsClash(block, className, state);
 	// No need to check for internal var name clash here as will be checked when creating
 	// the binding for class name accessed from inside class
-	createBindingWithoutNameCheck(block, className, {isFunction: true});
+	createBindingWithoutNameCheck(block, className, {isFrozenName: true});
 
 	// Visit class
 	visitClass(node, parent, key, className, state);

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
@@ -85,7 +86,7 @@ function FunctionDeclaration(node, state, parent, key) {
 	let binding = block.bindings[fnName];
 	if (!binding) {
 		const isTopLevel = block === hoistBlock;
-		binding = createBinding(block, fnName, {isVar: isTopLevel, isFunction: true}, state);
+		binding = createBinding(block, fnName, {isVar: isTopLevel, isFrozenName: true}, state);
 
 		// If is a sloppy-mode function declaration which may need to be hoisted, record that.
 		// Determine whether to hoist after 1st pass is complete, once all other bindings created.
@@ -98,10 +99,10 @@ function FunctionDeclaration(node, state, parent, key) {
 				parentFn: state.currentFunction
 			});
 		}
-	} else if (!binding.isFunction) {
+	} else if (!binding.isFrozenName) {
 		// Existing binding is a `var` declaration.
-		// Flag binding as a function so `var` declarations' identifiers don't get renamed.
-		binding.isFunction = true;
+		// Flag binding as frozen so `var` declarations' identifiers don't get renamed.
+		binding.isFrozenName = true;
 		binding.trails.length = 0;
 	}
 
@@ -152,7 +153,7 @@ function FunctionExpression(node, state, parent, key) {
  */
 function createAndEnterFunctionOrClassNameBlock(fnName, isSilentConst, state) {
 	const block = createAndEnterBlock(fnName, false, state);
-	createBinding(block, fnName, {isConst: true, isSilentConst, isFunction: true}, state);
+	createBinding(block, fnName, {isConst: true, isSilentConst, isFrozenName: true}, state);
 	return block;
 }
 
@@ -575,10 +576,10 @@ function hoistSloppyFunctionDeclaration(declaration) {
 	// Can be hoisted
 	if (!hoistBlockBinding) {
 		hoistBlock.bindings[varName] = {...declaration.binding, isVar: true};
-	} else if (!hoistBlockBinding.isFunction) {
+	} else if (!hoistBlockBinding.isFrozenName) {
 		// Existing binding is a `var` declaration.
-		// Flag binding as a function so `var` declarations' identifiers don't get renamed.
-		hoistBlockBinding.isFunction = true;
+		// Flag binding as frozen so `var` declarations' identifiers don't get renamed.
+		hoistBlockBinding.isFrozenName = true;
 		hoistBlockBinding.trails.length = 0;
 	}
 }
@@ -722,15 +723,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);
-	for (const {name: varName, trails, isFunction} 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 (isFunction) continue;
-		const internalVar = internalVars[varName] || (internalVars[varName] = []);
-		internalVar.push(...trails);
+	// Compile internal vars and reserved var names
+	const internalVars = Object.create(null),
+		reservedVarNames = new Set();
+	for (const {name: varName, trails, isFrozenName} of fn.bindings) {
+		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,7 +760,8 @@ function createFunctionInfoFunction(fn, state) {
 				return {
 					isReadFrom: varProps.isReadFrom || undefined,
 					isAssignedTo: varProps.isAssignedTo || undefined,
-					isFrozenInternalName: varProps.binding.isFunction || undefined,
+					isFrozenName: varProps.isFrozenName || undefined,
+					isFrozenInternalName: varProps.binding.isFrozenName || undefined,
 					trails: varProps.trails
 				};
 			})
@@ -756,10 +772,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

@@ -19,7 +19,7 @@ const visitEval = require('./eval.js'),
 	{getOrCreateExternalVar, activateBlock, activateBinding} = require('../blocks.js'),
 	{checkInternalVarNameClash} = require('../internalVars.js'),
 	{
-		CONST_VIOLATION_CONST, CONST_VIOLATION_FUNCTION_THROWING, CONST_VIOLATION_FUNCTION_SILENT
+		CONST_VIOLATION_NEEDS_VAR, CONST_VIOLATION_NEEDS_NO_VAR, CONST_VIOLATION_SILENT
 	} = require('../../shared/constants.js');
 
 // Exports
@@ -167,21 +167,31 @@ 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.trails.push(trail);
 		return;
 	}
 
 	// Record external var
 	if (isAssignedTo && binding.isConst) {
-		// Record const violation
+		// Record const violation.
+		// `CONST_VIOLATION_NEEDS_VAR` violation type is for if this const violation is output
+		// in a parent function where the const binding is internal to the function,
+		// it will need to be added to `internalVars`.
+		// 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 possible to know at this point whether binding is frozen or not (an `eval()` might be 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_FUNCTION_SILENT
-				: binding.isFunction
-					? CONST_VIOLATION_FUNCTION_THROWING
-					: CONST_VIOLATION_CONST,
+				? CONST_VIOLATION_SILENT
+				: isReadFrom || binding.isFrozenName
+					? CONST_VIOLATION_NEEDS_NO_VAR
+					: CONST_VIOLATION_NEEDS_VAR,
 			blockId: block.id,
-			trail
+			trail,
+			binding
 		});
 
 		if (!isReadFrom) return;
@@ -209,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);
 }