Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9461/

I will have a look at it soon. I know we had problems with changing locations of comments in the past and I experienced them in react-docgen so I will plug this PR into there and see what fails and if i can make the jsdoc parsing work correctly or it just happens to work. We can also try to run prettier with this change and see if their tests work.

Ok I plugged it into prettier and one test failed, but the recorded snapshots looks incorrect. The changed comment behavior in this PR seems to fix it 👍:

input:

const Component = branch/*::     <Props, ExternalProps> */(
  ({ src }) => !src,
  // $FlowFixMe
  renderNothing,
)(BaseComponent);

const C = b/*:: <A> */(foo) + c/*:: <B> */(bar);

foo/*::<bar>*/(baz);

foo/*::<bar>*/();

output diff:

- Expected
+ Received

 const Component = branch/*:: <Props, ExternalProps> */(
-   /*::     <Props, ExternalProps> */ ({ src }) => !src,
+   ({ src }) => !src,
    // $FlowFixMe
    renderNothing
 )(BaseComponent);
 
- const C = b/*:: <A> */(/*:: <A> */ foo) + c/*:: <B> */(/*:: <B> */ bar);
+ const C = b/*:: <A> */(foo) + c/*:: <B> */(bar);
 
- foo/*:: <bar> */(/*::<bar>*/ baz);
+ foo/*:: <bar> */(baz);
 
 foo/*:: <bar> */();

react-docgen has some problems, but only because it relies on the fact that comments are attached as trailing on the decorator and not only as leading on the id of the class or the classbody.
https://github.com/reactjs/react-docgen/blob/master/src/handlers/componentDocblockHandler.js#L39-L46

export default
@decorator
/** class description */
class A {}

I guess this can be fixed somehow, although I can't see an easy solution, as there is no location recorded in the AST where the class keyword is located.

so in the case of

export default /** Decorator description */ @decorator /** Description */ class /** Base */SomeName {}

it is possible to know where the first comment belongs to, but it is not really possible to know which of the other two comments come before and which after the class keyword and which react-docgen should treat as the description of the class. Both of the two comments are attached to the Identifier or the Classbody (if there is no id).

Any idea about this special case? It is the same case that broke in the past and whythis hack is present in react-docgen.

In general the ruleset for the comment attachment make sense to me and I feel having it clearly defined is really good.

I'm worried about the potential of breaking stuff and I think this change maybe should not be in a minor version. Not necessarily because of Babel users but for tools that use Babylon standalone. (As mentioned above)
Though maybe this change might be a good candidate for a feature flag as discussed in the last meeting. It could be enabled in Babel by default if we know it is not breaking the default Babel use cases.

I haven't looked at the code changes so far. Will do it tomorrow.

1. Leading comments are any comments that
   • Were not already consumed as trailing comments for a previous node
   • Come fully before the node.start offset
   • Will always be the shallowest node to satisfy the node.start

I think that we should add another rule:

• Are not separated from the current node by any token

This rule would change the behavior in cases like this one:

if /* a */ (x) {}

Currently /* x */ is a leading comment of the Identifier, but with the new rule it wouldn't be assigned to x and thus would become an inner comment of the IfStatement.

It would also fix this case, where /* x */ is considered a leading comment of the BlockStatement:

for (/* x */;;) {}

Apart from that, I think that these rules are pretty good. We should write them somewhere else other than in this PR so that they don't get lost.

I'll review the code later.

@danez

export default /** Decorator description */ @decorator /** Description */ class /** Base */SomeName {}

Special-casing that use-case would unfortunately break these:

/**
 * Class description
 */
@frozen class Foo {}

@frozen // Prevent modifications to the class
class Foo {}

@nicolo-ribaudo

This rule would change the behavior in cases like this one:

if /* a */ (x) {}

Currently /* x */ is a leading comment of the Identifier, but with the new rule it wouldn't be assigned to x and thus would become an inner comment of the IfStatement.

I think this would actually also help with the example I posted if i understood it correctly:

/** leading on ExportDeclaration */
export default 
/** leading on Decorator */ 
@decorator 
/** inner on class because of class keyword between comment and identifier */
class /** leading on Identifier */SomeName 
/** leading on body */
{}

Is that correct?

@nicolo-ribaudo

Currently /* x */ is a leading comment of the Identifier, but with the new rule it wouldn't be assigned to x and thus would become an inner comment of the IfStatement.

My aim with this change was to try to stay in the bounds of what we already do wherever possible. innerComments is very limited for now and I think that's realistically for the best. Since don't have a Concrete Syntax Tree, it's not at all clear how innerComments should behave in anything but the simplest of cases. I think this if probably is one of those cases, but I'd really love to avoid addings special cases for specific node types. I'm also consider moving to more innerComments to be more likely to qualify as breaking, since babel-generator only really expects innerComments in a few places.

It would also fix this case, where /* x */ is considered a leading comment of the BlockStatement:

for (/* x */;;) {}

The issue here is innerComments still isn't good enough resolution anyway. What would babel-generator do?

{
  type: "ForStatement",
  innerComments: [{ type: "CommentBlock", value: " x " }],
}

could serialize as any of

for /* x */(;;) {}
for (/* x */;;) {}
for (;/* x */;) {}
for (;;/* x */) {}

technically. My preference for this patch is to leave the preference for comments to attach as leading comments on the next-closest node.

Should we also add >, since it is used as a groupEnd for type parameters?

See below reply to @danez.

@danez

export default
@decorator
/** class description */
class A {}

This should be a trailing comment on the decorator, but it's a case I missed. I think this and the case above about > may point at a change I should make around how I handle sibling tokens. I actually wonder if I could avoid the groupEnd token ideal altogether. Let me explore and see if I run into any issues.

I'm worried about the potential of breaking stuff and I think this change maybe should not be in a minor version. Not necessarily because of Babel users but for tools that use Babylon standalone. (As mentioned above)
Though maybe this change might be a good candidate for a feature flag as discussed in the last meeting. It could be enabled in Babel by default if we know it is not breaking the default Babel use cases.

Yeah, I'm on the fence. I think partly it depends on how close we can get to the previous behavior. In an ideal worth, this patch would

1. Avoid putting comments in two places
2. Exclusively remove duplicates in the places where people didn't expect them to be anyway

You two have pointed out a big case I missed, so thanks a ton for that. I can also do another pass to see what cases might diverge from those 2 points. The logic I added around permeable is the only place I know of off the top of my head, since previously

[
  foo, // comment
  other
]

would place // comment as a leading comment on other rather than as a trailing comment on foo, but now it would be treated as a trailing comment on foo. If that is something that you feel is too aggressive, I'd be fine with just dropping it for now.

At the end of the day, if the only way to land this is a feature flag, I'm open to it, but I'd love to avoid that if at all possible.

I'll reopen once I have an update.