Skip to content

fix: Types containing comments generate invalid code#14762

Merged
nicolo-ribaudo merged 2 commits intobabel:mainfrom
liuxingbaoyu:fix-type-with-comment
Jul 18, 2022
Merged

fix: Types containing comments generate invalid code#14762
nicolo-ribaudo merged 2 commits intobabel:mainfrom
liuxingbaoyu:fix-type-with-comment

Conversation

@liuxingbaoyu
Copy link
Copy Markdown
Member

@liuxingbaoyu liuxingbaoyu commented Jul 15, 2022

Q                       A
Fixed Issues? Fixes #14751
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR also contains some very minor performance optimizations.

@liuxingbaoyu liuxingbaoyu added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: generator labels Jul 15, 2022
Comment on lines 13 to 15

if (node.returnType) {
if (node.type === "ArrowFunctionExpression") {
this._noLineTerminator = true;
this.print(node.returnType, node);
this._noLineTerminator = false;
} else {
this.print(node.returnType, node);
}
}
this.print(node.returnType, node);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reverted here #14758.

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Jul 15, 2022

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

Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I think it would be enough to disallow a line terminator in the following positions:

  • After the returnType of an ArrowFunctionExpression
  • After the typeName of a TSTypeReference with params
  • After the elementType of a TSArrayType
  • After the objectType of a TSIndexedAccessType

Also, could you add these generator tests with retainLines: true?

type T = U.
/* 1 */
C /* 2 */ [
  /* 3 */
  0];
type T = U.
/* 1 */
C /* 2 */ [
  /* 3 */
];
type T = U.
/* 1 */
C /* 2 */ <
  /* 3 */
  0>;
let f = (x)
/* 1 */
:
/* 2 */
void /* 3 */ => 1

@liuxingbaoyu
Copy link
Copy Markdown
Member Author

There seem to be two ways, one is like #14758 and the other is using _printStack in the print method, what do you think?
In addition this problem seems to exist in the flow, do we still need to consider it? (I don't know anything about flow, it will need your detailed guidance)

@nicolo-ribaudo
Copy link
Copy Markdown
Member

nicolo-ribaudo commented Jul 15, 2022

I prefer the #14758 approach, since it lets us get a "better" output where we can print newlines.

The flow rules are the same, except that the AST nodes are ArrayTypeAnnotation, IndexedAccessType and GenericTypeAnnotation (you can test the example above on https://astexplorer.net).

Comment on lines +6 to +11

type T2 = U.
/* 1 */
C /* 2 */[];


Copy link
Copy Markdown
Member Author

@liuxingbaoyu liuxingbaoyu Jul 15, 2022

Choose a reason for hiding this comment

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

There seems to be a bug here.

update: maybe not

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, the /* 3 */ comment should be printed somewhere. It's a bug that we already have regardless of this PR.

Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit 770c22f into babel:main Jul 18, 2022
@liuxingbaoyu liuxingbaoyu changed the title fix: TypeAnnotation with comments generates incorrect code fix: Types containing comments generate invalid code Jul 18, 2022
@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 Oct 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: flow area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Babel produces invalid TypeScript code if comment follows type annotation and is not stripped

4 participants