tree-sitter / tree-sitter-typescript

TypeScript grammar for tree-sitter
MIT License
335 stars 104 forks source link

Fix edge cases with type imports #240

Closed rtsao closed 1 year ago

rtsao commented 1 year ago

Fixes https://github.com/tree-sitter/tree-sitter-typescript/issues/234

The following should parse correctly (TS playground) (Flow playground):

import type from './User.js';

Before:

(program
  (import_statement
    (ERROR (identifier)) (string (string_fragment)))

After:

(program
  (import_statement
    (import_clause (identifier)) (string (string_fragment)))

Checklist:

rtsao commented 1 year ago

I am surprised the type is not allowed for the $.identifier in import_clause and import_specifier. Especially since it works in other places, such as let type = 42;.

Yeah, I was puzzled by this too. I thought it might have to do with it being a reserved identifier https://github.com/tree-sitter/tree-sitter-typescript/blob/c6e56d44c686a67c89e29e773e662567285d610f/common/define-grammar.js#L939 but removing that didn't seem affect how import statements were parsed.

After some time trying to figure it out, there didn't seem to be anything obvious so I went this workaround.

I've just pushed a fix for the additional edge case with import specifiers and added more test cases.

If someone with more knowledge of tree-sitter and this grammar can point out what is preventing type from being parsed as a normal identifier in these instances, I'd be happy to refactor this. But in the meantime, I think this approach is still a workable fix for the parse errors.

rtsao commented 1 year ago

Good idea, thanks!

hendrikvanantwerpen commented 1 year ago

Thanks for the contribution, @rtsao!