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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate TypeScript and TSX grammars to avoid ambiguity and support type arguments in TSX #68

Merged
merged 17 commits into from May 29, 2019

Conversation

@nathansobo
Copy link
Member

@nathansobo nathansobo commented May 23, 2019

馃崘'd with @as-cii

TSX and TypeScript are actually two slightly different languages. TypeScript includes syntax for type assertions that conflicts with JSX elements, so in TSX the type assertion syntax is not used. Previously, we handled this by just allowing the grammar to be ambiguous with respect to type assertions versus JSX expressions, but adding support for type arguments led to complications with that approach.

This PR splits the previous grammar into two separate grammars, one for Typescript and one for TSX. Now, when using this module, you need to specify the dialect when requiring as follows:

const ts = require("tree-sitter-typescript/typescript");
const tsx = require("tree-sitter-typescript/tsx");

In order to avoid ambiguity around the special tree-sitter fields in the package.json, there is no longer a default export. Each dialect-specific subfolder includes its own package.json with the custom tree-sitter fields in it. @maxbrunsfeld I'm not sure how you're consuming these fields in other components, so let me know if this makes sense.

Duplication between grammars is minimized by a JS helper function at the root of the repo called defineGrammar, which takes a dialect argument to conditionally define the expression rule. In TypeScript, we include type assertions and filter out JSX expressions inherited from the JS grammar. In JSX, we do the opposite. We don't conditionally filter the conflicts to exclude JSX-related rules in the TypeScript grammar because we're assuming that conflicts that reference rules that aren't used end up being ignored. @maxbrunsfeld, is that assumption correct?

To avoid duplication of the custom scanner code, we use a simple #include macro in the TSX scanner to include the scanner code from the TypeScript grammar. A little gross, but gets the job done.

Our biggest concern is duplication in the tests. They are slightly different due to the differences in grammars explained above, but largely duplicated. I could see potentially cutting down the TSX tests to the bare minimum since we define both grammars via a shared function anyways. Let me know if that makes sense and I'll do it.

Nathan Sobo and others added 13 commits May 23, 2019
This is hacked in for now. We need to separate TS and TSX for special 
handling of type assertions.

Co-Authored-By: Jason Rudolph <jason@jasonrudolph.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Still a lot of duplication here, but it's passing.

Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
Co-Authored-By: Antonio Scandurra <as-cii@github.com>
@nathansobo nathansobo requested a review from maxbrunsfeld May 23, 2019
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld left a comment

This looks like a great improvement.

We don't conditionally filter the conflicts to exclude JSX-related rules in the TypeScript grammar because we're assuming that conflicts that reference rules that aren't used end up being ignored.

Yeah that's right; unused conflicts are just ignored.

To avoid duplication of the custom scanner code, we use a simple #include macro in the TSX scanner to include the scanner code from the TypeScript grammar.

Reusing the C code via an #include seems like a good solution to me. I would suggest one minor tweak though:

With this setup, the tree-sitter-tsx shared library will export its own external scanner symbols and also the external scanner symbols for tree-sitter-typescript. When both libraries are loaded, this could cause problems with duplicate symbol definitions. For instance, in Treelights, we statically compile a bunch of Tree-sitter parsers into one executable. It might cause linker errors if two object files both defined _tree_sitter_type_script_external_scanner_scan.

So maybe you should have a third scanner.c file that defines the scan function as a static inline function, and then you could #include that file in both typescript/src/scanner.c and tsx/src/scanner.c.

@nathansobo
Copy link
Member Author

@nathansobo nathansobo commented May 23, 2019

@maxbrunsfeld I named the common file scanner.h because it seems like the .h naming convention is used for files meant to be included elsewhere. Considering the only definitions in the file are static, that seemed reasonable, but I'm not very familiar with C naming conventions.

@nathansobo nathansobo mentioned this pull request May 23, 2019
Nathan Sobo added 2 commits May 23, 2019
I'm worried that it's causing problems on Windows. TreeSitter wants to 
generate the binding files in the subfolders when you run `tree-sitter 
generate` so I replaced the previous files with placeholders.
@nathansobo nathansobo force-pushed the ns-as/tsx-type-arguments branch 4 times, most recently from 6a303bb to fc5d2c8 May 24, 2019
Co-Authored-By: Jason Rudolph <jason@jasonrudolph.com>
@nathansobo nathansobo force-pushed the ns-as/tsx-type-arguments branch from fc5d2c8 to 5c03083 May 24, 2019
@nathansobo nathansobo requested a review from maxbrunsfeld May 24, 2019
@nathansobo
Copy link
Member Author

@nathansobo nathansobo commented May 24, 2019

Hey @maxbrunsfeld! Got this green and addressed your concerns. My last concern is that I don't want to interfere with any other infrastructure that depends on this grammar. I moved the tree-sitter metadata from the root of the repository into the typescript and tsx subfolders. Will that be problematic for anything depending on this module?

Copy link
Contributor

@maxbrunsfeld maxbrunsfeld left a comment

I don't think that the tree-sitter metadata moving should be problem. Thanks!

@as-cii
Copy link
Contributor

@as-cii as-cii commented May 29, 2019

Thanks, @maxbrunsfeld! 馃檱

Do you mind merging this and publishing a new version? I would do it myself but I don't think I have write access to this repository.

@maxbrunsfeld maxbrunsfeld merged commit d90b629 into tree-sitter:master May 29, 2019
2 checks passed
@maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented May 29, 2019

Sorry for the delay there; I published this as 0.14.0.

@as-cii
Copy link
Contributor

@as-cii as-cii commented May 30, 2019

Thanks, Max! 鉂わ笍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants