Comment positioning issues

[jtbandes]

Hi! I recently used flow-to-ts and ran into some issues with comment positioning. It looks like you've been working on it recently in #212 and #216, but I found that even running with the latest master, there are some issues where whitespace between near-adjacent comments is not respected, and comments are moved onto preceding lines.

An example of changes produced by flow-to-ts is here: https://gist.github.com/jtbandes/17606101e1c17d4f8b979bee01a44bb0/revisions

Some examples of really surprising behavior:

• Comments are moved up to be on the same line as a preceding }, even though there was previously a blank line in between. The blank line is now after the comment.
  

• Roughly the same thing happens for variable declarations inside a class, meaning that these now appear to be same-line comments on the wrong variable:
  

I'd be open to helping fix this if you have some pointers to where the problem might be arising from.

I should add that I think the ideal behavior is to leave comments as close to their original positions as possible, and not try to be smart about which comments apply to declarations on the next/previous line.

[jtbandes]

I discovered that this is a problem with @babel/generator and not this library. Completely removing the call to traverse(), comments are still moved around unfavorably. 🙁

Perhaps switching to something like recast would be the best way to move forward.

[kevinbarabash]

I thought I did a better job of handling fixing trailing comments but I didn't consider the case where there were multiple comments between statements. 😞

I also didn't realize that you could pass recast an AST directly. It looks like recast uses the babel AST under the hood which is good because that's what flow-to-ts also uses.

[kevinbarabash]

I'd rather not maintain complicated comment code if it can be avoid. Hopefully recast just works. 🤞

[kevinbarabash]

I gave recast a try, but the results for some the test cases were quite bad. In one of them

switch (kind) {

    /**
     * foo
     */
    case "foo":
        console.log("foo");
        break;

    case "bar":
        console.log("bar");
        break;

    // default
    default:
        break;

    /**
     * trailing comment
     */
}

became

/**
     * foo
     */
// default
/**
     * trailing comment
     */
switch (kind) {
case "foo":
  console.log("foo");
  break;
case "bar":
  console.log("bar");
  break;
default:
  break;
}

