Clean up parse table representation, use 16 bits for production_id #943
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also remove the use of bitfields from the parse table format. In all cases, bitfields were not necessary to achieve the current binary sizes. Avoiding them makes the binaries more portable. There was no way to make this change backward-compatible, so we have finally dropped support for parsers generated with an earlier version of Tree-sitter. At some point, when Atom adopts this version of Tree-sitter, this change will affect Atom users who have installed packages using third-party Tree-sitter parsers. The packages will need to be updated to use a regenerated version of the parsers.
I think this is the last additional field that's needed so that every array member of TSLanguage has a length that can be calculated at runtime.
This was referenced Feb 26, 2021
|
Most Emacs users of |
maxbrunsfeld
added a commit
that referenced
this pull request
Mar 1, 2021
Due to the breaking ABI change in #943, this is our chance to reorder the fields in a more logical way.
This was referenced Mar 2, 2021
Closed
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #511
Background
Currently, because of the way that Tree-sitter's parser ABI incrementally evolved, the
production_idfield on parse actions (which is used to identify the sequence of fields and aliases that applies to a given node's children) is represented as auint8_t. So far, this has not been a problem for any of the grammars that we maintain, but other users have hit the limit, and when it happens, it's very confusing.Change
This PR changes
production_idto a niceuint16_t, which should always be more than large enough.Unfortunately, because of some shortcomings of the old struct layout, this was not feasible to do without making a breaking change to the parser ABI. So, for the first time in a long time, I have bumped tree-sitter's
TREE_SITTER_MIN_COMPATIBLE_LANGUAGE_VERSIONconstant. This means that new versions of the library will refuse to load parsers that were generated with previous versions of the CLI. Specificallyts_parser_set_languagewill returnfalse. Users will need to regenerate their parsers.Normally, this is not a problem: I think that for most use cases, users can easily upgrade the library and their parsers together. There are a few exceptions that I know of:
Atom - At some point, when Atom adopts this version of Tree-sitter, this change will affect Atom users who have installed packages using third-party Tree-sitter parsers. The packages will needto be updated to use a regenerated version of the parsers. /cc @darangi - I don't think Atom plans on upgrading Tree-sitter anytime soon, so this is probably not urgent for you I think?
Neovim - I think that some end-users of Neovim have installed their own third-party parsers. When Neovim upgrades to the latest version of Tree-sitter, Neovim will refuse to load these older parsers. The end users will need to install new versions of the parsers. /cc @bfredl @vigoux @theHamsta @tjdevries
Emacs-Tree-Sitter - /cc @ubolonton I'm not sure if users can install their own parsers. If so, then this is something to be aware of.
Details
Since an ABI change was already needed, I took this opportunity to fix some dumb aspects of the current ABI. There are no more bitfields (they were used unnecessarily before). Also, I've added one more field to the
TSLanguagestruct -production_id_count, which makes it so that all the arrays onTSLanguagecan have their length known at runtime.