Correctly associate hoisted variables with parameters (#2456)

* Associated hoisted variables with parameters, use LocalVariables for parameters

* Move test assertions into tested code

* Test parameter defaults are consistently deconflicted

src/ast/ExecutionPathOptions.ts

@@ -5,7 +5,6 @@ import CallExpression from './nodes/CallExpression';
 import Property from './nodes/Property';
 import { ExpressionEntity } from './nodes/shared/Expression';
 import { ObjectPath } from './values';
-import ParameterVariable from './variables/ParameterVariable';
 import ThisVariable from './variables/ThisVariable';
 
 export enum OptionTypes {
@@ -105,7 +104,7 @@ export class ExecutionPathOptions {
 			.setIgnoreNoLabels();
 	}
 
-	getReplacedVariableInit(variable: ThisVariable | ParameterVariable): ExpressionEntity {
+	getReplacedVariableInit(variable: ThisVariable): ExpressionEntity {
 		return this.optionValues.getIn([OptionTypes.REPLACED_VARIABLE_INITS, variable]);
 	}
 
@@ -184,7 +183,7 @@ export class ExecutionPathOptions {
 		return this.get(OptionTypes.IGNORE_RETURN_AWAIT_YIELD);
 	}
 
-	replaceVariableInit(variable: ThisVariable | ParameterVariable, init: ExpressionEntity) {
+	replaceVariableInit(variable: ThisVariable, init: ExpressionEntity) {
 		return this.setIn([OptionTypes.REPLACED_VARIABLE_INITS, variable], init);
 	}
 

src/ast/nodes/ArrowFunctionExpression.ts

@@ -17,7 +17,7 @@ export default class ArrowFunctionExpression extends NodeBase {
 	preventChildBlockScope: true;
 
 	createScope(parentScope: Scope) {
-		this.scope = new ReturnValueScope(parentScope);
+		this.scope = new ReturnValueScope(parentScope, this.context.deoptimizationTracker);
 	}
 
 	getReturnExpressionWhenCalledAtPath(path: ObjectPath) {
@@ -67,7 +67,7 @@ export default class ArrowFunctionExpression extends NodeBase {
 			this.body = new this.context.nodeConstructors.BlockStatement(
 				esTreeNode.body,
 				this,
-				new Scope(this.scope)
+				this.scope.hoistedBodyVarScope
 			);
 		}
 		super.parseNode(esTreeNode);

src/ast/nodes/CatchClause.ts

@@ -15,7 +15,7 @@ export default class CatchClause extends NodeBase {
 	preventChildBlockScope: true;
 
 	createScope(parentScope: Scope) {
-		this.scope = new CatchScope(parentScope);
+		this.scope = new CatchScope(parentScope, this.context.deoptimizationTracker);
 	}
 
 	initialise() {

src/ast/nodes/Identifier.ts

@@ -10,7 +10,6 @@ import { ImmutableEntityPathTracker } from '../utils/ImmutableEntityPathTracker'
 import { LiteralValueOrUnknown, ObjectPath, UNKNOWN_EXPRESSION, UNKNOWN_VALUE } from '../values';
 import LocalVariable from '../variables/LocalVariable';
 import Variable from '../variables/Variable';
-import AssignmentExpression from './AssignmentExpression';
 import AssignmentPattern from './AssignmentPattern';
 import * as NodeType from './NodeType';
 import Property from './Property';
@@ -66,10 +65,7 @@ export default class Identifier extends NodeBase {
 				);
 				break;
 			case 'parameter':
-				this.variable = (<FunctionScope>this.scope).addParameterDeclaration(
-					this,
-					this.context.deoptimizationTracker
-				);
+				this.variable = (<FunctionScope>this.scope).addParameterDeclaration(this);
 				break;
 			default:
 				throw new Error(`Unexpected identifier kind ${kind}.`);

src/ast/nodes/shared/FunctionNode.ts

@@ -2,7 +2,6 @@ import CallOptions from '../../CallOptions';
 import { ExecutionPathOptions } from '../../ExecutionPathOptions';
 import FunctionScope from '../../scopes/FunctionScope';
 import BlockScope from '../../scopes/FunctionScope';
-import Scope from '../../scopes/Scope';
 import { ObjectPath, UNKNOWN_EXPRESSION, UNKNOWN_KEY, UNKNOWN_PATH } from '../../values';
 import BlockStatement from '../BlockStatement';
 import Identifier from '../Identifier';
@@ -80,7 +79,11 @@ export default class FunctionNode extends NodeBase {
 
 	parseNode(esTreeNode: GenericEsTreeNode) {
 		this.body = <BlockStatement>(
-			new this.context.nodeConstructors.BlockStatement(esTreeNode.body, this, new Scope(this.scope))
+			new this.context.nodeConstructors.BlockStatement(
+				esTreeNode.body,
+				this,
+				this.scope.hoistedBodyVarScope
+			)
 		);
 		super.parseNode(esTreeNode);
 	}

src/ast/scopes/CatchScope.ts

@@ -3,11 +3,8 @@ import { ExpressionEntity } from '../nodes/shared/Expression';
 import { EntityPathTracker } from '../utils/EntityPathTracker';
 import LocalVariable from '../variables/LocalVariable';
 import ParameterScope from './ParameterScope';
-import Scope from './Scope';
 
 export default class CatchScope extends ParameterScope {
-	parent: Scope;
-
 	addDeclaration(
 		identifier: Identifier,
 		deoptimizationTracker: EntityPathTracker,

src/ast/scopes/FunctionScope.ts

@@ -20,7 +20,7 @@ export default class FunctionScope extends ReturnValueScope {
 	};
 
 	constructor(parent: Scope, deoptimizationTracker: EntityPathTracker) {
-		super(parent);
+		super(parent, deoptimizationTracker);
 		this.variables.arguments = new ArgumentsVariable(
 			super.getParameterVariables(),
 			deoptimizationTracker

src/ast/scopes/ParameterScope.ts

@@ -1,22 +1,41 @@
 import Identifier from '../nodes/Identifier';
 import { EntityPathTracker } from '../utils/EntityPathTracker';
-import ParameterVariable from '../variables/ParameterVariable';
+import { UNKNOWN_EXPRESSION } from '../values';
+import LocalVariable from '../variables/LocalVariable';
 import Scope from './Scope';
 
 export default class ParameterScope extends Scope {
 	parent: Scope;
+	hoistedBodyVarScope: Scope;
 
-	private parameters: ParameterVariable[] = [];
+	private parameters: LocalVariable[] = [];
+	private deoptimizationTracker: EntityPathTracker;
+
+	constructor(parent: Scope, deoptimizationTracker: EntityPathTracker) {
+		super(parent);
+		this.deoptimizationTracker = deoptimizationTracker;
+		this.hoistedBodyVarScope = new Scope(this);
+	}
 
 	/**
 	 * Adds a parameter to this scope. Parameters must be added in the correct
 	 * order, e.g. from left to right.
-	 * @param {Identifier} identifier
-	 * @returns {Variable}
 	 */
-	addParameterDeclaration(identifier: Identifier, deoptimizationTracker: EntityPathTracker) {
-		const variable = new ParameterVariable(identifier, deoptimizationTracker);
-		this.variables[identifier.name] = variable;
+	addParameterDeclaration(identifier: Identifier) {
+		const name = identifier.name;
+		let variable;
+		if (name in this.hoistedBodyVarScope.variables) {
+			variable = this.hoistedBodyVarScope.variables[name] as LocalVariable;
+			variable.addDeclaration(identifier, null);
+		} else {
+			variable = new LocalVariable(
+				name,
+				identifier,
+				UNKNOWN_EXPRESSION,
+				this.deoptimizationTracker
+			);
+		}
+		this.variables[name] = variable;
 		this.parameters.push(variable);
 		return variable;
 	}

src/ast/variables/ArgumentsVariable.ts

@@ -3,7 +3,6 @@ import { ExecutionPathOptions } from '../ExecutionPathOptions';
 import { EntityPathTracker } from '../utils/EntityPathTracker';
 import { ObjectPath, UNKNOWN_EXPRESSION } from '../values';
 import LocalVariable from './LocalVariable';
-import ParameterVariable from './ParameterVariable';
 
 const getParameterVariable = (path: ObjectPath, options: ExecutionPathOptions) => {
 	const firstArgNum = parseInt(<string>path[0], 10);
@@ -16,9 +15,9 @@ const getParameterVariable = (path: ObjectPath, options: ExecutionPathOptions) =
 };
 
 export default class ArgumentsVariable extends LocalVariable {
-	private parameters: ParameterVariable[];
+	private parameters: LocalVariable[];
 
-	constructor(parameters: ParameterVariable[], deoptimizationTracker: EntityPathTracker) {
+	constructor(parameters: LocalVariable[], deoptimizationTracker: EntityPathTracker) {
 		super('arguments', null, UNKNOWN_EXPRESSION, deoptimizationTracker);
 		this.parameters = parameters;
 	}

src/ast/variables/LocalVariable.ts

@@ -27,7 +27,7 @@ export default class LocalVariable extends Variable {
 	additionalInitializers: ExpressionEntity[] | null = null;
 
 	// Caching and deoptimization:
-	// We collect deoptimization when we do not return something unknown
+	// We track deoptimization when we do not return something unknown
 	private deoptimizationTracker: EntityPathTracker;
 	private expressionsToBeDeoptimized: DeoptimizableEntity[] = [];
 

src/ast/variables/ThisVariable.ts

@@ -1,8 +1,45 @@
+import CallOptions from '../CallOptions';
+import { ExecutionPathOptions } from '../ExecutionPathOptions';
+import { ExpressionEntity } from '../nodes/shared/Expression';
 import { EntityPathTracker } from '../utils/EntityPathTracker';
-import ReplaceableInitializationVariable from './ReplaceableInitializationVariable';
+import { LiteralValueOrUnknown, ObjectPath, UNKNOWN_EXPRESSION, UNKNOWN_VALUE } from '../values';
+import LocalVariable from './LocalVariable';
 
-export default class ThisVariable extends ReplaceableInitializationVariable {
+export default class ThisVariable extends LocalVariable {
 	constructor(deoptimizationTracker: EntityPathTracker) {
-		super('this', null, deoptimizationTracker);
+		super('this', null, null, deoptimizationTracker);
+	}
+
+	getLiteralValueAtPath(): LiteralValueOrUnknown {
+		return UNKNOWN_VALUE;
+	}
+
+	hasEffectsWhenAccessedAtPath(path: ObjectPath, options: ExecutionPathOptions) {
+		return (
+			this._getInit(options).hasEffectsWhenAccessedAtPath(path, options) ||
+			super.hasEffectsWhenAccessedAtPath(path, options)
+		);
+	}
+
+	hasEffectsWhenAssignedAtPath(path: ObjectPath, options: ExecutionPathOptions) {
+		return (
+			this._getInit(options).hasEffectsWhenAssignedAtPath(path, options) ||
+			super.hasEffectsWhenAssignedAtPath(path, options)
+		);
+	}
+
+	hasEffectsWhenCalledAtPath(
+		path: ObjectPath,
+		callOptions: CallOptions,
+		options: ExecutionPathOptions
+	) {
+		return (
+			this._getInit(options).hasEffectsWhenCalledAtPath(path, callOptions, options) ||
+			super.hasEffectsWhenCalledAtPath(path, callOptions, options)
+		);
+	}
+
+	_getInit(options: ExecutionPathOptions): ExpressionEntity {
+		return options.getReplacedVariableInit(this) || UNKNOWN_EXPRESSION;
 	}
 }

test/function/samples/deconflict-parameter-defaults/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'consistently deconflict variable names for parameter defaults'
+};

test/function/samples/deconflict-parameter-defaults/main.js

@@ -0,0 +1,27 @@
+import './other.js';
+
+var a = 'main';
+
+function paramDefault(a, b = a) {
+	assert.equal(b, 'param', 'main-param-b');
+}
+paramDefault('param');
+
+function outsideDefault(b = a) {
+	assert.equal(b, 'main', 'main-outside-b');
+}
+outsideDefault();
+
+function paramDefaultRedeclare(a, b = a) {
+	var a;
+	assert.equal(a, 'param', 'main-param-redeclare-a');
+	assert.equal(b, 'param', 'main-param-redeclare-b');
+}
+paramDefaultRedeclare('param');
+
+function outsideDefaultRedeclare(b = a) {
+	var a;
+	assert.equal(a, undefined, 'main-outside-redeclare-a');
+	assert.equal(b, 'main', 'main-outside-redeclare-a');
+}
+outsideDefaultRedeclare();

test/function/samples/deconflict-parameter-defaults/other.js

@@ -0,0 +1,25 @@
+var a = 'other';
+
+function paramDefault(a, b = a) {
+	assert.equal(b, 'param', 'other-param-b');
+}
+paramDefault('param');
+
+function outsideDefault(b = a) {
+	assert.equal(b, 'other', 'other-outside-b');
+}
+outsideDefault();
+
+function paramDefaultRedeclare(a, b = a) {
+	var a;
+	assert.equal(a, 'param', 'other-param-redeclare-a');
+	assert.equal(b, 'param', 'other-param-redeclare-b');
+}
+paramDefaultRedeclare('param');
+
+function outsideDefaultRedeclare(b = a) {
+	var a;
+	assert.equal(a, undefined, 'other-outside-redeclare-a');
+	assert.equal(b, 'other', 'other-outside-redeclare-a');
+}
+outsideDefaultRedeclare();

test/function/samples/redeclare-parameter/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'associates redeclared parameters (#2451)'
+};

test/function/samples/redeclare-parameter/main.js

@@ -0,0 +1,23 @@
+function fnDecl(a) {
+	var a;
+	if (!a) {
+		throw Error("a was incorrectly assumed to be undefined in a function declaration");
+	}
+}
+fnDecl(true);
+
+export const fnExp = function(a) {
+	var a;
+	if (!a) {
+		throw Error("a was incorrectly assumed to be undefined in a function expression");
+	}
+};
+fnExp(true);
+
+export const arrowFn = a => {
+	var a;
+	if (!a) {
+		throw Error("a was incorrectly assumed to be undefined in an arrow function");
+	}
+};
+arrowFn(true);