From 1cb7c0b2209fbf85c95b2d4fc896eee2dddfd586 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Tue, 21 Nov 2023 12:35:31 +0000 Subject: [PATCH] WIP 1 --- TODO.md | 18 +++++++ lib/init/eval.js | 2 +- lib/instrument/blocks.js | 11 ++++- lib/instrument/visitors/assignee.js | 20 +++++++- lib/instrument/visitors/eval.js | 33 +++++++++++-- lib/instrument/visitors/function.js | 25 +++++++--- lib/instrument/visitors/identifier.js | 4 +- lib/instrument/visitors/loop.js | 18 +++---- lib/serialize/blocks.js | 69 +++++++++++++-------------- lib/serialize/functions.js | 9 ++-- lib/serialize/parseFunction.js | 18 +++++-- 11 files changed, 155 insertions(+), 72 deletions(-) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 00000000..27294f58 --- /dev/null +++ b/TODO.md @@ -0,0 +1,18 @@ +# TODO + +* Handle setting `binding.isFrozenName` on `this` better. Better to set it, but just not to delete internal var trails. +* No point setting `isFrozenName` on external vars passed to serializer? (as it depends entirely on `containsEval` anyway) + * Right now, it is pointless, but in future when scope tree is built dynamically, it would allow + pulling vars which aren't accessible by `eval()` up to a higher level in the tree so they can be renamed. +* `serialize/blocks` + `serialize/functions` + `serialize/parseFunction` could have changes stripped out if so. +* TODO comment in `visitors/function` +* TODO comment in `visitors/assignee` +* TODO comment in `serialize/parseFunction` +* 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 +* 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. diff --git a/lib/init/eval.js b/lib/init/eval.js index 3e77d910..3dd35346 100644 --- a/lib/init/eval.js +++ b/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} ); diff --git a/lib/instrument/blocks.js b/lib/instrument/blocks.js index 347f0d7d..d927f825 100644 --- a/lib/instrument/blocks.js +++ b/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} [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} [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; diff --git a/lib/instrument/visitors/assignee.js b/lib/instrument/visitors/assignee.js index 2fb7c7ff..bb7e4286 100644 --- a/lib/instrument/visitors/assignee.js +++ b/lib/instrument/visitors/assignee.js @@ -178,9 +178,25 @@ 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.isFunction) { // TODO: Should `isFrozenName` be checked here too? 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 e8b16661..e68aa649 100644 --- a/lib/instrument/visitors/function.js +++ b/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() diff --git a/lib/instrument/visitors/identifier.js b/lib/instrument/visitors/identifier.js index d21b8d0e..120898e4 100644 --- a/lib/instrument/visitors/identifier.js +++ b/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); } 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/serialize/blocks.js b/lib/serialize/blocks.js index d6d219bc..278d177c 100644 --- a/lib/serialize/blocks.js +++ b/lib/serialize/blocks.js @@ -58,11 +58,15 @@ module.exports = { processBlock(block, getParentCreateScopeRecord, inheritedVars, frozenNames, isRoot) { const returnNodes = []; - // Init param objects - const paramsByName = Object.create(null); - const params = [...block.params.keys()].map((name) => { + // Init param objects and add frozen param names to `frozenNames` + const {containsEval} = block, + paramsByName = Object.create(null); + let frozenNamesIsCloned = false; + 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,14 @@ module.exports = { injectionVarNode: null }; paramsByName[name] = param; + // `super` and `new.target` are not actually frozen + if (isFrozenName && !['super', 'new.target'].includes(name)) { + if (!frozenNamesIsCloned) { + frozenNames = new Set(frozenNames); + frozenNamesIsCloned = true; + } + frozenNames.add(name); + } return param; }); @@ -93,7 +105,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,9 +354,6 @@ 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)]); - // Init vars to track strict/sloppy children const strictFns = []; let someSloppy = false; @@ -490,9 +499,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); @@ -500,30 +508,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 @@ -641,7 +637,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 +649,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 +665,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 33fe2aab..f30e38ea 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 + }; }) }); } @@ -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`).