Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leading comments incorrectly duplicated on variable initialization #264

Closed
kaicataldo opened this issue Mar 26, 2016 · 4 comments · Fixed by eslint/eslint#5922
Closed

Leading comments incorrectly duplicated on variable initialization #264

kaicataldo opened this issue Mar 26, 2016 · 4 comments · Fixed by eslint/eslint#5922

Comments

@kaicataldo
Copy link
Member

Leading comments are incorrectly duplicated on variable initializations. The current strategy employed to fix #256 only checks the array inside body properties and removes comments that exist in the range of the previous index's node, never checking the init property (and so not covering this case). Just to be clear, this is not a regression - this is just one more case that was being added incorrectly (or rather, in practice, not being removed correctly).

The difficulty with this one is that when we drill deeper into the tree, we'll need to have saved some kind of reference to the previously iterated over comments to know if we need to remove it or not. We may have to rethink our strategy on this.

Example:
The following code

//foo
var zzz /*aaa*/ = 777;
//bar

currently produces this output (notice the incorrect leading comment //foo on the literal 777):

{
    "type": "Program",
    "start": 0,
    "end": 34,
    "range": [
        6,
        28
    ],
    "body": [
        {
            "type": "VariableDeclaration",
            "start": 6,
            "end": 28,
            "range": [
                6,
                28
            ],
            "declarations": [
                {
                    "type": "VariableDeclarator",
                    "start": 10,
                    "end": 27,
                    "range": [
                        10,
                        27
                    ],
                    "id": {
                        "type": "Identifier",
                        "start": 10,
                        "end": 13,
                        "range": [
                            10,
                            13
                        ],
                        "name": "zzz",
                        "trailingComments": [
                            {
                                "type": "Block",
                                "value": "aaa",
                                "start": 14,
                                "end": 21,
                                "range": [
                                    14,
                                    21
                                ]
                            }
                        ]
                    },
                    "init": {
                        "type": "Literal",
                        "start": 24,
                        "end": 27,
                        "range": [
                            24,
                            27
                        ],
                        "value": 777,
                        "raw": "777",
                        "leadingComments": [
                            {
                                "type": "Line",
                                "value": "foo",
                                "start": 0,
                                "end": 5,
                                "range": [
                                    0,
                                    5
                                ]
                            },
                            {
                                "type": "Block",
                                "value": "aaa",
                                "start": 14,
                                "end": 21,
                                "range": [
                                    14,
                                    21
                                ]
                            }
                        ]
                    }
                }
            ],
            "kind": "var",
            "leadingComments": [
                {
                    "type": "Line",
                    "value": "foo",
                    "start": 0,
                    "end": 5,
                    "range": [
                        0,
                        5
                    ]
                }
            ],
            "trailingComments": [
                {
                    "type": "Line",
                    "value": "bar",
                    "start": 29,
                    "end": 34,
                    "range": [
                        29,
                        34
                    ]
                }
            ]
        }
    ],
    "sourceType": "script",
    "comments": [
        {
            "type": "Line",
            "value": "foo",
            "start": 0,
            "end": 5,
            "range": [
                0,
                5
            ]
        },
        {
            "type": "Block",
            "value": "aaa",
            "start": 14,
            "end": 21,
            "range": [
                14,
                21
            ]
        },
        {
            "type": "Line",
            "value": "bar",
            "start": 29,
            "end": 34,
            "range": [
                29,
                34
            ]
        }
    ]
}

I plan to look at this.

@kaicataldo
Copy link
Member Author

Haven't forgotten about this. It's been on my mind and I hope to get to it soon.

kaicataldo added a commit to kaicataldo/espree that referenced this issue Apr 18, 2016
@kaicataldo
Copy link
Member Author

kaicataldo commented Apr 18, 2016

After trying a few iterations (and trying to write the comment attachment from scratch), I have a much better understanding of why the current system for comment attachment works the way it does. With that newfound understanding, I think I have a much more elegant solution than my original that also fixes this issue.

I know we talked about moving comment attachment to ESLint, and I'm definitely willing to give that a shot - wanted to fix this issue here first at the parser level (we can always just not use the attachComment option if we decide to try to attach comments at the ESLint level).

@nzakas I've updated two comment attachment tests, but it looks like many of the library tests are failing - most likely from having duplicate comments attached to nested nodes. Is their purpose just to ensure the parser doesn't error? Or a better question: how would I go about fixing them and ensuring my change is good? The fixes to the individual comment attachment tests are correct (see PR). I believe I have correctly updated the library tests, and my only real concern now is the ESLint test failure below.

One final question: I've run the ESLint repo's suite of tests with my PR branch and all tests pass except for one:
screen shot 2016-04-18 at 1 39 45 am

I'm a little out of my depth here, and am not sure what this means - whether I should change the tests, or whether this test indicates my fix is no good. Interesting thing to note - the code snippet in the failing test is the same code snippet in the original post of this issue, so it might make sense that it has changed. Thanks for all your help with this one!

@kaicataldo
Copy link
Member Author

kaicataldo commented Apr 18, 2016

Sorry, one last bit of info. Changing the test's expected order of events from

var expected = [
    ["Program", ast],
    ["LineComment", ast.comments[0]], // foo
    ["VariableDeclaration", ast.body[0]],
    ["LineComment", ast.comments[2]], // bar
    ["VariableDeclarator", ast.body[0].declarations[0]],
    ["Identifier", ast.body[0].declarations[0].id],
    ["BlockComment", ast.comments[1]], /* aaa */
    ["BlockComment:exit", ast.comments[1]], /* aaa */
    ["Identifier:exit", ast.body[0].declarations[0].id],
    ["Literal", ast.body[0].declarations[0].init],
    ["Literal:exit", ast.body[0].declarations[0].init],
/* Move this --> */ ["LineComment:exit", ast.comments[0]], // foo
    ["VariableDeclarator:exit", ast.body[0].declarations[0]],
    ["LineComment:exit", ast.comments[2]], // bar
    ["VariableDeclaration:exit", ast.body[0]],
    ["Program:exit", ast]
];

to

var expected = [
    ["Program", ast],
    ["LineComment", ast.comments[0]], // foo
    ["VariableDeclaration", ast.body[0]],
    ["LineComment", ast.comments[2]], // bar
    ["VariableDeclarator", ast.body[0].declarations[0]],
    ["Identifier", ast.body[0].declarations[0].id],
    ["BlockComment", ast.comments[1]], /* aaa */
    ["BlockComment:exit", ast.comments[1]], /* aaa */
    ["Identifier:exit", ast.body[0].declarations[0].id],
    ["Literal", ast.body[0].declarations[0].init],
    ["Literal:exit", ast.body[0].declarations[0].init],
    ["VariableDeclarator:exit", ast.body[0].declarations[0]],
    ["LineComment:exit", ast.comments[2]], // bar
    ["VariableDeclaration:exit", ast.body[0]],
/* To here --> */ ["LineComment:exit", ast.comments[0]], // foo
    ["Program:exit", ast]
];

causes the failing test to pass. Sorry for the massive amount of info, and really appreciate any guidance on the issue!

@kaicataldo
Copy link
Member Author

@nzakas Pinging once more - let me know if you'd prefer we don't ping multiple times (don't want to bug you, but also wanted to make sure this didn't get lost). I know you've been super busy with logistical stuff this week (but yay for all the exciting things!), so whenever you get a chance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants