Rename top level require functions in CommonJS [fix]

Fixes #567.
overlookmotel · Dec 23, 2023 · 74abd31 · 74abd31
1 parent c354cef
commit 74abd31
Show file tree

Hide file tree

Showing 5 changed files with 223 additions and 3 deletions.
diff --git a/lib/init/getScopeId.js b/lib/init/getScopeId.js
@@ -5,7 +5,7 @@
 
 'use strict';
 
-const {defineProperties, getOwnPropertyDescriptors} = Object;
+const {defineProperty, defineProperties, getOwnPropertyDescriptors} = Object;
 
 // Exports
 
@@ -23,6 +23,7 @@ let nextScopeId = 1;
 
 // Add additional static methods
 getScopeId.toRest = toRest;
+getScopeId.renameRequireAlias = renameRequireAlias;
 
 /**
  * Convert object to array.
@@ -33,3 +34,12 @@ getScopeId.toRest = toRest;
 function toRest(obj) {
 	return defineProperties([], getOwnPropertyDescriptors(obj));
 }
+
+/**
+ * Set name property of a function to 'require'.
+ * @param {Function} req - Function
+ * @returns {Function} - Input function
+ */
+function renameRequireAlias(req) {
+	return defineProperty(req, 'name', {value: 'require'});
+}
diff --git a/lib/instrument/internalVars.js b/lib/instrument/internalVars.js
@@ -31,7 +31,8 @@ module.exports = {
 	createTempVarNode,
 	createFnInfoVarNode,
 	checkInternalVarNameClash,
-	renameInternalVars
+	renameInternalVars,
+	addToInternalVars
 };
 
 /*

diff --git a/lib/instrument/modify.js b/lib/instrument/modify.js
@@ -87,6 +87,7 @@ function modifyAst(ast, filename, isCommonJs, isStrict, sources, evalState) {
 		trackerVarNode: undefined,
 		getScopeIdVarNode: undefined,
 		getSourcesNode: undefined,
+		requireAliasTempVarNode: undefined,
 		functions: [],
 		fileContainsFunctionsOrEval: false,
 		secondPass: (fn, ...params) => secondPassQueue.push({fn, params})
@@ -197,6 +198,7 @@ function modifySecondPass(ast, secondPassQueue, isEvalCode, sources, state) {
 		state.getSourcesNode = createFnInfoVarNode(0, state);
 		processQueue(secondPassQueue, state);
 		insertBlockVarsIntoBlockStatement(state.programBlock, programNode, state);
+		insertRequireAlias(programNode, state);
 		insertFunctionInfoFunctions(programNode, isEvalCode, sources, state);
 	}
 
@@ -295,6 +297,32 @@ function insertFunctionInfoFunctions(programNode, isEvalCode, sources, state) {
 	}
 }
 
+/**
+ * Insert alias reassignment for `require`.
+ * If CommonJS code containing top-level function declaration(s) called 'require',
+ * they've been renamed to a temporary name earlier in instrumentation.
+ * Insert `var` statement at top of file to restore the binding.
+ * @param {Object} programNode - Program AST node
+ * @param {Object} state - State object
+ * @returns {undefined}
+ */
+function insertRequireAlias(programNode, state) {
+	if (!state.requireAliasTempVarNode) return;
+
+	// `var require = livepack_getScopeId.renameRequireAlias(livepack_temp_3);`
+	programNode.body.unshift(
+		t.variableDeclaration('var', [
+			t.variableDeclarator(
+				t.identifier('require'),
+				t.callExpression(
+					t.memberExpression(state.getScopeIdVarNode, t.identifier('renameRequireAlias')),
+					[state.requireAliasTempVarNode]
+				)
+			)
+		])
+	);
+}
+
 /**
  * Get current AST node, to get location where instrumentation failed and threw error.
  * Uses current function node and trail to get node. If no node at that trail, get parent node.

diff --git a/lib/instrument/visitors/function.js b/lib/instrument/visitors/function.js
@@ -38,7 +38,7 @@ const Statement = require('./statement.js'),
 		createNewTargetBinding, getOrCreateExternalVar
 	} = require('../blocks.js'),
 	{insertTrackerCodeIntoFunction} = require('../tracking.js'),
-	{createFnInfoVarNode} = require('../internalVars.js'),
+	{createFnInfoVarNode, createTempVarNode, addToInternalVars} = require('../internalVars.js'),
 	{insertComment, hasUseStrictDirective, stringLiteralWithSingleQuotes} = require('../utils.js'),
 	{combineArraysWithDedup} = require('../../shared/functions.js'),
 	{
@@ -109,6 +109,12 @@ function FunctionDeclaration(node, state, parent, key) {
 
 	// Insert tracker comment
 	insertFunctionDeclarationOrExpressionTrackerComment(fn, node, state);
+
+	// If top-level function declaration called 'require' in CommonJS, rename it in 2nd pass
+	if (
+		fnName === 'require' && block === state.programBlock
+		&& state.programBlock.parent.bindings.has('require')
+	) state.secondPass(renameRequireFunctionDeclaration, node, state);
 }
 
 /**
@@ -691,6 +697,26 @@ function insertArrowFunctionTrackerComment(fn, node, state) {
 	);
 }
 
+/**
+ * Rename top-level function declaration in CommonJS file called `require` to a temp name.
+ * Otherwise, this prevents loading Livepack's `init` code.
+ * At end of instrumentation, a `var require =` statement will be added at top of file
+ * to restore the binding.
+ * @param {Object} fnNode - Function declaration AST node
+ * @param {Object} state - State object
+ * @returns {undefined}
+ */
+function renameRequireFunctionDeclaration(fnNode, state) {
+	// Use same temp var name for all `require` functions in this file
+	const tempVarNode = state.requireAliasTempVarNode
+		|| (state.requireAliasTempVarNode = createTempVarNode(state));
+
+	// Set `name` of existing identifier, rather than replacing with `tempVarNode`
+	// to preserve any attached comments (including tracker comment)
+	fnNode.id.name = tempVarNode.name;
+	addToInternalVars(fnNode.id, state);
+}
+
 /**
  * Insert tracker comment.
  * @param {number} fnId - Function ID

diff --git a/test/commonjs.test.js b/test/commonjs.test.js
@@ -241,6 +241,161 @@ describe('`require`', () => {
 			);
 		});
 	});
+
+	describe('local functions called `require` do not break instrumentation', () => {
+		describe('1 function', () => {
+			itSerializes('sloppy mode', {
+				in: `
+					module.exports = require;
+					const ext = {x: 1};
+					function require() {
+						return ext;
+					}
+				`,
+				strictEnv: false,
+				out: '(a=>function require(){return a})({x:1})',
+				validate(fn) {
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toEqual({x: 1});
+				}
+			});
+
+			itSerializes('strict mode', {
+				in: `
+					'use strict';
+					module.exports = require;
+					const ext = {x: 1};
+					function require() {
+						return ext;
+					}
+				`,
+				out: '(a=>function require(){return a})({x:1})',
+				validate(fn) {
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toEqual({x: 1});
+				}
+			});
+		});
+
+		describe('multiple functions', () => {
+			itSerializes('sloppy mode', {
+				in: `
+					module.exports = require;
+					const extA = {x: 1},
+						extB = {y: 2},
+						extC = {z: 3};
+					function require() {
+						return extA;
+					}
+					function require() {
+						return extB;
+					}
+					function require() {
+						return extC;
+					}
+				`,
+				strictEnv: false,
+				out: '(a=>function require(){return a})({z:3})',
+				validate(fn) {
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toEqual({z: 3});
+				}
+			});
+
+			itSerializes('strict mode', {
+				in: `
+					'use strict';
+					module.exports = require;
+					const extA = {x: 1},
+						extB = {y: 2};
+					function require() {
+						return extA;
+					}
+					function require() {
+						return extB;
+					}
+				`,
+				out: '(a=>function require(){return a})({y:2})',
+				validate(fn) {
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toEqual({y: 2});
+				}
+			});
+		});
+
+		itSerializes('function prefixed with label', {
+			in: `
+				module.exports = require;
+				const ext = {x: 1};
+				foo: function require() {
+					return ext;
+				}
+			`,
+			strictEnv: false,
+			out: '(a=>function require(){return a})({x:1})',
+			validate(fn) {
+				expect(fn).toBeFunction();
+				expect(fn.name).toBe('require');
+				expect(fn()).toEqual({x: 1});
+			}
+		});
+
+		describe('function in nested block is not hoisted', () => {
+			itSerializes('when no top level function', {
+				in: {
+					'index.js': `
+						let inner;
+						{
+							inner = require;
+							function require() {
+								return 456;
+							}
+						}
+						module.exports = [require('./other.js'), inner];
+					`,
+					'other.js': 'module.exports = 123;'
+				},
+				out: '[123,function require(){return 456}]',
+				strictEnv: false,
+				validate([num, fn]) {
+					expect(num).toBe(123);
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toBe(456);
+				}
+			});
+
+			itSerializes('when also top level function', {
+				in: `
+					let inner;
+					{
+						inner = require;
+						function require() {
+							return 456;
+						}
+					}
+					function require() {
+						return 123;
+					}
+					module.exports = [require, inner];
+				`,
+				out: '[function require(){return 123},function require(){return 456}]',
+				strictEnv: false,
+				validate([fn, inner]) {
+					expect(fn).toBeFunction();
+					expect(fn.name).toBe('require');
+					expect(fn()).toBe(123);
+					expect(inner).toBeFunction();
+					expect(inner.name).toBe('require');
+					expect(inner()).toBe(456);
+				}
+			});
+		});
+	});
 });
 
 describe('`require.resolve`', () => {