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 instantiation expressions #14457

Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Apr 12, 2022

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

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 area: typescript labels Apr 12, 2022
@nicolo-ribaudo nicolo-ribaudo added this to the v7.18.0 milestone Apr 12, 2022
);
result.typeParameters = typeArguments;
return result;
if (tokenIsTemplate(this.state.type)) {
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 12, 2022

Choose a reason for hiding this comment

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

This code has just been moved.

@@ -1842,6 +1842,21 @@ export default class ExpressionParser extends LValParser {
// argument list.
// https://tc39.es/ecma262/#prod-NewExpression
parseNew(node: N.Expression): N.NewExpression {
this.parseNewCallee(node);

if (this.eat(tt.parenL)) {
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 12, 2022

Choose a reason for hiding this comment

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

This code has just been moved.

// a<b>
// if (0);
// is not valid TS code (https://github.com/microsoft/TypeScript/issues/48654)
// However, it should correctly parse anything that is correctly parsed by TS.
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 12, 2022

Choose a reason for hiding this comment

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

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Apr 12, 2022

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

@existentialism existentialism changed the title [ts] Add support for instantation expressions [ts] Add support for instantiation expressions Apr 12, 2022
existentialism
existentialism previously approved these changes Apr 12, 2022
@existentialism existentialism dismissed their stale review Apr 12, 2022

Didn't see TS parser tests fail

parent: t.Node,
) {
return (
(isCallExpression(parent) ||
Copy link
Contributor

@JLHwung JLHwung Apr 12, 2022

Choose a reason for hiding this comment

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

Will parent be an OptionalCallExpression?


(a<b>)<c>;
(a<b>)<c>();
new (a<b>)<c>();
Copy link
Contributor

@JLHwung JLHwung Apr 12, 2022

Choose a reason for hiding this comment

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

Can you add

(a<b>)<c>?.();

Copy link
Contributor

@JLHwung JLHwung left a comment

  • We may need to mark TSExpressionWithTypeArguments as an expression: Babel is throwing Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got "TSExpressionWithTypeArguments" when transpiling both TS and optional chaining (REPL):
a<b>?.();

Behaviour differences:

  • Babel accepts a<b><c>(); while TS rejects it with "Expression expected"
  • Babel rejects let a: typeof a<b> while TS accepts it
  • Babel parses a>b<x>>1 as TSExpressionWithTypeArguments while TS parses them as JS.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 13, 2022

I just realized that TSExpressionWithTypeArguments is a completely different thing:

  • it's "expression" can only be a TS type in the form A.B.C
  • it's meant to be a type-only annotation

I'm tempted to update this PR to use a new node, such as TSInstantiationExpression, however TS uses the same node for both. (cc @bradzacher maybe you have opinions)

@sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Apr 16, 2022

Babel rejects let a: typeof a<b> while TS accepts it

Regardless of whether we are adding a new node for Instantiation Expression, we should extend TSTypeQuery to add typeParameters.

@bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Apr 17, 2022

I just realized that TSExpressionWithTypeArguments is a completely different thing:

  • it's "expression" can only be a TS type in the form A.B.C
  • it's meant to be a type-only annotation

@nicolo-ribaudo are you sure this is correct?
Based on my playing around with the nightly build - ExpressionWithTypeArguments is just the node that gets used for this

declare function foo(): <T>() => T;

let a3 = foo()<string>;
//       ^^^^^^^^^^^^^ ExpressionWithTypeArguments

Based on TS's defined types the expression is any valid LHS expression:
https://github.com/microsoft/TypeScript/blob/56a4a93718ce016f038bcbce425bd3f47c3bec78/src/compiler/types.ts#L2473

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

However, interface A extends foo()<T> {} (which is the place where TSExpressionWithTypeArguments was originally used) is not valid 🤔

@bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Apr 17, 2022

Yeah TS uses the same node for all those usecase then blocks weird cases like that with a semantic error.

In our AST we have a separate node for that usecase - TSInterfaceHeritage.

I think class implements also uses the same node and the same parsing logic within TS. Again in our AST we have a special node for this - TSClassImplements.

So it's fine for us to treat the new node differently.

Side note - I wish TS was stricter with its parsing in these cases. Allowing so much and relying on semantic validation to narrow the scope feels so dirty.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

Non-fun fact: in type a = typeof import("b").c<d> and type e = typeof f.g<h>, the type argument is attached in two completely different places in TS's ast: <d> is part of the import node, while <h> of the typeof node.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

So, there are multiple possible options:

  1. Continue using TSExpressionWithTypeArguments for both interface heritage and for these new type instantiation expressions.
  2. Use TSExpressionWithTypeArguments for both, but in Babel 8 change the one used for interface heritage to TSInterfaceHeritage or TSQualifiedNameWithTypeArguments.
  3. Use TSInstantiationExpression for the new node, and keep TSExpressionWithTypeArguments for interfaces.
  4. Use TSInstantiationExpression for the new node, and also rename TSExpressionWithTypeArguments in Babel 8.

My preference is 3/4 first, then 2 and then 1. I think using the same node for both (like TS does) is particularly confusing because:

  • they have completely different rules for what's valid
  • one is a type, the other one is a JS expression

@JLHwung @existentialism WDYT?

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

It's also possible that in Babel 8 we will be able to just use TSTypeReference for interfaces heritage, since it seems to have the AST shape we need.

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 17, 2022

@nicolo-ribaudo I agree with approach 4. I support renaming TSExpressionWithTypeArguments for interfaces since the type is confusing.

@bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Apr 17, 2022

It's also possible that in Babel 8 we will be able to just use TSTypeReference for interfaces heritage, since it seems to have the AST shape we need.

A consideration for why the AST is designed the way it is; it's designed to support TS's AST shape - which in turn is designed to support the "recoverable parsing" in a type-safe manner.

So the reason that we use special nodes for these is because it's another one of those "TS uses semantic errors to ban weird syntax" things.
Technically interface Foo extends fn() {} is parses as syntactically valid TS - though it causes TS to report a semantic error - it parses fine.
I believe that's because TS reuses its "class extends" parser logic in this location.


(2) would align your AST shape with the typescript-eslint estree spec as it has been defined since the beginning.
I didn't realise that babel's AST differed in this regard! It looks like our alignment tests have taken this difference into consideration for a long while.

I could acquiesce to naming it TSInstantiationExpression to prevent naming clashes with bable's existing AST.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

@bradzacher Another reason I like TSInstantiationExpression is that it's name is consistent with TSAsExpression and TSNonNullExpression, which are the other two "expressions augmented with a type hint" nodes.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 17, 2022

Also, do you plan to represent typeof a<b> by adding a .typeParameters property to TSTypeQuery, or by allowing TSTypeQuery's .exprName to be a TSTypeReference?

I am currently implementing it in the first way (similarly to how tsc does), but I'm annoyed because .typeParameters are not valid if .exprName is a TSImportType (because it already includes its own type parameters).

@bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Apr 17, 2022

Interestingly I found #12884 which logs the difference in our ASTs for the heritage nodes!


Also, do you plan to represent typeof a<b> by adding a .typeParameters property to TSTypeQuery, or by allowing TSTypeQuery's .exprName to be a TSTypeReference?

When I saw @sosukesuzuki mention this above - I didn't hugely love the idea because it means that typeof further diverges from things like keyof.

However then I looked into it and noticed that in our AST - typeof maps to TSTypeQuery, which uses Identifier | TSQualifiedName for its expression - which obv doesn't support generics.

Looking into it more, for typeof import(''), we currently don't create a TSTypeQuery, instead we create a TSImportType with isTypeOf: true. We were looking to change this in our next major though (typescript-eslint/typescript-eslint#3076).

I don't know exactly what the better option is here.
My gut is telling me that allowing the AST types to describe a potentially invalid state and then just logically banning the invalid state is acceptable for the sake of keeping things simple.

i.e. I think this is the best AST type we can define

interface TSTypeQuery {
  exprName: Identifier | TSQualifiedName | TSImportType;
  typeParameters?: TSTypeParameterInstantiation;
}

if you wanted you could be fancy and define a union type for it?

interface TSTypeQueryWithImport {
  exprName: TSImportType;
  typeParameters?: never;
}
interface TSTypeQueryWithName {
  exprName: Identifier | TSQualifiedName;
  typeParameters?: TSTypeParameterInstantiation;
}
type TSTypeQuery = TSTypeQueryWithImport | TSTypeQueryWithName;

We've done similar sorts of magic to properly define types for things like properties and the differences between computed and non-computed keys.

@existentialism
Copy link
Member

@existentialism existentialism commented Apr 17, 2022

I second @JLHwung, (4) 👍

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 18, 2022

@bradzacher It's getting a bit out-of-scope for this PR (we might then migrate the discussion somewhere else), but since both your and our projects are considering AST breaking changes for the next major, we could make it a bit change and completely restructure the AST to something sensible like this:

// typeof ...
interface TSTypeQuery {
  typeAnnotation: TSImportType | TSTypeParameterInstantiation | TSQualifiedName | Identifier;
}

// ...<x, y, z>
interface TSTypeParameterInstantiation {
  typeName: TSQualifiedName | Identifier;
  typeParameters: Array<TSType>;
}

// ... .x
interface TSQualifiedName {
  left: TSQualifiedName | TSImportType | Identifier;
  right: Identifier;
}

// import("...")
interface TSImportType {
  argument: StringLiteral;
}

By doing so, type parameters are only represented in TSTypeReference nodes, and the ASTs would look like this:

  • a.b<c>
    TSTypeParameterInstantiation {
      typeName: TSQualifiedName { left: Identifier("a"), right: Identifier("b") },
      typeParameters: [ "c" ]
    }
  • typeof a.b
    TSTypeQuery {
      typeAnnotation: TSQualifiedName { left: Identifier("a"), right: Identifier("b") }
    }
  • import("a").b<c>
    TSTypeParameterInstantiation {
      typeName: TSQualifiedName { left: TSImportType("a"), right: Identifier("b") },
      typeParameters: [ "c" ]
    }
  • typeof import("a").b<c>
    TSTypeQuery {
      typeAnnotation: TSTypeParameterInstantiation {
        typeName: TSQualifiedName { left: TSImportType("a"), right: Identifier("b") },
        typeParameters: [ "c" ]
      }
    }

This also follows the AST design for JS nodes:

  • typeof a(b) is a UnaryExpression(typeof) whose argument is a CallExpression, and the arguments are not directly part of the outer UnaryExpression
  • import("a").b is a MemberExpression whose object is an ImportExpression, not an ImportExpression whose "qualifier" is b.

EDIT: to de-dupe even further the number of places where you can find type parameters in the AST, we could also represent a<x>() and new a<x>() as a CallExpression and a NewExpression whose callee is a TSInstantiationExpression. This would also give (a<x>)() and a<x>() the same AST, since they are effectively the same thing (like (1 * 2) + 3 and 1 * 2 + 3).

JLHwung
JLHwung approved these changes May 9, 2022
@existentialism existentialism added PR: Ready to be Merged and removed PR: Needs Review labels May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typescript PR: New Feature 🚀 PR: Ready to be Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants