diff --git a/lib/instrument/blocks.js b/lib/instrument/blocks.js index 1bb662a4..f29e1752 100644 --- a/lib/instrument/blocks.js +++ b/lib/instrument/blocks.js @@ -17,6 +17,7 @@ module.exports = { createArgumentsBinding, createNewTargetBinding, getOrCreateExternalVar, + createInternalVar, activateBlock, activateBinding, createBlockTempVar @@ -207,6 +208,25 @@ function getOrCreateExternalVar(externalVars, block, varName, bindingOrExternalV return externalVar; } +/** + * Create an internal var in a function. + * @param {Object} fn - Function object + * @param {string} varName - Var name + * @param {Object} binding - Binding object + * @param {Object} block - Block object + * @param {Array} [trail] - Trail for where binding appears (optional) + * @returns {undefined} + */ +function createInternalVar(fn, varName, binding, block, trail) { + const {internalVars} = fn; + let internalVar = internalVars.get(binding); + if (!internalVar) { + internalVar = {varName, blockId: block.id, trails: []}; + internalVars.set(binding, internalVar); + } + if (trail) internalVar.trails.push(trail); +} + /** * Activate block. * Called when block contains a binding which is referenced within a function. diff --git a/lib/instrument/visitors/assignee.js b/lib/instrument/visitors/assignee.js index 2c6877fa..da6b6be4 100644 --- a/lib/instrument/visitors/assignee.js +++ b/lib/instrument/visitors/assignee.js @@ -25,9 +25,8 @@ const assert = require('simple-invariant'); const Expression = require('./expression.js'), {IdentifierAssignOnly, IdentifierReadAndAssign} = require('./identifier.js'), MemberExpression = require('./memberExpression.js'), - {createBinding} = require('../blocks.js'), - {visitKey, visitKeyContainer, visitKeyContainerWithEmptyMembers} = require('../visit.js'), - {createArrayOrPush} = require('../../shared/functions.js'); + {createBinding, createInternalVar} = require('../blocks.js'), + {visitKey, visitKeyContainer, visitKeyContainerWithEmptyMembers} = require('../visit.js'); // Exports @@ -157,11 +156,11 @@ function visitConstOrLetIdentifier(node, isConst, state) { const varName = node.name, block = state.currentBlock; assertNoCommonJsVarsClash(block, varName, state); - createBinding(block, varName, {isConst}, state); + const binding = createBinding(block, varName, {isConst}, state); // Record as internal var const fn = state.currentFunction; - if (fn) createArrayOrPush(fn.internalVars, varName, [...state.trail]); + if (fn) createInternalVar(fn, varName, binding, block, [...state.trail]); } /** @@ -183,24 +182,10 @@ function IdentifierVar(node, state) { return; } - // Leave until later to add to `internalVars` in case binding is redeclared later - // as a function declaration. Vars defined as functions must not be - // added to `internalVars` to avoid their names being mangled. + // Record as internal var. + // If a function declaration reuses the same binding later, internal var will be deleted again. const fn = state.currentFunction; - if (fn) state.secondPass(addVarIdentifierToInternalVars, node, binding, varName, fn, [...state.trail]); -} - -/** - * Add identifier to parent function's internal vars. - * @param {Object} node - Identifier AST node (not needed but passed for debug reasons) - * @param {Object} binding - Binding object - * @param {string} varName - Variable name - * @param {Object} fn - Function object - * @param {Array} trail - Trail - * @returns {undefined} - */ -function addVarIdentifierToInternalVars(node, binding, varName, fn, trail) { - if (!binding.isFunction) createArrayOrPush(fn.internalVars, varName, trail); + if (fn) createInternalVar(fn, varName, binding, block, [...state.trail]); } /** diff --git a/lib/instrument/visitors/eval.js b/lib/instrument/visitors/eval.js index 76613198..32aa7d33 100644 --- a/lib/instrument/visitors/eval.js +++ b/lib/instrument/visitors/eval.js @@ -245,7 +245,7 @@ function activateSuperIfIsUsable(fn, state) { const maybeConstructorNode = getProp(fn.node, trail, 3); if (maybeConstructorNode.type === 'ClassMethod' && maybeConstructorNode.kind === 'constructor') { // In class constructor - need internal var for `this` as `eval()` code might include `super()` - createInternalVarForThis(fn); + createInternalVarForThis(fn, state); } } } diff --git a/lib/instrument/visitors/function.js b/lib/instrument/visitors/function.js index 4d77e917..98f63ff9 100644 --- a/lib/instrument/visitors/function.js +++ b/lib/instrument/visitors/function.js @@ -82,18 +82,26 @@ function FunctionDeclaration(node, state, parent, key) { // `() => { console.log(typeof x); if (true) function x() {} }` -> Logs 'undefined' const {currentBlock: block, currentHoistBlock: hoistBlock} = state; let binding = block.bindings[fnName]; - if (binding) { - // Existing binding must be a `var` declaration or another function declaration - binding.isFunction = true; - } else { + if (!binding) { const isTopLevel = block === hoistBlock; binding = createBinding(block, fnName, {isVar: isTopLevel, isFunction: 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. if (!isTopLevel && !state.isStrict && !node.async && !node.generator) { - state.sloppyFunctionDeclarations.push({varName: fnName, binding, block, hoistBlock}); + state.sloppyFunctionDeclarations.push({ + varName: fnName, + binding, + block, + hoistBlock, + parentFn: state.currentFunction + }); } + } else if (!binding.isFunction) { + // Existing binding is a `var` declaration. + // Remove from parent function's `internalVars` so `var` declarations' identifiers don't get renamed. + binding.isFunction = true; + state.currentFunction?.internalVars.delete(binding); } // Insert tracker comment @@ -423,7 +431,7 @@ function createFunction(id, node, isStrict, state) { parent: parentFunction, children: [], trail: parentFunction ? [...state.trail] : undefined, - internalVars: Object.create(null), // Keyed by var name + internalVars: new Map(), // Keyed by binding externalVars: new Map(), // Keyed by block globalVarNames: new Set(), amendments: [], @@ -535,6 +543,7 @@ function hoistSloppyFunctionDeclarations(state) { * @param {Object} declaration.binding - Binding object for binding in block where originally declared * @param {Object} declaration.block - Block object for block where originally declared * @param {Object} declaration.hoistBlock - Block object for block where could be hoisted to + * @param {Object} [declaration.parentFn] - Function object for parent function * @returns {undefined} */ function hoistSloppyFunctionDeclaration(declaration) { @@ -561,10 +570,13 @@ function hoistSloppyFunctionDeclaration(declaration) { } // Can be hoisted - if (hoistBlockBinding) { - hoistBlockBinding.isFunction = true; - } else { + if (!hoistBlockBinding) { hoistBlock.bindings[varName] = {...declaration.binding, isVar: true}; + } else if (!hoistBlockBinding.isFunction) { + // Existing binding is a `var` declaration. + // Remove from parent function's `internalVars` so `var` declarations' identifiers don't get renamed. + hoistBlockBinding.isFunction = true; + declaration.parentFn?.internalVars.delete(hoistBlockBinding); } } @@ -716,6 +728,14 @@ function insertTrackerComment(fnId, fnType, commentHolderNode, commentType, stat * @returns {undefined} */ function createFunctionInfoFunction(fn, scopes, astJson, fnInfoVarNode, state) { + // Compile internal vars + const internalVars = Object.create(null); + for (const {varName, blockId, trails} of fn.internalVars.values()) { + const internalVar = internalVars[varName] || (internalVars[varName] = {blockIds: [], trails: []}); + internalVar.blockIds.push(blockId); + internalVar.trails.push(...trails); + } + // Create JSON function info string const {children} = fn; let argNames; @@ -737,7 +757,7 @@ function createFunctionInfoFunction(fn, scopes, astJson, fnInfoVarNode, state) { containsEval: fn.containsEval || undefined, containsImport: fn.containsImport || undefined, argNames, - internalVars: fn.internalVars, + internalVars, 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 e9d4dba8..f2b1bb20 100644 --- a/lib/instrument/visitors/identifier.js +++ b/lib/instrument/visitors/identifier.js @@ -16,9 +16,8 @@ module.exports = { // Imports const visitEval = require('./eval.js'), - {getOrCreateExternalVar, activateBlock, activateBinding} = require('../blocks.js'), + {getOrCreateExternalVar, activateBlock, activateBinding, createInternalVar} = require('../blocks.js'), {checkInternalVarNameClash} = require('../internalVars.js'), - {createArrayOrPush} = require('../../shared/functions.js'), { CONST_VIOLATION_CONST, CONST_VIOLATION_FUNCTION_THROWING, CONST_VIOLATION_FUNCTION_SILENT } = require('../../shared/constants.js'); @@ -83,7 +82,7 @@ function ThisExpression(node, state) { if (block.id < fn.id) { recordExternalVar(binding, block, 'this', fn, [...state.trail], true, false, state); } else if (fn.hasSuperClass) { - createArrayOrPush(fn.internalVars, 'this', [...state.trail]); + createInternalVar(fn, 'this', binding, block, [...state.trail]); } } @@ -183,7 +182,7 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign // Record if internal var if (block.id >= fn.id) { - if (!binding.isFunction && !binding.argNames) createArrayOrPush(fn.internalVars, varName, trail); + if (!binding.isFunction && !binding.argNames) fn.internalVars.get(binding).trails.push(trail); return; } diff --git a/lib/instrument/visitors/super.js b/lib/instrument/visitors/super.js index 260ee71d..8112b7af 100644 --- a/lib/instrument/visitors/super.js +++ b/lib/instrument/visitors/super.js @@ -19,7 +19,8 @@ module.exports = { // Imports const { - createBindingWithoutNameCheck, getOrCreateExternalVar, activateBlock, createBlockTempVar + createBindingWithoutNameCheck, getOrCreateExternalVar, activateBlock, createBlockTempVar, + createInternalVar } = require('../blocks.js'), {getProp} = require('../../shared/functions.js'), {SUPER_CALL, SUPER_EXPRESSION} = require('../../shared/constants.js'); @@ -63,7 +64,7 @@ function Super(node, state, parent, key) { } else if (fn.firstSuperStatementIndex === undefined) { // First `super()` statement - record statement index fn.firstSuperStatementIndex = statementIndex; - createInternalVarForThis(fn); + createInternalVarForThis(fn, state); } } else if ( // Need to check parent node type as `ThrowStatement` also has an `argument` property @@ -80,7 +81,7 @@ function Super(node, state, parent, key) { } else { // `super()` appears in constructor, but not as top level statement fn.firstSuperStatementIndex = -1; - createInternalVarForThis(fn); + createInternalVarForThis(fn, state); } } else if (isInArrowFunction) { // `super` expression in arrow function @@ -89,10 +90,10 @@ function Super(node, state, parent, key) { // If inside class constructor or prototype class property of class with super class, // class requires `this$0`. `this$0` not needed in methods. const fullFn = getParentFullFunction(fn); - if (fullFn && fullFn.hasSuperClass) createInternalVarForThis(fullFn); + if (fullFn && fullFn.hasSuperClass) createInternalVarForThis(fullFn, state); } else if (fn.hasSuperClass) { // `super` expression in class constructor or class property of class with super class - createInternalVarForThis(fn); + createInternalVarForThis(fn, state); } // Create external var for `super` @@ -143,11 +144,12 @@ function activateSuperBinding(superBlock, state) { /** * Create internal var for `this` on function. * @param {Object} fn - Function object + * @param {Object} state - State object * @returns {undefined} */ -function createInternalVarForThis(fn) { - const {internalVars} = fn; - if (!internalVars.this) internalVars.this = []; +function createInternalVarForThis(fn, state) { + const block = state.currentThisBlock; + createInternalVar(fn, 'this', block.bindings.this, block, null); } /** diff --git a/lib/serialize/parseFunction.js b/lib/serialize/parseFunction.js index 7fa24f00..a6c41921 100644 --- a/lib/serialize/parseFunction.js +++ b/lib/serialize/parseFunction.js @@ -89,13 +89,17 @@ module.exports = function parseFunction( // Add child functions into AST, get external/internal var nodes, and get amendments to be made // (const violations / incidences of `super` to be transpiled) - const internalVars = Object.create(null), + const internalVarsWithBlockIds = Object.create(null), globalVarNames = new Set(), functionNames = new Set(), amendments = []; resolveFunctionInfo( fnInfo, getChildFnInfos, false, fnId, scopeDefs, - externalVars, internalVars, globalVarNames, functionNames, amendments + externalVars, internalVarsWithBlockIds, globalVarNames, functionNames, amendments + ); + + const internalVars = Object.fromEntries( + Object.entries(internalVarsWithBlockIds).map(([varName, {nodes}]) => [varName, nodes]) ); let node = fnInfo.ast; @@ -836,8 +840,11 @@ function resolveFunctionInfo( // Initializing properties of `internalVars` needs to happen before processing child functions, // as external vars in child functions can become internal vars in this function. const internalVarTrails = []; - for (const [varName, trails] of Object.entries(fnInfo.internalVars)) { - if (!internalVars[varName]) internalVars[varName] = []; + for (const [varName, {blockIds, trails}] of Object.entries(fnInfo.internalVars)) { + let internalVar = internalVars[varName]; + if (!internalVar) internalVar = internalVars[varName] = {blockIds: new Set(), nodes: []}; + blockIds.forEach(blockId => internalVar.blockIds.add(blockId)); + internalVarTrails.push({varName, trails}); } @@ -878,15 +885,17 @@ function resolveFunctionInfo( } else { // Var which is external to current function, but internal to function being serialized for (const [varName, {trails}] of Object.entries(scope.vars)) { - const internalVarNodes = internalVars[varName]; - if (internalVarNodes) internalVarNodes.push(...trailsToNodes(fnNode, trails, varName)); + const internalVar = internalVars[varName]; + if (internalVar && internalVar.blockIds.has(blockId)) { + internalVar.nodes.push(...trailsToNodes(fnNode, trails, varName)); + } } } } // Get internal var nodes for (const {varName, trails} of internalVarTrails) { - internalVars[varName].push(...trailsToNodes(fnNode, trails, varName)); + internalVars[varName].nodes.push(...trailsToNodes(fnNode, trails, varName)); } // Get global var names @@ -905,7 +914,7 @@ function resolveFunctionInfo( // 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. const node = getProp(fnNode, trail); - internalVars[node.name].push(node); + internalVars[node.name].nodes.push(node); } } } diff --git a/test/functions.test.js b/test/functions.test.js index 2aab81c2..7f81fdc1 100644 --- a/test/functions.test.js +++ b/test/functions.test.js @@ -7268,6 +7268,117 @@ describe('Functions', () => { } }); + // Tests for https://github.com/overlookmotel/livepack/issues/494 + describe('function declaration where other internal bindings with same name', () => { + itSerializes('in nested block', { + in() { + return () => { + function fn() { + return 123; + } + let n; + { + const fn = 456; // eslint-disable-line no-shadow + n = fn; + } + return [fn, n]; + }; + }, + out: '()=>{function fn(){return 123}let a;{const b=456;a=b}return[fn,a]}', + validate(outerFn) { + expect(outerFn).toBeFunction(); + const [fn, n] = outerFn(); + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + expect(n).toBe(456); + } + }); + + describe('when function top level', () => { + itSerializes('var statement before', { + in() { + return () => { + var fn; // eslint-disable-line no-var + function fn() { // eslint-disable-line no-redeclare + return 123; + } + return fn; + }; + }, + out: '()=>{var fn;function fn(){return 123}return fn}', + validate(outerFn) { + expect(outerFn).toBeFunction(); + const fn = outerFn(); + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + + itSerializes('var statement after', { + in() { + return () => { + function fn() { + return 123; + } + var fn; // eslint-disable-line no-var, no-redeclare, vars-on-top + return fn; + }; + }, + out: '()=>{function fn(){return 123}var fn;return fn}', + validate(outerFn) { + expect(outerFn).toBeFunction(); + const fn = outerFn(); + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + }); + + describe('when function in nested block and hoisted', () => { + itSerializes('var statement before', { + // Sloppy mode + in: `module.exports = () => { + var fn; + { + function fn() { + return 123; + } + } + return fn; + }`, + strictEnv: false, + out: '()=>{var fn;{function fn(){return 123}}return fn}', + validate(outerFn) { + expect(outerFn).toBeFunction(); + const fn = outerFn(); + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + + itSerializes('var statement after', { + // Sloppy mode + in: `module.exports = () => { + { + function fn() { + return 123; + } + } + var fn; + return fn; + }`, + strictEnv: false, + out: '()=>{{function fn(){return 123}}var fn;return fn}', + validate(outerFn) { + expect(outerFn).toBeFunction(); + const fn = outerFn(); + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + }); + }); + itSerializes('function expression', { in() { return () => function fn() {