Comments on types are stripped

[cdaringe]

Describe the bug

Comments are stripped due to unknown reasons, against expectation.

Input code

// FILE bad.ts:

/* istanbul ignore next */
type Z = number;
x = 1;


// FILE good.ts
/* istanbul ignore next */
x = 1;

Config

{
  "jsc": {
    "transform": {}
  },
  "swcrc": true
}

Playground link

https://play.swc.rs/?version=1.2.117&code=H4sIAAAAAAAAAzXMOwrAIBBF0d5VvFoCklqykXQahjigY4hjPruPTdpz4ToLbhok9gzepZ4EoUdhnXEO%2Bh6EFQukl0inx7AuWy2FRKGJGzILTbiDbmkA4Y8h1otwBeGWzDMWszcfPlBa620AAAA%3D&config=H4sIAAAAAAAAA6tWys3My0yrVLJKS8wpTtVRyipOVrKqVipILCpOLQKxiivzShIrlKyUSioLUouTizILSpRqa2sBrzFAMDkAAAA%3D

Expected behavior

bad.ts should emit:

/* istanbul ignore next */
x = 1;

but does emit:

x = 1;

dropping the istanbul comment

Version

1.2.117

Additional context

No response

[kdy1]

Seems like the comment is on type alias and the type node is removed.

[kdy1]

@kwonoj Any thoughts? Do you think we should preserve the comment on type nodes?

[kwonoj]

In my opinion this is somewhat undefined behavior, there's no clear expectation.

i.e tsc strips comment out

while babel preserves it. If we're aiming for babel's model probably better to mimic babel's behavior but even in that case I'd consider this is improvement, not a bug.

[cdaringe]

TS has a compiler flag, removeComments, to control the behavior

[kdy1]

I think removal of comment is expected, because the comment is on type definition.

[kwonoj]

@cdaringe That doesn't apply for this case, what @kdy1 says is correct. Even if you set removeComments: false, tsc strips comments along with type definition: https://www.typescriptlang.org/play?removeComments=false#code/PQKgBAlgzgLghgOwEYFcA2kDmCD2AnAUzAQIA8YwRgAoGATwAciAtMAXmJQFskC8BuamgIVS7MAEZ+QA

Locally verified it as well.

My take is still same, I'd consider this as improvement if we want to support.

[dpchamps]

It's worth mentioning that currently the behavior is not 1:1, take a look at @kwonoj's example but with a newline: https://www.typescriptlang.org/play?&removeComments=false#code/PQKgBAlgzgLghgOwEYFcA2kDmCD2AnAUzAQIA8YwRgAoamATwAciAtMAXmJQFskC8A3NTQEKpDmACMAoA

This emits the comment as expected, however https://play.swc.rs/?version=1.2.115&code=H4sIAAAAAAAAA9PXUsgsLknMSyrNUchMz8svSlXIS60oUdDS5%2BIqqSxIVYhSsFXIK81NSi2y5spJLVGoAPINrQFh8abFNwAAAA%3D%3D&config=H4sIAAAAAAAAA6tWys3My0yrVLJKS8wpTtVRyipOVrKqVipILCpOLQKxiivzShIrlKyUSioLUouTizILSpRqa2sBrzFAMDkAAAA%3D does not.

[kwonoj]

Yes, that's known - while issue is different, this comments explains it:

microsoft/TypeScript#26079 (comment)

Comments at the beginning of a line followed by a new line are considered "unattached", and these are preserved,

While SWC doesn't strictly distinguish comment attachment in this case.

But also, if desire is let comment directive to work for given statement for this specific case, since actual line's already strippted not sure what those comment supposed to do here.

[DawChihLiou]

Just to provide a visual to @kwonoj 's comment. Please see this TS Playground.

let y  =  3;

/* istanbul ignore next */

type Z = number;
let x = 1;

is compiled into:

"use strict";
let y = 3;
let x = 1;

The comment above the type statement is stripped.

[joshyboyrules]

Has anyone found a workaround for this? Is there a setting on .swcrc config or .tsconfig that can prevent /* instabul ignore next*/ type comments from being stripped?

