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

Don't convert line comments containing */ to block comments #15135

Merged
merged 1 commit into from Nov 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/babel-generator/src/generators/methods.ts
Expand Up @@ -10,7 +10,6 @@ export function _params(
this.token("(");
this._parameters(node.params, node);
this.token(")");
this._noLineTerminator = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix, can you confirm it? I'm not sure about the purpose here.
@nicolo-ribaudo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In arrow functions, ) cannot be followed by a newline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it doesn't matter because inner comments in arrow functions would already be printer earlier.


this.print(node.returnType, node, node.type === "ArrowFunctionExpression");
}
Expand Down
43 changes: 28 additions & 15 deletions packages/babel-generator/src/printer.ts
Expand Up @@ -23,6 +23,7 @@ const ZERO_DECIMAL_INTEGER = /\.0+$/;
const NON_DECIMAL_LITERAL = /^0[box]/;
const PURE_ANNOTATION_RE = /^\s*[@#]__PURE__\s*$/;
const HAS_NEWLINE = /[\n\r\u2028\u2029]/;
const HAS_BlOCK_COMMENT_END = /\*\//;

const { needsParens } = n;

Expand Down Expand Up @@ -904,7 +905,13 @@ class Printer {

if (this._printedComments.has(comment)) return false;

if (this._noLineTerminator && HAS_NEWLINE.test(comment.value)) {
const noLineTerminator = this._noLineTerminator;

if (
noLineTerminator &&
(HAS_NEWLINE.test(comment.value) ||
HAS_BlOCK_COMMENT_END.test(comment.value))
) {
Comment on lines -907 to +914
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the fix for the root cause, see the test.

return true;
}

Expand Down Expand Up @@ -957,7 +964,7 @@ class Printer {

val = val.replace(/\n(?!$)/g, `\n${" ".repeat(indentSize)}`);
}
} else if (!this._noLineTerminator) {
} else if (!noLineTerminator) {
val = `//${comment.value}`;
} else {
// It was a single-line comment, so it's guaranteed to not
Expand All @@ -972,7 +979,7 @@ class Printer {
this.source("start", comment.loc);
this._append(val, isBlockComment);

if (!isBlockComment && !this._noLineTerminator) {
if (!isBlockComment && !noLineTerminator) {
this.newline(1, true);
}

Expand All @@ -997,12 +1004,16 @@ class Printer {
const nodeEndLine = hasLoc ? nodeLoc.end.line : 0;
let lastLine = 0;
let leadingCommentNewline = 0;
const { _noLineTerminator } = this;

const maybeNewline = this._noLineTerminator
? function () {}
: this.newline.bind(this);

for (let i = 0; i < len; i++) {
const comment = comments[i];

if (hasLoc && comment.loc && !this._printedComments.has(comment)) {
const printed = this._printedComments.has(comment);
if (hasLoc && comment.loc && !printed) {
const commentStartLine = comment.loc.start.line;
const commentEndLine = comment.loc.end.line;
if (type === COMMENT_TYPE.LEADING) {
Expand All @@ -1022,11 +1033,11 @@ class Printer {
}
lastLine = commentEndLine;

if (!_noLineTerminator) this.newline(offset);
maybeNewline(offset);
this._printComment(comment, COMMENT_SKIP_NEWLINE.SKIP_ALL);

if (!_noLineTerminator && i + 1 === len) {
this.newline(
if (i + 1 === len) {
maybeNewline(
Math.max(nodeStartLine - lastLine, leadingCommentNewline),
);
lastLine = nodeStartLine;
Expand All @@ -1036,28 +1047,30 @@ class Printer {
commentStartLine - (i === 0 ? nodeStartLine : lastLine);
lastLine = commentEndLine;

if (!_noLineTerminator) this.newline(offset);
maybeNewline(offset);
if (this._printComment(comment, COMMENT_SKIP_NEWLINE.SKIP_ALL)) break;

if (!_noLineTerminator && i + 1 === len) {
this.newline(Math.min(1, nodeEndLine - lastLine)); // TODO: Improve here when inner comments processing is stronger
if (i + 1 === len) {
maybeNewline(Math.min(1, nodeEndLine - lastLine)); // TODO: Improve here when inner comments processing is stronger
lastLine = nodeEndLine;
}
} else {
const offset =
commentStartLine - (i === 0 ? nodeEndLine - lineOffset : lastLine);
lastLine = commentEndLine;

if (!_noLineTerminator) this.newline(offset);
maybeNewline(offset);
this._printComment(comment, COMMENT_SKIP_NEWLINE.SKIP_ALL);
}
} else {
hasLoc = false;

if (printed) continue;

if (len === 1) {
const singleLine = comment.loc
? comment.loc.start.line === comment.loc.end.line
: !comment.value.includes("\n");
: !HAS_NEWLINE.test(comment.value);

const shouldSkipNewline =
singleLine &&
Expand Down Expand Up @@ -1091,15 +1104,15 @@ class Printer {
// /*:: b: ?string*/
// }

const skippedDueToNewlie = this._printComment(
const skippedDueToNewline = this._printComment(
comment,
i === 0
? COMMENT_SKIP_NEWLINE.SKIP_LEADING
: i === len - 1
? COMMENT_SKIP_NEWLINE.SKIP_TRAILING
: COMMENT_SKIP_NEWLINE.DEFAULT,
);
if (skippedDueToNewlie) break;
if (skippedDueToNewline) break;
} else {
this._printComment(comment, COMMENT_SKIP_NEWLINE.DEFAULT);
}
Expand Down
@@ -0,0 +1,9 @@
// https://github.com/babel/babel/issues/15132

var gen_bitlen = function gen_bitlen(s, desc) // deflate_state *s;
// tree_desc *desc; /* the tree descriptor */
{
};
async (
// 11111 /* */
) => {};
@@ -0,0 +1,9 @@
// https://github.com/babel/babel/issues/15132

var gen_bitlen = function gen_bitlen(s, desc)
// deflate_state *s;
// tree_desc *desc; /* the tree descriptor */
{};
async (
// 11111 /* */
) => {};