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

fix(es/compat): Fix loose mode of the spread pass #7608

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

lsdyi
Copy link
Contributor

@lsdyi lsdyi commented Jun 30, 2023

@CLAassistant
Copy link

CLAassistant commented Jun 30, 2023

CLA assistant check
All committers have signed the CLA.

@lsdyi
Copy link
Contributor Author

lsdyi commented Jun 30, 2023

@kdy1 Sorry to push code without tests. I'm new to this repo🥹, kind of confused where I can complete whole testing process. I noticed we can write input/index.js and inpuit/.swcrc which I think are input example, but I'm confused how I can generate output, which is output/index.js

@kdy1
Copy link
Member

kdy1 commented Jun 30, 2023

You can do UPDATE=1 cargo test

@kdy1 kdy1 added this to the Planned milestone Jun 30, 2023
@kdy1
Copy link
Member

kdy1 commented Jun 30, 2023

Also, can you sign the CLA?

@kdy1
Copy link
Member

kdy1 commented Jul 3, 2023

CI is failing. You can update snapshots with UPDATE=1 cargo test

@lsdyi lsdyi closed this Jul 3, 2023
@lsdyi
Copy link
Contributor Author

lsdyi commented Jul 3, 2023

@kdy1 Thank you for the reminder :) I'm going to working on this pr today. Updating the Snapshots.

@lsdyi lsdyi reopened this Jul 3, 2023
@lsdyi lsdyi force-pushed the issue-7354 branch 2 times, most recently from bb8dc90 to 7af6eb3 Compare July 4, 2023 08:43
@lsdyi
Copy link
Contributor Author

lsdyi commented Jul 4, 2023

Hello @kdy1 , I update the testing file and snapshots. Could you please help me check the code?

@kdy1 kdy1 self-assigned this Jul 5, 2023
for arg in args {
if let Some(arg) = arg {
let ExprOrSpread { expr, spread } = arg;
arg_list.push(expr.clone().as_arg());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clone seems redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exprused without clonemethod will cause use of moved value error in next *expr location.


Error stack is as follows.

error[E0382]: use of moved value: `*expr`
   --> crates/swc_ecma_transforms_compat/src/es2015/spread.rs:286:34
    |
258 |                 arg_list.push(expr.as_arg());
    |                                    -------- `*expr` moved due to this method call
...
286 |                         buf.push(match *expr {
    |                                  ^^^^^^^^^^^ value used here after move
    |
    = note: move occurs because `*expr` has type `swc_ecma_ast::Expr`, which does not implement the `Copy` trait
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
258 |                 arg_list.push(expr.clone().as_arg());
    |                                    ++++++++

For more information about this error, try `rustc --explain E0382`.
error: could not compile `swc_ecma_transforms_compat` (lib) due to previous error

$ rustc --version
rustc 1.70.0-nightly (da7c50c08 2023-03-19)

@kdy1 kdy1 removed their assignment Jul 5, 2023
@lsdyi
Copy link
Contributor Author

lsdyi commented Jul 6, 2023

Thanks for the CR yesterday @kdy1 , is there anything I can do with this PR?

@kdy1
Copy link
Member

kdy1 commented Jul 6, 2023

Please remove the redundant clone. It's removable

@lsdyi
Copy link
Contributor Author

lsdyi commented Jul 6, 2023

Good afternoon ☕️ @kdy1 I've removed the redundant clone() method and collected list elements in a specific new for loop. Could you please help me check whether the code meets expectation?

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swc-bump:

  • swc_ecma_transforms_compat

@kdy1 kdy1 changed the title fix(es/compat): change array spread syntax result on loose mode fix(es/compat): Fix loose mode of the spread pass Jul 7, 2023
@kdy1 kdy1 merged commit a7daa5b into swc-project:main Jul 7, 2023
127 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.3.69 Jul 13, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Aug 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

spread syntax differs between swc loose mode and babel loose mode
3 participants