We're trying to migrate from ts-jest to @swc/jest and we have 100% coverage requirement... 🙏 Any help much appreciated.

cc: @kwonoj @kdy1

[cdaringe]

@dpchamps wrote a jest plugin that tests for the comment, calls into this swc/jest, then reinjects the comment before handing off jest. it's imperfect, but gets us most of the way there

[joshyboyrules]

@dpchamps wrote a jest plugin that tests for the comment, calls into this swc/jest, then reinjects the comment before handing off jest. it's imperfect, but gets us most of the way there

Can you link me to the plugin? 🙏

@dpchamps @cdaringe

[dpchamps]

workaround

// wrap-swc-transformer.js
const { createTransformer: createSwcTransformer } = require("@swc/jest");

const FILE_LEVEL_IGNORE = "\n/* istanbul ignore file */";
const istanbulFileLevelMatcher = /(istanbul ignore) (?!.*(next|branch|if|else))/gm;
/**
 * search the contents of the file for a file-level istanbul-ignore
 * e.g. `istanbul ignore file`
 * if found, append the ignore text to the bottom of the file
 **/
const wrapTransformer = (swcOptions) => {
    const swcTransformer = createSwcTransformer(swcOptions);
    return {
        ...swcTransformer,
        process: (src, filename, jestOptions) => {
            const result = swcTransformer.process(src, filename, jestOptions);
            if( src.match(istanbulFileLevelMatcher) ){
                result.code += FILE_LEVEL_IGNORE;
                console.log(result.code)
            }

            return result;
        },
    };
};

module.exports = {
    createTransformer: wrapTransformer
}

//jest config

transform: {
  "^.+\\.[tj]sx?$": [
    "./wrap-swc-transformer.js",
    {
      sourceMaps: true,
      jsc: {
        parser: {
          syntax: "typescript",
        },
        transform: {},
        target: "es5",
      },
    },
  ],
}

This is heuristic based on the observation that this issue only impacts comments attached to type nodes (and import nodes depending on what you're compiling for).

Assuming that/* istanbul ignore <token> */ should never precede types, and if they do, they can always be moved to the appropriate position so that they're not stripped without an impact to coverage. Therefore, the only ignore valid comments that can be stripped are file-level directives. In which case, we can put them at the bottom of the file after the transformation.

@kdy1 I think that assumption is robust enough that you might consider adding something like this into the transformer. Especially if the proper fix will take time to realize.

Side note: The creator of esbuild considers problems like this "out of scope", which makes it basically infeasible for an existing project to migrate while using other tools part of the status-quo js ecosystem. I know that this coverage issue prevents projects from migrating, so having something that works out of the box will increase adoption.

[kdy1]

Special handling of istanbul ignore is out of scope while preserving comments is in the scope of the project.
I think handling tool-specific comments is a hack and I'll reject such patches.

[joshyboyrules]

Special handling of istanbul ignore is out of scope while preserving comments is in the scope of the project. I think handling tool-specific comments is a hack and I'll reject such patches.

Thanks for the quick response. I think as long as comments can be preserved then there's a workaround for this type of situation.

[dpchamps]

@kdy1 yep thats fair. It's a total hack, but this will help you @joshyudeep in the interim

[joshyboyrules]

@kdy1 yep thats fair. It's a total hack, but this will help you @joshyudeep in the interim

Thanks for the suggestion.. Looks like your suggested wrapper only deals with /* istanbul ignore file */ comments, how would you go about handling /* istanbul ignore next/else/if */ comments? Looks like .swcrc compiler strips all kinds of comments during transformation. @dpchamps

Much appreciate the quick response 👍

[joshyboyrules]

@kdy1 I'm assuming there's currently not a config option as a workaround for this (disabling removing of comments). Do you have a rough estimate as to when this functionality would be worked on / released? Trying to see whether it'd be worth the time to come up with my own custom solution in the interim.

Thanks! Also, much appreciate all that you (swc contributors) have put into this project, our test suites are running 5x faster with @swc/jest.

[kwonoj]

I'd suggest to go with workaround. We'll eventually have this but there's no promised timeline or eta to provide.

[dpchamps]

@joshyudeep, sorry my explanation probably wasn't clear 😅 :

Comments are only dropped when they precede types and import statements. Types and import statements don't impact coverage, therefore/* istanbul ignore <token> */ comments shouldn't ever precede a type or an import statement. If they are, they can be adjusted to precede the particular code block you're actually trying to omit from coverage.

That leaves you with one remaining case: file-level istanbul directives. That's what this workaround solves.

[joshyboyrules]

Comments are only dropped when they precede types and import statements.

@dpchamps thanks for the follow up -- I don't think I completely follow cause I don't think the above assumption is true. Check out this playground where all comments are stripped out except the last comment that's at the EOF.

It looks like you're solution that you suggested basically just checks for istanbul comments, and if one of those comment type exists then slap a /* istanbul ignore file */ at the EOF. So it's a pretty heavy handed approach cause it treats if|else|next comments as ignore file.

Is there something I'm missing?

[dpchamps]

@joshyudeep

Yes that would be prettyheavy handed 😂. But:

Is there something I'm missing?

note the negative look ahead :)

