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

[ts] Add support for expr satisfies Type expressions #14211

Merged
merged 7 commits into from Oct 26, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 27, 2022

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

microsoft/TypeScript#46827 is in the TypeScript 4.6 iteration plan: I'm opening this PR so that we are ready in case it gets merged before Babel 7.17.0. However, I'm marking this PR as a draft because that one is still marked as a draft (@DanielRosenwasser, do you think that there is any chance that that PR will be actually included in 4.6, even if you already released a beta without it?).


I'm not convinced by the new AST node I introduced for expr satisfies Type expressions. It feels like it should be the same node as TSAsExpression, similarly to how in and instanceof both are BinaryExpressions. Ideally, something like this:

type TSBinaryExpression = { // or TSTypeExpression
  type: "TSBinaryExpression",
  operator: "as" | "satisfies",
  expression: Expression,
  typeAnnotation: TSType,
}

however, that would be a breaking change. I also considered just adding operator to TSAsExpression, but { type: "TSAsExpression", operator: "satisfies" } looks wrong.

Maybe we could go for the breaking change in Babel 8, and for now provide a TSBinaryExpression alias in @babel/types?

TSBinaryExpression or TSTypeExpression? The new node would not cover <Type> expr, because it needs to visit its childrens in a different order (type before expression).

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories area: typescript labels Jan 27, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jan 27, 2022

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

@nicolo-ribaudo nicolo-ribaudo changed the title Avoid lookahead when parsing TS type casts [ts] Add support for expr satisfies Type expressions Jan 27, 2022
@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft January 27, 2022 22:43
this.next(); // "as" or "satisfies"
if (this.match(tt._const)) {
if (isSatisfies) {
this.raise(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't have to specialize tt._const after tt._satisfies: tsParseType should throw when const is seen.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 28, 2022

do you think that there is any chance that that PR will be actually included in 4.6, even if you already released a beta without it?

This feature won't make its way into TypeScript 4.6 - it's still being iterated on, so if we can figure out the issues with it, 4.7 will be the earliest it can land.

@nicolo-ribaudo
Copy link
Member Author

Thank you!

@nicolo-ribaudo
Copy link
Member Author

The TS pull request has been merged, I'll rebase this.

@nicolo-ribaudo nicolo-ribaudo added this to the 7.19.0 milestone Aug 31, 2022
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review August 31, 2022 19:28
@nicolo-ribaudo
Copy link
Member Author

Depending on the 7.19 timing, we might need to postpone this to 7.20.

// name, since const isn't allowed as a type name. So in this instance we
// get to pretend we're the type checker.
if (typeReference.typeParameters) {
this.raise(TSErrors.CannotFindName, {
Copy link
Member

Choose a reason for hiding this comment

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

CannotFindName seems to be used only here.

typeAnnotation,
});
}
export { tsSatisfiesExpression as tSSatisfiesExpression };
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a bug.😕

edited: No, just a little ugly.🙃

@thorn0
Copy link
Contributor

thorn0 commented Oct 24, 2022

"Type expression" sounds like something that operates on types only whereas in as and satisfies one operand is a value and the other one a type.

@ngregory-rbi
Copy link

"Type expression" sounds like something that operates on types only...

I disagree. An adjective does not describe an operand but a property of the thing itself. In math, we don't call the addition symbol a "number expression". It operates on numbers, it itself is not a number.

This is a "type expression" because it expresses a type to either:

  • cast the operand value to, or
  • ensure the operand value satisfies

Makes sense to me.

@nicolo-ribaudo
Copy link
Member Author

The exposed names are TSAsExpression and TSSatisfiesExpression; TSTypeExpression is just an internal variable name.

@thorn0
Copy link
Contributor

thorn0 commented Oct 24, 2022

@ngregory-rbi You have a point, but in TypeScript there is this dichotomy between values and types. And when it's needed to specify one of these contexts, this kind of wording is used. E.g. "type operators" here.

On the other hand, I just noticed that the TS team strictly avoids using the word "expression" for type-only things, which makes TSTypeExpression a better choice than I thought.

we don't call the addition symbol a "number expression". It operates on numbers, it itself is not a number.

Can't help myself, excuse me :) , but what about "dog food" then? Is it a dog?

@ngregory-rbi
Copy link

You have a point, but in TypeScript there is this dichotomy between values and types. And when it's needed to specify one of these contexts, this kind of wording is used. E.g. "type operators" here.

Fair enough.

On the other hand, I just noticed that the TS team strictly avoids using the word "expression" for type-only things, which makes TSTypeExpression a better choice than I thought.

Not sure how your conclusion connects to the preceding statement.

Can't help myself, excuse me :) , but what about "dog food" then? Is it a dog?

Fair point as well. Touché.

Regardless of anything, wording may be a tangential subject to this PR.

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Oct 24, 2022
@thorn0
Copy link
Contributor

thorn0 commented Oct 24, 2022

Not sure how your conclusion connects to the preceding statement.

The preceding statement was against applying the wording "type expression" to as and satisfies whereas the following one was in support. Both statements are a result of checking what wording is used in the TS docs.

upd: an example where "type expression" has the meaning I expected.

@nicolo-ribaudo There are your questions about naming in the first post here, so I thought they were still relevant.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 24, 2022

Oh thanks, I forgot about it 🙃

Re-reading my original post, I don't like TSBinaryExpression because it feels like it should be an expression between two TS things, but this one is between a JS value and a type. For Babel 8, TSTypeExpression might be fine, however I'm open to alternative suggestions!

@ngregory-rbi
Copy link

@thorn0 I meant more the two segments here:

On the other hand, I just noticed that the TS team strictly avoids using the word "expression" for type-only things, which makes TSTypeExpression a better choice than I thought.

If the TS team "strictly avoids" using the word "expression", why does it make Babel using the term a "better choice than [you] thought"? Would you not want to keep the same wording as the TS team?

@thorn0
Copy link
Contributor

thorn0 commented Oct 25, 2022

@ngregory-rbi They avoid it for type-only things. as and satisfies aren't type-only.

@ngregory-rbi
Copy link

Please merge this as soon as possible :)

@nicolo-ribaudo
Copy link
Member Author

Release tomorrow!

@nicolo-ribaudo nicolo-ribaudo merged commit df733b1 into babel:main Oct 26, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the ts-parse-satisfies branch October 26, 2022 19:34
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Babel TypeScript transformer does not support satisfies operator
7 participants