Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly create outside variable when shadowed by catch parameter #4178

Merged
merged 1 commit into from Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/ast/nodes/CatchClause.ts
Expand Up @@ -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] ||
Expand Down
6 changes: 5 additions & 1 deletion 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';

Expand All @@ -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);
}
}
3 changes: 3 additions & 0 deletions test/function/samples/catch-scope-shadowing/_config.js
@@ -0,0 +1,3 @@
module.exports = {
description: 'correctly associates shadowed variables in catch scopes'
};
23 changes: 23 additions & 0 deletions 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');