search

HiPhish / rainbow-delimiters.nvim

Rainbow delimiters for Neovim with Tree-sitter
https://gitlab.com/HiPhish/rainbow-delimiters.nvim
Apache License 2.0
532 stars 39 forks source link

fix: improve global strategy #92

Closed Danielkonge closed 8 months ago

Danielkonge commented 8 months ago

I am not sure if it was always this bad and I just started noticing or if this is because of a recent upstream neovim update, but currently rainbow-delimiters struggles with highlighting "live" text. (I don't think it used to be this bad, so I think it is an upstream change affecting us.)

E.g. writing print() on a blank line in Lua in the middle of some code doesn't highlight the parentheses correctly until I do something extra (e.g. reload the buffer or make some edits around the text to get it captured correctly).

This should fix that problem at least on new versions on neovim 0.10. (I haven't yet tested if this works as expected in neovim 0.9.5, because my usual way of uninstalling and reinstalling neovim got a bit messed up recently.)

The reason the following works is that iter_captures gives us the containers for the surrounding delimiters, so we get the right level as usual, but it won't include the surrounding containers actual delimiters and sentinels, thus we need to do a little extra work after the loop ourselves in cases where we only look at e.g. one line of updates. (Did iter_captures use to include all delimiters and sentinels? Maybe that is the change?)

HiPhish commented 8 months ago

I cannot see any problems in Neovim v0.10.0-dev-2164+gdc466f9a6 with the current master. Can you please provide some minimal code example or record a short GIF or video?

Danielkonge commented 8 months ago

I am using NVIM v0.10.0-dev-d9946006c (I think the neovim update that might have changed the behavior is recent -- if there is a change. I am still not sure...).

Current master of rainbow-delimiters:

https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/878d9929-1b3f-4286-b11c-c1aa312f1d35

This pull request:

https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/9addd98f-364a-47f7-a56f-83ccf1ee3465

HiPhish commented 8 months ago

What OS and which build are you using? I am running the current AppImage on Linux.

Danielkonge commented 8 months ago

What OS and which build are you using? I am running the current AppImage on Linux.

macOS and I built from the commit 10 hours ago (commit d994600). (I don't think this should depend on the OS though.)

If you are trying to reproduce it, are you on the same line of lua/rainbow-delimiter/strategy/global.lua? It mainly is a problem when you have a bit of code with nesting around the line you are writing on, so looking at the exact same line makes it easier to compare.

HiPhish commented 8 months ago

Neovim 0.10 crashes for me with an assertion error when I scroll through that file. Even if rainbow delimiters are disabled. Disabling the language server helps, but on line 81 when I try to replicate your print it crashes regardless. I guess that line is cursed somehow?

nvim: /home/runner/work/neovim/neovim/src/nvim/marktree.c:1529: marktree_itr_next_skip: Assertion `itr->x->ptr[itr->i]->parent == itr->x' failed.

I don't think this should depend on the OS though.

I don't know. The binaries are compiled differently, so maybe we both have a bug, but it manifests less badly for you.

Danielkonge commented 8 months ago

Neovim 0.10 crashes for me with an assertion error when I scroll through that file. Even if rainbow delimiters are disabled. Disabling the language server helps, but on line 81 when I try to replicate your print it crashes regardless. I guess that line is cursed somehow?

nvim: /home/runner/work/neovim/neovim/src/nvim/marktree.c:1529: marktree_itr_next_skip: Assertion `itr->x->ptr[itr->i]->parent == itr->x' failed.

That sounds like this https://github.com/neovim/neovim/issues/27137, which should be fixed in the commit about 13 hours ago.

Also, it is not just that line. I have the behavior on a lot of lines, and the reason I started trying to fix it was because I ran into it a lot in some of my python code.

I don't know. The binaries are compiled differently, so maybe we both have a bug, but it manifests less badly for you.

Of course you are right, but so far I have just never really experienced any OS specific behavior with treesitter.

HiPhish commented 8 months ago

That sounds like this neovim/neovim#27137, which should be fixed in the commit about 13 hours ago.

Good to know, I'll try again once the new nightly AppImage is built. I don't feel like building myself form source.

HiPhish commented 8 months ago

I can reproduce it now, but only with the rainbow-blocks query. I'll try to see if I can write a test for this (yes, we have proper Busted tests now, it's really cool)

Screenshot_20240124_175910

Danielkonge commented 8 months ago

I can reproduce it now, but only with the rainbow-blocks query. I'll try to see if I can write a test for this (yes, we have proper Busted tests now, it's really cool)

To reproduce with rainbow-delimiters queries, you can go to line 62 and delete the () after new and then try to write it again.

https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/50f0659d-ce8b-4e08-b368-1b8361f761f2

The problem is happening when the captured change is on only one line, but the changed container(s) is/are inside a container spanning more surrounding lines. E.g. when we have something like:

{ -- outer container, different line
    print() -- change only on this line
} -- outer container, different line

With this pull request:

https://github.com/HiPhish/rainbow-delimiters.nvim/assets/7075380/03f762c7-910f-4e5f-90bb-9a016be1d2d3

HiPhish commented 8 months ago

Looks like I cannot write the test after all. The on_changedtree callback does not get called when the change happens over RPC in an embedded Neovim, even if I use nvim_feedkeys. I would have to further investigate whether this is an actual Neovim bug, but for now let's skip writing a test.

Danielkonge commented 8 months ago

Looks like I cannot write the test after all. The on_changedtree callback does not get called when the change happens over RPC in an embedded Neovim, even if I use nvim_feedkeys. I would have to further investigate whether this is an actual Neovim bug, but for now let's skip writing a test.

If there is really no way to test this, I think it makes sense to create an issue upstream asking about it.

Also, we could probably make an issue upstream about including the full content of the nested queries captured in iter_captures, though this is probably one of the only plugins (at least that I can think of) affected by this.

HiPhish commented 8 months ago

If there is really no way to test this, I think it makes sense to create an issue upstream asking about it.

I had to do further investigation to figure out what exactly the issue is, and it's really obvious in hindsight. It takes a while after the change for the callback to run through. It does not take much time, but because the callback is run asynchronously the test will be done probing for extmarks before they have been applied. This makes it look as if the callback had never run wheres in reality the callback just had not yet finished. This also means there is no upstream issue to report.

I should be able to make the test work by letting the test sleep for half a second or so. I don't like this solution; if I have to sleep in many tests it will slow down the entire test process. I cannot think of a better solution though.

HiPhish commented 8 months ago

I am merging this now as it is. The test has always succeeded even without your fix, so it's not really much of a test, but still better than nothing I guess.

The reason the test was not working previously is not because I needed to wait, but because I had to call vim.treesitter.start first. My own configuration does it implicitly through nvim-treesitter, which is why I never noticed anything wrong, but tests are isolated from my config. This means there is no need to sleep and slow down testing.