IndianBoy42 / tree-sitter-just

Treesitter grammar for Justfiles (casey/just)
Apache License 2.0
144 stars 26 forks source link

Variable declaration and receipe parameters crashes NeoVim #69

Closed eshepelyuk closed 7 months ago

eshepelyuk commented 9 months ago

Hello @tgross35

eshepelyuk commented 9 months ago

Adding another usecase - receipe parameter declaration with default value

test param="qwe": 
    echo {{param}}

As soon as typing double quote after = - NeoVim crashes.

tgross35 commented 9 months ago

Thanks for the report, I am not sure why nvim seems to struggle so much when every other editor I test with works fine.

I think you can nvim -V9nvim.log to dump a very verbose log, but I am unsure whether anything useful will be there.

I'll take a look...

eshepelyuk commented 9 months ago

Thanks for the report, I am not sure why nvim seems to struggle so much when every other editor I test with works fine.

I think you can nvim -V9nvim.log to dump a very verbose log, but I am unsure whether anything useful will be there.

I'll take a look...

Tried this and there's nothing useful or suspicious in logs :(

getchoo commented 9 months ago

i'm able to reliably reproduce this on f807ab33c36651ecb503a291aed933932754864d with this justfile

tgross35 commented 9 months ago

Thanks for the report. Are you (or anyone) able to play around with the queries? Just comment things out and see if hiding anything fixes this crash.

Unfortunately I have no way to debug this since I haven't been able to replicate it on my system.

eshepelyuk commented 9 months ago

Thanks for the report. Are you (or anyone) able to play around with the queries? Just comment things out and see if hiding anything fixes this crash.

Unfortunately I have no way to debug this since I haven't been able to replicate it on my system.

what is your "system" ? os \ nvim version \ minimal config

getchoo commented 9 months ago

after some testing, it appears this was broken in 5766e756ff5bfe9caddeb8c5fe369558f5dc5092. this commit and all after exhibit the same behavior when building against treesitter 0.20.8 on neovim v0.9.5

Are you (or anyone) able to play around with the queries? Just comment things out and see if hiding anything fixes this crash.

commenting out the following lines from the previously mentioned file lets neovim start. editing lines to not include strings (i.e., test: (rebuild "test") -> test: (rebuild)) also fixes this, which lines up with the breakage appearing with the new string scanner

image

Unfortunately I have no way to debug this since I haven't been able to replicate it on my system.

i actually came across this using my configuration found in this nix flake. if you install nix, you should be able to reproduce this by running nix shell --experimental-features 'nix-command flakes' github:getchoo/getchvim/bc1489046e0ddf3ac9460e3b227455d3cb3606e5, which will give you a shell with my configuration

tgross35 commented 9 months ago

Thanks for the report. Are you (or anyone) able to play around with the queries? Just comment things out and see if hiding anything fixes this crash. Unfortunately I have no way to debug this since I haven't been able to replicate it on my system.

what is your "system" ? os \ nvim version \ minimal config

I most recently tested with 0.10.0-dev on aarch64 macos, and nothing in the config outside of Plug and tree-sitter. But I have also tried a few different versions, and in docker.

after some testing, it appears this was broken in 5766e75. this commit and after all exhibit the same behavior when building against treesitter 0.20.8 on neovim v0.9.5

Unfortunately that commit is about the biggest change yet since it reworked everything to do with strings :)

Are you (or anyone) able to play around with the queries? Just comment things out and see if hiding anything fixes this crash.

commenting out the following lines from the previously mentioned file lets neovim start. editing lines to not include strings (i.e., test: (rebuild "test") -> test: (rebuild)) also fixes this, which lines up with the breakage appearing with the new string scanner

Ah, sorry for the confusion. I meant editing the query files, these should be some .scm files likely located wherever Plug (or the relevant manager) downloaded these items, and tells the editor how to apply highlighting to structured grammar. I am curious whether commenting out parts of those files makes things work correctly since queries are the only thing that is unique to nvim (tree-sitter-cli and helix share the grammar, and those two tools can parse everything fine).

Unfortunately I have no way to debug this since I haven't been able to replicate it on my system.

