fix(transformer-optional-catch-binding): the unused binding is not in the correct scope

[Dunqing]

Blocked by #5008

In the SemanticBuilder, we insert all CatchClause parameters in the BlockStatement scope, a child-scope of CatchClause

oxc/crates/oxc_semantic/src/builder.rs

Lines 709 to 718 in 858f510

	// Move all bindings from catch clause param scope to catch clause body scope
	// to make it easier to resolve references and check redeclare errors
	if self.scope.get_flags(parent_scope_id).is_catch_clause() {
	let parent_bindings =
	self.scope.get_bindings_mut(parent_scope_id).drain(..).collect::<Bindings>();
	parent_bindings.values().for_each(|symbol_id| {
	self.symbols.set_scope_id(*symbol_id, self.current_scope_id);
	});
	*self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings;
	}

So we should do the same thing in this plugin, but because CatchClause has no parameters, SemanticBuilder doesn't create scope for CatchClause. This cause we cannot find the BlockStatement scope_id.

This PR has changed to the correct logic. Just to wait #5008 solved

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

[Dunqing]

• fix(transformer-optional-catch-binding): the unused binding is not in the correct scope #5066 👈
• refactor(transformer/es2019): move all entry points to implementation of Traverse trait #5065
• docs(transformer): add documentation for optional-catch-binding plugin #5064
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #5066 will not alter performance

_{Comparing 08-22-fix_transformer-optional-catch-binding_the_unused_binding_is_not_in_the_correct_scope (47e69a8) with main (eba5033)}

Summary

✅ 29 untouched benchmarks

[overlookmotel]

Personally I'm not a fan of the way we move the bindings from catch param up into catch block. I understand it's to make identifying illegal declarations easier, but it's confusing/surprising.

We only have to check for illegal declarations in one place (syntax checker) whereas we have to deal with this non-standard scoping everywhere else. So on balance, in my view the downsides probably outweigh the upsides.

If we add scopes for function bodies (#5017), that'd require a mechanism in syntax checker to know when need to check parent scope for a clashing binding anyway, and could use the same mechanism for catch too.

[Dunqing]

Personally I'm not a fan of the way we move the bindings from catch param up into catch block. I understand it's to make identifying illegal declarations easier, but it's confusing/surprising.

We only have to check for illegal declarations in one place (syntax checker) whereas we have to deal with this non-standard scoping everywhere else. So on balance, in my view the downsides probably outweigh the upsides.

Yes, I agree with your point. I am not a fan of the way as well! But so far I haven't come up with a better way to address this

If we add scopes for function bodies (#5017), that'd require a mechanism in syntax checker to know when need to check parent scope for a clashing binding anyway, and could use the same mechanism for catch too.

Yes, I am looking forward we having a good mechanism to solve this. But we're stuck with the status quo for now 🥲.

[overlookmotel]

#5109 unblocks this.

[Dunqing]

#5109 unblocks this.

All tests passed, Thanks!

[overlookmotel]

Great to see some of the scope tests being taken off the "fail" list. Everything I've been doing over past 2 days has been just adding more and more to that list. Now the tide is turning!

[graphite-app]

Merge activity

• Aug 23, 8:46 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 23, 8:46 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 23, 9:12 AM EDT: overlookmotel merged this pull request with the Graphite merge queue.