Add more LuaLS types for better LSP info

[Danielkonge]

I added a lot of type information, so we can get proper LSP information when setting up the plugin:

I also made a few other small edits that might be worth discussing. Here is a list of things to think about before this is ready to be committed:

• [x] Right now I allow random strings in place of a known language, strategy, highlight color or query. This is because a user of the plugin could define their own, but this also means that the LSP won't really help out with spelling mistakes, since e.g. vim misspelled as vin is still a string. We could change this or see if there is some good workaround. (Spelling mistakes will be caught by :checkhealth, so we should be good with the current code.)
• [x] Since we don't want setup in rainbow-delimiters.lua, I have added an __index meta method for now. This is a temporary solution, that I think should make lazy work as expected, but I am not sure if that is a good solution. I assume you would still like to avoid putting setup in rainbow-delimiters.lua? (Think about this later.)
• [x] I think the standard for Neovim is to use integer instead of number for stuff like bufnr (or anything else that you know is an integer). Do you want to change number to integer or leave it as it is now? (Yes, use integer.)
• [x] I added 'comment' to the blacklist, since that seems like a good idea based on #53. I can remove it again, if you prefer? (Remove it for now and instead investigate why comment causes problems.)

Other notes:

• If you ever do a rewrite, note that it is easier to give types for strings that don't need to be inside ['<name>'] in a Lua table (like ['local'] or ['']).
• I couldn't figure out a way for the plugin to capture the fact that the type of vim.g.rainbow_delimiters should be of type rainbow_delimiters.config. So for now a user of the plugin will have to add ---@type rainbow_delimiters.config before vim.g.rainbow_delimiters to get the LSP to work as expected. I have added this to the docs for now, but we can remove it if we figure out how to avoid this step. If you use the setup function the type is already correct now without the user adding anything.
• I haven't figured out a way to remove used strings from the arrays with strings (like highlight or blacklist). So for those the LSP will continue to suggest a string that has already been used right now, but this should be easy enough to work with still.

[HiPhish]

I am all in favour of more type hints, but I think you went a bit too far in typing out everything.

Right now I allow random strings in place of a known language, strategy, highlight color or query. This is because a user of the plugin could define their own, but this also means that the LSP won't really help out with spelling mistakes, since e.g. vim misspelled as vin is still a string. We could change this or see if there is some good workaround.

I don't think we need to type-check arbitrary strings. This is already handled by :checkhealth at runtime: for each language listed check if there is a corresponding grammar installed and report an error if there isn't.

==============================================================================
rainbow-delimiters: require("rainbow-delimiters.health").check()

Custom strategies ~
- OK Valid custom strategy for 'commonlisp'.
- ERROR No parser installed for 'asdfwerwer'

How would your suggestion handle language names we don't have registered in the plugin?

Since we don't want setup in rainbow-delimiters.lua, I have added an __index meta method for now. This is a temporary solution, that I think should make lazy work as expected, but I am not sure if that is a good solution. I assume you would still like to avoid putting setup in rainbow-delimiters.lua?

This is a whole can of worms I have been kicking down the road. If the user calls a setup function (or any function) and that function in return has require it kicks off a whole chain of require calls, each of which has to traverse the file system to find the module. That is one of the reasons why setup functions are bad, and people have been working their way around that with lazy-loading package managers, people are trying to solve a problem they should not be having in the first place. This is why it is better to have a g: variable instead, defining variables is cheap in comparison.

I will have to look into this issue deeper because my code still does require calls that I should avoid.

I think the standard for Neovim is to use integer instead of number for stuff like bufnr (or anything else that you know is an integer). Do you want to change number to integer or leave it as it is now?

Can the language server handle integer? I know Lua 5.1 only has a number type, integers were only added in Lua 5.3. If the server can handle integer then that would be a better annotation, the more accurate information is, the better.

I added 'comment' to the blacklist, since that seems like a good idea based on https://github.com/HiPhish/rainbow-delimiters.nvim/issues/53. I can remove it again, if you prefer?

I would rather know why comment needs to be blacklisted in the first place, since we don't have any query for it. The problem with populating the blacklist by default is what happens if the user has his own blacklist. Does the user's blacklist override the default one? Do they get merged? If they get merged, what if the user for whatever reason actually does want comment to work?

If you ever do a rewrite, note that it is easier to give types for strings that don't need to be inside ['<name>'] in a Lua table (like ['local'] or ['']).

