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

ozanhonamlioglu · 2022-01-11T20:45:40Z

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

In this issue #13778, we solved trailing comma end of single type usage in typescript plugin. But we didn't raised any error yet to notice developer about their wrong usage. Now with this PR, we are informing developer via raising an error.

Here is demo code

// INPUT
<T>(a: T): T => a; 

// ERROR
"Single type definition T should have a trailing comma end of it. Example usage: <T,>."

@nicolo-ribaudo you mentioned the bug in the discussion.
#14113 (comment)

babel-bot · 2022-01-11T20:48:01Z

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

nicolo-ribaudo · 2022-01-11T21:04:57Z

packages/babel-parser/src/plugins/typescript/index.js

+      ) {
+        const typeName = arrow.node.typeParameters.params[0].name;
+        const start = arrow.node.start;
+        this.raise(start, TSErrors.SingleTypeWithoutTrailingComma, typeName);


The Babel 8 tests (you can run them with BABEL_8_BREAKING=true yarn jest parser) are failing, I think it's becaus typeName isn't a string anymore but a node: #12829

nicolo-ribaudo · 2022-01-11T22:08:19Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -2930,6 +2932,17 @@ export default (superClass: Class<Parser>): Class<Parser> =>
        return expr;
      }, state);

+      // report error if single type parameter used without trailing comma.
+      if (
+        arrow.node?.type === "ArrowFunctionExpression" &&


We should only report the error if this.hasPlugin("jsx")

nicolo-ribaudo · 2022-01-11T22:08:53Z

packages/babel-parser/src/plugins/typescript/index.js

+      // report error if single type parameter used without trailing comma.
+      if (
+        arrow.node?.type === "ArrowFunctionExpression" &&
+        arrow.node?.typeParameters?.params.length === 1 &&


After the previous check we know that node is defined, so we don't need the first ?.. Also, we don't need it after .typeParameters in the next check.

ozanhonamlioglu · 2022-01-12T20:03:07Z

I have updated the PR to adapt to babel 8 features.

I have a question but it can maybe out of topic @nicolo-ribaudo

What do you suggest me to be good on Babel library? I started to educate myself on compilers and interpreters with a book named "Crafting Interpreters". The book is so good, it teach me so much stuff about compilers. But do you have any suggestion specificly on Babel.

Thanks

nicolo-ribaudo · 2022-01-13T12:54:48Z

We have some videos about how Babel works at https://babeljs.io/videos, and https://github.com/jamiebuilds/babel-handbook shows different methods that are used in Babel plugins. Unfortunately we don't have much more 😅

I started to educate myself on compilers and interpreters with a book named "Crafting Interpreters". The book is so good, it teach me so much stuff about compilers.

I will probably buy it too one day 👀

nicolo-ribaudo · 2022-01-13T12:56:59Z

CI is now failing on this test: https://github.com/microsoft/TypeScript/blob/main/tests/cases/compiler/jsxChildrenGenericContextualTypes.tsx

I think it's because of <T extends string>(p: LitProps<T>) => <div></div>: apparently in that case the comma is not required, so we shouldn't throw an error if there is extends.

ozanhonamlioglu · 2022-01-13T12:58:03Z

CI is now failing on this test: https://github.com/microsoft/TypeScript/blob/main/tests/cases/compiler/jsxChildrenGenericContextualTypes.tsx

I think it's because of <T extends string>(p: LitProps<T>) => <div></div>: apparently in that case the comma is not required, so we shouldn't throw an error if there is extends.

Oh you right

nicolo-ribaudo · 2022-01-13T12:59:28Z

Btw, you can run that external TS tests with make test-typescript (you first need to run make bootstrap-typescript the first time, to clone the TS repo in the correct folder).

JLHwung · 2022-01-13T20:06:59Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -2930,6 +2932,33 @@ export default (superClass: Class<Parser>): Class<Parser> =>
        return expr;


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.

JLHwung · 2022-01-13T20:17:36Z

packages/babel-parser/src/plugins/typescript/index.js

+          const node = parameter.name;
+          let identifierName = "";
+          if (typeof node === "object") identifierName = node.name;
+          else identifierName = node;


Suggested change

const node = parameter.name;

let identifierName = "";

if (typeof node === "object") identifierName = node.name;

else identifierName = node;

const identifierName = process.env.BABEL_8_BREAKING ? parameter.name.name : parameter.name;

So we can remove Babel 7 AST support when we release Babel 8.

JLHwung · 2022-01-13T20:22:12Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -148,6 +148,8 @@ const TSErrors = makeErrorTemplates(
      "A 'set' accessor cannot have rest parameter.",
    SetAccesorCannotHaveReturnType:
      "A 'set' accessor cannot have a return type annotation.",
+    SingleTypeWithoutTrailingComma:
+      "Single type definition %0 should have a trailing comma end of it. Example usage: <%0,>.",


Suggested change

"Single type definition %0 should have a trailing comma end of it. Example usage: <%0,>.",

"Single type parameter %0 should have a trailing comma. Example usage: <%0,>.",

JLHwung · 2022-01-13T20:24:40Z

packages/babel-parser/src/plugins/typescript/index.js

+      ) {
+        const parameter = arrow.node.typeParameters.params[0];
+        if (parameter.constraint) {
+          // If Type has any constraints, means that it is not alone their.


Suggested change

// If Type has any constraints, means that it is not alone their.

// If parameter has any constraints, it must contain multiple tokens

ozanhonamlioglu · 2022-01-14T20:32:58Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -2908,6 +2910,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
      // Either way, we're looking at a '<': tt.jsxTagStart or relational.

      let typeParameters: ?N.TsTypeParameterDeclaration;
+      let hasSingleTypeError = { status: false, start: 0, identifierName: "" };


@JLHwung I needed to raise error outside const arrow = .... method because it should return expr end of it instead an error.
This just a quick type which is defined in here but if you suggest me to move the type to anywhere else, that is okay too.

Can we do let invalidSingleType: ?N.TSTypeParameter, and then just do invalidSingleType = parameter when it's invalid? It's similar to what we already do with the typeParameters variable, and we avoid allocationg a few objects unnecessary.

nicolo-ribaudo · 2022-01-15T14:17:21Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -148,6 +148,8 @@ const TSErrors = makeErrorTemplates(
      "A 'set' accessor cannot have rest parameter.",
    SetAccesorCannotHaveReturnType:
      "A 'set' accessor cannot have a return type annotation.",
+    SingleTypeWithoutTrailingComma:


Nit: I'd prefer to name this SingleTypeParameterWithoutTrailingComma

nicolo-ribaudo · 2022-01-15T14:21:10Z

packages/babel-parser/src/plugins/typescript/index.js

@@ -2908,6 +2910,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
      // Either way, we're looking at a '<': tt.jsxTagStart or relational.

      let typeParameters: ?N.TsTypeParameterDeclaration;
+      let hasSingleTypeError = { status: false, start: 0, identifierName: "" };


Can we do let invalidSingleType: ?N.TSTypeParameter, and then just do invalidSingleType = parameter when it's invalid? It's similar to what we already do with the typeParameters variable, and we avoid allocationg a few objects unnecessary.

nicolo-ribaudo · 2022-01-15T14:21:42Z

packages/babel-parser/src/plugins/typescript/index.js

        return expr;
      }, state);

+      if (hasSingleTypeError.status) {
+        this.raise(
+          hasSingleTypeError.start,


Rather than at .start, we could report the error at parameter.end + 1 which is where we expect the trailing comma.

nicolo-ribaudo

Thanks!

nicolo-ribaudo · 2022-01-18T12:40:36Z

packages/babel-parser/src/plugins/typescript/index.js

+        this.raise(
+          invalidSingleType.end + 1,


In 478a970 we changed the signature of .raise; could you rebase this PR? 🙏 (otherwise I can do it)

As the new location, you can probably use { at: this.state.startLoc } (which is the beginning of the token after the type parameter).

Hi @nicolo-ribaudo , of course I can do it. I Will do it today night. But if you have any urgency, you can do it for me as well.

nicolo-ribaudo · 2022-01-19T18:15:34Z

packages/babel-parser/test/fixtures/typescript/arrow-function/generic-tsx-babel-7/output.json

@@ -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)"


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 } 🙁

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

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

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.

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

@nicolo-ribaudo do you think it is okay?

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

JLHwung · 2022-01-24T19:51:14Z

packages/babel-parser/src/plugins/typescript/index.js

+          {
+            at: {
+              ...invalidSingleType.loc.end,
+              column: invalidSingleType.loc.end.column + 1,


There is createPositionWithColumnOffset in ./util/location serving the same purpose.

Oh I didn't know that. So you want me to use that method for the position and update the PR right?
@JLHwung

Yes, please!

ozanhonamlioglu · 2022-02-23T21:22:08Z

@nicolo-ribaudo @JLHwung Hey guys
Sorry for delay but I had soo much busy days over the last weeks. I wan't to keep moving from where I left and want to solve more issues...
Can you review my PR and give some comments or merge please... I just updated it
Thanks

packages/babel-parser/src/plugins/typescript/index.js

Thulof · 2022-03-16T03:09:01Z

packages/babel-parser/src/plugins/typescript/index.js

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


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

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

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

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

nicolo-ribaudo added area: typescript pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels Jan 11, 2022

nicolo-ribaudo reviewed Jan 11, 2022

View reviewed changes

nicolo-ribaudo mentioned this pull request Jan 11, 2022

Missing trailing comma in TSX arrow single type param prettier/prettier#12076

Closed

nicolo-ribaudo reviewed Jan 11, 2022

View reviewed changes

JLHwung reviewed Jan 13, 2022

View reviewed changes

ozanhonamlioglu commented Jan 14, 2022

View reviewed changes

ozanhonamlioglu requested review from JLHwung and nicolo-ribaudo January 14, 2022 21:29

nicolo-ribaudo reviewed Jan 15, 2022

View reviewed changes

ozanhonamlioglu requested a review from nicolo-ribaudo January 15, 2022 20:23

nicolo-ribaudo approved these changes Jan 16, 2022

View reviewed changes

nicolo-ribaudo reviewed Jan 18, 2022

View reviewed changes

ozanhonamlioglu added 7 commits January 19, 2022 15:09

code is adapted to babel 8

a7fd060

some tests are updated for babel 8

0b0d2e9

typescript parser tests are fixed

117e71d

check is moved into arrow parse

aa29eb1

babel 8 support check is added

c22c322

hasSingleTypeError changed as invalidSingleType

4621b27

adopted for new raise pattern

c7d16af

ozanhonamlioglu requested a review from nicolo-ribaudo January 19, 2022 13:33

nicolo-ribaudo reviewed Jan 19, 2022

View reviewed changes

column location fix

a0df5f9

ozanhonamlioglu requested a review from nicolo-ribaudo January 20, 2022 06:55

JLHwung reviewed Jan 24, 2022

View reviewed changes

ozanhonamlioglu added 2 commits February 23, 2022 23:54

code simplifying fix

07083a8

conflicts are solved

c396454

tests are overwritten

6a2e55d

ozanhonamlioglu requested a review from JLHwung February 24, 2022 07:25

nicolo-ribaudo approved these changes Feb 24, 2022

View reviewed changes

packages/babel-parser/src/plugins/typescript/index.js Outdated Show resolved Hide resolved

Update packages/babel-parser/src/plugins/typescript/index.js

15059cc

JLHwung approved these changes Feb 25, 2022

View reviewed changes

nicolo-ribaudo changed the title ~~raise error on single type usage without comma~~ [tsx] raise error on single arrow type argument without comma Feb 27, 2022

nicolo-ribaudo merged commit 5749c16 into babel:main Feb 27, 2022

Thulof reviewed Mar 16, 2022

View reviewed changes

nicolo-ribaudo mentioned this pull request Mar 17, 2022

Defer <T>() => {} TSX error to Babel 8 #14367

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 16, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2022

		@@ -2930,6 +2932,33 @@ export default (superClass: Class<Parser>): Class<Parser> =>
		return expr;

	"Single type definition %0 should have a trailing comma end of it. Example usage: <%0,>.",
	"Single type parameter %0 should have a trailing comma. Example usage: <%0,>.",

	// If Type has any constraints, means that it is not alone their.
	// If parameter has any constraints, it must contain multiple tokens

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

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

Conversation

ozanhonamlioglu commented Jan 11, 2022 • edited by gitpod-io bot

babel-bot commented Jan 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozanhonamlioglu commented Jan 12, 2022 • edited

nicolo-ribaudo commented Jan 13, 2022 • edited

nicolo-ribaudo commented Jan 13, 2022

ozanhonamlioglu commented Jan 13, 2022

nicolo-ribaudo commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Jan 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozanhonamlioglu Jan 19, 2022 • edited

Choose a reason for hiding this comment

nicolo-ribaudo Jan 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozanhonamlioglu commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozanhonamlioglu commented Jan 11, 2022 •

edited by gitpod-io bot

babel-bot commented Jan 11, 2022 •

edited

ozanhonamlioglu commented Jan 12, 2022 •

edited

nicolo-ribaudo commented Jan 13, 2022 •

edited

nicolo-ribaudo Jan 19, 2022 •

edited

ozanhonamlioglu Jan 19, 2022 •

edited

nicolo-ribaudo Jan 19, 2022 •

edited