So, thats to say "if one file global directive is found, then the whole file will ignored regardless of individual comments". That's inline with expectations.

also, compiler settings matter

lmk if that still isn't clear happy to help

[kdy1]

Comments are not removed. Those are simply not printed because the node which the comments are for is not printed as it's removed.

[kdy1]

Also, I don't have any estimate and I don't think I will work on this.

[dpchamps]

@kdy1 , yes -- it's clear to me why comments are sometimes not emitted in the final output. Which I'll summarize here. Please correct me if I'm wrong:

• Preserving comments is not part of the ECMA spec, they don't get AST nodes
• SWC associates comments with ast nodes in order to preserve them through the compilation process
• In the event where the AST node is dropped during compilation, the comment is also dropped.

I actually have some interest in working on a holistic solution for this. Would you be open to a patch? I have some PTO coming up, and am already pretty familiar with the SWC codebase.

[kdy1]

I'm open to patch, but please add a flag to enable moving of comments.

I think preserving comments of dropped nodes is counter-intuitive

[longlho]

would it be possible to just capture comments as a list of CommentLine nodes as babel does it?

[kdy1]

@longlho No

[longlho]

is that a technical limitation or resourcing limitation?

[kdy1]

What's resourcing limitation?

[kdy1]

It's technical,

[longlho]

To be clear I'm not specifically asking for emitting/preserving comment nodes, but rather parsing them into AST so plugins can use them for static analysis/directives.

[kdy1]

It's not possible and I'll reject PRs doing so

[longlho]

Can you elaborate? I'm reading through the thread and wasn't able to see the technical limitations other than lack of spec

[kdy1]

Adding a field for comment to all nodes is simply not acceptable.

[longlho]

hmm no that's not what I meant. I meant like https://astexplorer.net/#/gist/5523e56fe73439101ec0d38e43734dd7/9d2d9e5dea8587085ada67d1a731785783eb7cd4 where comments is a field on File node that has a list of CommentLines

[kdy1]

It will make writing transforms much harder, so it's no anyway.

[longlho]

Can you explain that point? Assuming comment transformation/emit is still unsupported what's the complexity in just parsing?

[kdy1]

I mean lifetime will be headachr if comments are stored in Module type.

[longlho]

Can you clarify that? That's how babel does it so there is 1 reference implementation.
I think the bottom line is that comments need to be captured so the decision is whether you wanna attach to a node the same way TS does it or just combining all of them and store it at a top level like babel.
The former has a lot of complexity and the latter seems like a good stepping stone. Right now your AST is also technically a private API so there's no compatibility SLA here. Therefore I'm not sure what "lifetime" means here.

[coderaiser]

Would be amazing to have comments in AST tree. I'm working on swc-to-babel, and want to use it in 🐊Putout linter, and if some file in source tree has comments, they need to be preserved 🤷‍♂️. Someone put comments there for a reason.

Here is example of Babel:

