Some comments not emitted - upgrade 1.0.1 to 1.3.0

[jbondc]

more file.ts

// test
var f = ''
// test #2
module foo {
 function bar() { }
}
// test #3
module empty {
}
// test #4

more file.js

// test
var f = '';
// test #2
var foo;
(function (foo) {
 function bar() {
 }
})(foo || (foo = {}));

Looks like comments after a 'block' aren't emitted. Could it be a missing emitTrailingComments(node) ?

[DanielRosenwasser]

Just to be clear, when you say 1.0.3, do you mean 1.3.0?

[jbondc]

Yip, corrected the title. I attempted a patch here:
https://github.com/jbondc/TypeScript/compare/debug

Tried to get rid of the trailingSeparator logic, still a few fixes needed.

[DanielRosenwasser]

Not to deflect from your current work, but it may be worth seeing what Sheetal Nandi (@sheetalkamat) has to say about this issue and what the most appropriate fix is in general

[jbondc]

Thanks Daniel Rosenwasser (@DanielRosenwasser), I'll wait for feedback.

Updated the patch with a couple of fixes. 2729 tests failing though most are because newlines are preserved for single line comments.

[jbondc]

So I think I found the right idea to fix & improve, mainly simplify scanner & parser using:

 export interface CommentRange extends TextRange {
 leadingWhitespace?: string;
 trailingWhitespace?: string;
 isAttached?: boolean; /* attached to a Node (trailingWhitespace has at most 1 newline) */
 isFollowed?: boolean; /* followed *//* by another comment */
 }

The other bit is not emitting comments attached to !isDeclarationVisible(node)

As a heads up, only working on compiler scanner.ts emitter.ts, parser.ts

[CyrusNajmabadi]

Hey Jon (@jbondc) I'm going to be doing a little bit of work related to this. Namely, the introduction of an EndOfFile node to the ast. This should address the case where // test #4 is not in the final tree.

Note: it is by design that test #3 is not in the final tree.

[jbondc]

Super Cyrus Najmabadi (@CyrusNajmabadi), EndOfFile node sounds great. Pushed latest experiment and test case I'm using:
https://github.com/jbondc/TypeScript/blob/debug/tests/cases/compiler/commentsWhitespace.ts

Thought: when scanning can determine 'isAttached', 'isFollowed' & keep whitespace?
https://github.com/jbondc/TypeScript/blob/debug/src/compiler/scanner.ts#L872

Would avoid re-scanning (though don't know about services.ts / keeping in memory):
https://github.com/jbondc/TypeScript/blob/debug/src/compiler/scanner.ts#L373

[synaesthesia]

The bug still exists in 1.5.3. And it's not all blocks:

function hello(ok: boolean) {
 if (ok) {
 console.log("ok1");
 // will be removed
 }
 // will be removed too
 else {
 console.log("ok2");
 }
 // will stay
}

hello(true);

compiles to

function hello(ok) {
 if (ok) {
 console.log("ok1");
 }
 else {
 console.log("ok2");
 }
 // will stay
}
hello(true);

[alvarorahul]

So, this reproduces in TypeScript 1.6 as well

Here's another case where it gets stripped out

export = Main;
module Main {
 // stays

 class Foo {
 // stays
 public bar = 5;
 // gets removed
 }

 // gets removed
}

[DickvdBrink]

The last example is solved and it looks likes the first post is now correct to; only test is removed but as Cyrus Najmabadi (@CyrusNajmabadi) mentioned #1311 (comment) that is by design.
So maybe close this one?