/CC @weswigham

@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.

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 ;-)

@rbuckton can you please review this change.

@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.

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.

@rbuckton friendly ping.

thanks!