Fix edge cases with type imports

[rtsao]

Fixes #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:

• All tests pass in CI.
• There are sufficient tests for the new fix/feature.
• Grammar rules have not been renamed unless absolutely necessary.
• The conflicts section hasn't grown too much.
• The parser size hasn't grown too much (check the value of STATE_COUNT in src/parser.c).

[hendrikvanantwerpen]

Thank you for willing contributing to this grammar!

I played a bit with the TS playground to get a feel for the different cases. Some interesting variants I found. The ones that parsed in Tree-sitter:

• import A from './User.js'; // import identifier 'A'
• import type A from './User.js'; // import type 'A'
• import type { A } from './User.js'; // import type 'A'
• import { type A } from './User.js'; // import type 'A'
• import A, { type type } from './User.js'; // import identifier 'A' and type 'type'
• import type type from './User.js'; // import type 'type'

And those that didn't:

• import type from './User.js'; // import identifier 'type'
• import { type, A } from './User.js'; // import identifiers 'type' and 'A'
• import type, { type A } from './User.js'; // import identifier 'type' and type 'A'
• import { type A } from './User.js'; // import type 'A'
• import type, A from './User.js'; // syntax error
• import { A, type } from './User.js'; // import identifiers 'type' and 'A'

The comments indicate the behavior in TSC.

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;.

I think the fix worked, but fixing it like this will require doing it on more places (at least import_specifier). It would be better if we understand why type is not accepted in those positions and see if we can fix that, so it works everywhere where $.identifier is used.

[rtsao]

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

tree-sitter-typescript/common/define-grammar.js

Line 939 in c6e56d4

'type',

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.

[hendrikvanantwerpen]

Thanks for adding more tests! It seems that this is the best solution for now. One last nit: I think we should create a separate production for the new choice, instead of replicating it several times. Something like:

_import_identifier: $ => choice($.identifier, alias('type', $.identifier))

and then use $._import_identifier in those places. That way we keep this localized, and if we find a more principled solution later, it will be easier to fix.

[rtsao]

Good idea, thanks!

[hendrikvanantwerpen]

That's much cleaner. There's one place where the change wasn't applied, but otherwise this looks good.

[hendrikvanantwerpen]

One more place where we can use $._import_identifier.

[rtsao]

I think replacing choice($._module_export_name, alias('type', $.identifier)) with $._import_identifier would change the semantics of the grammar because $._module_export_name is equivalent to choice($.string, $.identifier) not merely $.identifier. See tree-sitter/tree-sitter-javascript#234 for more context.

If you'd like, I could factor out alias('type', $.identifier) from choice($.identifier, alias('type', $.identifier)) into its own rule and use it here.

[hendrikvanantwerpen]

You're absolutely right, this is not the same case!

[hendrikvanantwerpen]

Thanks for the contribution, @rtsao!