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(cli): Use the source-file-name and source-root options #6973

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

realtimetodie
Copy link
Contributor

Adds the --source-file-name and --source-root options to allow the user to set the source file URL to the original source file and the URL root from which all sources are relative respectively.

The implementation follows the Node.js @swc/cli

https://github.com/swc-project/cli/blob/461231917181133fd2e98b954fce07b599711502/src/swc/compile.ts#L20

https://github.com/swc-project/cli/blob/461231917181133fd2e98b954fce07b599711502/src/swc/util.ts#L58-L59

Example

$ swc compile --source-maps true --source-file-name demo.ts --source-root "./foo" input.ts
...
{"version":3,"sources":["demo.ts"],"sourceRoot":"./foo","sourcesContent":["export const a: string = \"a\";\n\n"],"names":["a"],"mappings":"AAAA,OAAO,IAAMA,IAAY,IAAI"}

The source_root feature was added to the sourcemap crate in v6.2.0 (kudos @kamilogorek).

Related

@realtimetodie
Copy link
Contributor Author

CI fails due to an unrelated error

Error: Couldn't find package "@babel/code-frame@^7.18.6" required by "@babel/template@^7.20.7" on the "npm" registry.

@kdy1 kdy1 requested a review from kwonoj February 22, 2023 08:55
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.

Thank you!


swc-bump:

  • dbg-swc

bindings/swc_cli/src/commands/compile.rs Outdated Show resolved Hide resolved
bindings/swc_cli/src/commands/compile.rs Outdated Show resolved Hide resolved
@realtimetodie
Copy link
Contributor Author

I pinned the version of the sourcemap crate to 6.2 globally.

kwonoj
kwonoj previously approved these changes Feb 22, 2023
@realtimetodie
Copy link
Contributor Author

After updating the sourcemap crate for swc_common, a fixture test fails. The string of base64 VLQs which contain the actual mappings of the generated source map are different.

crates/swc/tests/fixture/sourcemap/004
- "mappings": "AAACA,CAAAA,KAAKC,gBAAmB,GAAGD,KAAKC,gBAAmB,IAAI,EAAC,AAAD,EAAIC,IAAI,CAAC;IAC7D;QACI;KACH;IACD;QACU,MAAY,SAAUC,CAAuB,EAAEC,CAAmB,EAAEC,CAAmB,EAAE;YAC3F;YACA,IAAIC,IAAa,SAAoBC,CAAI,EAAE;gBACvC,IAAIC,IAAOD,EAAKC,IAAI;gBACpB,OAAsB,CAAA,GAAGC,EAA+CC,GAAE,AAAFA,EAAK,OAAO;oBAChFC,UAAUH,EAAKI,GAAG;gBACtB;YACJ;YACAP,EAAoBQ,CAAC,CAACT,IACDC,EAAoBS,CAAC,CAACV,GAAqB;gBACvCW,SAAS,WAAoB;oBAC9C,OAAqBA;gBACzB;gBACqBC,SAAS,WAAoB;oBAC9C,OAAqBV;gBACzB;YACJ,EAAA;YACqB,IAAIG,IAAiDJ,EAAoB,OAC1FU,IAAU,CAAA;QAE1B;QACc,MAAY,SAAUZ,CAAuB,EAAEc,CAAwB,EAAEZ,CAAmB,EAAE;YAC/Fa,CAAAA,OAAOC,QAAQ,GAAGD,OAAOC,QAAQ,IAAI,EAAC,AAAD,EAAIjB,IAAI,CAAC;gBAC3C;gBACA,WAAY;oBACR,OAAOG,EAAoB;gBAC/B;aACH;QAGb;IACI;IACS,SAAUA,CAAmB,EAAE;QAK3BA,EAAoBe,CAAC,CAAC,GAAG;YAC9B;YACA;YACA;SACH,EAAE,WAAY;YACX,OAPOf,EAAoBA,EAAoBgB,CAAC,GAOxB;QAC5B,IAESC,OAD0BjB,EAAoBe,CAAC,EAAA;IAGhE;CACC",
+ "mappings": "AAACA,CAAAA,KAAKC,gBAAmB,GAAGD,KAAKC,gBAAmB,IAAI,EAAC,AAAD,EAAIC,IAAI,CAAC;IAC7D;QACI;KACH;IACD;QACU,MAAY,SAAUC,CAAuB,EAAEC,CAAmB,EAAEC,CAAmB,EAAE;YAC3F;YACA,IAAIC,IAAa,SAAoBC,CAAI,EAAE;gBACvC,IAAIC,IAAOD,EAAKC,IAAI;gBACpB,OAAqB,AAAC,CAAA,GAAGC,EAA+CC,GAAE,AAAFA,EAAK,OAAO;oBAChFC,UAAUH,EAAKI,GAAG;gBACtB;YACJ;YACAP,EAAoBQ,CAAC,CAACT,IACDC,EAAoBS,CAAC,CAACV,GAAqB;gBACvCW,SAAS,WAAoB;oBAC9C,OAAqBA;gBACzB;gBACqBC,SAAS,WAAoB;oBAC9C,OAAqBV;gBACzB;YACJ,EAAA;YACqB,IAAIG,IAAiDJ,EAAoB,OAC1FU,IAAU,CAAA;QAE1B;QACc,MAAY,SAAUZ,CAAuB,EAAEc,CAAwB,EAAEZ,CAAmB,EAAE;YAC/Fa,CAAAA,OAAOC,QAAQ,GAAGD,OAAOC,QAAQ,IAAI,EAAC,AAAD,EAAIjB,IAAI,CAAC;gBAC3C;gBACA,WAAY;oBACR,OAAOG,EAAoB;gBAC/B;aACH;QAGb;IACI;IACS,SAAUA,CAAmB,EAAE;QAK3BA,EAAoBe,CAAC,CAAC,GAAG;YAC9B;YACA;YACA;SACH,EAAE,WAAY;YACX,OAPOf,EAAoBA,EAAoBgB,CAAC,GAOxB;QAC5B,IAESC,OAD0BjB,EAAoBe,CAAC,EAAA;IAGhE;CACC",

