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.

Sign up for GitHub

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

Jump to bottom

[tsx] raise error on single arrow type argument without comma #14135

Merged
merged 12 commits into from Feb 27, 2022
Merged

[tsx] raise error on single arrow type argument without comma #14135

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a7fd060
code is adapted to babel 8
ozanhonamlioglu Jan 12, 2022
0b0d2e9
some tests are updated for babel 8
ozanhonamlioglu Jan 12, 2022
117e71d
typescript parser tests are fixed
ozanhonamlioglu Jan 13, 2022
aa29eb1
check is moved into arrow parse
ozanhonamlioglu Jan 13, 2022
c22c322
babel 8 support check is added
ozanhonamlioglu Jan 14, 2022
4621b27
hasSingleTypeError changed as invalidSingleType
ozanhonamlioglu Jan 15, 2022
c7d16af
adopted for new raise pattern
ozanhonamlioglu Jan 19, 2022
a0df5f9
column location fix
ozanhonamlioglu Jan 19, 2022
07083a8
code simplifying fix
ozanhonamlioglu Feb 23, 2022
c396454
conflicts are solved
ozanhonamlioglu Feb 23, 2022
6a2e55d
tests are overwritten
ozanhonamlioglu Feb 23, 2022
15059cc
Update packages/babel-parser/src/plugins/typescript/index.js
nicolo-ribaudo Feb 24, 2022
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
2 changes: 1 addition & 1 deletion packages/babel-generator/test/fixtures/typescript/arrow-function-generic-tsx/input.js
View file
Open in desktop
@@ -1,2 +1,2 @@
// Same as `generic`. Verify that JSX doesn't change things.
<T>(a: T): T => a;
<T,>(a: T): T => a;
28 changes: 28 additions & 0 deletions packages/babel-parser/src/plugins/typescript/index.js
View file
Open in desktop
Expand Up @@ -148,6 +148,8 @@ const TSErrors = makeErrorTemplates(
"A 'set' accessor cannot have rest parameter.",
SetAccesorCannotHaveReturnType:
"A 'set' accessor cannot have a return type annotation.",
SingleTypeParameterWithoutTrailingComma:
"Single type parameter %0 should have a trailing comma. Example usage: <%0,>.",
StaticBlockCannotHaveModifier:
"Static class blocks cannot have any modifier.",
TypeAnnotationAfterAssign:
Expand Down Expand Up @@ -2922,6 +2924,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// Either way, we're looking at a '<': tt.jsxTagStart or relational.

let typeParameters: ?N.TsTypeParameterDeclaration;
let invalidSingleType: ?N.TsTypeParameter;
state = state || this.state.clone();

const arrow = this.tryParse(abort => {
Expand All @@ -2941,9 +2944,34 @@ export default (superClass: Class<Parser>): Class<Parser> =>
this.resetStartLocationFromNode(expr, typeParameters);
}
expr.typeParameters = typeParameters;

// report error if single type parameter used without trailing comma.
if (
this.hasPlugin("jsx") &&
Copy link

Choose a reason for hiding this comment

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

Is it possible to check it only in tsx file? @ozanhonamlioglu
(#14361 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo Sounds logical to me, shall I change it with tsx? What is your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

TSX is "jsx in TypeScript". This check is only executed when TypeScript is enabled, so it's already only checking for tsx.

Copy link

Choose a reason for hiding this comment

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

TSX is "jsx in TypeScript". This check is only executed when TypeScript is enabled, so it's already only checking for tsx.

In our project, we used the same presets (@babel/preset-env + @babel/preset-react + @babel/preset-typescript) to transform all the files, which caused the .ts files to be processed by the jsx plugin, so the generics in the .ts files also triggered the SingleTypeParameterWithoutTrailingComma error.

This is a wrong usage, we will improve it. Thank you! @ozanhonamlioglu @nicolo-ribaudo

expr.typeParameters.params.length === 1 &&
!expr.typeParameters.extra?.trailingComma
) {
const parameter = expr.typeParameters.params[0];
if (parameter.constraint) {
// If parameter has any constraints, it must contain multiple tokens.
// <T extends U> is a valid declaration.
// <T extends {name: string}> is also a valid declaration.
} else invalidSingleType = parameter;
nicolo-ribaudo marked this conversation as resolved.
Show resolved Hide resolved
}

return expr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the check before this line? It should simplify arrow.node checks and we have typeParameters ready. We can leak isSingleTypeWithoutTrailingComma out of tryParse and throw afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course

}, state);

if (invalidSingleType) {
this.raise(
TSErrors.SingleTypeParameterWithoutTrailingComma,
{ at: this.state.startLoc },
process.env.BABEL_8_BREAKING
? invalidSingleType.name.name
: invalidSingleType.name,
);
}

/*:: invariant(arrow.node != null) */
if (!arrow.error && !arrow.aborted) {
// This error is reported outside of the this.tryParse call so that
Expand Down
3 changes: 3 additions & 0 deletions ...ages/babel-parser/test/fixtures/typescript/arrow-function/generic-tsx-babel-7/output.json
View file
Open in desktop
@@ -1,6 +1,9 @@
{
"type": "File",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":18}},
"errors": [
"SyntaxError: Single type parameter T should have a trailing comma. Example usage: <T,>. (2:17)"
Copy link
Member

Choose a reason for hiding this comment

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

Uh, this made me realize that this.state.startLoc doesn't work because we didn't just finish parsing the type parameters, but the whole arrow. I guess we can go back to { at: invalidSingleType.end } 🙁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right, I actually realized that something wrong with that. Taking it back so and will update the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo but now it says 2:2 actually it should be 2:3 right? { at: invalidSingleType.loc.end }

Copy link
Member

Choose a reason for hiding this comment

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

2:2 is way better than 2:17 🤷

If we still want to report 2:3 (which is slightly better than 2:2, since it's consistent with the "unexpected token, expected ," errors) we could store this.state.startLoc after the typeParameters = this.tsParseTypeParameters(); call a few lines above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did something different, lets see if it passes from tests, and if a valid usage for you also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo do you think it is okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @JLHwung can you give a last review for the PR? thanks

],
"program": {
"type": "Program",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":18}},
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-parser/test/fixtures/typescript/arrow-function/generic-tsx/input.ts
View file
Open in desktop
@@ -1,2 +1,2 @@
// Same as `generic`. Verify that JSX doesn't change things.
<T>(a: T): T => a;
<T,>(a: T): T => a;
31 changes: 17 additions & 14 deletions packages/babel-parser/test/fixtures/typescript/arrow-function/generic-tsx/output.json
View file
Open in desktop
@@ -1,27 +1,27 @@
{
"type": "File",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":18}},
"start":0,"end":80,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":19}},
"program": {
"type": "Program",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":18}},
"start":0,"end":80,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":19}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "ExpressionStatement",
"start":61,"end":79,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":18}},
"start":61,"end":80,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":19}},
"expression": {
"type": "ArrowFunctionExpression",
"start":61,"end":78,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":17}},
"start":61,"end":79,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":18}},
"returnType": {
"type": "TSTypeAnnotation",
"start":70,"end":73,"loc":{"start":{"line":2,"column":9},"end":{"line":2,"column":12}},
"start":71,"end":74,"loc":{"start":{"line":2,"column":10},"end":{"line":2,"column":13}},
"typeAnnotation": {
"type": "TSTypeReference",
"start":72,"end":73,"loc":{"start":{"line":2,"column":11},"end":{"line":2,"column":12}},
"start":73,"end":74,"loc":{"start":{"line":2,"column":12},"end":{"line":2,"column":13}},
"typeName": {
"type": "Identifier",
"start":72,"end":73,"loc":{"start":{"line":2,"column":11},"end":{"line":2,"column":12},"identifierName":"T"},
"start":73,"end":74,"loc":{"start":{"line":2,"column":12},"end":{"line":2,"column":13},"identifierName":"T"},
"name": "T"
}
}
Expand All @@ -32,17 +32,17 @@
"params": [
{
"type": "Identifier",
"start":65,"end":69,"loc":{"start":{"line":2,"column":4},"end":{"line":2,"column":8},"identifierName":"a"},
"start":66,"end":70,"loc":{"start":{"line":2,"column":5},"end":{"line":2,"column":9},"identifierName":"a"},
"name": "a",
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":66,"end":69,"loc":{"start":{"line":2,"column":5},"end":{"line":2,"column":8}},
"start":67,"end":70,"loc":{"start":{"line":2,"column":6},"end":{"line":2,"column":9}},
"typeAnnotation": {
"type": "TSTypeReference",
"start":68,"end":69,"loc":{"start":{"line":2,"column":7},"end":{"line":2,"column":8}},
"start":69,"end":70,"loc":{"start":{"line":2,"column":8},"end":{"line":2,"column":9}},
"typeName": {
"type": "Identifier",
"start":68,"end":69,"loc":{"start":{"line":2,"column":7},"end":{"line":2,"column":8},"identifierName":"T"},
"start":69,"end":70,"loc":{"start":{"line":2,"column":8},"end":{"line":2,"column":9},"identifierName":"T"},
"name": "T"
}
}
Expand All @@ -51,12 +51,12 @@
],
"body": {
"type": "Identifier",
"start":77,"end":78,"loc":{"start":{"line":2,"column":16},"end":{"line":2,"column":17},"identifierName":"a"},
"start":78,"end":79,"loc":{"start":{"line":2,"column":17},"end":{"line":2,"column":18},"identifierName":"a"},
"name": "a"
},
"typeParameters": {
"type": "TSTypeParameterDeclaration",
"start":61,"end":64,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":3}},
"start":61,"end":65,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":4}},
"params": [
{
"type": "TSTypeParameter",
Expand All @@ -67,7 +67,10 @@
"name": "T"
}
}
]
],
"extra": {
"trailingComma": 63
}
}
},
"leadingComments": [
Expand Down