WIP 5

TODO.md

@@ -14,3 +14,20 @@
 * 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; };
+};
+```

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
@@ -740,6 +741,19 @@ function createFunctionInfoFunction(fn, state) {
 		}
 	}
 
+	// 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
 	const {children} = fn;
 	let argNames;
@@ -767,9 +781,7 @@ function createFunctionInfoFunction(fn, state) {
 		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,

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
@@ -173,15 +173,26 @@ function resolveIdentifier(node, block, varName, fn, trail, isReadFrom, isAssign
 
 	// 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 a function name or frozen var, as they're not renamed
+		// and so aren't 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 || binding.isFunction
+					? CONST_VIOLATION_NEEDS_NO_VAR
+					: CONST_VIOLATION_NEEDS_VAR,
 			blockId: block.id,
-			trail
+			trail,
+			binding
 		});
 
 		if (!isReadFrom) return;

lib/instrument/visitors/super.js

@@ -206,7 +206,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
 	});
 }
 

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');
 
@@ -338,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);
 		}
 	}
@@ -916,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);
 			}

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,