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
[ts 4.7] Support extends
constraints for infer
#14476
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51953/ |
@@ -1260,6 +1260,7 @@ export default (superClass: Class<Parser>): Class<Parser> => | |||
this.expectContextual(tt._infer); | |||
const typeParameter = this.startNode(); | |||
typeParameter.name = this.tsParseTypeParameterName(); | |||
typeParameter.constraint = this.tsEatThenParseType(tt._extends); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add this to the type definitions in https://github.com/babel/babel/blob/main/packages/babel-parser/src/types.js, in @babel/types
and in @babel/generator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,3 @@ | |||
{ | |||
"BABEL_8_BREAKING": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this behavior different in Babel 8? If it is, can you also add a Babel 8 test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In babel 8, the AST for type parameters will be changed. I'll add tests for babel 8 later!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7c7d753 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to handle the precedence of extends
vs ?
. Can you add a new test case
type X<U, T> = T extends [infer U extends T ? U : T] ? U : T;
It is valid in tsc 4.7.0.
Can you add a new test case? type X<U, T> = [(infer U extends T)?]; is invalid according to tsc. Currently Babel 7 was passing while Babel 8 is throwing. In microsoft/TypeScript#48112, tsc introduced a new parsing context |
f47bd4c
to
c9a8e60
Compare
@JLHwung Thank you for your review. I've refactored according to tsc implementation. c9a8e60
It is semantically invalid, but seems to parse successfully(TS Playground link). Should Babel throw a recoverable error for it? If Babel needs to throw a recoverable error for it, I think the fix should be in a separate PR. This is because even with infer without extends, tsc throws a similar semantic error(TS Playground link), and Babel does not throw an error as well(Babel repl link). |
c9a8e60
to
1509f4e
Compare
"type": "TSTypeParameter", | ||
"start":30,"end":46,"loc":{"start":{"line":1,"column":30,"index":30},"end":{"line":1,"column":46,"index":46}}, | ||
"name": "U", | ||
"constraint": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really surprised by this placement. I'd expect constraint
to be on TSInferType
, not on TSTypeParameter
. The new syntax is infer T extends U
, not just T extends U
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of adding a new constraint
field to TSInferType
, we should add a constraint
to the typeParamter
of TSInferType
. Because it is the type parameter, not the infer
, that is constrained.
FYI, the AST in TypeScript Compiler adds constaint
to TypeParameter
. (TypeScript AST Viewer Link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I really dislike many choices that the TS team made with their AST 😛
However, I realized that your AST design follows the shape we already have for type X<A extends B>
so it's fine.
@@ -0,0 +1,5 @@ | |||
type X3<T> = T extends [infer U extends number] ? MustBeNumber<U> : never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add some tests without tuples?
type X<U, T> = T extends infer U extends number ? U : T;
type X<U, T> = T extends (infer U extends number ? U : T) ? U : T;
1509f4e
to
a965c3e
Compare
828d3ad
to
7c29839
Compare
extends
constraints for infer
extends
constraints for infer
Support https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-beta/#extends-constraints-on-infer-type-variables