feat: get supertype info - #3938

@ribru17 I'll close this out until it's ready, no offense to you but the constant notification everytime you push is a little annoying 😅

You can unsubscribe, you know. Pushing to a closed PR breaks GitHub.

(Marking as ready was me fat fingering on mobile. Will redraft when I get back home.)

You can unsubscribe, you know. Pushing to a closed PR breaks GitHub.

I would like to be subscribed, when it's ready. And how does it even break GH? Even if it does, that sounds like a GH issue.

Yeah I think it is a really annoying GitHub issue, I'll re-mark as draft and stop pushes haha. (Though I think this is almost ready.) Sorry!

Yes, it's a GH issue. But pushing to a closed PR prevents it from being opened again.

Alright from my perspective it looks like this is pretty much done, but I am having some perplexing issues that look like some data offset weirdness (i.e. field_count returns max_alias_sequence_length, supertype_count returns field_count, etc.). May need some help with that because I can't tell where it's coming from, but in my limited understanding I feel like 90% of the heavy lifting is done

Note that the nvim-treesitter CI check here will inherently fail because this changes the TSLanguage struct. In the future it would be nice to run this check building Neovim with the local version of TS rather than just unpacking the Neovim tarball, but this sounds like a job for the mighty @dundargoc :) Running Neovim with the updated local version of TS on my branch shows things working still.

Note that the nvim-treesitter CI check here will inherently fail because this changes the TSLanguage struct.

Which means that nvim-treesitter will fail in the wild, too, so the check is valuable. (Note that the generate workflow explicitly generates parsers with ABI 14 to match the tree-sitter lib Neovim is built with.)

@ObserverOfTime do you have any idea why the rust bindings have some (I am guessing) data offset issues mentioned in this #3938 (comment)? I ported these changes to Neovim locally and everything worked when just called from C directly

Nope.

Test failures due to out of date fixtures I believe. All failing tests use cached fixtures and all passing tests regenerated.

Side question, is there a way to manually tell CI to regenerate fixtures or ignore cache for a workflow run?

Test failures due to out of date fixtures I believe. All failing tests use cached fixtures and all passing tests regenerated.

I think you're right, it looks the cache hit on your last force push because it only modified lib/src/tree_cursor.c.

The fixtures cache is indexed by a hash of several files, but doesn't include that one:

Ahh, thank you that makes sense. Yeah I had modified that as a mistake during some debugging... 😅 Seems like lib/src/parser.h should be included in that list

@ribru17 I cleaned up some of the code, though some notable changes are:

• I completely removed the usage of production infos to store supertype info, because conceptually, a field is associated with the children a given parent at runtime, but we don't store this relationship globally because fields are not directly linked to one set of symbols, whereas supertypes are for all supertypes (they must have one visible child only), and we're doing a lot of redundant cloning to store the exact same "global" set of supertypes for each production info, when we don't need to

• I changed the signature of ts_language_supertype_map to not take a double pointer, since the reason the internal field map version does is because it's used more like a range rather than a list, and also because it's a littler nicer for downstream consumers

If this looks good to you we can get this in :)

Looks good to me, thanks for the help!

One last thing to note now that we have moved the function to return symbol IDs: say we have a supertype:

supertype_sym: $ => choice (/* ...stuff */ alias($.symbol, $.unique_alias)),

right now the map will return the symbol ID for both $.symbol and the $.unique_alias, even though we cannot query supertype_sym/symbol, only supertype_sym/unique_alias. In this case would it be possible for e.g. query.c to differentiate visibility in this way? Before with returning strings we only had "unique_alias" here

Imo an argument for returning strings was that with supertypes, most of their purpose relates to queries, and the notion of symbols is not as strong because of situations like this. But I also could just be unaware of a simple way to rectify this

Yeah true, why not just keep the alias symbol? I think we'd just remove the extra logic for fetching the underlying symbol of an alias in get_supertype_symbol_map

If we do that, then for the rust rule alias(choice(...primitiveTypes), $.primitive_type), we no longer get any symbols that correspond to the primitive_type alias, sadly. After inspecting parser.c, maybe it would be enough to keep the current implementation and read the names from ts_symbol_names? (And also maybe expose that data in the rust bindings; it already is, oops)?

Edit: keeping the current implementation would still give two symbols when aliasing a named node, which is undesirable

Edit2: Actually, this makes me think there is a bug? There should exist a alias_sym_primitive_type in the rust case, no?

Ah right, aliasing a terminal is a little different than a non terminal (there's a slight optimization made here), so we can remove the fetching logic when getting the supertype map, and then, if there's no aliased symbol that exists in alias_ids (which is for non-terminal aliases), we'll just do a lookup of the parse table's symbols and fetch all the underlying terminal symbols that map to this alias.

I just pushed some changes to account for this, I think this should be correct now. Sorry that this was kind of more difficult than I expected 😅

Everything is working perfectly in my tests as well, thx again for the help 😄

cool, this is great to have - thanks @ribru17!

@ribru17 @amaanq - Just a heads up that backwards-compatibility wasn't handled correctly in this PR, in the case of WASM. If Tree-sitter 0.25.1 tries to load a WASM-compiled language that was generated by an earlier version of Tree-sitter, a crash will occur when executing this line.

I'm going to push up a fix shortly.

Ack, sorry about that. Thanks for pushing a fix!

No worries, it's a great PR anyway. I wish we had a better setup for checking backwards compatibility in tests. Right now, the tests all run against freshly-generated parsers, which makes sense for the most part. And I don't want to make the test suite super complicated.

Maybe we just need one additional test that regenerates a trivial parser with each supported ABI, and tests that it can be loaded, both natively, and via WASM.