{
  "type": "File",
  "start": 0,
  "end": 29,
  "loc": {
    "start": {
      "line": 1,
      "column": 0
    },
    "end": {
      "line": 3,
      "column": 1
    }
  },
  "errors": [],
  "program": {
    "type": "Program",
    "start": 0,
    "end": 29,
    "loc": {
      "start": {
        "line": 1,
        "column": 0
      },
      "end": {
        "line": 3,
        "column": 1
      }
    },
    "sourceType": "module",
    "interpreter": null,
    "body": [
      {
        "type": "FunctionDeclaration",
        "start": 9,
        "end": 29,
        "loc": {
          "start": {
            "line": 2,
            "column": 0
          },
          "end": {
            "line": 3,
            "column": 1
          }
        },
        "id": {
          "type": "Identifier",
          "start": 18,
          "end": 23,
          "loc": {
            "start": {
              "line": 2,
              "column": 9
            },
            "end": {
              "line": 2,
              "column": 14
            },
            "identifierName": "world"
          },
          "name": "world"
        },
        "generator": false,
        "async": false,
        "params": [],
        "body": {
          "type": "BlockStatement",
          "start": 26,
          "end": 29,
          "loc": {
            "start": {
              "line": 2,
              "column": 17
            },
            "end": {
              "line": 3,
              "column": 1
            }
          },
          "body": [],
          "directives": []
        },
        "leadingComments": [
          {
            "type": "CommentLine",
            "value": " hello",
            "start": 0,
            "end": 8,
            "loc": {
              "start": {
                "line": 1,
                "column": 0
              },
              "end": {
                "line": 1,
                "column": 8
              }
            }
          }
        ]
      }
    ],
    "directives": []
  },
  "comments": [
    {
      "type": "CommentLine",
      "value": " hello",
      "start": 0,
      "end": 8,
      "loc": {
        "start": {
          "line": 1,
          "column": 0
        },
        "end": {
          "line": 1,
          "column": 8
        }
      }
    }
  ]
}

And swc:

{
  "type": "Module",
  "span": {
    "start": 0,
    "end": 25,
    "ctxt": 0
  },
  "body": [
    {
      "type": "FunctionDeclaration",
      "identifier": {
        "type": "Identifier",
        "span": {
          "start": 9,
          "end": 14,
          "ctxt": 0
        },
        "value": "world",
        "optional": false
      },
      "declare": false,
      "params": [],
      "decorators": [],
      "span": {
        "start": 0,
        "end": 25,
        "ctxt": 0
      },
      "body": {
        "type": "BlockStatement",
        "span": {
          "start": 17,
          "end": 25,
          "ctxt": 0
        },
        "stmts": []
      },
      "generator": false,
      "async": false,
      "typeParameters": null,
      "returnType": null
    }
  ],
  "interpreter": null
}

Almost everything can be converted, but where should I get comments from 🤷‍♂️?

[kdy1]

@coderaiser

swc to babel already exists.
See https://github.com/swc-project/swc/tree/bfada04b338a70323eb205ee147a2157d328ad46/crates/swc_estree_compat

[kdy1]

Currently there's no js api to get comments

[coderaiser]

swc to babel already exists.
See https://github.com/swc-project/swc/tree/bfada04b338a70323eb205ee147a2157d328ad46/crates/swc_estree_compat

Nice :)! How can I use it from js?

[coderaiser]

Currently there's no js api to get comments

OK, but is it possible to use it (event if without comments) from @swc/core, for example, with a method parseSync:

const {parseSync} = require('@swc/core');
const ast = parseSync(`const hello = 'world'`);
const babelAst = swcToBabel(ast);

Or even:

const {parseSync, swcToBabel} = require('@swc/core');
const ast = parseSync(`const hello = 'world'`, {
    babel: true,
});

swc to babel already exists.
See https://github.com/swc-project/swc/tree/bfada04b338a70323eb205ee147a2157d328ad46/crates/swc_estree_compat

Why does it called swc_estree_compat? Is it related to Babel AST or ESTree AST or both :)?

The thing is they are quite different and that's why estree-to-babel exist.

[kdy1]

It can emit acorn ast/babel ast

[coderaiser]

It can emit acorn ast/babel ast

That’s awesome :)!
Could you please show me an example of

• ✅parsing source to SWC AST;
• ✅converting SWC AST to acorn and Babel;

[swc-bot]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.