From ccdf124caaf1f470e1117dceba4eaaba4c968305 Mon Sep 17 00:00:00 2001
From: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
Date: Thu, 15 Jul 2021 06:05:52 +0200
Subject: [PATCH] Correctly create outside variable when shadowed by catch
 parameter (#4178)

---
 src/ast/nodes/CatchClause.ts                  |  6 ++---
 src/ast/scopes/CatchScope.ts                  |  6 ++++-
 .../samples/catch-scope-shadowing/_config.js  |  3 +++
 .../samples/catch-scope-shadowing/main.js     | 23 +++++++++++++++++++
 4 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 test/function/samples/catch-scope-shadowing/_config.js
 create mode 100644 test/function/samples/catch-scope-shadowing/main.js

diff --git a/src/ast/nodes/CatchClause.ts b/src/ast/nodes/CatchClause.ts
index f9e8057c94b..6d3f87c4a12 100644
--- a/src/ast/nodes/CatchClause.ts
+++ b/src/ast/nodes/CatchClause.ts
@@ -18,9 +18,9 @@ export default class CatchClause extends NodeBase {
 	}
 
 	parseNode(esTreeNode: GenericEsTreeNode): void {
-		// Parameters need to be declared first as the logic is that hoisted body
-		// variables are associated with outside vars unless there is a parameter,
-		// in which case they are associated with the parameter
+		// Parameters need to be declared first as the logic is that initializers
+		// of hoisted body variables are associated with parameters of the same
+		// name instead of the variable
 		const { param } = esTreeNode;
 		if (param) {
 			(this.param as GenericEsTreeNode) = new (this.context.nodeConstructors[param.type] ||
diff --git a/src/ast/scopes/CatchScope.ts b/src/ast/scopes/CatchScope.ts
index 9a4e928674b..fd34d32675d 100644
--- a/src/ast/scopes/CatchScope.ts
+++ b/src/ast/scopes/CatchScope.ts
@@ -1,6 +1,7 @@
 import { AstContext } from '../../Module';
 import Identifier from '../nodes/Identifier';
 import { ExpressionEntity } from '../nodes/shared/Expression';
+import { UNDEFINED_EXPRESSION } from '../values';
 import LocalVariable from '../variables/LocalVariable';
 import ParameterScope from './ParameterScope';
 
@@ -13,10 +14,13 @@ export default class CatchScope extends ParameterScope {
 	): LocalVariable {
 		const existingParameter = this.variables.get(identifier.name) as LocalVariable;
 		if (existingParameter) {
+			// While we still create a hoisted declaration, the initializer goes to
+			// the parameter. Note that technically, the declaration now belongs to
+			// two variables, which is not correct but should not cause issues.
+			this.parent.addDeclaration(identifier, context, UNDEFINED_EXPRESSION, isHoisted);
 			existingParameter.addDeclaration(identifier, init);
 			return existingParameter;
 		}
-		// as parameters are handled differently, all remaining declarations are hoisted
 		return this.parent.addDeclaration(identifier, context, init, isHoisted);
 	}
 }
diff --git a/test/function/samples/catch-scope-shadowing/_config.js b/test/function/samples/catch-scope-shadowing/_config.js
new file mode 100644
index 00000000000..6a7cd9de660
--- /dev/null
+++ b/test/function/samples/catch-scope-shadowing/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'correctly associates shadowed variables in catch scopes'
+};
diff --git a/test/function/samples/catch-scope-shadowing/main.js b/test/function/samples/catch-scope-shadowing/main.js
new file mode 100644
index 00000000000..5cc45ec7ff6
--- /dev/null
+++ b/test/function/samples/catch-scope-shadowing/main.js
@@ -0,0 +1,23 @@
+var e = 'failed1',
+	x = 'value';
+
+(function () {
+	try {
+		// empty
+	} catch (e) {
+		var e = 'failed2';
+	}
+
+	assert.strictEqual(e, undefined);
+	assert.strictEqual(x, undefined);
+	x = 'failed3';
+	return;
+
+	try {
+		not_reached();
+	} catch (x) {
+		var x = 'failed4';
+	}
+})();
+
+assert.strictEqual(x, 'value');