feat(transformer): add object-spread plugin

[magic-akari]

No description provided.

[magic-akari]

• feat(transformer): add object-spread plugin #3133 👈
• main

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

Join @magic-akari and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #3133 will not alter performance

_{Comparing 04-29-feat_transformer_add_object-rest-spread_plugin (08dc0ad) with main (5c4c001)}

Summary

✅ 29 untouched benchmarks

[magic-akari]

module_imports.rs may need to be improved to enable testing, but I do not want to make changes to it in this PR.

[overlookmotel]

Hi @magic-akari. Just wondered how complete this is and whether we should try to finish it off?

Transformer has changed a bit since April - it now uses Traverse rather than VisitMut - but I doubt updating it for that change would be difficult, as they're largely the same.

[magic-akari]

Hi @magic-akari. Just wondered how complete this is and whether we should try to finish it off?

It's up to you; I'm not in a rush.

Transformer has changed a bit since April - it now uses Traverse rather than VisitMut - but I doubt updating it for that change would be difficult, as they're largely the same.

Wow, I am very willing to learn new methods.

[magic-akari]

I am currently refactoring this pull request.

[overlookmotel]

Hi @magic-akari. Sorry I didn't reply before. My Github notifications is like a snowdrift!

In case you've not noticed it, we recently adopted a "style guide" for writing transformers: https://github.com/oxc-project/oxc/blob/main/crates/oxc_transformer/README.md

Most of the existing transformers have not yet been brought in line with the style guide, but this one has, so it's a good model to follow.

If you have any trouble figuring out Traverse, please feel free to ping me.

[magic-akari]

❯ cargo run -p oxc_transformer --example transformer tasks/coverage/babel/packages/babel-parser/test/fixtures/es2018/object-rest-spread/5/input.js
    Finished `dev` profile [unoptimized] target(s) in 0.15s
     Running `target/debug/examples/transformer tasks/coverage/babel/packages/babel-parser/test/fixtures/es2018/object-rest-spread/5/input.js`
Original:

z = {x, ...y}

Transformed:

import { default as _objectSpread } from "@babel/helpers/lib/helpers/objectSpread2.js";
z = _objectSpread({ x }, y);

[Dunqing]

Oh, this plugin used @babel/helpers, we may hold this until #4753 has a conclusion.

[Dunqing]

https://github.com/babel/babel/blob/f42e24d6451d333052900e1ae073e4bc5d484cca/packages/babel-plugin-transform-object-rest-spread/test/fixtures/assumption-pureGetters/spread-single-call/output.js#L1-L7

In Babel's test cases, babelHelpers.objectSpread2 is used instead of the imported helpers. I should make a special return when running the tests. For example,

oxc/crates/oxc_transformer/src/es2018/object_spread.rs

Lines 138 to 159 in 91e2652

	fn get_extend_object_callee(&mut self, ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
	if self.options.set_spread_properties {
	Self::object_assign(ctx)
	} else {
	let ident = if self.ctx.source_type.is_module() {
	self.add_import_statement(
	"_objectSpread",
	"@babel/helpers/lib/helpers/objectSpread2.js".into(),
	ctx,
	)
	} else {
	self.add_require_statement(
	"_objectSpread",
	"@babel/helpers/lib/helpers/objectSpread2.js".into(),
	false,
	ctx,
	)
	};
	ctx.ast.expression_from_identifier_reference(ident.create_read_reference(ctx))
	}
	}

         Expression::from(ctx.ast.member_expression_static(SPAN, object, property, false))
     }
 
+    #[cfg(feature = "external_helper")]
+    fn babel_external_helper(ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
+        let ident_object =
+            ctx.create_reference_id(SPAN, ctx.ast.atom("babelHelpers"), None, ReferenceFlags::Read);
+        let object = ctx.ast.expression_from_identifier_reference(ident_object);
+        let property = ctx.ast.identifier_name(SPAN, "_objectSpread");
+
+        Expression::from(ctx.ast.member_expression_static(SPAN, object, property, false))
+    }
+
     fn get_extend_object_callee(&mut self, ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
+        if cfg!(feature = "external_helper") {
+            return Self::babel_external_helper(ctx);
+        }
+
         if self.options.set_spread_properties {
             Self::object_assign(ctx)
         } else {

What do you think?

We're not sure how we're going to solve #4753. So let's make it as simple as possible, so we'll just return babel_external_helper

[overlookmotel]

Can I just check: Are we running the conformance tests from this Babel plugin? If so, they're all passing - in that case, bravo! With the introduction of the transform semantic checker, that's no mean feat.

[magic-akari]

Can I just check: Are we running the conformance tests from this Babel plugin? If so, they're all passing - in that case, bravo! With the introduction of the transform semantic checker, that's no mean feat.

I'm not sure about the workings of conformance testing.
However, I think that achieving a 100% pass is something wrong.
This is because there's an unimplemented part of the plugin.
It implies that some parts of the test code may not be working as expected.

[overlookmotel]

I'm not sure about the workings of conformance testing.

Me neither! @Dunqing Can you advise?

[magic-akari]

I've caught it. It's been additionally disabled here

oxc/tasks/transform_conformance/src/constants.rs

Lines 66 to 68 in 3fa8635

	"transform-modules-commonjs",
	"transform-object-rest-spread",
	"transform-optional-chaining",

[overlookmotel]

OK great. Now how do we disable just the rest-related tests? Or maybe we don't? @Dunqing?

[overlookmotel]

I've made a bunch of comments. I hope they're useful.

As I said in one of them, please don't feel the need to address everything immediately. The priority is to get the tests passing first.

[Dunqing]

OK great. Now how do we disable just the rest-related tests? Or maybe we don't? @Dunqing?

These disabled plugins are used in other plugins. i.e. when we develop the transform-typescript plugin, and the transform-object-rest-spread plugin is not supported yet, so we don't care about tests where transform-typescript and transform-object-rest-spread are both enabled.

Now, we can remove transform-object-rest-spread because we are doing support this plugin right now

[overlookmotel]

I'm not sure if I was clear. This PR adds support for the spread part of the transform-object-rest-spread but the rest part is left as a TODO. So should we:

1. Enable all the tests for this plugin, and the rest-related tests will show as failing in snapshot?
2. Disable the rest-related tests, so only the spread-related tests run (and should all pass)?

[Dunqing]

1. Enable all the tests for this plugin, and the rest-related tests will show as failing in snapshot?

Yes

2. Disable the rest-related tests, so only the spread-related tests run (and should all pass)?

This depends on whether the spread part is fully completed or not

[Dunqing]

This PR is now large, and there will always be conflicts. I think we should merge first and then iterate for improvement. @overlookmotel What do you think?

Merge conflicts now resolved.

[overlookmotel]

This PR is now large, and there will always be conflicts. I think we should merge first and then iterate for improvement.

Agreed. I've merged latest main. If CI passes, I'll merge it.

[overlookmotel]

Thanks @magic-akari for your work on this. Sorry it's taken a while to get merged.

[graphite-app]

Merge activity

• Aug 28, 11:56 AM EDT: The merge label 'merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Aug 28, 11:56 AM EDT: overlookmotel added this pull request to the Graphite merge queue.
• Aug 28, 12:01 PM EDT: overlookmotel merged this pull request with the Graphite merge queue.