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 all 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;
32 changes: 31 additions & 1 deletion packages/babel-parser/src/plugins/typescript/index.js
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ import {
} from "../../tokenizer/types";
import { types as tc } from "../../tokenizer/context";
import * as N from "../../types";
import type { Position } from "../../util/location";
import { Position, createPositionWithColumnOffset } from "../../util/location";
import type Parser from "../../parser";
import {
type BindingTypes,
Expand Down 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 @@ -2933,6 +2935,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 @@ -2952,9 +2955,36 @@ 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) {
// A single type parameter must either have constraints
// or a trailing comma, otherwise it's ambiguous with JSX.
invalidSingleType = parameter;
}
}

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: createPositionWithColumnOffset(invalidSingleType.loc.end, 1),
},
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,"index":0},"end":{"line":2,"column":18,"index":79}},
"errors": [
"SyntaxError: Single type parameter T should have a trailing comma. Example usage: <T,>. (2:3)"
],
"program": {
"type": "Program",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":2,"column":18,"index":79}},
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,"index":0},"end":{"line":2,"column":18,"index":79}},
"start":0,"end":80,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":2,"column":19,"index":80}},
"program": {
"type": "Program",
"start":0,"end":79,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":2,"column":18,"index":79}},
"start":0,"end":80,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":2,"column":19,"index":80}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "ExpressionStatement",
"start":61,"end":79,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":18,"index":79}},
"start":61,"end":80,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":19,"index":80}},
"expression": {
"type": "ArrowFunctionExpression",
"start":61,"end":78,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":17,"index":78}},
"start":61,"end":79,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":18,"index":79}},
"returnType": {
"type": "TSTypeAnnotation",
"start":70,"end":73,"loc":{"start":{"line":2,"column":9,"index":70},"end":{"line":2,"column":12,"index":73}},
"start":71,"end":74,"loc":{"start":{"line":2,"column":10,"index":71},"end":{"line":2,"column":13,"index":74}},
"typeAnnotation": {
"type": "TSTypeReference",
"start":72,"end":73,"loc":{"start":{"line":2,"column":11,"index":72},"end":{"line":2,"column":12,"index":73}},
"start":73,"end":74,"loc":{"start":{"line":2,"column":12,"index":73},"end":{"line":2,"column":13,"index":74}},
"typeName": {
"type": "Identifier",
"start":72,"end":73,"loc":{"start":{"line":2,"column":11,"index":72},"end":{"line":2,"column":12,"index":73},"identifierName":"T"},
"start":73,"end":74,"loc":{"start":{"line":2,"column":12,"index":73},"end":{"line":2,"column":13,"index":74},"identifierName":"T"},
"name": "T"
}
}
Expand All @@ -32,17 +32,17 @@
"params": [
{
"type": "Identifier",
"start":65,"end":69,"loc":{"start":{"line":2,"column":4,"index":65},"end":{"line":2,"column":8,"index":69},"identifierName":"a"},
"start":66,"end":70,"loc":{"start":{"line":2,"column":5,"index":66},"end":{"line":2,"column":9,"index":70},"identifierName":"a"},
"name": "a",
"typeAnnotation": {
"type": "TSTypeAnnotation",
"start":66,"end":69,"loc":{"start":{"line":2,"column":5,"index":66},"end":{"line":2,"column":8,"index":69}},
"start":67,"end":70,"loc":{"start":{"line":2,"column":6,"index":67},"end":{"line":2,"column":9,"index":70}},
"typeAnnotation": {
"type": "TSTypeReference",
"start":68,"end":69,"loc":{"start":{"line":2,"column":7,"index":68},"end":{"line":2,"column":8,"index":69}},
"start":69,"end":70,"loc":{"start":{"line":2,"column":8,"index":69},"end":{"line":2,"column":9,"index":70}},
"typeName": {
"type": "Identifier",
"start":68,"end":69,"loc":{"start":{"line":2,"column":7,"index":68},"end":{"line":2,"column":8,"index":69},"identifierName":"T"},
"start":69,"end":70,"loc":{"start":{"line":2,"column":8,"index":69},"end":{"line":2,"column":9,"index":70},"identifierName":"T"},
"name": "T"
}
}
Expand All @@ -51,12 +51,12 @@
],
"body": {
"type": "Identifier",
"start":77,"end":78,"loc":{"start":{"line":2,"column":16,"index":77},"end":{"line":2,"column":17,"index":78},"identifierName":"a"},
"start":78,"end":79,"loc":{"start":{"line":2,"column":17,"index":78},"end":{"line":2,"column":18,"index":79},"identifierName":"a"},
"name": "a"
},
"typeParameters": {
"type": "TSTypeParameterDeclaration",
"start":61,"end":64,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":3,"index":64}},
"start":61,"end":65,"loc":{"start":{"line":2,"column":0,"index":61},"end":{"line":2,"column":4,"index":65}},
"params": [
{
"type": "TSTypeParameter",
Expand All @@ -67,7 +67,10 @@
"name": "T"
}
}
]
],
"extra": {
"trailingComma": 63
}
}
},
"leadingComments": [
Expand Down
2 changes: 1 addition & 1 deletion test/jest-light-runner/src/worker-runner.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import { performance } from "perf_hooks";
import snapshot from "jest-snapshot";
import expect from "expect";
import * as circus from "jest-circus";
import { inspect } from "util"
import { inspect } from "util";

import "./global-setup.js";

Expand Down