Retain synthetic comments on exports. #25604

mprobst · 2018-07-12T10:50:51Z

Variables that do not have a local variable created get transformed into
a single exports assignment expression. TypeScript previously just
created a new expression and set the text range to retain original
comments, but for synthetic comments, merging the emit nodes by setting
the original node is required.

Fixes #17594

mprobst · 2018-07-12T10:50:58Z

/CC @weswigham

mprobst · 2018-07-12T12:41:30Z

@weswigham I'm not 100% sure we want the changes in the last commit, i.e. retaining comments on export specifiers. The code paths setting allowComments=false (or rather, not passing it) sound as if they expect the comments to be emitted elsewhere, at least for export assignment and hoisted exports.

mprobst · 2018-07-12T15:02:15Z

Actually I take it back, emitting comments for export specifiers is not a good idea.

I'm having a really productive conversation with myself here, it seems ;-)

mhegazy · 2018-07-12T23:56:10Z

@rbuckton can you please review this change.

This pulls in the changes from microsoft/TypeScript#25604 that are required for tsickle to run purely based on transformers. This allows deleting a number of hacks in `transformer_utils.ts` on our side.

mprobst · 2018-07-16T16:45:57Z

@rbuckton I'm seeing other problems, such as comments on initialized comments being emitted, e.g. from:

class Foo {
  // comment goes here
  bar = 1;
}

If I set NoLeadingComments on bar and add a synthetic comment, I still get:

class Foo {
  constructor() {
    // my synthetic comment
    // comment goes here
    this.bar = 1;
  }
}

The cause in this case I think is: https://github.com/Microsoft/TypeScript/blob/master/src/compiler/factory.ts#L3141

The code sets the text range of the new code, but ignores emit flags such as NoComments
I'm wondering if tracking all these individual issues down is the scalable solution, or whether we should change an underlying API to make them impossible. E.g. I wonder if it's ever reasonable to setTextRange or setCommentRange without also copying the emit flags from whatever we're setting the range to.

Variables that do not have a local variable created get transformed into a single exports assignment expression. TypeScript previously just created a new expression and set the text range to retain original comments, but for synthetic comments, merging the emit nodes by setting the original node is required.

Previously, TypeScript would only set the text range when transforming import and export declarations, leading it to drop synthetic comments and emit flags. This commit sets the original node on the statements that contains the generated `require()` call (or similar, depending on module system), retaining emit flags and synthetic comments.

This pulls in the changes from microsoft/TypeScript#25604 that are required for tsickle to run purely based on transformers. This allows deleting a number of hacks in `transformer_utils.ts` on our side.

Keep emit flags set on the original node on a namespace or enum node. This prevents dropping flags like NoComments, which caused duplicated comment emits. Additionally, TypeScript would emit synthetic comments twice, once for the variable declaration, once for the module statement. This explicitly clears away synthetic comments on namespaces and enums if their synthetic comments have already been emitted on the corresponding variable statement.

mprobst · 2018-07-19T06:50:42Z

The approach that I'm taking in a7224ec now is to not consider comments "consumed" if a container node has a NoComments flag set - any child nodes contained in it will then not emit the skipped comment on their own. This passes the regression test suite and Google's internal repository, with a single change to avoid setting the text range on the captured this for super calls.

mprobst · 2018-07-20T18:43:31Z

@rbuckton friendly ping.

This pulls in the changes from microsoft/TypeScript#25604 that are required for tsickle to run purely based on transformers. This allows deleting a number of hacks in `transformer_utils.ts` on our side.

weswigham · 2018-07-24T23:32:44Z

src/compiler/transformers/ts.ts

@@ -2977,8 +2986,13 @@ namespace ts {
            );

            setOriginalNode(moduleStatement, node);
+            if (varAdded) {
+                // If a variable was added, synthetic comments are mitted on it, not on the moduleStatement.


Typo: mitted

weswigham · 2018-07-24T23:32:47Z

src/compiler/transformers/ts.ts

@@ -2689,8 +2692,13 @@ namespace ts {
            );

            setOriginalNode(enumStatement, node);
+            if (varAdded) {
+                // If a variable was added, synthetic comments are mitted on it, not on the moduleStatement.


Typo: mitted

mhegazy · 2018-07-25T00:41:56Z

thanks!

mprobst force-pushed the fix-exported-var-comments branch 2 times, most recently from 410ca8d to 075690d Compare July 12, 2018 12:32

mprobst changed the title ~~Retain synthetic comments on exported variables.~~ Retain synthetic comments on exports. Jul 12, 2018

mprobst force-pushed the fix-exported-var-comments branch 2 times, most recently from 71f0393 to eba8e49 Compare July 12, 2018 14:24

mprobst force-pushed the fix-exported-var-comments branch from eba8e49 to cdfed60 Compare July 12, 2018 15:08

mhegazy requested review from weswigham and rbuckton and removed request for weswigham July 12, 2018 23:55

mprobst force-pushed the fix-exported-var-comments branch from 30a650a to f35428c Compare July 17, 2018 09:19

mprobst added 3 commits July 18, 2018 16:30

Reduce duplication of addSyntheticComment.

1bd79af

mprobst force-pushed the fix-exported-var-comments branch from f35428c to b7ee0e9 Compare July 18, 2018 14:31

mprobst added 3 commits July 18, 2018 17:23

Retain synthetic comments on classes and their properties.

2529b86

Do not emit comments if container had a comment suppression.

a7224ec

mprobst force-pushed the fix-exported-var-comments branch from 11681ef to c50a6f7 Compare July 18, 2018 15:25

weswigham reviewed Jul 24, 2018

View reviewed changes

Fix typos

7d44014

mhegazy merged commit fd2eb49 into microsoft:master Jul 25, 2018

mprobst deleted the fix-exported-var-comments branch August 1, 2018 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Retain synthetic comments on exports. #25604

Retain synthetic comments on exports. #25604

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mhegazy commented Jul 12, 2018

Uh oh!

mprobst commented Jul 16, 2018

Uh oh!

mprobst commented Jul 19, 2018

Uh oh!

mprobst commented Jul 20, 2018

Uh oh!

weswigham Jul 24, 2018

Uh oh!

weswigham Jul 24, 2018

Uh oh!

mhegazy commented Jul 25, 2018

Uh oh!

Uh oh!

Retain synthetic comments on exports. #25604

Retain synthetic comments on exports. #25604

Uh oh!

Conversation

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mprobst commented Jul 12, 2018

Uh oh!

mhegazy commented Jul 12, 2018

Uh oh!

mprobst commented Jul 16, 2018

Uh oh!

mprobst commented Jul 19, 2018

Uh oh!

mprobst commented Jul 20, 2018

Uh oh!

weswigham Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Jul 25, 2018

Uh oh!

Uh oh!