This repository has been archived by the owner. It is now read-only.

TypeScript parser #523

Merged
merged 74 commits into from Jun 28, 2017

Conversation

Projects
None yet
10 participants
@andy-ms
Contributor

andy-ms commented May 12, 2017

Q A
Bug fix? no
Breaking change? no
New feature? yes
Deprecations? no
Spec compliancy? no
Tests added/pass? Tests added, and pass
Fixed tickets #320
License MIT

This is a WIP, so please don't merge yet. The AST spec is likely to change. I am just looking to get comments on the implementation.
For those interested in just the AST spec, just look at the changes to src/types.js. I am tracking differences with typescript-eslint-parser here.

CC @stephen @JamesHenry @DanielRosenwasser

Notes:

  • It shares a lot in common with the flow plugin, but for now I am just using identical code (and marking it as such). In the future they should probably share.

  • Test files should really have a .ts extension, but that would require editing babel-helper-fixtures. Shall I make a pull request there?

andy-ms added some commits May 12, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot May 12, 2017

Codecov Report

Merging #523 into master will decrease coverage by 2.29%.
The diff coverage is 89.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #523     +/-   ##
=========================================
- Coverage   98.27%   95.97%   -2.3%     
=========================================
  Files          22       23      +1     
  Lines        3530     4477    +947     
  Branches      981     1240    +259     
=========================================
+ Hits         3469     4297    +828     
- Misses         22       86     +64     
- Partials       39       94     +55
Flag Coverage Δ
#babel 65.04% <13.98%> (-16.01%) ⬇️
#babylon 95.06% <89.05%> (-2.05%) ⬇️
Impacted Files Coverage Δ
src/tokenizer/state.js 100% <ø> (ø) ⬆️
src/tokenizer/types.js 100% <ø> (ø) ⬆️
src/tokenizer/context.js 100% <ø> (ø) ⬆️
src/tokenizer/index.js 98.36% <100%> (ø) ⬆️
src/plugins/estree.js 99.16% <100%> (ø) ⬆️
src/parser/lval.js 98.12% <100%> (+0.11%) ⬆️
src/parser/util.js 92.3% <100%> (+2.3%) ⬆️
src/plugins/flow.js 98.37% <100%> (-0.01%) ⬇️
src/index.js 94.44% <75%> (-5.56%) ⬇️
src/plugins/typescript.js 86.13% <86.13%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a738870...fc81c23. Read the comment docs.

codecov bot commented May 12, 2017

Codecov Report

Merging #523 into master will decrease coverage by 2.29%.
The diff coverage is 89.05%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #523     +/-   ##
=========================================
- Coverage   98.27%   95.97%   -2.3%     
=========================================
  Files          22       23      +1     
  Lines        3530     4477    +947     
  Branches      981     1240    +259     
=========================================
+ Hits         3469     4297    +828     
- Misses         22       86     +64     
- Partials       39       94     +55
Flag Coverage Δ
#babel 65.04% <13.98%> (-16.01%) ⬇️
#babylon 95.06% <89.05%> (-2.05%) ⬇️
Impacted Files Coverage Δ
src/tokenizer/state.js 100% <ø> (ø) ⬆️
src/tokenizer/types.js 100% <ø> (ø) ⬆️
src/tokenizer/context.js 100% <ø> (ø) ⬆️
src/tokenizer/index.js 98.36% <100%> (ø) ⬆️
src/plugins/estree.js 99.16% <100%> (ø) ⬆️
src/parser/lval.js 98.12% <100%> (+0.11%) ⬆️
src/parser/util.js 92.3% <100%> (+2.3%) ⬆️
src/plugins/flow.js 98.37% <100%> (-0.01%) ⬇️
src/index.js 94.44% <75%> (-5.56%) ⬇️
src/plugins/typescript.js 86.13% <86.13%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a738870...fc81c23. Read the comment docs.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot May 12, 2017

Codecov Report

Merging #523 into master will decrease coverage by 2.32%.
The diff coverage is 88.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   98.14%   95.82%   -2.33%     
==========================================
  Files          22       23       +1     
  Lines        3674     4695    +1021     
  Branches     1024     1287     +263     
