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

fix: Types containing comments generate invalid code #14762

Merged
merged 2 commits into from Jul 18, 2022

Conversation

liuxingbaoyu
Copy link
Contributor

@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

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
Contributor 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.

Reverted here #14758.

@babel-bot
Copy link
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
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

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
Contributor Author

liuxingbaoyu commented Jul 15, 2022

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
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).


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


Copy link
Contributor 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
Member

@nicolo-ribaudo nicolo-ribaudo Jul 16, 2022

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
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit 770c22f into babel:main Jul 18, 2022
38 checks passed
@liuxingbaoyu liuxingbaoyu changed the title fix: TypeAnnotation with comments generates incorrect code fix: Types containing comments generate invalid code Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: flow area: typescript 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.

4 participants