with statements WIP 1

README.md

@@ -573,8 +573,6 @@ NB Applications can *use* any of these within functions, just that instances of
 * Unsupported: `export default Promise.resolve();` (Promise instance serialized directly)
 * Unsupported: `const p = Promise.resolve(); export default function f() { return p; };` (Promise instance in outer scope of exported function)
 
-`with (...) {...}` is also not supported where it alters the scope of a function being serialized.
-
 ### Browser code
 
 This works in part. You can, for example, build a simple React app with Livepack.

lib/init/eval.js

@@ -119,7 +119,8 @@ function evalDirect(args, filename, blockIdCounter, externalPrefixNum, evalDirec
 		currentBlock: undefined,
 		currentThisBlock: undefined,
 		currentSuperBlock: undefined,
-		currentSuperIsProto: false
+		currentSuperIsProto: false,
+		currentWithBlock: undefined
 	};
 	const tempVars = [];
 	let allowNewTarget = false;
@@ -147,6 +148,11 @@ function evalDirect(args, filename, blockIdCounter, externalPrefixNum, evalDirec
 				createBindingWithoutNameCheck(
 					block, varName, {isConst: !!isConst, isSilentConst: !!isSilentConst, argNames}
 				);
+
+				if (varName === 'with') {
+					state.currentWithBlock ||= block;
+					tempVars.push({value: tempVarValue, block, varName});
+				}
 			}
 		}
 	}

lib/init/tracker.js

@@ -11,7 +11,8 @@ module.exports = createTracker;
 
 // Imports
 const addEvalFunctionsToTracker = require('./eval.js'),
-	{tracker} = require('../shared/tracker.js');
+	{tracker, getIsGettingScope} = require('../shared/tracker.js'),
+	{INTERNAL_VAR_NAMES_PREFIX} = require('../shared/constants.js');
 
 // Exports
 
@@ -25,5 +26,53 @@ const addEvalFunctionsToTracker = require('./eval.js'),
 function createTracker(filename, blockIdCounter, prefixNum) {
 	const localTracker = (getFnInfo, getScopes) => tracker(getFnInfo, getScopes);
 	addEvalFunctionsToTracker(localTracker, filename, blockIdCounter, prefixNum);
+	addWrapWithFunctionToTracker(localTracker, prefixNum);
 	return localTracker;
 }
+
+function addWrapWithFunctionToTracker(localTracker, prefixNum) {
+	// `.wrapWith` wraps an object used as object of a `with ()` statement.
+	//
+	// Filter out any accesses to Livepack's internal vars, to prevent the `with` object
+	// obscuring access to them.
+	//
+	// There can be no valid accesses to Livepack's internal vars, otherwise they'd have
+	// been renamed to avoid clashes with any existing vars.
+	// Only exception is in `eval()`, where var names cannot be predicted in advance
+	// e.g. `with ({livepack_tracker: 1}) { eval('livepack_tracker = 2'); }`.
+	// This should set the property on the `with` object.
+	// However, this is a pre-existing problem with `eval()`, and can manifest without `with`,
+	// so not going to try to solve it here either.
+	// TODO: Try to solve this.
+	//
+	// Also always returns false from `has` trap if getting scope vars for a function.
+	// This renders the `with` object transparent, so tracker can get values of variables
+	// outside of the `with () {}` block. e.g. `let f, x = 123; with ({x: 1}) { f = () => x; }`
+	// Tracker in `f` needs to be able to get the value of `x` in outer scope.
+	//
+	// Proxy an empty object, and forward operations to the `with` object,
+	// rather than proxying the `with` object directly, to avoid breaking Proxy `has` trap's invariant
+	// that cannot report a property as non-existent if it's non-configurable.
+	// e.g. `with (Object.freeze({livepack_tracker: 123})) { ... }`
+	const internalVarsPrefix = `${INTERNAL_VAR_NAMES_PREFIX}${prefixNum || ''}_`;
+	localTracker.wrapWith = withObj => new Proxy(Object.create(null), {
+		// NB: These are the only traps which can be triggered
+		has(target, key) {
+			// Act as if object has no properties if currently getting scope vars for a function
+			if (getIsGettingScope()) return false;
+			// Act as if properties named like Livepack's internal vars don't exist
+			if (key.startsWith(internalVarsPrefix)) return false;
+			// Forward to `with` object
+			return Reflect.has(withObj, key);
+		},
+		get(target, key) {
+			return Reflect.get(withObj, key, withObj);
+		},
+		set(target, key, value) {
+			return Reflect.set(withObj, key, value, withObj);
+		},
+		deleteProperty(target, key) {
+			return Reflect.deleteProperty(withObj, key);
+		}
+	});
+}