==========================================
+ Hits         3606     4499     +893     
- Misses         25      107      +82     
- Partials       43       89      +46
Flag Coverage Δ
#babel 64.21% <10.56%> (-16.6%) ⬇️
#babylon 94.78% <88.36%> (-2.04%) ⬇️
Impacted Files Coverage Δ
src/tokenizer/types.js 100% <ø> (ø) ⬆️
src/tokenizer/state.js 100% <ø> (ø) ⬆️
src/tokenizer/context.js 100% <ø> (ø) ⬆️
src/parser/node.js 96.87% <100%> (+0.2%) ⬆️
src/parser/util.js 91.89% <100%> (+1.89%) ⬆️
src/tokenizer/index.js 98.44% <100%> (ø) ⬆️
src/plugins/estree.js 99.18% <100%> (-0.01%) ⬇️
src/index.js 94.44% <75%> (-5.56%) ⬇️
src/parser/lval.js 96.83% <83.33%> (-1.21%) ⬇️
src/plugins/flow.js 98.17% <84.61%> (-0.23%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b4fba4...5b56e59. Read the comment docs.

codecov bot commented May 12, 2017

Codecov Report

Merging #523 into master will decrease coverage by 2.32%.
The diff coverage is 88.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   98.14%   95.82%   -2.33%     
==========================================
  Files          22       23       +1     
  Lines        3674     4695    +1021     
  Branches     1024     1287     +263     
==========================================
+ Hits         3606     4499     +893     
- Misses         25      107      +82     
- Partials       43       89      +46
Flag Coverage Δ
#babel 64.21% <10.56%> (-16.6%) ⬇️
#babylon 94.78% <88.36%> (-2.04%) ⬇️
Impacted Files Coverage Δ
src/tokenizer/types.js 100% <ø> (ø) ⬆️
src/tokenizer/state.js 100% <ø> (ø) ⬆️
src/tokenizer/context.js 100% <ø> (ø) ⬆️
src/parser/node.js 96.87% <100%> (+0.2%) ⬆️
src/parser/util.js 91.89% <100%> (+1.89%) ⬆️
src/tokenizer/index.js 98.44% <100%> (ø) ⬆️
src/plugins/estree.js 99.18% <100%> (-0.01%) ⬇️
src/index.js 94.44% <75%> (-5.56%) ⬇️
src/parser/lval.js 96.83% <83.33%> (-1.21%) ⬇️
src/plugins/flow.js 98.17% <84.61%> (-0.23%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b4fba4...5b56e59. Read the comment docs.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 14, 2017

Member

Test files should really have a .ts extension, but that would require editing babel-helper-fixtures. Shall I make a pull request there?

I think that should be covered by babel/babel#5511 but I didn't think we really wanted it earlier?

Member

hzoo commented May 14, 2017

Test files should really have a .ts extension, but that would require editing babel-helper-fixtures. Shall I make a pull request there?

I think that should be covered by babel/babel#5511 but I didn't think we really wanted it earlier?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo May 16, 2017

Member

I know it's easier to modify the code in src/expression etc, but is there a way for us to make the parser plugin like in estree/flow/jsx? https://github.com/babel/babylon/tree/master/src/plugins.

The concern is how we are going to do versioning? In flow I guess everything is a minor/patch though. Maybe we can discuss is at the meeting (not typescript specific issue though)

Member

hzoo commented May 16, 2017

I know it's easier to modify the code in src/expression etc, but is there a way for us to make the parser plugin like in estree/flow/jsx? https://github.com/babel/babylon/tree/master/src/plugins.

The concern is how we are going to do versioning? In flow I guess everything is a minor/patch though. Maybe we can discuss is at the meeting (not typescript specific issue though)

andy-ms added some commits May 17, 2017

Merge pull request #1 from andy-ms/ts-indexSignature
Change TSIndexSignatureDeclaration to TSIndexSignature
Merge pull request #2 from andy-ms/ts-namespace-body
Change TSModuleBlock `.statements` to `.body`
Merge pull request #3 from andy-ms/ts-TypeParameterDeclaration
Use TypeParameterDeclaration and TypeParameter nodes instead of a TsTypeParameterDeclaration[]
Merge pull request #4 from andy-ms/ts-TypeAnnotation
Use TypeAnnotation node for type annotations
Remove optional "name" on TSTypeElementBase, because some type elemen…
…ts may never have "name", others are TSNamedTypeElementBase
Merge pull request #5 from andy-ms/ts-interface-body-body
Add TSInterfaceBody intermediate node
Merge pull request #6 from andy-ms/ts-name-id-key
Change "name" fields to "id" or "key"
Merge pull request #7 from andy-ms/ts-declarefunction
Introduce TSDeclareFunction and TSDeclareMethod node types
Show outdated Hide outdated src/parser/expression.js
node.generator = !!isGenerator;
this.parseFunctionBody(node);
const missingBody = this.maybeParseFunctionBody(node, /* allowExpression */ false, /* allowMissingBody */ type === "ClassMethod");

This comment has been minimized.

@xtuc

xtuc May 19, 2017

Member

I think we could pass an object here, like:

maybeParseFunctionBody({ allowExpression: false, allowMissingBody: type === "ClassMethod"});

Looks better than comments to me.

@xtuc

xtuc May 19, 2017

Member

I think we could pass an object here, like:

maybeParseFunctionBody({ allowExpression: false, allowMissingBody: type === "ClassMethod"});

Looks better than comments to me.

Show outdated Hide outdated src/parser/expression.js
// Strict mode words may be allowed as in `declare namespace N { const static: number; }`.
// And we have a type checker anyway, so don't bother having the parser do it.
return;
}

This comment has been minimized.

@xtuc

xtuc May 19, 2017

Member

What about a better integration with TypeScript here? Early grammar errors are great for online editor (like Babel's REPL). Babylon has that kind of logic for JavaScript.

@xtuc

xtuc May 19, 2017

Member

What about a better integration with TypeScript here? Early grammar errors are great for online editor (like Babel's REPL). Babylon has that kind of logic for JavaScript.

This comment has been minimized.

@andy-ms

andy-ms May 19, 2017

Contributor

We would have to know of whether we're in a declaration context or not. And knowing that requires knowing the current file extension. export let package: number; is a perfectly legal .d.ts file.

@andy-ms

andy-ms May 19, 2017

Contributor

We would have to know of whether we're in a declaration context or not. And knowing that requires knowing the current file extension. export let package: number; is a perfectly legal .d.ts file.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 13, 2017

Member

And knowing that requires knowing the current file extension.

We could also add a "sourceType": "declaration" which treats files as .d.ts

@nicolo-ribaudo

nicolo-ribaudo Jun 13, 2017

Member

And knowing that requires knowing the current file extension.

We could also add a "sourceType": "declaration" which treats files as .d.ts

andy-ms added some commits May 19, 2017

Merge pull request #9 from andy-ms/ts-parameter-property-decorator
Attach decorators to the TSParameterProperty, not to the Identifier
Merge pull request #10 from andy-ms/ts-TypePredicate-annotation
Make TSTypePredicate store a TypeAnnotation node instead of directly storing a type

@avindra avindra referenced this pull request May 21, 2017

Closed

TypeScript Support #13

@existentialism

existentialism approved these changes Jun 21, 2017 edited

Changes to parser core LGTM 👍

Did not spend a ton of time on typescript plugin proper, but my POV is that we can land it (to prevent needing to constantly update this huge PR) and continue to iterate.

Show outdated Hide outdated src/parser/lval.js
}
// Overridden in typescript.js
parseParameterModifiers(): { accessibility: ?Accessibility, readonly: boolean } {

This comment has been minimized.

@existentialism

existentialism Jun 21, 2017

Member

Is this still needed? Couldn't see where it was used (or overridden)

@existentialism

existentialism Jun 21, 2017

Member

Is this still needed? Couldn't see where it was used (or overridden)

@@ -1341,6 +1340,12 @@ export default (superClass: Class<Parser>): Class<Parser> => class extends super
parseAssignableListItemTypes(param: N.Pattern): N.Pattern {
if (this.eat(tt.question)) {
if (param.type !== "Identifier") {

This comment has been minimized.

@existentialism

existentialism Jun 21, 2017

Member

Is this expected behavior in flow? Using the fixture from the matching typescript test in astexplorer seems to produce a funky result.

If it is, we should probably add a matching flow test fixture.

@existentialism

existentialism Jun 21, 2017

Member

Is this expected behavior in flow? Using the fixture from the matching typescript test in astexplorer seems to produce a funky result.

If it is, we should probably add a matching flow test fixture.

This comment has been minimized.

@andy-ms

andy-ms Jun 23, 2017

Contributor

flow itself (the command-line tool, not the babylon parser) fails for your example with Unexpected token ?. I'll add a test.

@andy-ms

andy-ms Jun 23, 2017

Contributor

flow itself (the command-line tool, not the babylon parser) fails for your example with Unexpected token ?. I'll add a test.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 24, 2017

Member

@danez @stephen @nicolo-ribaudo any more thoughts?

Member

hzoo commented Jun 24, 2017

@danez @stephen @nicolo-ribaudo any more thoughts?

Show outdated Hide outdated src/parser/expression.js
@@ -786,6 +795,8 @@ export default class ExpressionParser extends LValParser {
node.callee = this.parseNoCallExpr();
const optional = this.eat(tt.questionDot);
this.parseNewTypeArguments(node);

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 25, 2017

Member

I didn't notice this before: is it possible to implement new type arguments only in the typescript plugin?

Something like

parseNew() {
    const node = super.parseNew();
    if (/* node is NewExpression */ && !/* node has args parentheses*/ && this.match("<")) {
       // parse type arguments
       // maybe parse arguments
    }
}

Anyway, apart from this detail this pr looks good to me! (well, I have to admit I haven't reviwed all of it because I don't know typescript very well 😅 )

@nicolo-ribaudo

nicolo-ribaudo Jun 25, 2017

Member

I didn't notice this before: is it possible to implement new type arguments only in the typescript plugin?

Something like

parseNew() {
    const node = super.parseNew();
    if (/* node is NewExpression */ && !/* node has args parentheses*/ && this.match("<")) {
       // parse type arguments
       // maybe parse arguments
    }
}

Anyway, apart from this detail this pr looks good to me! (well, I have to admit I haven't reviwed all of it because I don't know typescript very well 😅 )

This comment has been minimized.

@andy-ms

andy-ms Jun 26, 2017

Contributor

I don't think the AST gives us a way to distinguish between new X and new X(). But I found a different way that works just as well, see the latest commit 102be0e.

@andy-ms

andy-ms Jun 26, 2017

Contributor

I don't think the AST gives us a way to distinguish between new X and new X(). But I found a different way that works just as well, see the latest commit 102be0e.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jun 25, 2017

Member

I have a concern about versioning: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes makes me think that typescript can have breaking changes. Does this PR make babylon potentially need a major version bump every time a new ts version is released?

Member

nicolo-ribaudo commented Jun 25, 2017

I have a concern about versioning: https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes makes me think that typescript can have breaking changes. Does this PR make babylon potentially need a major version bump every time a new ts version is released?

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jun 25, 2017

Typescript never had syntactic breaking changes. Meaning valid syntax that is supported by one version and disallowed by a newer version. All breaking changes are semantic in nature; either new error checks or new behaviors for the type system. All these are outside the scope of Babylon. There are some changes in the resulting code from tsc but these are not relevant here either.
So from parsing perspective we have a guarantee for no breaking changes movinging forward.
Hope that addresses your concern.

mhegazy commented Jun 25, 2017

Typescript never had syntactic breaking changes. Meaning valid syntax that is supported by one version and disallowed by a newer version. All breaking changes are semantic in nature; either new error checks or new behaviors for the type system. All these are outside the scope of Babylon. There are some changes in the resulting code from tsc but these are not relevant here either.
So from parsing perspective we have a guarantee for no breaking changes movinging forward.
Hope that addresses your concern.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Jun 25, 2017

Member

Thank you 👍

Member

nicolo-ribaudo commented Jun 25, 2017

Thank you 👍

Show outdated Hide outdated src/plugins/typescript.js
// 99% certain this is `new C<T>();`. But may be `new C < T;`, which is also legal.
const typeParameters = this.tsTryParseAndCatch(this.tsParseTypeArguments.bind(this));
if (typeParameters) {
node.typeParameters = typeParameters;

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 27, 2017

Member

I think you should check that this.match(tt.parenL), otherwise new Foo<T> will be allowed

@nicolo-ribaudo

nicolo-ribaudo Jun 27, 2017

Member

I think you should check that this.match(tt.parenL), otherwise new Foo<T> will be allowed

This comment has been minimized.

@andy-ms

andy-ms Jun 27, 2017

Contributor

👍

@andy-ms

andy-ms Jun 27, 2017

Contributor

👍

@hzoo

hzoo approved these changes Jun 27, 2017

Super job @andy-ms, I agree with landing and iterating. Sorry for the delays

@hzoo hzoo changed the title from WIP: TypeScript parser to TypeScript parser Jun 28, 2017

@hzoo hzoo merged commit 97c2346 into babel:master Jun 28, 2017

1 of 3 checks passed

codecov/patch 88.8% of diff hit (target 98%)
Details
codecov/project 95.9% (-2.1%) compared to f976bdd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JamesHenry

This comment has been minimized.

Show comment
Hide comment
@JamesHenry

JamesHenry Jun 28, 2017

Member

Congrats, @andy-ms! Awesome work 😄

We can keep fine tuning the AST together but it's fantastic to have this merged!

Member

JamesHenry commented Jun 28, 2017

Congrats, @andy-ms! Awesome work 😄

We can keep fine tuning the AST together but it's fantastic to have this merged!

@andy-ms andy-ms deleted the andy-ms:ts-wip branch Jun 28, 2017

@andy-ms

This comment has been minimized.

Show comment
Hide comment
@andy-ms

andy-ms Jun 28, 2017

Contributor

@hzoo Please notify me when this is published.

Contributor

andy-ms commented Jun 28, 2017

@hzoo Please notify me when this is published.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Jun 28, 2017

Member

yep doing it now!

Member

hzoo commented Jun 28, 2017

yep doing it now!

@hzoo

This comment has been minimized.

Show comment
Hide comment
@yahiko00

This comment has been minimized.

Show comment
Hide comment
@yahiko00

yahiko00 Jun 29, 2017

Oh. My. God. <3

yahiko00 commented Jun 29, 2017

Oh. My. God. <3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.