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/init/eval.js b/lib/init/eval.js index 3e77d910..7d133eec 100644 --- a/lib/init/eval.js +++ b/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} diff --git a/lib/instrument/blocks.js b/lib/instrument/blocks.js index 347f0d7d..367f3d20 100644 --- a/lib/instrument/blocks.js +++ b/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} [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} [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; diff --git a/lib/instrument/visitors/assignee.js b/lib/instrument/visitors/assignee.js index 2fb7c7ff..f6f02c3e 100644 --- a/lib/instrument/visitors/assignee.js +++ b/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; } diff --git a/lib/instrument/visitors/class.js b/lib/instrument/visitors/class.js index 7e034af2..9951728c 100644 --- a/lib/instrument/visitors/class.js +++ b/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); 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..11c344a6 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 @@ -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, diff --git a/lib/instrument/visitors/identifier.js b/lib/instrument/visitors/identifier.js index d21b8d0e..43c277f2 100644 --- a/lib/instrument/visitors/identifier.js +++ b/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); } 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 04d6bd53..16d99bbd 100644 --- a/lib/instrument/visitors/super.js +++ b/lib/instrument/visitors/super.js @@ -132,10 +132,13 @@ function activateSuperBinding(superBlock, state) { activateBlock(superBlock, state); let binding = superBlock.bindings.super; if (!binding) { - // NB: No need to add this binding to function, as `super` is always an external var + // No need to add this binding to function, as `super` is always an external var. + // `isFrozenName: true` because it shouldn't be added to internal vars if a function + // containing the class/object which creates `super` is serialized. binding = createBindingWithoutNameCheck(superBlock, 'super', { varNode: createBlockTempVar(superBlock, state), - isConst: true + isConst: true, + isFrozenName: true }); } return binding; @@ -148,10 +151,12 @@ function activateSuperBinding(superBlock, state) { * @returns {undefined} */ function createInternalVarForThis(fn, state) { - if (!fn.hasThisBindingForSuper) { - fn.bindings.push(state.currentThisBlock.bindings.this); - fn.hasThisBindingForSuper = true; - } + if (fn.hasThisBindingForSuper) return; + + const binding = state.currentThisBlock.bindings.this; + binding.isFrozenName = false; + fn.bindings.push(binding); + fn.hasThisBindingForSuper = true; } /** @@ -203,7 +208,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 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..0e638fe6 100644 --- a/lib/serialize/parseFunction.js +++ b/lib/serialize/parseFunction.js @@ -18,7 +18,7 @@ const {isJsIdentifier, isNumberKey, setAddFrom} = require('./utils.js'), getProp, getProps, setProp, traverseAll } = require('../shared/functions.js'), { - SUPER_CALL, SUPER_EXPRESSION, CONST_VIOLATION_CONST, CONST_VIOLATION_FUNCTION_SILENT + SUPER_CALL, SUPER_EXPRESSION, CONST_VIOLATION_SILENT, CONST_VIOLATION_NEEDS_VAR } = require('../shared/constants.js'), assertBug = require('../shared/assertBug.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 + }; }) }); } @@ -334,10 +338,10 @@ module.exports = function parseFunction( replaceSuperExpression( trail, trailNodes, superVarNode, thisVarNode, superIsProto, internalVars, copyLoc ); - } else if (amendmentType === CONST_VIOLATION_FUNCTION_SILENT) { + } else if (amendmentType === CONST_VIOLATION_SILENT) { replaceConstViolation(trail, trailNodes, true, internalVars, copyLoc); } else { - // amendmentType === CONST_VIOLATION_CONST || amendmentType === CONST_VIOLATION_FUNCTION_THROWING + // A non-silent const violation replaceConstViolation(trail, trailNodes, false, internalVars, copyLoc); } } @@ -873,20 +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) { - internalVars[varName]?.push(...trailsToNodes(fnNode, trails, varName)); + internalVars[varName].push(...trailsToNodes(fnNode, trails, varName)); } } } @@ -897,7 +905,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`). @@ -907,10 +916,13 @@ function resolveFunctionInfo( for (const [type, blockId, ...trail] of thisAmendments) { if (blockId < fnId) { amendments.push({type, trail, trailNodes: getProps(fnNode, trail)}); - } else if (type === CONST_VIOLATION_CONST) { + } else if (type === CONST_VIOLATION_NEEDS_VAR) { // Const violation where var is internal to function. Add to internal vars instead. - // Ignore CONST_VIOLATION_FUNCTION_THROWING and CONST_VIOLATION_FUNCTION_SILENT violation types - // because they refer to function names, which are not treated as internal vars. + // If var is frozen, no internal var is needed. In these cases, type will be + // `CONST_VIOLATION_SILENT` or `CONST_VIOLATION_NEEDS_NO_VAR`. + // If violation is read-write (e.g. `x += 1`), then it would also appear in `externalVars` + // so will have been converted to an internal var already above. Type will be + // `CONST_VIOLATION_NEEDS_NO_VAR`. const node = getProp(fnNode, trail); internalVars[node.name].push(node); } diff --git a/lib/shared/constants.js b/lib/shared/constants.js index 5ef48dec..9b615ec8 100644 --- a/lib/shared/constants.js +++ b/lib/shared/constants.js @@ -27,9 +27,9 @@ module.exports = { FN_TYPE_CLASS: 'c', SUPER_CALL: 1, SUPER_EXPRESSION: 2, - CONST_VIOLATION_CONST: 3, - CONST_VIOLATION_FUNCTION_THROWING: 4, - CONST_VIOLATION_FUNCTION_SILENT: 5, + CONST_VIOLATION_NEEDS_VAR: 3, + CONST_VIOLATION_NEEDS_NO_VAR: 4, + CONST_VIOLATION_SILENT: 5, GLOBAL: 1, MODULE: 2, VALUE: 3,