Should I revert the update of the sourcemap crate for swc_common or should I update the fixture test instead?

@kamilogorek
Copy link

kamilogorek commented Feb 22, 2023

The snapshot itself should be updated in this case.

The diff is adding an additional AAAC, which in mappings terms translates to "add 1 to a column number". This is a valid behavior due to the fix that we had to apply here getsentry/rust-sourcemap#54 and which is based on discussion here getsentry/rust-sourcemap#53

@realtimetodie
Copy link
Contributor Author

I updated the fixture test. Thank you @kamilogorek.

@realtimetodie
Copy link
Contributor Author

realtimetodie commented Feb 22, 2023

Blocked by servo/string-cache#271

@realtimetodie
Copy link
Contributor Author

Some tests fail. GitHub actions is unable to run the git checkout step: HTTP status 429. I will retry tomorrow.

unable to access 'https://github.com/swc-project/swc/': The requested URL returned error: 429

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.

Thank you!

@kdy1 kdy1 enabled auto-merge (squash) February 23, 2023 01:22
@kdy1 kdy1 added this to the Planned milestone Feb 23, 2023
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

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

Automated review comment generated by auto-rebase script

@kdy1 kdy1 disabled auto-merge February 23, 2023 02:37
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.

CI failed

@realtimetodie
Copy link
Contributor Author

realtimetodie commented Feb 23, 2023

The tests basically compare the generated source maps from the sourcemap crate with the generated source maps from the Node.js terser package

test result: FAILED. 1929 passed; 54 failed;