My hunch is that recast is using ast.comments (I had to remove // @flow comments from this array) and relies on the loc of those comment nodes instead of the leadingComments/trailingComments attached to each node and ignore the loc.

We can't "fix" the loc since converting to TypeScript can change the length of lines and even how many lines there are and we don't know the lengths of the lines and such until we print them out.

[kevinbarabash]

@jtbandes I wasn't able to reproduce this using a smaller chunk of code with multiple comments in between lines, see playground example. Can you submit a minimal reproducible example as a playground link?

[jtbandes]

Here's what I was testing (just modified one of your test cases):

https://flow-to-ts.netlify.app/#code=Ly8gZmlyc3QgbGVhZGluZyBjb21tZW50Ci8vIHNlY29uZCBsZWFkaW5nIGNvbW1lbnQKY29uc3QgYSA9IDU7IC8vIHgKCi8vIGJhcgpjb25zdCBiID0gMTA7IC8vIHkKLy8gYmF6CgovLyBmaXJzdCBsZWFkaW5nIGNvbW1lbnQKLy8gc2Vjb25kIGxlYWRpbmcgY29tbWVudApjb25zdCBjID0gNTsKCi8vIGJhcgpjb25zdCBkID0gMTA7Ci8vIGJheg==&semi=1&singleQuote=0&tabWidth=4&trailingComma=all&bracketSpacing=0&arrowParens=undefined&printWidth=80&prettier=1&inlineUtilityTypes=0

For a larger test case you can also see the file I shared above: https://gist.github.com/jtbandes/17606101e1c17d4f8b979bee01a44bb0/revisions

[kevinbarabash]

Thanks. There are two issues:

• blank lines are not preserved
• in some cases what should be leadingComments become trailingComments

It looks like the latter issue is happening certain statements, but not all statements. This should be easy to fix, we just need to call trackComments inside of more node handlers in transform.js. Unfortunately

The issue with blanks lines not being preserved is more difficult. Previously I had vendored @babel/generator and made a bunch of custom tweaks to it. I really didn't want to maintain those tweaks especially since it and the other babel dependencies needed to be upgraded to support newer syntax.

That being said I think the current approach could be improved to better handle const b = 10; // y. I don't think that maintaining the whitespace between baz and first leading comment is that important since it doesn't seem like comments would normally have whitespace there:

const b = 10; // y
// baz

// first leading comment
// second leading comment
const c = 5;

[jtbandes]

I don't think that maintaining the whitespace between baz and first leading comment is that important since it doesn't seem like comments would normally have whitespace there

I think that's true, but there might be some cases where a totally separate comment block in the middle is surrounded by blank lines:

const b = 10; // y

// baz

// first leading comment
const c = 5;

Right now all these comments get squashed into one block.

[kevinbarabash]

I think there's probably a way to maintain the blank lines without having to vendor @babel/generator. I'm going to solve the easier (and more glaring) problems first. 🙂

[kevinbarabash]

@jtbandes I've fixed some of the issues with #233. Please give https://deploy-preview-233--flow-to-ts.netlify.app a try and let me know if you're seeing any issues not pertaining to the removal of blank lines (I haven't got to that issue yet).

[jtbandes]

Thanks! This does look a lot better so far.

I am noticing that some comments are still being moved between adjacent class members, although maybe this is partly because of the blank lines issue:

https://deploy-preview-233--flow-to-ts.netlify.app/#code=Y2xhc3MgRm9vIHsKICAvLyBUaGlzIGlzIGJhcgogIGJhcjogbnVtYmVyOwogIAogIC8vIFRoaXMgaXMgYmF6CiAgLy8gVGhpcyBpcyBhbHNvIGJhegogIGJhejogbnVtYmVyOwogIAogIC8vIEluIGJldHdlZW4KICAKICAvLyBBbmQgYSBxdXV4CiAgcXV1eDogbnVtYmVyOwp9Ow==&prettier=1&semi=1&singleQuote=0&tabWidth=4&trailingComma=all&bracketSpacing=0&arrowParens=avoid&printWidth=80&inlineUtilityTypes=0

[kevinbarabash]

// This is baz and // In between should not have become trailing comments. I'll look into it tomorrow.

[kevinbarabash]

I wasn't tracking comments in class methods/properties. I've updated the PR to handle those now and have also enabled private class methods/properties.

[kevinbarabash]

I figured out kind of a hacky way to maintain blank lines between single line comments. Adding a \n to the end of the comments value will result in a blank line after the comment. This wonn't work for block comments though.

[jtbandes]

I'll try it out soon.

Regarding recast, did you see their note in the readme that you need to use recast.parse as well? You can't just pass it a pre-existing AST.

[jtbandes]

flow-to-ts now seems to be adding blank lines where there weren't any:

https://flow-to-ts.netlify.app/#code=Ly8gQGZsb3cKZXhwb3J0IGRlZmF1bHQgZnVuY3Rpb24gZmlsdGVyTWFwPFQsIFU+KHZhbHVlczogSXRlcmFibGU8VD4sIG1hcEZuOiAoVCwgbnVtYmVyKSA9PiA/VSk6IFVbXSB7CiAgY29uc3QgcmVzdWx0cyA9IFtdOwogIGxldCBpbmRleCA9IDA7CiAgZm9yIChjb25zdCBpdGVtIG9mIHZhbHVlcykgewogICAgY29uc3QgbWFwcGVkSXRlbSA9IG1hcEZuKGl0ZW0sIGluZGV4KyspOwogICAgaWYgKG1hcHBlZEl0ZW0pIHsKICAgICAgcmVzdWx0cy5wdXNoKG1hcHBlZEl0ZW0pOwogICAgfQogIH0KICByZXR1cm4gcmVzdWx0czsKfQ==&prettier=1&semi=1&singleQuote=0&tabWidth=4&trailingComma=all&bracketSpacing=0&arrowParens=avoid&printWidth=80&inlineUtilityTypes=0

[jtbandes]

Comment still moving around in some cases:

const a = 1;

// foo
export default {};

const a = 1;

// foo
class X {};

Is there any way to ensure that your trackComments function is being run for every type of AST node?

[jtbandes]

Same-line comment gets moved to next line:

const next = {
  foo, // a
  bar,
};

Note that there is also some surprising behavior with inner comments, like:

const next = {
  foo /*hi*/,
  bar,
};

But the code I'm working with almost never uses inner comments.

[jtbandes]

Blank line added before comment:

if (true) {
}
// foo
let x;

[jtbandes]

Next-line comment gets moved to same line if there is no next sibling node:

function foo() {
  let x;
  // foo
}

[kevinbarabash]

I tried an older version of the playground and the same blank lines are being inserted there too.

Comment still moving around in some cases:
Is there any way to ensure that your trackComments function is being run for every type of AST node?

I believe that trackComments is being called for those node types. I'll double check, but I suspect something else is at play. It wouldn't be hard to call it every node. That's something I've considered doing. I'd like to investigate these cases first and go from there.

Blank line added before comment:

Not sure if we'll be able to do anything about this without vendoring @babel/generator. 😞

Next-line comment gets moved to same line if there is no next sibling node:

I can fix this. The right only processed comments that appear in both leadingComments and trailingComments of two nodes.

[kevinbarabash]

Regarding recast, did you see their note in the readme that you need to use recast.parse as well? You can't just pass it a pre-existing AST.

I was skimming the README for how to pretty print an AST. I missed the important part:

Why is this so important? When you call recast.parse, it makes a shadow copy of the AST before returning it to you, giving every copied AST node a reference back to the original through a special .original property. This information is what enables recast.print to detect where the AST has been modified, so that it can preserve formatting for parts of the AST that were not modified.

I also didn't realize that you can specify the parser for recast to use which is great because it means we can use recast with changing transform.js. I'm going to give recast another try.

[kevinbarabash]

recast seems to be working better now that I'm use it to parse and pretty print. Some of the comments have gone missing. recast stores the comments slightly differently, hopefully I can get those comments back.

I also try parsing and pretty print some code without running any transforms on it and it added an extra blank line. Here's the code I used:

  const {parse} = require("@babel/parser");

  const ast = recast.parse(flowCode, {
    parser: {
      parse(source) {
        return parse(source, parseOptions);
      }
    }
  });

  console.log(flowCode);
  console.log("-------")
  console.log(recast.prettyPrint(ast, {tabWidth: 2}).code);

and here's the console output:

const b = 10; // y
// baz

// first leading comment
// second leading comment
const c = 5;
-------
const b = 10; // y

// baz

// first leading comment
// second leading comment
const c = 5;

[kevinbarabash]

Here's another test case, no transforms:

// @flow
declare class Path<T> extends Node<U> {
    // converts Path to a string
    toString(): string;
}
-------
// @flow
declare class Path { // converts Path to a string
toString: : () => string }

[jtbandes]

Yeah, it seems like comment positioning is simply a weak point of the Babel AST. Recast may have better workarounds but I think it still needs workarounds. :(

It claims to be "nondestructive" so maybe it is worth filing an issue against recast.

[kevinbarabash]

I looked at the following example:

function foo() {
  let x;
  // foo
}

There isn't much we can do about this without patching @babel/generator. The code in convert only controls whether a comment appears in trailingComments or not. // foo is already in trailingComments and @babel/generator decides it how it wants to print trailingComments.

I think we're going to have to bring back a patched version of @babel/generator to fix these issues. I want to do it right though. Instead of copying what's in node_modules/@babel/generator we should fork the babel repo and patch it in the source.

I'm going to focus on conversion bugs instead of formatting bugs for the next while. Once more of the conversion issues are addressed I'll return to formatting issues.

[kevinbarabash]

Now that we're consuming the babel source (instead of vendored built files) I can start work on re-adding the blank-line handling that we had a while go.

[Pike]

Another few examples of reordered comments from my Pontoon conversion is at https://flow-to-ts.netlify.app/#code=aW1wb3J0ICogYXMgdW5zYXZlZGNoYW5nZXMgZnJvbSAnbW9kdWxlcy91bnNhdmVkY2hhbmdlcyc7CgovLyBDb21iaW5lIHJlZHVjZXJzIGZyb20gYWxsIG1vZHVsZXMsIHVzaW5nIHRoZWlyIE5BTUUgY29uc3RhbnQgYXMga2V5LgpleHBvcnQgZGVmYXVsdCAoYnJvd3Nlckhpc3Rvcnk6IGFueSk6IGFueSA9PiBhbnk7CgovLyBJZiB0aGVyZSBpcyBubyBlbnRpdHkgdGhlbiBpdCBpcyBhIHNlYXJjaCB0ZXJtIGFuZCBpdCBpcwovLyBhZGRlZCB0byB0aGUgZWRpdG9yIGluc3RlYWQgb2YgcmVwbGFjaW5nIHRoZSBlZGl0b3IgY29udGVudAppZiAoIWVudGl0eSkgewogIGFkZFRleHRUb1RyYW5zbGF0aW9uKHRyYW5zbGF0aW9uLnRyYW5zbGF0aW9uKTsKfQovLyBUaGlzIGlzIGEgRmx1ZW50IE1lc3NhZ2UsIHRodXMgd2UgYXJlIGluIHRoZSBSaWNoRWRpdG9yLgovLyBIYW5kbGUgbWFjaGluZXJ5IGRpZmZlcmVudGx5LgplbHNlIGlmIChpc0ZsdWVudFRyYW5zbGF0aW9uTWVzc2FnZSkgewogIGFkZFRleHRUb1RyYW5zbGF0aW9uKHRyYW5zbGF0aW9uLnRyYW5zbGF0aW9uLCAnbWFjaGluZXJ5Jyk7Cn0KLy8gQnkgZGVmYXVsdCByZXBsYWNlIGVkaXRvciBjb250ZW50CmVsc2UgewogIHVwZGF0ZVRyYW5zbGF0aW9uKHRyYW5zbGF0aW9uLnRyYW5zbGF0aW9uLCAnbWFjaGluZXJ5Jyk7Cn0K&prettier=1&semi=1&singleQuote=1&tabWidth=2&trailingComma=all&bracketSpacing=0&arrowParens=avoid&printWidth=80&inlineUtilityTypes=0