WIP 1

TODO.md

@@ -0,0 +1,10 @@
+# TODO
+
+* Blocks in serializer code rename internal vars which aren't frozen, even in prescence of `eval()`
+* No point setting `isFrozenName` on external vars passed to serializer? (as it depends entirely on `containsEval` anyway)
+* `serialize/blocks` + `serialize/functions` + `serialize/parseFunction` could have changes stripped out if so.
+* TODO comment in `visitors/eval`
+* TODO comment in `serialize/blocks`
+* TODO comment in `serialize/functions`
+* TODO comment in `serialize/parseFunction`
+* Deal with duplicate bindings - `for (let x of x)` and `x => { var x; }`

experiment/index.js

@@ -0,0 +1,27 @@
+/* eslint-disable no-console */
+
+'use strict';
+
+const options = {
+	cache: false,
+	esm: false,
+	jsx: false,
+	minify: false,
+	mangle: true,
+	inline: true,
+	comments: true,
+	format: 'cjs',
+	strictEnv: undefined
+};
+
+const {cache, esm, jsx, ...serializeOpts} = options;
+
+require('../register.js')({cache, esm, jsx});
+
+const {serialize} = require('../index.js');
+
+let fn = require('./src/index.js');
+
+if (esm) fn = fn.default;
+
+console.log(serialize(fn, serializeOpts));

experiment/instrument.js

@@ -0,0 +1,29 @@
+/* eslint-disable no-console */
+
+'use strict';
+
+const {readFileSync} = require('fs'),
+	pathJoin = require('path').join,
+	{instrumentCodeImpl} = require('../lib/instrument/instrument.js');
+
+const filename = pathJoin(__dirname, 'src/index.js');
+const codeIn = readFileSync(filename, 'utf8');
+
+// eeslint-disable-next-line no-unused-vars
+const options = {
+	isEsm: false,
+	isCommonJs: true,
+	isJsx: false,
+	isStrict: false,
+	sourceMaps: true,
+	retainLines: false
+};
+
+const startTime = new Date();
+const codeOut = instrumentCodeImpl(
+	codeIn, filename, options.isEsm, options.isCommonJs, options.isJsx, options.isStrict,
+	options.sourceMaps, undefined, options.retainLines
+).code;
+const endTime = new Date();
+console.log(codeOut);
+console.log(`-----\nInstrumented in ${endTime - startTime} ms`);

experiment/src/index.js

@@ -0,0 +1,18 @@
+/* eslint-disable no-eval, no-unused-vars */
+
+'use strict';
+
+class S {}
+class X extends S {
+	constructor(module, exports) {
+		super();
+		eval('0');
+		this.x = X;
+	}
+}
+
+{
+	const Klass = X;
+	X = 1; // eslint-disable-line no-class-assign
+	module.exports = Klass;
+}

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

@@ -127,6 +127,17 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 
 			varNamesUsed.add(varName);
 
+			// `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;
+			}
+
 			// 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.
@@ -152,6 +163,12 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 				const externalVar = getOrCreateExternalVar(externalVars, block, varName, binding);
 				externalVar.isReadFrom = true;
 				if (!isConst) externalVar.isAssignedTo = true;
+				if (!externalVar.isFrozenName) {
+					// TODO: Should also do same for all functions above
+					// TODO: Should also do this for `super` and `new.target`
+					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/serialize/blocks.js

@@ -59,10 +59,14 @@ module.exports = {
 		const returnNodes = [];
 
 		// Init param objects
-		const paramsByName = Object.create(null);
-		const params = [...block.params.keys()].map((name) => {
+		const {containsEval} = block,
+			paramsByName = Object.create(null),
+			blockFrozenNames = [];
+		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,8 @@ module.exports = {
 				injectionVarNode: null
 			};
 			paramsByName[name] = param;
+			// `super` and `new.target` are not actually frozen
+			if (isFrozenName && !['super', 'new.target'].includes(name)) blockFrozenNames.push(name);
 			return param;
 		});
 
@@ -93,7 +99,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,8 +348,10 @@ 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)]);
+		// Flag any frozen var names.
+		// Var names are frozen because potentially accessed by `eval()`.
+		// TODO: Can this be done earlier when `blockFrozenNames` is filled?
+		if (blockFrozenNames.length > 0) frozenNames = new Set([...frozenNames, ...blockFrozenNames]);
 
 		// Init vars to track strict/sloppy children
 		const strictFns = [];
@@ -641,7 +649,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 +661,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 +677,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;
 					}
 				}
 

lib/serialize/parseFunction.js

@@ -82,7 +82,11 @@ module.exports = function parseFunction(
 			blockName,
 			vars: mapValues(vars, (varProps, varName) => {
 				externalVars[varName] = [];
-				return {isReadFrom: !!varProps.isReadFrom, isAssignedTo: !!varProps.isAssignedTo};
+				return {
+					isReadFrom: !!varProps.isReadFrom,
+					isAssignedTo: !!varProps.isAssignedTo,
+					isFrozenName: !!varProps.isFrozenName
+				};
 			})
 		});
 	}
@@ -873,19 +877,24 @@ function resolveFunctionInfo(
 		const {blockId} = scope;
 		if (blockId < fnId) {
 			// External var
-			for (const [varName, {isReadFrom, isAssignedTo, trails}] of Object.entries(scope.vars)) {
+			for (
+				const [varName, {isReadFrom, isAssignedTo, isFrozenName, trails}]
+				of Object.entries(scope.vars)
+			) {
 				if (isNestedFunction) {
 					const scopeDefVar = scopeDefs.get(blockId).vars[varName];
 					if (isReadFrom) scopeDefVar.isReadFrom = true;
 					if (isAssignedTo) scopeDefVar.isAssignedTo = true;
+					if (isFrozenName) scopeDefVar.isFrozenName = true;
 				}
 
-				externalVars[varName].push(...trailsToNodes(fnNode, trails, varName));
+				if (!isFrozenName) externalVars[varName].push(...trailsToNodes(fnNode, trails, varName));
 			}
 		} else {
 			// Var which is external to current function, but internal to function being serialized
 			for (const [varName, {isFrozenInternalName, trails}] of Object.entries(scope.vars)) {
 				if (!isFrozenInternalName) {
+					// TODO: Could remove the `?.` if flagged `super` and `new.target` as `isFrozenInternalName`
 					internalVars[varName]?.push(...trailsToNodes(fnNode, trails, varName));
 				}
 			}
@@ -897,7 +906,8 @@ function resolveFunctionInfo(
 		internalVars[varName].push(...trailsToNodes(fnNode, trails, varName));
 	}
 
-	// Get global var names
+	// Get reserved internal var names + global var names
+	if (fnInfo.reservedVarNames) setAddFrom(reservedVarNames, fnInfo.reservedVarNames);
 	if (fnInfo.globalVarNames) setAddFrom(globalVarNames, fnInfo.globalVarNames);
 
 	// Get amendments (const violations and `super`).