Harmony branch attaches comments to different location

[GoogleCodeExporter]

There are several instances where comments are attached at a different location 
in the Harmony branch than in master. 

1) Sample:

var o = {
   /** Desc */
   foo: function(){}
}

Harmony branch: /** Desc */ is attached to Property.key.leadingComments.
Master branch: /** Desc */ is attached to Property.leadingComments.

2) Sample:

/** File desc */
/** Desc*/
Foo.bar = function (){}

Harmony branch: /** Desc */ is attached to 
ExpressionStatement.AssignmentExpression.left.leadingComments.
Master branch: /** Desc */ is attached to ExpressionStatement.leadingComments.

3) Sample:

/** desc */
(function() {
    // todo
});

Harmony branch: /** Desc */ is attached to Program.comments only.
Master branch: /** Desc */ is attached to both Program.comments and 
ExpressionStatement.leadingComments.

Originally brought up in 
https://github.com/eslint/eslint/issues/1291#issuecomment-57890928

I can't think of a valid reason for this to be different, it would be really 
nice if the Harmony branch was made to work like master.

Original issue reported on code.google.com by nicho...@nczconsulting.com on 26 Oct 2014 at 3:01

[GoogleCodeExporter]

Update: It appears my initial description was inaccurate. The difference in 
comment attachment appears to be between the npm published version and 
everything that's in GitHub. Both the master and harmony branches show the same 
behavior, both of which are different from what's published on npm.

I also checked the online demo of Esprima, and it displays the same behavior as 
in GitHub rather than the behavior on npm.

Was this change intentional or not?

Original comment by nicho...@nczconsulting.com on 1 Nov 2014 at 5:18

[GoogleCodeExporter]

Comment from GitHub issue above.

"The difference is in the function processComment(node) { function in Esprima, 
replacing it with the 1.2 version makes the UTs pass, I will investigate some 
more. My guess is it's either this checkin or this one".

Original comment by off...@gmail.com on 5 Nov 2014 at 6:36

[GoogleCodeExporter]

PR for issue https://github.com/ariya/esprima/pull/292

Original comment by off...@gmail.com on 5 Nov 2014 at 7:59

[GoogleCodeExporter]

Unfortunately, there is no really specification as to where to attach the 
comment. However, if we follow the spirit where each comment needs to be as 
close as possible to the syntax node, then the version implemented in master 
branch makes more sense.

Consider the first case:

var o = {
   /** Desc */
   foo: function(){}
}

Argueably, the block comment there is the closest to the Identifier "foo" which 
signifies the property key in that object.

Original comment by ariya.hi...@gmail.com on 7 Nov 2014 at 1:53

[GoogleCodeExporter]

The problem with this approach is that it makes locating comments much more 
difficult in the AST. With the 1.2.x approach, given any node, I can find the 
nearest related comment simply by going to the parent, then grandparent, etc., 
until I find a node with leading comments. With the 2.x approach, for every 
ancestor node, I need to check the type to see if it's one of the types where 
the comment is attached on one of its children or not. 

I'd argue the way it is in 1.2.x is more useful., and also that it makes more 
sense   In your example, the comment applies to the whole property (including 
the value), not just the key. 

Original comment by nicho...@nczconsulting.com on 7 Nov 2014 at 2:03

[GoogleCodeExporter]

There is also the backward compatibility argument, there may well be code out 
there, like ESLint rules, that rely on comments being in those locations.

Original comment by off...@gmail.com on 9 Nov 2014 at 10:50

[GoogleCodeExporter]

Can you please update this issue with whether or not you intend to fix this? 
ESLint is stalled on ES6 support until we know how Esprima is going to address 
(or not) this issue.

I believe this is a bug that breaks compatibility with anything that is using 
the currently published version, so I believe it should be fixed, but if you 
disagree it would be good to know so we can move on.

Original comment by nicho...@nczconsulting.com on 12 Nov 2014 at 12:29

[GoogleCodeExporter]

To close the loop on this.

Doing a consistent attachment is pretty tough and unfortunately neither 1.2 nor 
2.0 is perfect. What is being done in 2.0 is another step so that hopefully the 
attachment becomes more consistent. As I mentioned in previous comment, the 
consistency is achieved with the comment can be associated with a particular 
node without ambiguity. As an example in a practical sense, if comment C is 
considered as the leading comment for node N, then there can not be another 
node M, the direct parent or an indirect ancestor of N, that also be considered 
as the owner of that comment C.

The switch statement illustrates this problem. If we consider the following 
code snippet:

switch (x) {
/*foo*/ case 42: /*bar*/ answer();
}

This will result in a single SwitchCase node. SwitchCase has two important 
child nodes: test (of Literal node) and consequent (an array).

With version 1.2, "foo" is the leading comment of the entire SwitchCase node 
while "bar" is the leading comment of the consequent's first item 
(ExpressionStatement that is a CallExpression for answer).

With version 2.0, "bar" is exactly the same as above. However, "foo" is the 
leading comment of the test property only (that representing the Literal 42). 
How did it choose between that Literal node and SwitchCase node? In this case, 
the Literal node has a larger depth compared to the SwitchCase node. After all, 
the Literal node is a leaf and the SwitchCase is its parent.

Maybe there is a world where both attachment styles (the next largest node vs 
the next most granular node) need to co-exist. In all cases, the discussion 
should continue (if at all) on the new Github issue tracker.

Original comment by ariya.hi...@gmail.com on 11 Feb 2015 at 8:13

• Changed state: AsExpected

[GoogleCodeExporter]

I've left a comment asking if I should close the PR on GitHub.

https://github.com/jquery/esprima/pull/292/files#r24318548

Original comment by off...@gmail.com on 11 Feb 2015 at 10:12