diff --git a/TODO.md b/TODO.md new file mode 100644 index 00000000..add460c3 --- /dev/null +++ b/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; }; +}; +``` diff --git a/lib/instrument/blocks.js b/lib/instrument/blocks.js index f44d0d50..cbe188d9 100644 --- a/lib/instrument/blocks.js +++ b/lib/instrument/blocks.js @@ -210,6 +210,7 @@ function getOrCreateExternalVar(externalVars, block, varName, binding) { binding, isReadFrom: false, isAssignedTo: false, + isFrozenName: false, trails: [] }; blockVars[varName] = externalVar; diff --git a/lib/instrument/visitors/assignee.js b/lib/instrument/visitors/assignee.js index e9ed590c..54739b54 100644 --- a/lib/instrument/visitors/assignee.js +++ b/lib/instrument/visitors/assignee.js @@ -178,8 +178,24 @@ function IdentifierVar(node, state) { block = state.currentHoistBlock; let binding = block.bindings[varName]; if (!binding) { - binding = createBinding(block, varName, {isVar: true}, state); - if (fn) fn.bindings.push(binding); + // 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; } diff --git a/lib/instrument/visitors/eval.js b/lib/instrument/visitors/eval.js index 4be0ef8e..72e62437 100644 --- a/lib/instrument/visitors/eval.js +++ b/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; + } + } } } diff --git a/lib/instrument/visitors/function.js b/lib/instrument/visitors/function.js index 23d64dcb..38c6fa5f 100644 --- a/lib/instrument/visitors/function.js +++ b/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 @@ -724,15 +725,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} = fn; 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 @@ -747,6 +762,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 }; @@ -758,11 +774,9 @@ function createFunctionInfoFunction(fn, state) { containsImport: fn.containsImport || undefined, argNames, internalVars, - reservedVarNames: fn.reservedVarNames.size !== 0 ? [...fn.reservedVarNames] : undefined, + 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, diff --git a/lib/instrument/visitors/identifier.js b/lib/instrument/visitors/identifier.js index be02abbb..41fc96e5 100644 --- a/lib/instrument/visitors/identifier.js +++ b/lib/instrument/visitors/identifier.js @@ -180,6 +180,9 @@ 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 @@ -187,7 +190,8 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign ? 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); } diff --git a/lib/instrument/visitors/loop.js b/lib/instrument/visitors/loop.js index 1eefae16..0e83ab3c 100644 --- a/lib/instrument/visitors/loop.js +++ b/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 diff --git a/lib/instrument/visitors/super.js b/lib/instrument/visitors/super.js index f8676d7e..45492234 100644 --- a/lib/instrument/visitors/super.js +++ b/lib/instrument/visitors/super.js @@ -207,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 }); } diff --git a/lib/serialize/blocks.js b/lib/serialize/blocks.js index 55e0b71a..83464dd3 100644 --- a/lib/serialize/blocks.js +++ b/lib/serialize/blocks.js @@ -62,9 +62,11 @@ module.exports = { const {containsEval} = block, paramsByName = Object.create(null); let frozenNamesIsCloned = false; - const params = [...block.params.keys()].map((name) => { + 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, @@ -83,7 +85,7 @@ module.exports = { paramsByName[name] = param; // `super` and `new.target` are not actually frozen - if (containsEval && !['super', 'new.target'].includes(name)) { + if (isFrozenName && !['super', 'new.target'].includes(name)) { if (!frozenNamesIsCloned) { frozenNames = new Set(frozenNames); frozenNamesIsCloned = true; @@ -499,9 +501,8 @@ 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); @@ -509,30 +510,18 @@ module.exports = { // 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 @@ -650,7 +639,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) { @@ -662,9 +651,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) { @@ -677,7 +667,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. diff --git a/lib/serialize/functions.js b/lib/serialize/functions.js index 99b95ba9..2593b3ef 100644 --- a/lib/serialize/functions.js +++ b/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; } } diff --git a/lib/serialize/parseFunction.js b/lib/serialize/parseFunction.js index 6acc0a50..0f0a7fcd 100644 --- a/lib/serialize/parseFunction.js +++ b/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 + }; }) }); } @@ -867,14 +871,18 @@ 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