lib/instrument/modify.js

@@ -74,6 +74,7 @@ function modifyAst(ast, filename, isCommonJs, isStrict, sources, evalState) {
 		currentThisBlock: undefined,
 		currentSuperBlock: undefined,
 		currentHoistBlock: undefined,
+		currentWithBlock: undefined,
 		fileBlock: undefined,
 		programBlock: undefined,
 		currentFunction: undefined,

lib/instrument/visitors/eval.js

@@ -16,6 +16,7 @@ const {
 		activateSuperBinding, getParentFullFunction, createInternalVarForThis, createExternalVarForThis,
 		setSuperIsProtoOnFunctions
 	} = require('./super.js'),
+	{activateWithBinding} = require('./with.js'),
 	{getOrCreateExternalVar, activateBlock, activateBinding} = require('../blocks.js'),
 	{createTempVarNode} = require('../internalVars.js'),
 	{copyLocAndComments} = require('../utils.js'),
@@ -138,35 +139,40 @@ 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;
-			}
+			if (varName === 'with') {
+				// Variables accessed from within `eval()` may pass through this with block
+				activateWithBinding(block, state);
+			} else {
+				// 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;
+				// Skip var if it's shadowed by var with same name in higher scope
+				if (varNamesUsed.has(varName)) continue;
 
-			// If `eval()` is strict mode, skip vars which are illegal to use in strict mode (e.g. `package`)
-			// as `eval()` won't be able to access them
-			if (isStrict && varName !== 'this' && varName !== 'super' && isReservedWord(varName)) continue;
+				// If `eval()` is strict mode, skip vars which are illegal to use in strict mode (e.g. `package`)
+				// as `eval()` won't be able to access them
+				if (isStrict && varName !== 'this' && varName !== 'super' && isReservedWord(varName)) continue;
 
-			// Ignore `require` as it would render the function containing `eval()` un-serializable.
-			// Also ignore CommonJS wrapper function's `arguments` as that contains `require` too.
-			if ((varName === 'require' || varName === 'arguments') && block === state.fileBlock) continue;
+				// Ignore `require` as it would render the function containing `eval()` un-serializable.
+				// Also ignore CommonJS wrapper function's `arguments` as that contains `require` too.
+				if ((varName === 'require' || varName === 'arguments') && block === state.fileBlock) continue;
 
-			// Ignore `super` if it's not accessible
-			if (varName === 'super' && !canUseSuper) continue;
+				// Ignore `super` if it's not accessible
+				if (varName === 'super' && !canUseSuper) continue;
 
-			varNamesUsed.add(varName);
+				varNamesUsed.add(varName);
+			}
 
 			// Create array for var `[varName, isConst, isSilentConst, argNames, tempVarValue]`.
 			// NB: Assignment to var called `arguments` or `eval` is illegal in strict mode.
@@ -179,7 +185,7 @@ function instrumentEvalCall(callNode, block, fn, isStrict, canUseSuper, superIsP
 				while (varDefNodes.length !== 3) varDefNodes.push(null);
 				varDefNodes.push(t.arrayExpression(binding.argNames.map(argName => t.stringLiteral(argName))));
 			}
-			if (varName === 'super') {
+			if (varName === 'super' || varName === 'with') {
 				while (varDefNodes.length !== 4) varDefNodes.push(null);
 				varDefNodes.push(binding.varNode);
 			}

lib/instrument/visitors/identifier.js

@@ -16,6 +16,7 @@ module.exports = {
 
 // Imports
 const visitEval = require('./eval.js'),
+	{activateWithBinding} = require('./with.js'),
 	{getOrCreateExternalVar, activateBlock, activateBinding} = require('../blocks.js'),
 	{checkInternalVarNameClash} = require('../internalVars.js'),
 	{
@@ -80,7 +81,18 @@ function ThisExpression(node, state) {
 	// Ignore if internal to function, unless in class constructor or prototype class property
 	// of class with super class (where `this` will be substituted when `super()` is transpiled).
 	if (block.id < fn.id) {
-		recordExternalVar(binding, block, 'this', fn, [...state.trail], true, false, state);
+		// Check if a `with` binding in between, because if so, `this` needs to be frozen, to avoid whatever
+		// var is used to stand in for `this` being blocked by a property of the `with` object.
+		// No need to activate the `with` binding, as `this` doesn't read from it.
+		// Actually, `this` only needs to be frozen if the `with` block ends up in the output's blocks tree.
+		// e.g. `with (o) return () => this` does not require `this` to be frozen.
+		// But this can't be determined until serialization time, as it could be *another* function
+		// in same block which causes the `with` block to be used.
+		// To avoid complicating matters for this edge case, don't worry about it, and always freeze `this`.
+		// `this` may be frozen unnecessarily, which produces longer output, but it won't be incorrect.
+		const isBehindWith = !!state.currentWithBlock && state.currentWithBlock.id > block.id;
+
+		recordExternalVar(binding, block, 'this', fn, [...state.trail], true, false, isBehindWith, state);
 	} else if (fn.hasSuperClass) {
 		binding.trails.push([...state.trail]);
 	}
@@ -103,8 +115,19 @@ function NewTargetExpression(node, state) {
 	// parser will have already rejected the code as syntax error.
 	const block = state.currentThisBlock;
 	if (block.id < fn.id) {
+		// Check if a `with` binding in between, because if so, `new.target` needs to be frozen,
+		// to avoid whatever var is used to stand in for `new.target` being blocked by a property
+		// of the `with` object.
+		// At present, `new.target` won't actually be frozen, but setting `isFrozenName` will at least
+		// cause var name to be prefixed with "_".
+		// No need to activate the `with` binding, as `new.target` doesn't read from it.
+		// Same as with `this`, `new.target` only actually needs to be frozen if the `with` block ends
+		// up in the output's blocks tree. But to simplify matters, just freeze it anyway.
+		const isBehindWith = !!state.currentWithBlock && state.currentWithBlock.id > block.id;
+
 		recordExternalVar(
-			block.bindings['new.target'], block, 'new.target', fn, [...state.trail], true, false, state
+			block.bindings['new.target'], block, 'new.target', fn, [...state.trail],
+			true, false, isBehindWith, state
 		);
 	}
 }
@@ -126,7 +149,7 @@ function visitIdentifier(node, varName, isReadFrom, isAssignedTo, state) {
 		// Resolve the binding var refers to in 2nd pass
 		state.secondPass(
 			resolveIdentifier,
-			node, state.currentBlock, varName, fn, [...state.trail],
+			node, state.currentBlock, state.currentWithBlock, varName, fn, [...state.trail],
 			isReadFrom, isAssignedTo, state.isStrict, state
 		);
 	} else {
@@ -140,6 +163,7 @@ function visitIdentifier(node, varName, isReadFrom, isAssignedTo, state) {
  * Resolve binding an identifier refers to and record its usage on function it's used within.
  * @param {Object} node - Identifier AST node (not needed but passed for debug reasons)
  * @param {Object} block - Block object
+ * @param {Object} [withBlock] - Block object for first `with ()` block above identifier
  * @param {string} varName - Variable name
  * @param {Object} fn - Function object
  * @param {Array<string|number>} trail - Trail
@@ -149,13 +173,27 @@ function visitIdentifier(node, varName, isReadFrom, isAssignedTo, state) {
  * @param {Object} state - State object
  * @returns {undefined}
  */
-function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssignedTo, isStrict, state) {
+function resolveIdentifier(
+	node, block, withBlock, varName, fn, trail, isReadFrom, isAssignedTo, isStrict, state
+) {
 	// Find binding
 	let binding;
 	do {
 		binding = block.bindings[varName];
 	} while (!binding && (block = block.parent)); // eslint-disable-line no-cond-assign
 
+	// If `with ()` block between identifier and its binding, the `with ()` could block access
+	// to the binding. Create external vars for `with` blocks in function, and flag the `binding`
+	// as frozen name, so var isn't renamed in output.
+	const isBehindWith = !!withBlock && (!block || withBlock.id > block.id);
+	if (isBehindWith) {
+		if (binding) {
+			binding.isFrozenName = true;
+			binding.trails.length = 0;
+		}
+		createExternalVarsForWiths(withBlock, block, fn, state);
+	}
+
 	// Record if global var
 	if (!binding) {
 		// Vars which are local bindings have var name checked for internal var name clashes
@@ -198,7 +236,34 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign
 		isAssignedTo = false;
 	}
 
-	recordExternalVar(binding, block, varName, fn, trail, isReadFrom, isAssignedTo, state);
+	recordExternalVar(binding, block, varName, fn, trail, isReadFrom, isAssignedTo, isBehindWith, state);
+}
+
+/**
+ * Find all `with` blocks above identifier's binding.
+ * Activate those `with` blocks and create external vars for them in function.
+ * @param {Object} withBlock - Block object for `with` block
+ * @param {Object} [bindingBlock] - Block object for variable binding (which will be above `with` block)
+ *   or `undefined` if no binding (global var)
+ * @param {Object} fn - Function object
+ * @param {Object} state - State object
+ * @returns {undefined}
+ */
+function createExternalVarsForWiths(withBlock, bindingBlock, fn, state) {
+	while (true) { // eslint-disable-line no-constant-condition
+		if (withBlock.id < fn.id) {
+			// `with ()` is external to function - create external var
+			const binding = activateWithBinding(withBlock, state);
+			const externalVar = getOrCreateExternalVar(fn, withBlock, 'with', binding);
+			externalVar.isReadFrom = true;
+		}
+
+		// Find next `with` block above binding (if any)
+		do {
+			withBlock = withBlock.parent;
+			if (withBlock === bindingBlock) return;
+		} while (!withBlock.bindings.with);
+	}
 }
 
 /**
@@ -210,15 +275,19 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign
  * @param {Array<string|number>} trail - Trail
  * @param {boolean} isReadFrom - `true` if variable is read from
  * @param {boolean} isAssignedTo - `true` if variable is assigned to
+ * @param {boolean} isBehindWith - `true` if variable may be shadowed by a `with () {}` statement
  * @param {Object} state - State object
  * @returns {undefined}
  */
-function recordExternalVar(binding, block, varName, fn, trail, isReadFrom, isAssignedTo, state) {
+function recordExternalVar(
+	binding, block, varName, fn, trail, isReadFrom, isAssignedTo, isBehindWith, state
+) {
 	activateBlock(block, state);
 	activateBinding(binding, varName);
 	const externalVar = getOrCreateExternalVar(fn, block, varName, binding);
 	if (isReadFrom) externalVar.isReadFrom = true;
 	if (isAssignedTo) externalVar.isAssignedTo = true;
+	if (isBehindWith) externalVar.isFrozenName = true;
 	// `new.target` isn't actually frozen so needs trail recorded
 	if (!externalVar.isFrozenName || varName === 'new.target') externalVar.trails.push(trail);
 }

lib/instrument/visitors/statement.js

@@ -19,6 +19,7 @@ const VariableDeclaration = require('./variableDeclaration.js'),
 	SwitchStatement = require('./switch.js'),
 	TryStatement = require('./try.js'),
 	ThrowStatement = require('./unary.js'),
+	{WithStatement} = require('./with.js'),
 	{visitKey, visitKeyMaybe} = require('../visit.js');
 
 // Exports
@@ -71,13 +72,6 @@ function ReturnStatement(node, state) {
 	visitKeyMaybe(node, 'argument', Expression, state);
 }
 
-function WithStatement(node, state) {
-	// TODO: Maintain a state property `currentWithBlock` which can be used in `resolveBinding()`
-	// to flag functions which access a var which would be affected by `with`
-	visitKey(node, 'object', Expression, state);
-	visitKey(node, 'body', Statement, state);
-}
-
 function LabeledStatement(node, state) {
 	visitKey(node, 'body', Statement, state);
 }