Incorrect leading comments.

[mysticatea]

From eslint/eslint#5512

In the following case:

var foo = {
  // foo
};
var bar;

// foo comment has attached to the leading of var bar.

---

"use strict";

const espree = require("espree");
const ast = espree.parse(
    `var foo = {
  // foo
};
var bar;`,
    {
        range: true,
        attachComment: true
    }
);

console.log(JSON.stringify(ast, null, 4));

{
    "type": "Program",
    "start": 0,
    "end": 32,
    "range": [
        0,
        32
    ],
    "body": [
        {
            "type": "VariableDeclaration",
            "start": 0,
            "end": 23,
            "range": [
                0,
                23
            ],
            "declarations": [
                {
                    "type": "VariableDeclarator",
                    "start": 4,
                    "end": 22,
                    "range": [
                        4,
                        22
                    ],
                    "id": {
                        "type": "Identifier",
                        "start": 4,
                        "end": 7,
                        "range": [
                            4,
                            7
                        ],
                        "name": "foo"
                    },
                    "init": {
                        "type": "ObjectExpression",
                        "start": 10,
                        "end": 22,
                        "range": [
                            10,
                            22
                        ],
                        "properties": [],
                        "trailingComments": [
                            {
                                "type": "Line",
                                "value": " foo",
                                "start": 14,
                                "end": 20,
                                "range": [
                                    14,
                                    20
                                ]
                            }
                        ]
                    }
                }
            ],
            "kind": "var"
        },
        {
            "type": "VariableDeclaration",
            "start": 24,
            "end": 32,
            "range": [
                24,
                32
            ],
            "declarations": [
                {
                    "type": "VariableDeclarator",
                    "start": 28,
                    "end": 31,
                    "range": [
                        28,
                        31
                    ],
                    "id": {
                        "type": "Identifier",
                        "start": 28,
                        "end": 31,
                        "range": [
                            28,
                            31
                        ],
                        "name": "bar"
                    },
                    "init": null
                }
            ],
            "kind": "var",
            "leadingComments": [
                {
                    "type": "Line",
                    "value": " foo",
                    "start": 14,
                    "end": 20,
                    "range": [
                        14,
                        20
                    ]
                }
            ]
        }
    ],
    "sourceType": "script",
    "comments": [
        {
            "type": "Line",
            "value": " foo",
            "start": 14,
            "end": 20,
            "range": [
                14,
                20
            ]
        }
    ]
}

[kaicataldo]

I'd like to work on this :)

[nzakas]

Ooh, go for it!

[kaicataldo]

Spent some time with this tonight and think I understand what's going on now. What I'm not sure of is how or where to put this fix. Seems to me like the best fix is to check something like

if (node.leadingComments[x].range[1] < previousNode.range[1]) {
    delete node.leadingComments[x];
}

where node is the current node being checked. As of now, there is only a mechanism to ensure the comment ends before the current node, but no checks to ensure that the comment comes after the previous node.

Ideally the change outlined above would happen in lib/comment-attachment, but the nodes appear to be checked in isolation there, and I don't think we have access to the previous node.

We could perform this check and remove offending leadingComments somewhere after the comments are attached and we have access to all the nodes (somewhere after this line, I think?). Wanted to see what someone with a little more knowledge/experience thought about this before I try to make this change.

One final thing - an alternative would be to see why acorn is attaching these comments to the node in the first place. Definitely does not seem like desired behavior.

Sorry for the wall of text, but looking forward to seeing what you all think and figuring this out. Cheers!

EDIT: It's always easier to show someone code than try to explain it, so here's an example of what I was thinking about (this would happen on this line):

    var i, j;

    if (extra.comment || extra.attachComment) {
        // Remove leading comments that are added when no expression exists between comment and the relevant node
        for (i = 1; i < program.body.length; i++) {
            if (program.body[i].leadingComments) {
                for (j = 0; j < program.body[i].leadingComments.length; j++) {
                    if (program.body[i].leadingComments[j].range[1] < program.body[i - 1].range[1]) {
                        program.body[i].leadingComments.splice(j, 1);
                    }
                }

                if (program.body[i].leadingComments.length < 1) {
                    delete program.body[i].leadingComments;
                }
            }
        }

        program.comments = extra.comments;
    }

I tried this out locally and it seems to be working, though not sure how it affects performance. I can make a PR if this seems like a good way forward!

[kaicataldo]

Sorry, thought I was going to put this down when I made my last comment...but here I am, still looking at it :)

This actually can work in lib/comment-attachment by putting the following code here:

        var i, j;

        if (node.type === astNodeTypes.Program) {
            // Remove leading comments that are added when no expression exists between comment and the relevant node
            for (i = 1; i < node.body.length; i++) {
                if (node.body[i].leadingComments) {
                    for (j = 0; j < node.body[i].leadingComments.length; j++) {
                        if (node.body[i].leadingComments[j].range[1] < node.body[i - 1].range[1]) {
                            node.body[i].leadingComments.splice(j, 1);
                        }
                    }

                    if (node.body[i].leadingComments.length < 1) {
                        delete node.body[i].leadingComments;
                    }
                }
            }

            if (node.body.length > 0) {
                return;
            }
        }

[nzakas]

Comment attachment is a big pile of hacks anyway, so this looks fine to me.