Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kmoschcau
Copy link

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

  • file type detection
  • parser registration
  • injections queries

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.

First, makes this repo installable via plugin manager. In addition it
already takes care of most setup for the user:
- file type detection
- parser registration
- injections queries

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
Copy link
Collaborator

Looking nice 😄

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
Copy link
Author

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.

This fixes installation problems and allows the user to pick which
additional languages to install and configure.
@kmoschcau
Copy link
Author

@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
Copy link
Collaborator

qvalentin commented Jan 5, 2024

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 😕 :

  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.

@@ -0,0 +1 @@
; inherits: gotmpl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can add helm specific functions here later?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, that's a good idea. Do you have some in mind that you want to specifically highlight beyond their gotmpl highlighting?

local parser_config = require("nvim-treesitter.parsers").get_parser_configs()

--- @param additional_langs string[]
local function configure_filetype(additional_langs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functions seems to be generic, but then it contains details for the helm filetype. Maybe we should just make one configure_helm_filetype function instead, which is called, when additional_langs contains "helm". Or known_additional_langs could also contain the patterns of the known languages.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is just a misleading name. I named it after vim.filetype. Basically this means "configure filetype detection". Should I just rename the function to configure_filetype_detection?

@kmoschcau
Copy link
Author

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:
nvim-treesitter/nvim-treesitter#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
Copy link
Collaborator

qvalentin commented Jan 6, 2024

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
Copy link
Author

kmoschcau commented Jan 6, 2024

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
Copy link
Collaborator

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

I thought of another solution and opened up #15, please check it out.

@kmoschcau
Copy link
Author

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.

Copy link
Collaborator

@qvalentin qvalentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if we should turn this repo into a nvim plugin.
For helm you could just use https://github.com/towolf/vim-helm/tree/master and for gotemplate maybe https://github.com/fatih/vim-go/tree/master?

([
(field)
(field_identifier)
] @variable.member (#set! priority 105))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can now move the priorities to the nivm-treesitter repo and delete the queries here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR for the queries in nvim-treesitter.

end

M.setup = function(options)
local additional_langs = options.additional_langs or { "helm" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we just just add the helm filetype always and remove the whole config option to keep things simple.


## Installation

Install with your favorite package manager.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be more like:

  1. get filetype detection working (you can install this repo as a plugin)
  2. Install the grammar you need with :TSInstall (gotmpl|helm)

@kmoschcau
Copy link
Author

kmoschcau commented May 31, 2024

@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
Copy link
Collaborator

Please check out #20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants