ngalaiko / tree-sitter-go-template

Golang template grammar for tree-sitter
MIT License
74 stars 27 forks source link

Change the parser repo to an installable plugin and update queries/injections #13

Open kmoschcau opened 8 months ago

kmoschcau commented 8 months ago

First, makes this repo installable via plugin manager. In addition it already takes care of most setup for the user:

Second, it defines helm as a language that uses the gotmpl language, but with different injection queries. This allows the standard gotmpl language to use simple text for its text nodes and only injects yaml as a language in helm templates. The injections are also set to combined, to fix some highlighting issues.

Third, it increases the priority of gotmpl highlight queries, so that the gotmpl highlights are always visible above the highlights of any injection language.

qvalentin commented 8 months ago

Looking nice :smile:

Using this minimal config: https://github.com/qvalentin/helm-ls/blob/feat-tree-sitter-nivm-plugin/examples/nvim/init.lua I get the following error:

Error detected while processing BufReadPost Autocommands for "*":
Error executing lua callback: ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24: Error executing lua: ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:25: BufReadPost Autocommands for "*"..FileType Autocommands for "*"..function <SN
R>1_LoadFTPlugin[19]..script /home/qv/.local/share/nvim/lazy/tree-sitter-go-template/ftplugin/helm.lua: Vim(runtime):E5113: Error while calling lua chunk: ...0.9.5/share/nvim/runtime/lua/vim/treesitter/language.lua:93: no parser for 'helm' language, see :h
elp treesitter-parsers
stack traceback:
        [C]: in function 'error'
        ...0.9.5/share/nvim/runtime/lua/vim/treesitter/language.lua:93: in function 'add'
        ...hare/nvim/lazy/tree-sitter-go-template/ftplugin/helm.lua:2: in main chunk
        [C]: in function 'nvim_cmd'
        ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:25: in function <...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24>
        [C]: in function 'nvim_buf_call'
        ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24: in function <...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:10>
stack traceback:
        [C]: in function 'nvim_cmd'
        ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:25: in function <...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24>
        [C]: in function 'nvim_buf_call'
        ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24: in function <...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:10>
stack traceback:
        [C]: in function 'nvim_buf_call'
        ...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:24: in function <...3-neovim-unwrapped-0.9.5/share/nvim/runtime/filetype.lua:10>

Running :TSInstall helm seems to fix it, but this does not work for my main nvim config.

kmoschcau commented 8 months ago

The installation of the parsers is a bit of a weak point at the moment. I'm not entirely happy with it either. Usually it worked for me by just reloading the helm file (:e) after the parser is installed.

I couldn't find any easy nvim-treesitter API, that allows installing a configured parser. If you have a hint how this could work, I can update the plugin.

kmoschcau commented 8 months ago

@qvalentin I think I fixed it. I had one last error until the end, but that was because gotmpl was set in my own treesitter config under ensure_installed. Once I removed that, everything worked smoothly.

I also added the ability for the user to only pick a subset of the additional languages to install.

qvalentin commented 8 months ago

Maybe we should add instructions on how to migrate from having the grammar installed manually. And also how to make a clean reinstall. For me this worked:

:TSUninstall helm
:TSUninstall gotmpl

restart nvim (twice for helm to work).

While the minimal config is working pretty good now, for my person config (which is based on AstoNvim I had to change how lazy loads the plugin :confused: :

  event = { "BufReadPre", "BufNewFile", "BufEnter" }

is working. I was expecting ft="helm" to also work, but it did not sadly. EDIT: ft="helm" works now, I guess we should use this, or better

    ft = { "helm", "gotmpl" }

EDIT 2: A file inside the folder ftdetect would be needed for this to work (see vim-helm)

Do you know any other grammar that can be installed with a nvim plugin? Maybe we can look into how they implemented things.

kmoschcau commented 8 months ago

Agreed, I will have a look at adding migration instructions.

I also discovered that this plugin will cause errors when just running :TSUpdate. Sadly this is unavoidable I believe. The reason for that is this: There is currently no way to tell treesitter to use the same parser for two different filetypes with distinct queries for each filetype. There is already a related discussion here: https://github.com/nvim-treesitter/nvim-treesitter/issues/2241

What is possible is to simply use the same parser for multiple filetypes, but then you only get to use a single set of queries. And since queries don't have a mechanism to match on the filetype, we need separate queries. So the only way we have right now is to install the same parser twice. Once as "gotmpl" and once as "helm" (and more for future languages). Just so we can have queries for "gotmpl" and queries for "helm". But if you then look at the ftplugin/helm.lua file, you can see me telling treesitter to load the "helm" parser (which is just the "gotmpl" parser in a file named "helm") and with the C entry symbol "gotmpl". Otherwise this would break. Because treesitter expects parser files to have a C symbol in them that corresponds to their file/lang name by default.

As for lazy loading I'm not sure that's a good idea. I don't lazy load this plugin at the moment either and it works fine. The only thing you would lazy load is our setup code and that's arguably to late, because the setup code sets up filetype detection. Treesitter already only loads the parsers it needs for the corresponding file types, so there really shouldn't be a need to lazy load. That's also the reason why a file in ftdetect would work, because that would always be loaded by neovim, regardless of plugin setup in Lazy. But ftdetect is deprecated and instead we should use vim.filetype, as I already do in our setup function.

Lastly, no I don't know of any other parser that can be loaded as a plugin. Though from the docs, neovim's treesitter should load any parser/*.so files in any plugin directory. We currently only have this step with nvim-treesitter, because we don't ship compiled parsers. If we did, we could avoid this whole nvim-treesitter setup and just be treated as a normal plugin. Then our own plugin updates would basically be equivalent to parser updates as well.

qvalentin commented 8 months ago

Wouldn't it be better if we tried to add the parser to https://github.com/nvim-treesitter/nvim-treesitter? Following this example PR should be easy. This should at least work for gotmpl, for helm a good solution would still be needed I guess?

kmoschcau commented 8 months ago

I never looked into adding a parser to the official repo, but sure that's also a solution. Though it would only be the parser then. We'd have to still provide stuff like the commentstring to either core or ourselves as a separate plugin.

qvalentin commented 7 months ago

Hi @kmoschcau, sorry for being a little bit inactive on this.

I thought of another solution and opened up https://github.com/ngalaiko/tree-sitter-go-template/pull/15, please check it out.

kmoschcau commented 6 months ago

We need to discuss what we want to do with this PR and the repo as a whole. Queries now live in the treesitter repo, but the parser still comes from here. Also stuff like the filetype detection and comment string are still in here. The latter two would be candidates for PRs on neovim core I believe.

kmoschcau commented 3 months ago

@qvalentin I think with the queries moved over, we can probably close the PR. You are right, this repo should focus on the parser. The only thing really left in this PR would be file type detection and other plugins are better suited for that.

qvalentin commented 1 month ago

Please check out https://github.com/ngalaiko/tree-sitter-go-template/pull/20