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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Inner comments for anonymous functions/classes printed in leading position #15138

Closed
1 task done
overlookmotel opened this issue Nov 5, 2022 · 8 comments 路 Fixed by #15143 or #15144
Closed
1 task done
Labels
area: comments i: bug i: discussion i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator

Comments

@overlookmotel
Copy link
Contributor

overlookmotel commented Nov 5, 2022

馃捇

  • Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

const {parse} = require('@babel/parser'),
  generate = require('@babel/generator').default;

{
  const ast = parse('(function() {})');
  const fnNode = ast.program.body[0].expression;
  fnNode.innerComments = [{type: 'CommentBlock', value: 'foo'}];
  console.log(generate(ast, {compact: true}).code);
  // Prints (/*foo*/function(){});
}

{
  const ast = parse('(class {})');
  const classNode = ast.program.body[0].expression;
  classNode.innerComments = [{type: 'CommentBlock', value: 'foo'}];
  console.log(generate(ast, {compact: true}).code);
  // Prints (/*foo*/class{});
}

Configuration file name

No response

Configuration

n/a

Current and expected behavior

Previous to 7.20.1, innerComments were printed after the function or class keyword. Now they are printed before.

Previously: function /*foo*/ () {}
Now: /*foo*/function() {}

So now there's no difference between innerComments and leadingComments in these cases. In my opinion the old behavior made more sense ("inner" implies that position of comment is somewhere inside the node).

I assume this change occurred in #15080.

Environment

System:
OS: macOS 12.4
Binaries:
Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
npmPackages:
@babel/core: ^7.20.2 => 7.20.2
@babel/generator: ^7.20.2 => 7.20.2
@babel/helper-module-transforms: ^7.20.2 => 7.20.2
@babel/parser: ^7.20.2 => 7.20.2
@babel/plugin-transform-arrow-functions: ^7.18.6 => 7.18.6
@babel/plugin-transform-modules-commonjs: ^7.19.6 => 7.19.6
@babel/plugin-transform-react-jsx: ^7.19.0 => 7.19.0
@babel/plugin-transform-strict-mode: ^7.18.6 => 7.18.6
@babel/traverse: ^7.20.1 => 7.20.1
@babel/types: ^7.20.2 => 7.20.2
babel-plugin-dynamic-import-node: ^2.3.3 => 2.3.3
eslint: ^8.26.0 => 8.26.0
jest: ^29.2.2 => 29.2.2

Possible solution

No response

Additional context

No response

@babel-bot
Copy link
Collaborator

Hey @overlookmotel! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@liuxingbaoyu
Copy link
Member

@overlookmotel
Copy link
Contributor Author

Also found a case where the inner comments don't get printed at all:

const ast = parse('x = class {};');
const classNode = ast.program.body[0].expression.right;
classNode.innerComments = [{type: 'CommentBlock', value: 'foo'}];
console.log(generate(ast).code);
// Prints x = class {};

@nicolo-ribaudo
Copy link
Member

In class {} there isn't really a position where you would find inner comments of ClassExpression. If you try parsing /* 1 */ class /* 2 */ { /* 3 */ } /* 4 */, you see:

  • 1 is a leading comment of the class expression
  • 2 is a leading comment of the class body
  • 3 is an inner comment of the class body
  • 4 is a trailing comment of the class expression

As a workaround, we could print inner comments in trailing positions.

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Nov 5, 2022

I know of one example where parser does create inner comments on the ClassExpression:

(class /* foo */ extends X {})

I'm not at all familiar with generate()'s code, or what implications #15080 has, so I can't judge how difficult it would be restore the previous behavior - or whether it's worthwhile trying.

But personally I don't think it's ideal that whether inner comments are printed at all is inconsistent (as in #15138 (comment)).

If it helps, I'd be happy to submit a PR with failing tests, but I don't think I'm in a position to make the actual changes.

@nicolo-ribaudo
Copy link
Member

The problem with the old approach was that we had to manually specify for each node the possible locations of inner comments, and we often missed some of them.

With the new approach, the generator prints the inner comments in the first position that cannot be represented as leading/trailing comments of something (because if the comments were there, they wouldn't be marked as "inner comments").

I agree that we should always fallback to printing comments somewhere, even if we cannot figure out the precise location.

@overlookmotel
Copy link
Contributor Author

overlookmotel commented Nov 5, 2022

Ah I see. That makes sense. Is there ability to manually override and flush comments earlier?

By the way, the first example I gave where inner comments result in (/*foo*/function(){}) would seem to break the rule of printing inner comments "in the first position that cannot be represented as leading/trailing comments". This position can be leading comments.

@nicolo-ribaudo
Copy link
Member

That bug is because the printer considered ( as part of the node, and so assumed that leading comments would have been before (.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: comments i: bug i: discussion i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator
Projects
None yet
4 participants