i actually came across this using my configuration found in this nix flake. if you install nix, you should be able to reproduce this by running nix shell --experimental-features 'nix-command flakes' github:getchoo/getchvim/bc1489046e0ddf3ac9460e3b227455d3cb3606e5, which will give you a shell with my configuration

Hm, I wonder if I could get that working on CI somehow... https://github.com/IndianBoy42/tree-sitter-just/pull/64 could use some help if you have any idea how to just run nvim headlessly on files

woojiq commented 8 months ago

FYI, I can reproduce the same thing (SIGSEGV) with Helix. I tried to update just ts revision to the latest because right now helix has 3y.o revision. Queries do not affect this in any way, without queries the crash still exists.

tgross35 commented 8 months ago

FYI, I can reproduce the same thing (SIGSEGV) with Helix. I tried to update just ts revision to the latest because right now helix has 3y.o revision. Also it breaks after fixing 5766e75 for me. Queries do not affect this in any way, without queries the crash still exists.

Ah thanks, knowing it's a segfault is very helpful. Must be a bug in the external scanner then.

I should hook up a fuzzer in CI, unless somebody spots the issue (PRs welcome for either, I won't be able to get to it for a few days)

tgross35 commented 8 months ago

This could actually also be related to the Windows timeout https://github.com/IndianBoy42/tree-sitter-just/pull/67 (even though that started later)

tgross35 commented 8 months ago

Also I have some sorta-helpful debug statements in scanner.c that can help trace this back, either compile with -DDEBUG_PRINT or uncomment https://github.com/tgross35/tree-sitter-just/blob/3e55519c162997eb1809d9c05a0e9aa073cf754d/src/scanner.c#L13

ievgenii-shepeliuk commented 8 months ago

Dear all, Unfortunately I can't help on debugging / fixing this.

But the same time the plugin is unusable, so maybe it worth rolling it back to some usable state ?

I've been trying to "freeze" to particular commit in vim-plug to some older version, but there are always problems with complilation, even with year old commits.

Maybe you can suggest other ways to use older version. Thanks in advance.

woojiq commented 8 months ago

But the same time the plugin is unusable, so maybe it worth rolling it back to some usable state ?

I was actually updating to have the shebang feature. If you can't fix it now, do a temporary revert or mention it in the Readme. imo

UPD. I thought it was the author's answer, xd.

woojiq commented 8 months ago

The easiest reproduction:

echo -n '"' > /tmp/Justfile && tree-sitter parse /tmp/Justfile -0

full-valgrind-report.txt

I'd be able to investigate more on the next weekends (I hope) if no one comes up with a solution faster.

tgross35 commented 8 months ago

Thanks for the report! I’ll revert this when I’m back at a computer if I don’t spot an easy fix.

tgross35 commented 8 months ago

This was reverted for now https://github.com/IndianBoy42/tree-sitter-just/pull/72

eshepelyuk commented 8 months ago

This was reverted for now #72

Hello, after this update - when opening simple justfile - I receive an error

test:
    echo "test"

Error

Error detected while processing BufReadPost Autocommands for "{.,}justfile\c"..FileType Autocommands for "*":
Error executing lua callback: ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: table index is nil
stack traceback:
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: in function 'add_injection'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:708: in function 'f'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:137: in function 'tcall'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:283: in function 'parse'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:89: in function '_create_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:131: in function 'get_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:460: in function 'start'
        ...lugged/nvim-treesitter/lua/nvim-treesitter/highlight.lua:20: in function 'attach'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:509: in function 'attach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:532: in function 'reattach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:133: in function <.../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:132>
Error executing lua callback: ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: table index is nil
stack traceback:
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: in function 'add_injection'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:708: in function 'f'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:137: in function 'tcall'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:283: in function 'parse'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:89: in function '_create_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:131: in function 'get_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:460: in function 'start'
        ...lugged/nvim-treesitter/lua/nvim-treesitter/highlight.lua:20: in function 'attach'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:509: in function 'attach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:532: in function 'reattach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:133: in function <.../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:132>
Error executing lua callback: ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: table index is nil
stack traceback:
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: in function 'add_injection'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:708: in function 'f'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:137: in function 'tcall'
        ...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:283: in function 'parse'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:89: in function '_create_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:131: in function 'get_parser'
        ...r/neovim/0.9.5/share/nvim/runtime/lua/vim/treesitter.lua:460: in function 'start'
        ...lugged/nvim-treesitter/lua/nvim-treesitter/highlight.lua:20: in function 'attach'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:509: in function 'attach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:532: in function 'reattach_module'
        .../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:133: in function <.../plugged/nvim-treesitter/lua/nvim-treesitter/configs.lua:132>
eshepelyuk commented 8 months ago

Even more, I tried to uninstall and install just with TSUninstall / TSInstall commands And I am currently receiving error during compliation

Failed to execute the following command:
{
  cmd = <function 1>
}
"...5/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:582: table index is nil"
tgross35 commented 8 months ago

Does pinning an old version work? Those errors don't look like they are from this repo.

eshepelyuk commented 8 months ago

Does pinning an old version work? Those errors don't look like they are from this repo.

As I already mentioned, I can't make any older version work, even year old ones. So I am currently completely unable to edit justfiles from nvim. Any suggestions are appreciated.

tgross35 commented 8 months ago

The readme has and nvim-treesitter has instructions for using a local repo. Follow those and play around as needed. I would recommend trying an older version (specifically if 27b2d8a07e409025cd4160e56c1c1af939a2c556 works, since it is the point before reverted commits) and trying with queries commented out (starting with injections.scm since it seems like it may be struggling there).

If you find a fix then I can review a PR, but my focus needs to be on adding back what was lost in the revert.

eshepelyuk commented 8 months ago

Sorry, you probably misunderstood me.

I don't want and I am unable to manually compile something locally, I just need a way how to make this pluging work. Now, after latest revert - I expect it to work, but it still doesn't compile by NeoVim, so apparently latest revert still has some bugs preventing the normal installation.

tgross35 commented 8 months ago

Sorry, you probably misunderstood me.

I don't want and I am unable to manually compile something locally, I just need a way how to make this pluging work.

Tree-sitter grammars always get compiled locally, regardless of whether nvim does it or NPM.

Now, after latest revert - I expect it to work, but it still doesn't compile by NeoVim, so apparently latest revert still has some bugs preventing the normal installation.

Unfortunately, I really just need to ask that you either pin an older version (which others have been able to do) or help by submitting a fix to help the situation (which can also be a revert of specific commits). I am not going to dig deep into getting this temporary revert to work when it doesn't even have correct string parsing. I would much rather work on getting the original issue solved.

eshepelyuk commented 8 months ago

Hello @tgross35

Unfortunately, I really just need to ask that you either pin an older version (which others have been able to do)

Unfortunately, as I mentioned few times already, I was unable to do this - because I am receiving those compilation errors when try to pin any previous version, even one year old.

So I am asking for any suggestion how can I make older version work for me, because right now I can't edit any of my justfiles :(

woojiq commented 8 months ago

because right now I can't edit any of my justfiles

Then don't use tree-sitter-just. You won't have syntax highlighting, that's it.

because I am receiving those compilation errors when try to pin any previous version

99% that the problem is not in tree-sitter-just because the author, me and probably a bunch of other people can compile it (CI works too). Try to find error in your vim plugin configuratoin or idk, I personally don't use nvim.

Sorry if this sounds aggressive, but you've already been explained several times and you're still trying to convince the author to solve your problem, which he can't even reproduce.

eshepelyuk commented 8 months ago

The problem is that author doing changes without testing and breaking the plugin functionality completely. I am not using any specific setup, it's just regular ubuntu with latest neovim from Homebrew, i bet a lot of people are using the same.

And starting from New Year approx, I am only doing bug reports that plugin stopped working because author is not doing proper testing and alwasy just replies that he can't reproduce in his setup.

I am jsut trying to ask for getting some stable state like it was before intorducing those breaking changes And that state jsut be tested on very widely used setup of Ubuntu + NeoVim from homebrew.

I had zero issues with this plugin before those breaking changes were introduced in the end of previous year. So I do have a feeling that the reason is that plugin has not enough testing on simple old Ubuntu.

While I can pin any other plugin to previous version if they crash, I can't do the same with only this plugin. And I don't even see an intention to provide any help with pinning old version.

tgross35 commented 8 months ago

This is frustrating. I need to remind you of a few things:

  1. I have not been able to reproduce some of these issues. The segfault was not showing up on my machine (aarch64), leading me to believe it is primarily x86, and it does not show up with tree-sitter-cli (which is run in CI). Please appreciate how this makes these things extremely hard to track down.
  2. This project did not have any CI testing at all until I started working on it this month, before which highlighting was completely broken for plenty of things. I also do try it out on local with both Helix and neoVim but cannot guarantee 100% coverage. Please do not accuse me of not doing proper testing.
  3. The only highlighter that can be easily tested in CI is tree-sitter-cli, and this project has much more complete highlighting than many other TS grammars. I have stated multiple times that I would love to test editors in CI (to which you are more than welcome to add your configuration) but I need help getting it over the line. link.
  4. Unfortunately, as I mentioned few times already, I was unable to do this - because I am receiving those compilation errors when try to pin any previous version, even one year old.

    How do you expect me to help you? If there is no point along this project's history ever worked for you, then why do you expect top of tree to work? I want something that "just works" as much as you do, but it takes effort to get there. And so far that has been my effort.

    If you are on MacOS, pin a version after I changed scaner.cc to scanner.c https://github.com/IndianBoy42/tree-sitter-just/issues/23.

  5. The readme makes it clear that this project is a work in progress. You are not obligated to use the top of main. I tried to be helpful with a revert, but I am not going to start maintaining a LTS version of this plugin. If that does not work for you and you cannot pin a version, use the more stable but less correct (for Just) tree-sitter-make.

The license states Licensor provides the Work (and each Contributor provides its Contributions) on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND. This is FOSS: if you do not appreciate how I am doing my contributions, you are welcome to bring your own. I realize that sounds harsh, but I am imperfect and my time is limited so you need to lower your expectations.

I do not intend to comment more on this thread as it is nonproductive. Submit PRs, wait patiently until I get a chance to fix everything, or do not use this plugin.

tgross35 commented 8 months ago

This segfault is proving extremely difficult to track down. If anyone has ideas, I'm open to them. Details:

  1. I was finally able to reproduce this issue on a different machine
  2. I reimplemented the improved string parser and have it on the next branch (permalink), but it still has this segfault
  3. This issue appears x86-only and does not reliably reproduce in CI.
  4. Error is sometimes SIGBUS sometimes SIGSEGV, makes me think that there is a misaligned read somewhere
  5. The actual error happens within tree-sitter's advance function, more details here https://github.com/tree-sitter/tree-sitter/issues/2876
  6. I can't reproduce this with a simple parser, debug.c in this branch https://github.com/tgross35/tree-sitter-just/tree/1bce36f0243523a2669d22c8a29f6e7d31297d6a (just debug-build then LD_LIBRARY_PATH=tree-sitter-src/ ./debug.out '\nfoo := "abc"\n' to run).
  7. This means I can only reproduce the issue with tree-sitter test --debug-build -f 'assignment' (or an editor), which seems to test with an optimized and stripped binary despite the --debug-build flag. So my stack trace uselessly jumps directly from just.so to libc.so and I can't debug anything.

All of this is sort of pointing me to an issue that might be fixed upstream, since lexer->advance doesn't seem like it should ever be a point of failure. But I have no clue why it hasn't been reported before.

If anyone has any ideas, I could use a few 🙂

tgross35 commented 8 months ago

Actually it looks like TS does indeed pass -g regardless of the debug-build flag, so I'm not sure why I have no debug info https://github.com/tree-sitter/tree-sitter/blob/660481dbf71413eba5a928b0b0ab8da50c1109e0/cli/loader/src/lib.rs#L450-L467

tgross35 commented 7 months ago

Has anybody run into this recently? The scanner changes from #79 were merged a couple weeks ago, which hopefully fixed things.

aleprovencio commented 7 months ago

It's working fine here, thanks for you efforts @tgross35

tgross35 commented 7 months ago

Thanks for the report :) I'll close this, please feel free to open a new issue if there are further problems