feat(transformer): transform explicit resource management

[camc314]

wanted to have a shot at implementing this, this is rough draft.

Dunqing or overlookmotel feel free to take over or close out this PR. not sure how much time i'll have to finish it off

@Dunqing @overlookmotel this should be ready when you guys have time 🙏

closes #9168

[camc314]

• feat(transformer): transform explicit resource management #9310 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #9310 will degrade performances by 5.43%

_{Comparing c/02-23-feat_transformer_transform_explicit_resource_management (beed50c) with main (4ca62ab)}

Summary

❌ 3 regressions
✅ 36 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	transformer[antd.js]	53.3 ms	55 ms	-3.15%
❌	transformer[checker.ts]	24 ms	25.4 ms	-5.43%
❌	transformer[pdf.mjs]	10.6 ms	11.1 ms	-4.42%

[Dunqing]

Thank you for working on this, I will review this next week, before that, can you add some documentation like other plugins do at the top of file?

For example:

oxc/crates/oxc_transformer/src/es2020/nullish_coalescing_operator.rs

Lines 1 to 30 in 3c1d7f6

	//! ES2020: Nullish Coalescing Operator
	//!
	//! This plugin transforms nullish coalescing operators (`??`) to a series of ternary expressions.
	//!
	//! > This plugin is included in `preset-env`, in ES2020
	//!
	//! ## Example
	//!
	//! Input:
	//! ```js
	//! var foo = object.foo ?? "default";
	//! ```
	//!
	//! Output:
	//! ```js
	//! var _object$foo;
	//! var foo =
	//! (_object$foo = object.foo) !== null && _object$foo !== void 0
	//! ? _object$foo
	//! : "default";
	//! ```
	//!
	//! ## Implementation
	//!
	//! Implementation based on [@babel/plugin-transform-nullish-coalescing-operator](https://babeljs.io/docs/babel-plugin-transform-nullish-coalescing-operator).
	//!
	//! ## References:
	//! * Babel plugin implementation: <https://github.com/babel/babel/tree/v7.26.2/packages/babel-plugin-transform-nullish-coalescing-operator>
	//! * Nullish coalescing TC39 proposal: <https://github.com/tc39-transfer/proposal-nullish-coalescing>

[overlookmotel]

Thanks very much for tackling this. And bravo for getting all the tests to pass, including all the semantic IDs (don't worry about that 1 weird one).

I've made comments below about some small things.

The bigger thing is the performance hit. My assumption is it's due to looping over statements. Usually we try to avoid that by setting some state in enter_statements, then let the traversal run through the children with smaller visitors which update that state as they go, and then you check the state in exit_statements and act on it if there's work to be done. This avoids visiting every statement twice.

However, I'm not familiar with this transform, and found it hard to follow what the code here is doing, so can't judge how amenable it'd be to that approach. So I suggest we merge this, and then work on the performance in later PRs.

What would help a lot is if you'd be able to add more comments, so the logic is easier to follow. The AstBuilder calls are so verbose (bad API, sorry!) that it helps a lot to have a comment before a bunch of ctx.ast.blah_blah calls saying what the code it produces. e.g. add a comment before this:

// `var var_id = <expr>;`

oxc/crates/oxc_transformer/src/proposals/explicit_resource_management.rs

Lines 270 to 289 in 69a7a41

	inner_block.push(Statement::VariableDeclaration(ctx.ast.alloc(
	ctx.ast.variable_declaration(
	span,
	VariableDeclarationKind::Var,
	ctx.ast.vec_from_array([ctx.ast.variable_declarator(
	span,
	VariableDeclarationKind::Var,
	ctx.ast.binding_pattern(
	BindingPatternKind::BindingIdentifier(
	ctx.ast.alloc(var_id.create_binding_identifier(ctx)),
	),
	NONE,
	false,
	),
	Some(expr),
	false,
	)]),
	false,
	),
	)));

Also methods should ideally have a high-level comment with a before vs after example (exactly like you did on enter_for_of_statement).

Once the logic of the transform is clearer, it'll be easier to see what perf optimization is possible.

Sorry for the deluge of feedback. I hope it's helpful, rather than annoying!

[overlookmotel]

This change seems extraneous.

[camc314]

i need to think more here, i'm using it

oxc/crates/oxc_transformer/src/proposals/explicit_resource_management.rs

Line 414 in fc2520a

let child_ids = scopes.get_child_ids(current_scope_id);

, but there might be a better way.

will look properly tommorow

[overlookmotel]

I didn't explain myself well. This file is an example. Did you mean to change the example?

[overlookmotel]

I don't think this is quite correct. current_scope_id isn't updated until after enter_for_of_statement is called:

oxc/crates/oxc_traverse/src/generated/walk.rs

Lines 1802 to 1813 in 4ca62ab

	unsafe fn walk_for_of_statement<'a, Tr: Traverse<'a>>(
	traverser: &mut Tr,
	node: *mut ForOfStatement<'a>,
	ctx: &mut TraverseCtx<'a>,
	) {
	traverser.enter_for_of_statement(&mut *node, ctx);
	let previous_scope_id = ctx.current_scope_id();
	let current_scope_id = (*((node as *mut u8).add(ancestor::OFFSET_FOR_OF_STATEMENT_SCOPE_ID)
	as *mut Cell<Option<ScopeId>>))
	.get()
	.unwrap();
	ctx.set_current_scope_id(current_scope_id);

So the correct parent scope is for_of_stmt_scope_id, not ctx.current_scope_id().

This is rather unintutive, I know! If I remember right, the reason was that for some types the scope is entered late/exited early (scope doesn't start before the 1st field e.g. Class). So we chose to always call enter_* before entering the scope, and exit_* after exiting the scope, so you don't have to remember which types have this "late entry" / "early exit" behavior, and which don't.

If making that change doesn't fix/fail any tests, I guess there's no tests for the case where for statement doesn't already have a statement block. If so, we should add one.

[camc314]

Thank you for the detailed review! I appreciate it.

For the perf hit, i had this thought. Would it be something like:

1. enter var decl.
2. if using, add address of parent (e.g. block stmt) to a hash map
3. when leaving a block, check if the address is in the hash map, if so, do some work. else skip.

If that makes sense, perhapse we merge this once the other feedback has been resolved and fixed in a foillowup?

I've actioned most of your comments just now. the more complex ones i need to think about more before actioning, but i'll try to do tomorrow/friday.

NOTE: still need to add some more code comments about ast builder changes

[Boshen]

This is a great help for future maintenance: https://github.com/oxc-project/oxc/blob/main/crates/oxc_transformer/README.md#style-guide-for-implementing-transforms