Please, no more rewrites :( Yeah, local was maybe not the best choice. Instead of the empty string I originally wanted the index 1, which looked really cool in Lua: {foo, x = 'bar', y = 'baz'}. This is similar to *args and **kwargs in Python. Unfortunately tables which have both an array- and a dict part do not convert well between Lua and Vim script.

[Danielkonge]

I am all in favour of more type hints, but I think you went a bit too far in typing out everything.

This is really more of a draft where I played around with the types a bit. I plan to clean up everything a bit more soon. The main focus is what I did in rainbow-delimiters.types.lua. (I changed the status to draft in Github for now.)

I don't think we need to type-check arbitrary strings. This is already handled by :checkhealth at runtime: for each language listed check if there is a corresponding grammar installed and report an error if there isn't.

==============================================================================
rainbow-delimiters: require("rainbow-delimiters.health").check()

Custom strategies ~
- OK Valid custom strategy for 'commonlisp'.
- ERROR No parser installed for 'asdfwerwer'

How would your suggestion handle language names we don't have registered in the plugin?

I am not really including the types for the strings in e.g. whitelist or blacklist to type check them, it is more to get the completions to help writing your lists. I think the current system works fine, so maybe the current solution is good enough. You will get completions while writing e.g. commonlisp, and if you don't use the completion and accidentally write comonlisp or similar, then :checkhealth will catch it.

Do you not think it is worth it to include a list of the language strings to give you completions?

This is a whole can of worms I have been kicking down the road. If the user calls a setup function (or any function) and that function in return has require it kicks off a whole chain of require calls, each of which has to traverse the file system to find the module. That is one of the reasons why setup functions are bad, and people have been working their way around that with lazy-loading package managers, people are trying to solve a problem they should not be having in the first place. This is why it is better to have a g: variable instead, defining variables is cheap in comparison.

I will have to look into this issue deeper because my code still does require calls that I should avoid.

Ok, for now I will remove this code from this pull request then. It sounds like we should think about that more.

Can the language server handle integer? I know Lua 5.1 only has a number type, integers were only added in Lua 5.3. If the server can handle integer then that would be a better annotation, the more accurate information is, the better.

LuaLS can handle integer no matter which version of Lua you use (i.e., for 5.1-5.4 + LuaJIT)

I would rather know why comment needs to be blacklisted in the first place, since we don't have any query for it. The problem with populating the blacklist by default is what happens if the user has his own blacklist. Does the user's blacklist override the default one? Do they get merged? If they get merged, what if the user for whatever reason actually does want comment to work?

I will remove comment from the blacklist for now and try to see if I can figure out why it causes problems instead.

Please, no more rewrites :( Yeah, local was maybe not the best choice. Instead of the empty string I originally wanted the index 1, which looked really cool in Lua: {foo, x = 'bar', y = 'baz'}. This is similar to *args and **kwargs in Python. Unfortunately tables which have both an array- and a dict part do not convert well between Lua and Vim script.

I don't plan on doing work towards a rewrite either. :/ It was more of a general comment.

I will add some of the updates later tonight.

[Danielkonge]

I have cleaned up this draft a bit now (removed superfluous type annotations and removed the __index code and comment blacklist mentioned above).

Please let me know if there is anything in the types you would want me to simplify or remove. I think the current setup in rainbow-delimiters.types.lua gives me good suggestions/completions when setting up the plugin, but I can easily change it.

[HiPhish]

I have reviewed all changes except for lua/rainbow-delimiters.types.lua and lua/rainbow-delimiters/stack.lua, I will take a bit more time for those. I noticed that you have also made some corrections to the code and the manual. Ideally those should be separate PRs so we can merge then sooner, but it's OK for now.

[Danielkonge]

I have reviewed all changes except for lua/rainbow-delimiters.types.lua and lua/rainbow-delimiters/stack.lua, I will take a bit more time for those. I noticed that you have also made some corrections to the code and the manual. Ideally those should be separate PRs so we can merge then sooner, but it's OK for now.

I thought it was mostly minor things that might as well be included here, but if you prefer I can quickly move the small cleanup stuff to another pull request.

Update: Since I saw the error in #59 affected some people, I made a separate pull request with the small cleanup stuff. That pull request should fix that issue too.

[HiPhish]

How does the file rainbow-delimiters.types.lua work? Is it used automatically by the language server or do I need to specify some project settings (e.g. in .luarc.json)?

[Danielkonge]

How does the file rainbow-delimiters.types.lua work? Is it used automatically by the language server or do I need to specify some project settings (e.g. in .luarc.json)?

neodev should ensure that everything is set up correctly for all plugins by default.

Without neodev I think you need to set your runtime path correctly in a .luarc.json or in your Lua LSP setup.

[Danielkonge]

Sorry the sync on this branch got messed up. I will fix it soon.

Update: It should be fixed now.

[HiPhish]

I'm done with stack.lua, this just leaves the type definitions file.

[HiPhish]

Two notes:

• I removed the whitespace from the description because it shows up in the hover window
• All configuration fields are optional; this plugins works even without any configuration

[Danielkonge]

Two notes:

• I removed the whitespace from the description because it shows up in the hover window
• All configuration fields are optional; this plugins works even without any configuration

In that case I guess [''] should also be optional in the strategies and queries. I have updated this now.

[Danielkonge]

Also, should we maybe add a note in the docs and/or README about making sure to set up the LSP correctly (e.g. via Neodev)?

[HiPhish]

Also, should we maybe add a note in the docs and/or README about making sure to set up the LSP correctly (e.g. via Neodev)?

I don't think so. People either already know how to set up the Lua language server correctly, in which case that's just noise, or they don't, in which case that's just useless and confusing bloat.

I feel the same about plugin installation instructions that list standard code snippets for five different plugin managers, when it should be just one sentence that says "install it like any other plugin". Unless the plugin has special requirements (compiled binary, dependencies, extra configuration steps) of course. Maybe my brain is wired differently, but when I keep seeing the same text over and over again by brain starts to block out that boilerplate, and that's when mistakes are made because there was one time where it was actually useful, but I did not see it.

[HiPhish]

I have removed the class definition for the plugin library. The language server can derive the type of all fields from their values, and we never pass the table to any other function. Is there something that would work better if we had a separate class for the table?

[HiPhish]

I don't really like that we have to list all supported languages three times. I tried this annotations:

---@field query table<rainbow_delimiters.language, string>?

In my mind this should work, the key type is rainbow_delimiters.language, so the language server should suggest on the valid values, but it doesn't suggest any of them. Do you have an idea how we could reuse the rainbow_delimiters.language type? Otherwise will will have to put up with the huge list.

[HiPhish]

I have removed the type for highlight groups. People will either use the default list, in which case they don't need auto-completion, or they will use custom highlight groups from some theme, in which case the auto-completion is actually counter-productive.

[Danielkonge]

I have removed the class definition for the plugin library. The language server can derive the type of all fields from their values, and we never pass the table to any other function. Is there something that would work better if we had a separate class for the table?

From what I can tell this doesn't make anything worse than before, so it makes sense to remove it, yes.

Update: Actually the completions don't seem to work as well for me with this setup when choosing e.g. global via the enter key inside rainbow_delimiters.strategy['glo|'] ( | = cursor ). But this seems more like a LuaLS bug than an actual problem with the types.

[HiPhish]

I have nothing else to add, so if you are done as well let's squash and merge everything.

But this seems more like a LuaLS bug than an actual problem with the types.

Yes, I noticed it as well, but it should be handled by the language server.

[Danielkonge]

I don't really like that we have to list all supported languages three times. I tried this annotations:

---@field query table<rainbow_delimiters.language, string>?

In my mind this should work, the key type is rainbow_delimiters.language, so the language server should suggest on the valid values, but it doesn't suggest any of them. Do you have an idea how we could reuse the rainbow_delimiters.language type? Otherwise will will have to put up with the huge list.

It is hard to avoid multiple "duplicate" types like the current setup, because LuaLS doesn't really allow us to construct the types programatically. I don't know any good way than the current definitions that gives us proper completions.

Something like

---@class rainbow_delimiters.config.strategies
---@field [''] (rainbow_delimiters.strategy | fun(): rainbow_delimiters.strategy?)?
---@field [rainbow_delimiters.language] (rainbow_delimiters.strategy | fun(): rainbow_delimiters.strategy?)?

should give the correct type checks, but not the correct completions, which I think is a big part of the help from the LSP, and I think it is probably the same for what you wrote; you will get the type checks but not completions.

[Danielkonge]

I have nothing else to add, so if you are done as well let's squash and merge everything.

I have squashed it now.

[Danielkonge]

One note before you commit it:

If we write ['global'] = ... instead of global = ..., then we do get what I would consider better completions (as long as we recommend using ['global'] in the docs anyways). So maybe we should change this back (that was how it used to be), unless you prefer to keep it as global = ...?

Update: The Cleanup commit shows what I mean. I can squash it again if you are fine with that change?

[HiPhish]

Either way is fine by me. I like .global better, but since we cannot have .local let's be consistent.

[Danielkonge]

Either way is fine by me. I like .global better, but since we cannot have .local let's be consistent.

Okay, I have made it ['global'] now to be consistent (since we can't have .local as you said), and I have squashed the commits again.

[Danielkonge]

I added a note on types to CONTRIBUTING.rst in a separate commit. I can squash this commit too if you prefer. Also, if you commit the luadoc pull request first, I can add its types in this commit.

[Danielkonge]

I added the luadoc types mentioned above now.

[HiPhish]

I have merged it now, thank you.