feat(swc): Add comment preservation #3815

dpchamps · 2022-03-02T05:52:20Z

Problem

Generated code will drop comments in a variety of situations. Some of these situations may include:

Comments that precede types
Comments that precede / trail nodes that are transformed / moved / removed

The JS Ecosystem is (unfortunately) reliant on maintaining comment continuity between source and compiled code. There are many existing tools that use comments as annotations for various functionality, the most entrenched probably being istanbul coverage annotations. Not maintaining good parity between source and compiled output causes adopting SWC for test runners to be non-tenable for a lot of customers.

Solution

Implement a "comment fixer" visitor, dropped_comments_preserver, and introduce a feature flag preserve_dropped_comments into the jsc configuration object.

Overview of changes

Given the way SWC internals track comments, the comment fixer makes a best effort to preserve comments that may have otherwise been dropped. The output may not be 1:1 with what babel or other transpilers output, but it will preserve all expected functionality from existing js tooling where possible.

When preserve_dropped_comments is enabled, all trailing and leading comments will be shifted to the first non-dummy span that exists in the ast. This guarantees that all existing comments will be transported to the compiled output, orphaned comments will precede comments that would otherwise be preserved, and comments that would be preserved remain in a consistent position.

I tried my best to organize things as I thought they should be according to the current standards of the repo. Open to any and all suggestions 🙏 would really like to support this feature as it is currently blocking us over here at Walmart from adopting the SWC jest transformer fully. The gains are too good to ignore.

BREAKING CHANGE:

**Related issue (if exists): #2964 **

Closes #2964

dpchamps · 2022-03-02T06:04:09Z

crates/swc_ecma_transforms_base/src/dropped_comments_preserver.rs

+    noop_visit_mut_type!();
+
+    fn visit_mut_span(&mut self, span: &mut Span) {
+        if span.is_dummy() || self.is_first_span {


When rearranging comments, we have to stop and think what we should do with the module / script span. A comment at the top could be dropped, could be preserved in place, or could be transported to the next available span. Example of this ambiguity:

On the one hand, I could have something like this:

/* Some doc block, this very clearly should remain at the top of the file */ type X = number; ...

Or I could have something like this:

/* istanbul ignore next-line */ type X = number; const somethingFancy = (...x) => x;

Because during transpilation, code such as the second example will be shifted down to support utility functions, I opted to solve this ambiguity by assuming that the comment must be moved to the next nearest existing span.

This way at the very minimum tooling is supported.

kdy1

Code changes looks fine, but I think we need some tests to ensure that we does not use comments of other modules.

Can you add some tests js test suite at https://github.com/swc-project/swc/tree/ce78344a7c3597559ef32b0ee727dc0c7512c492/node-swc/__tests__ ?

Also, please fix the clippy errors.

dpchamps · 2022-03-03T07:30:14Z

@kdy1 happy to add the tests, will start early tomorrow pst.

Could you help me understand what you mean by "use comments of other modules"?

Lets say I call transformSync(someSource, config) with config including preserveDroppedComments: true

I'd expect to see all comments within someSource to be preserved. Is there a case where there's potentially other comments that might get included from somewhere else? I'm having a hard time envisioning it, but I'm certain you're calling out an important edge case.

I'll also update the node-swc config types in the follow-up commit.

Thanks!

kdy1 · 2022-03-03T08:54:00Z

SWC has a global storage for comments, and the storage is shared among files. And the storage uses BytePos to store/get comments.
The problem I'm worrying about is BytePos used incorrectly so the storage takes comments of other module, or stores comments in other module.

This may change in the future, but for now, we have to ensure that BytePos is used correctly.

magic-akari · 2022-03-03T09:37:24Z

crates/swc/src/config/mod.rs

@@ -1021,6 +1028,9 @@ pub struct JscConfig {

    #[serde(default)]
    pub lints: LintConfig,
+
+    #[serde(default)]
+    pub preserve_dropped_comments: bool,


is it necessary?
I suppose swc should always preserve comments.
And It's ok to remove comments at the minify phase.

It's because this patch tries to preserve comments on dropped nodes.

Btw, I think preserve_all_comments is better name. (Even if it does not preserve all comments)

@magic-akari I added this because it was requested here: #2964 (comment)

@kdy1 : agreed, see 3ce4af7

crates/swc/src/config/mod.rs

dpchamps · 2022-03-03T23:49:39Z

@kdy1 lmk if this is what you had in mind: https://github.com/swc-project/swc/blob/8b971afa46f9359b7b7ac8fa5d843b35f3de4dd0/node-swc/__tests__/preserve_comments.mjs

Clippy errors fixed, unmarking this as a draft and requesting another pass. thanks!

kdy1 · 2022-03-03T23:51:28Z

Yes, that's what I meant.
Thank you!

kdy1

I'm not sure about the way to fix this, so I'll think about it more.

kdy1 · 2022-03-04T02:04:35Z

crates/swc_ecma_transforms_base/src/dropped_comments_preserver.rs

+        let mut leading_comments = Vec::new();
+        let mut trailing_comments = Vec::new();
+
+        for idx in (self.comment_position..=span.lo.0).map(BytePos) {


Oh, this will be extremely slow.

It removes whole advantage of storing comments in a shared hash map.

@kdy1 Sorry, it was an extremely busy day for me. I'm just now sitting down and thinking about how to make this more this more performant.

Reviewing the discord convo from last night, your suggestion to use SingleThreadedComments is unclear to me. I don't see a way to depart from the SwcComments data structure that is used top-level in Compiler without replacing the compiler type, because this needs to be implemented as a pass baked into existing compiler.

Instead, I think we need to pull hashing operations out of the visitor entirely.

This seems to be a more appropriate way to solve this problem:

Collect all candidate spans with a visitor

iterate exactly once over the comments entries, shifting orphaned comments to candidate spans

I understand the reasons that iterating over the concurrent hashmaps is undesirable, but reducing that iteration to one time outside of the visitor sounds like an acceptable concession. Do you agree?

If not, could you provide some guidance on how exactly you intend single threaded comments to be used.

because this needs to be implemented as a pass baked into existing compiler

No, it should not and it does not have to.
It will used inside fn transform and it will be dropped once fn transform() is finished.

dpchamps · 2022-03-09T04:57:26Z

@kdy1 addressed the performance concerns you had by reworking the approach to comment shifting. Also followed the general feedback from our discord chat.

The summary of the comment shifting refactor is:

Use SingleThreadedComments
Shift known comment positions to known spans only, no unnecessary checks

Otherwise, moved dropped_comments_preserver into the swc crate as per your suggestion.

Unmarking as draft and requesting another pass, thanks a ton!

kdy1

Thank you!

kdy1 · 2022-03-11T06:22:29Z

crates/swc/src/dropped_comments_preserver.rs

+            let (comments_to_move, next_byte_positions): (CommentEntries, CommentEntries) =
+                existing_comments
+                    .drain(..)
+                    .partition(|(bp, _)| bp <= &span.lo);


Please use *bp <= span.lo instead

kdy1 · 2022-03-11T06:22:51Z

crates/swc/src/dropped_comments_preserver.rs

+/// comments.
+
+pub fn dropped_comments_preserver(
+    comments: Option<&SingleThreadedComments>,


I think we don't need a lifetime here.

Comments are cheap to clone, and it's part of the api.

kdy1 · 2022-03-11T06:24:09Z

crates/swc/src/dropped_comments_preserver.rs

+    /// similar location
+    fn shift_trailing_comments(&mut self, remaining_comment_entries: CommentEntries) {
+        let last_trailing =
+            self.known_spans.iter().fold(


Please use .iter().copied() instead.
Span is Copy, so we don't need indirection here.

dpchamps · 2022-03-11T07:02:35Z

Feedback addressed 7b3d8e3

kdy1

Thank you!

swc-bump:

swc --breaking

Docs update following swc-project/swc#3815 Will be valid post `v1.2.153` milestone release

dpchamps changed the title ~~Issue 2964 dpchamps~~ feat(swc): Add comment preservation Mar 2, 2022

dpchamps commented Mar 2, 2022

View reviewed changes

kdy1 requested changes Mar 3, 2022

View reviewed changes

kdy1 marked this pull request as draft March 3, 2022 06:46

magic-akari reviewed Mar 3, 2022

View reviewed changes

dpchamps force-pushed the issue-2964-dpchamps branch from 986242e to 8b971af Compare March 3, 2022 23:45

dpchamps commented Mar 3, 2022

View reviewed changes

crates/swc/src/config/mod.rs Show resolved Hide resolved

dpchamps marked this pull request as ready for review March 3, 2022 23:51

kdy1 requested changes Mar 4, 2022

View reviewed changes

kdy1 marked this pull request as draft March 5, 2022 04:13

dpchamps force-pushed the issue-2964-dpchamps branch from 8b971af to 891a316 Compare March 9, 2022 04:55

dpchamps marked this pull request as ready for review March 9, 2022 04:59

dpchamps added 13 commits March 8, 2022 20:59

add base case test fixture

129e234

feat(swc): Add comment preservation

ce0acd5

simplify comment preserver visitor

cf6d926

add newlines to fixture files

69e21f1

fix(swc): default preserve comments when true. Exercise more full .swcrc

fdfe293

feat(node-swc): Add configuration, test comment preservation

cfb58dc

run clippy --fix

c46a4e3

rename fixture folder

2453f89

rename to preserveAllComments

27774f8

format

479eba2

update test fixtures for swc helper integration

a78cdca

feat(swc): Optimize comment shifter

4a5d460

fix: migrate comment fixer to swc lib per PR feedback

bcd52f0

dpchamps force-pushed the issue-2964-dpchamps branch from 891a316 to bcd52f0 Compare March 9, 2022 05:00

dpchamps added 3 commits March 8, 2022 22:03

update shift_trailing_comments doc block

f49b93a

make trailing comment doc block more clear

13bc92b

no need to pass around mutable state

08d501e

kdy1 requested changes Mar 11, 2022

View reviewed changes

PR feedback

7b3d8e3

kdy1 approved these changes Mar 11, 2022

View reviewed changes

Merge branch 'main' into issue-2964-dpchamps

0b1a159

kdy1 added this to the v1.2.153 milestone Mar 11, 2022

kdy1 enabled auto-merge (squash) March 11, 2022 07:06

kdy1 merged commit c5a0c9a into swc-project:main Mar 11, 2022

dpchamps added a commit to dpchamps/website that referenced this pull request Mar 11, 2022

feat(docs): Add preserveAllComments feature flag documentation

e72b5d3

Docs update following swc-project/swc#3815 Will be valid post `v1.2.153` milestone release

dpchamps mentioned this pull request Mar 11, 2022

feat(docs): Add preserveAllComments feature flag documentation swc-project/website#148

Merged

kdy1 pushed a commit to swc-project/website that referenced this pull request Mar 12, 2022

feat(docs): Add preserveAllComments feature flag documentation (#148)

697e79e

Docs update following swc-project/swc#3815 Will be valid post `v1.2.153` milestone release

bestlucky0825 pushed a commit to bestlucky0825/swc-website that referenced this pull request Jun 1, 2022

feat(docs): Add preserveAllComments feature flag documentation (#148)

e213c21

Docs update following swc-project/swc#3815 Will be valid post `v1.2.153` milestone release

swc-project locked as resolved and limited conversation to collaborators Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(swc): Add comment preservation #3815

feat(swc): Add comment preservation #3815

dpchamps commented Mar 2, 2022 •

edited by kdy1

Loading

dpchamps Mar 2, 2022 •

edited

Loading

kdy1 left a comment

dpchamps commented Mar 3, 2022 •

edited

Loading

kdy1 commented Mar 3, 2022

magic-akari Mar 3, 2022 •

edited

Loading

kdy1 Mar 3, 2022

dpchamps Mar 3, 2022

dpchamps commented Mar 3, 2022

kdy1 commented Mar 3, 2022

kdy1 left a comment

kdy1 Mar 4, 2022

kdy1 Mar 4, 2022

dpchamps Mar 5, 2022 •

edited

Loading

kdy1 Mar 5, 2022

dpchamps commented Mar 9, 2022 •

edited

Loading

kdy1 left a comment

kdy1 Mar 11, 2022

kdy1 Mar 11, 2022

kdy1 Mar 11, 2022

kdy1 Mar 11, 2022

dpchamps commented Mar 11, 2022

kdy1 left a comment

feat(swc): Add comment preservation #3815

feat(swc): Add comment preservation #3815

Conversation

dpchamps commented Mar 2, 2022 • edited by kdy1 Loading

Problem

Solution

Overview of changes

dpchamps Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

kdy1 left a comment

Choose a reason for hiding this comment

dpchamps commented Mar 3, 2022 • edited Loading

kdy1 commented Mar 3, 2022

magic-akari Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpchamps commented Mar 3, 2022

kdy1 commented Mar 3, 2022

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpchamps Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpchamps commented Mar 9, 2022 • edited Loading

kdy1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpchamps commented Mar 11, 2022

kdy1 left a comment

Choose a reason for hiding this comment

dpchamps commented Mar 2, 2022 •

edited by kdy1

Loading

dpchamps Mar 2, 2022 •

edited

Loading

dpchamps commented Mar 3, 2022 •

edited

Loading

magic-akari Mar 3, 2022 •

edited

Loading

dpchamps Mar 5, 2022 •

edited

Loading

dpchamps commented Mar 9, 2022 •

edited

Loading