Tree-sitter: Tree-sitter generates invalid code for grammars with large numbers of fields and/or aliases

[eugmes]

I'm trying to implement a quite complicated grammar using tree-sitter.

I'm getting compiler warnings like this:

> node-gyp rebuild

  CC(target) Release/obj.target/tree_sitter_coccinelle_binding/src/parser.o
../src/parser.c:14813:90: warning: implicit conversion from 'int' to 'uint8_t' (aka 'unsigned char') changes value from 256 to 0 [-Wconstant-conversion]
  [1841] = {.count = 1, .reusable = true}, REDUCE(sym_metavariables, 9, .production_id = 256),
                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
../src/tree_sitter/parser.h:203:9: note: expanded from macro 'REDUCE'
        __VA_ARGS__                              \
        ^~~~~~~~~~~

Everything is then built without errors, but the grammar obviously is not working.

I think it is due to definitions like this one (there are more uses of uint8_t in that files): https://github.com/tree-sitter/tree-sitter/blob/9a7327738940d6726c2e01c23b598e1704830ea7/lib/include/tree_sitter/parser.h#L70

It would be good if tree-sitter would support more productions, and if the number of productions is more than maximum supported it would be good to have a compile error.

I'm attaching the grammar I'm using now.
grammar.zip

[issue-label-bot[bot]]

Issue-Label Bot is automatically applying the label bug to this issue, with a confidence of 0.82. Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

[maxbrunsfeld]

Ah yeah, using uint8_t for production_id was a mistake.

Just to clarify what the production_id is for: It's only used for storing the field names (and aliases) of child nodes in a production. So I think that in your case, it's happening because of the number of unique field names and orderings that you're using. So the workaround for the moment would be to remove some of the field calls.

One other semi-related suggestion (this is more of a general thing with LR parsers) - very long sequences require a lot more parse states (and a larger binary, and more production ids) than shorter, more general, repeating sequences, so it's oftentimes a worthwhile optimization to generalize things like this:

    metavariables: $ => seq(
      '@',
      optional(field('rule_name', $.pure_ident)),
      optional(field('extends', $.extends)),
      optional(field('depends', $.depends)),
      optional(field('choose_iso', $.choose_iso)),
      optional(field('disable_iso', $.disable_iso)),
      optional(field('exists', $.exists)),
      optional(field('is_expression', $.is_expression)),
      '@',
      optional(field('declarations', $.metadecls)),
      '@@'
    ),

by turning it into something more like this:

metavariables: $ => seq(
  '@',
  repeat(choice(
    field('rule_name', $.pure_ident),
    field('extends', $.extends),
    field('depends', $.depends),
    field('choose_iso', $.choose_iso),
    field('disable_iso', $.disable_iso),
    field('exists', $.exists),
    field('is_expression', $.is_expression),
  )),
  '@',
  optional(field('declarations', $.metadecls)),
  '@@'
),

The latter form will accept some input that may be semantically-invalid, but you can always catch that problem at a higher level, rather than making it unrepresentable in the parse tree.

Anyway, apologies for this bug. Its easy to fix, but requires a bit more work to do so in a way that will still maintain ABI backward compatibility (i.e. allowing a new version of the library to load parsers that were generated with an older version of the CLI). I'll try and come up with a fix, or you're welcome to as well if you feel so inclined.

[eugmes]

@maxbrunsfeld Thanks for suggestions. I've removed all fields from the grammar and now it compiles fine. It's too early to add those anyway, I first need a grammar that can parse those files. After that I'll if I can optimize it.

[maxbrunsfeld]

By the way, coccinelle is awesome; hope your project works out.