fn assert_eq_same_map(expected: &SourceMap, actual: &SourceMap) {

@kdy1 Is this a requirement to have parity with terser?

The reason why the tests fail seems to be related to getsentry/rust-sourcemap#53 as mentioned by @kamilogorek in #6973 (comment). Terser uses the Node.js @jridgewell/gen-mapping package to generate source maps. Was this issue in getsentry/rust-sourcemap#53 fixed in Node.js too?

swc/yarn.lock

Line 7266 in 447c1da

"@jridgewell/source-map": ^0.3.2

Ping @jridgewell short summary: this PR updates the sourcemap crate to a version that includes your PR getsentry/rust-sourcemap#53. However, some tests are failing (see above).

@kdy1
Copy link
Member

kdy1 commented Feb 23, 2023

It does not have to be same as terser. I just looked for a way to ensure that source maps are correct enough.
But not sure how to verify it without relying on some other tool or reviewing an enormous amount of source map changes.

@jridgewell Do you have any idea?

@jridgewell
Copy link
Contributor

jridgewell commented Feb 23, 2023

So the sourcemap itself hasn't changed:

[crates/swc_ecma_codegen/tests/sourcemap.rs:456] actual.tokens().collect::<Vec<_>>() = [
    <Token 017a45a1919f4006.js:0:0 (0:0)>,
    <Token 017a45a1919f4006.js:0:3 (0:2)>,
    <Token 017a45a1919f4006.js:0:10 (0:8)>,
    <Token 017a45a1919f4006.js:0:14 (0:12)>,
    <Token 017a45a1919f4006.js:0:16 (0:13)>,
    <Token 017a45a1919f4006.js:0:18 (0:14)>,
    <Token 017a45a1919f4006.js:0:27 (0:23)>,
    <Token 017a45a1919f4006.js:0:28 (0:24)>,
    <Token 017a45a1919f4006.js:0:29 (0:24)>,
]

Notice those last two tokens. They have the same generated location (0:24), but different source locations (0:28 vs 0:29). The bug we're seeing is the lookup token returning one or the other. Before, we'd see the 28, and now we're seeing the 29.

In getsentry/rust-sourcemap#54, we switched from a manual greatest-lower-bound binary search to one using Rust's binary_search_by_key. The big difference that I can see between the impls is the exact match condition: https://github.com/rust-lang/rust/blob/07c993eba8b76eae497e98433ae075b00f01be10/library/core/src/slice/mod.rs#L2529-L2532. Before, we'd continue searching even after finding an exact match, guaranteeing we'd get the lowest index exact-match. Now, we'll stop on any exact-match.

I think this is a bug in sourcemap, caused because I didn't really look up the binary search behavior when it was suggested we switch to the stdlib impl. For reference, my trace-mapping library guarantees the first/last item when tracing, Mozilla's v0.6.1 (the JS one) source-map does, but Mozilla's v0.7 (the Rust/WASM one) doesn't.

jridgewell added a commit to jridgewell/rust-sourcemap that referenced this pull request Feb 24, 2023
When we switched to Rust's `binary_search_by_key`, we accidentally changed the behavior when a token matches exactly. Before, we were guaranteed to always return the lowest index because we didn't stop iterating when we found a match. Now, we exit as soon as we find a match, which can be any matching based on the size of the slice.

Fixes swc-project/swc#6973
Swatinem pushed a commit to getsentry/rust-sourcemap that referenced this pull request Feb 24, 2023
When we switched to Rust's `binary_search_by_key`, we accidentally changed the behavior when a token matches exactly. Before, we were guaranteed to always return the lowest index because we didn't stop iterating when we found a match. Now, we exit as soon as we find a match, which can be any matching based on the size of the slice.

Fixes swc-project/swc#6973
@kamilogorek
Copy link

6.2.2 has been released, you can bump it here now

@jridgewell
Copy link
Contributor

Ah god, I think I read it in the wrong direction. The test was failing because we received 28 where before it received 29 (the opposite of what I said in #6973 (comment)). From that, I assumed the old sourcemap code was returning the lowest match index, when in fact it seems like it was returning the highest match index. I assumed that because that's what the JS sourcemap packages do.

@kamilogorek: We could revert 6.2.2 and implement the highest-index behavior to match the old 6.2.0 behavior. Or we could align with the JS ecosystem, in which case SWC would need to skip these failing tests (or stop inserting multiple tokens for the same generated position).

@kdy1
Copy link
Member

kdy1 commented Feb 24, 2023

I think it's better to align with js ecosystem, so we may inline source maps for those tests. But I think it should be done in a separate PR

@kamilogorek
Copy link

@Swatinem what do you think?

@Swatinem
Copy link

I’m not sure I fully understand the problem at hand.

stop inserting multiple tokens for the same generated position

I believe this might be the solution. We shouldn’t even get to the situation where there is more than one token matching (and picking the lowest or higher match). What would that even mean in sourcemaps that you have more than one token for one minified source position?

@realtimetodie
Copy link
Contributor Author

@kdy I created a follow-up PR in #6989 and reverted the update for the swc component crates.

bindings/swc_cli/Cargo.toml Show resolved Hide resolved
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:

  • dbg-swc

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.

Please revert the top level Cargo.lock

@realtimetodie
Copy link
Contributor Author

There were no changes in the main Cargo.lock file, the git history differed.

@kdy1 kdy1 enabled auto-merge (squash) February 27, 2023 02:36
@kdy1 kdy1 merged commit 3e3e41a into swc-project:main Feb 27, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.37 Feb 28, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Mar 30, 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.

7 participants