Type parameter variance

[mads-hartmann]

I played around with the grammar this morning and tried to add support for specifying the variance of type parameters.

I checked in changes to src/parser.c which seems to have been automatically generated. Is that the right thing to do?

I also updated the README slightly. The main motivation for this is that I'd like to get the Scala community involved and they might not be familiar with tree-sitter so I wanted to lower the barrier to working on the grammar.

Cheers,
Mads

[maxbrunsfeld]

I checked in changes to src/parser.c which seems to have been automatically generated. Is that the right thing to do?

👍 Yes, perfect. The reason I do this is so that these parsers are easy to obtain when using languages other than JavaScript (in which case the parsers are available on npm) and without users having to run tree-sitter-cli.

Thanks for the PR, and nice work! I just left a couple of minor suggestions.

[maxbrunsfeld]

Adding a development guide is a great idea. Since you don't have to run npm install every time, maybe we should move that piece into a separate paragraph - something like:

---

First, install the project's dependencies:

npm install

Then add a test case to ./corpus, make the required changes to grammar.js, regenerate and recompile the parser, and run the tests:

npm run build
npm test

[maxbrunsfeld]

I would probably make these two syntax nodes visible by removing the leading _.

My rule of thumb is that nodes should be visible unless they are simply wrappers around other nodes (like _type simply wraps a number of specific type nodes, and _expression simply wraps a bunch of specific expression nodes). Since the - and + carry important information, I'd want to see them associated with their own visible syntax node.

[mads-hartmann]

@maxbrunsfeld Thanks for the feedback! Should be fixed now :)

[mads-hartmann]

@maxbrunsfeld I spent a few more hours playing around with this. In the beginning I tried to follow the EBNF very closely but that led me into some fairly deep rabbit holes that resulted in ambiguities in the grammar so I started following it a bit more loosely. I've tried to chunk the changes into readable commits but I've rebased heavily during the development so the checked in C code might not be the actual result of compiling the parser - I hope that isn't an issue (the last commit definitely has an up to date parser generated)

On a side note: The tooling is great! The error messages when you have ambiguities in your grammar are really helpful. Being able to so easily add tests also makes it a much more enjoyable experience. Great job!

[maxbrunsfeld]

I started following it a bit more loosely

👍 That has always been my approach as well.

the checked in C code might not be the actual result of compiling the parser

That's totally fine. In fact, it's probably better. We don't need to be able to revert back to every single intermediate commit, and that approach avoids some unnecessary large objects in the git history.

On a side note: The tooling is great! The error messages when you have ambiguities in your grammar are really helpful. Being able to so easily add tests also makes it a much more enjoyable experience. Great job!

That's so great to hear. Thank you!

[maxbrunsfeld]

After merging this, I realized I had a few more suggestions. Let me know what you think.

[maxbrunsfeld]

The repeat rule actually means zero-or-more (there's also a repeat1 built-in, which means one-or-more), so you can remove the optional here.

[maxbrunsfeld]

I'm curious about the possibility of a type_parameters node within a type parameter. What does it mean when a type parameter has its own parameters? I think we should add a test for this case as well.

[maxbrunsfeld]

Now that I'm seeing how much structure there is to type parameters, I think we should tweak their structure so that there is always a visible syntax node corresponding to each parameter (as opposed to the identifier and upper_bound node appearing as siblings within the type_parameters node).

Maybe something like this:

==================
Type parameters
==================

class A[B, +C <: D, -E : F : G]

---

(compilation_unit
  (class_definition
    (identifier)
    (type_parameters
      (invariant_type_parameter (type_identifier))
      (covariant_type_parameter
        (type_identifier)
        (upper_bound (type_identifier)))
      (contravariant_type_parameter
        (type_identifier)
        (context_bound (type_identifier))
        (context_bound (type_identifier))))))

Does that make sense? We could have a hidden helper rule called _type_parameter that is shared between the three. In addition, we should add the _type_parameter to the inline section of the grammar. That will avoid actually creating that node at runtime for performance reasons.

[mads-hartmann]

@maxbrunsfeld Definitely. I'll create a separate issue for this so we don't forget :)