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

Fix issues with parameters and flags #144

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Fix issues with parameters and flags #144

merged 1 commit into from
Nov 18, 2024

Conversation

blindFS
Copy link
Contributor

@blindFS blindFS commented Nov 18, 2024

This PR fixes those parameter/flag related issues in #142

@fdncred fdncred merged commit e8bdcb9 into nushell:main Nov 18, 2024
3 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Nov 18, 2024

thanks

@melMass
Copy link

melMass commented Nov 18, 2024

I'm not sure if it's related to this PR but I'm now spammed with these in neovim:
image

EDIT: It's unrelated to this, updating treesitter solved the issue

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass Yes, :TSUpdate nu is required since PR #143 changed highlights.scm file to support the new spread operators.

@melMass
Copy link

melMass commented Nov 18, 2024

Thanks, it was something else in the neovim ts plugin and updating the plugin fixed the issue! I use TSUpdate as my build command so it's ran whenever I update nvim-treesitter (very often)

@fdncred
Copy link
Collaborator

fdncred commented Nov 18, 2024

I also created a similar issue #145 that I ran upon trying to test out melmass's report.

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass Sorry, but I'd like to figure out the root cause here since other users may face the same problem. In my understanding,
neovim ts plugin hosts a stand-alone copy of queries scheme files for nu and other languages. There's delay of updating, so it can cause inconsistency between the grammar and queries files. I don't see why updating neovim ts can solve the issue (their highlights.scm for nu is still out of date).

Would you please share your nvim config about this?

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@fdncred I'm afraid #145 is related to the inconsistency that I mentioned above.

@fdncred
Copy link
Collaborator

fdncred commented Nov 18, 2024

@blindFS How so? The queries are the same in this repo and the repo that nvim pulls down. Neither queries work.

Update:
LOL, they're the same because I copied the queries over from here to my nvim-data folder. But they still didn't work until I commented the two items out that I mentioned.

@melMass
Copy link

melMass commented Nov 18, 2024

I'd like to figure out the root cause

I think it's completely unrelated to this and just an upstream "bug" in nvim-treesitter. I realized that after having the same error message in a lua file (new session, no hidden nu buffers) which prompted me to update the plugin that fixed it in all languages

Would you please share your nvim config about this?

Oh wait I actually went a commit ealier while trying to solve the issue...:

{
    'nvim-treesitter/nvim-treesitter',
    build = ':TSUpdate',

    dependencies = {
      { 'nushell/tree-sitter-nu', commit = '45f9e51e5ee296dc0965a80f3d00178d985dffbd' }
}

I'll remove it now, retry and report how it went

@melMass
Copy link

melMass commented Nov 18, 2024

Oh right, the issue is there my bad

stack traceback:
	[C]: in function '_ts_parse_query'
	...0.10.2_1/share/nvim/runtime/lua/vim/treesitter/query.lua:252: in function 'fn'
	...im/0.10.2_1/share/nvim/runtime/lua/vim/func/_memoize.lua:58: in function 'fn'
	...im/0.10.2_1/share/nvim/runtime/lua/vim/func/_memoize.lua:58: in function 'get'
	..._1/share/nvim/runtime/lua/vim/treesitter/highlighter.lua:28: in function 'new'
	..._1/share/nvim/runtime/lua/vim/treesitter/highlighter.lua:243: in function 'get_query'
	..._1/share/nvim/runtime/lua/vim/treesitter/highlighter.lua:191: in function 'fn'
	...1/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:491: in function 'for_each_tree'
	..._1/share/nvim/runtime/lua/vim/treesitter/highlighter.lua:178: in function 'prepare_highlight_states'
	..._1/share/nvim/runtime/lua
Error in decoration provider treesitter/highlighter.win:
Error executing lua: ...0.10.2_1/share/nvim/runtime/lua/vim/treesitter/query.lua:252: Query error at 193:12. Impossible pattern:
(long_flag ["="] @punctuation.special)
           ^

I can provide more info as needed

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

I think when you do :TSUpdate nu, it installs the grammar in neovim-ts's repo, which is older version than this. However your highlights.scm file is updated with this repo, which is newer. This can explain both of your problems.

@melMass
Copy link

melMass commented Nov 18, 2024

I think when you do :TSUpdate nu, it installs the grammar in neovim-ts's repo, which is older version than this. However your highlights.scm file is updated with this repo, which is newer. This can explain both of your problems.

I use Lazy to manage it so it's possible yep, but this confirms the issues is introduced by this merge

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass would you please try to remove the dependency here:

{
    'nvim-treesitter/nvim-treesitter',
    build = ':TSUpdate',
}

And also clean-up the tree-sitter-nu plugin, then try again.

@melMass
Copy link

melMass commented Nov 18, 2024

@melMass would you please try to remove the dependency here:
And also clean-up the tree-sitter-nu plugin, then try again.

Ohh I see what you meant, so it's now merged upstream and doesn't require the explicit dependency, this seems to work but I'm not sure how to know which "version" TS install on it's own.

I get why they do that but it would be so much simpler to point to the maintained repos instead of PRing updates "manually"

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass I think it is specified in the lockfile https://github.com/nvim-treesitter/nvim-treesitter/blob/master/lockfile.json

@melMass
Copy link

melMass commented Nov 18, 2024

Hmmm seems like it uses some old ones:

$ cd .local/share/nvim/lazy/nvim-treesitter/queries
$ ls nu/
╭───┬───────────────────┬──────┬────────┬─────────────╮
│ # │       name        │ type │  size  │  modified   │
├───┼───────────────────┼──────┼────────┼─────────────┤
│ 0 │ nu/indents.scm    │ file │  376 B │ 2 weeks ago │
│ 1 │ nu/highlights.scm │ file │ 5.6 KB │ 2 weeks ago │
│ 2 │ nu/injections.scm │ file │  219 B │ 2 weeks ago │
╰───┴───────────────────┴──────┴────────┴─────────────╯

Ah but it's expected.

Tbh I'm regularly confused by Treesitter even though I went quite deep into it at some point...

Shouldn't we still be able to use this repo to get the latest updates?

@melMass
Copy link

melMass commented Nov 18, 2024

@melMass I think it is specified in the lockfile https://github.com/nvim-treesitter/nvim-treesitter/blob/master/lockfile.json

But that's "just" the parser, what I don't understand is why this PR doesn't work? A feature not yet in the treesitter version used in the plugin?

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass Sure, but when you do :TSUpdate nu, you only install the locked version. If you want to use the latest grammar, I'm afraid that you have to use the method of my config as in #145

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

This PR's update of highlights.scm only works with its corresponding grammar. If your grammar is some older version, the query of the "=" token in long_flag is simply invalid ("=" is in long_flag_equals_value which is removed in this PR).

@melMass
Copy link

melMass commented Nov 18, 2024

I think I get it now, so the saner approach for me seems to accept the slight delay upstream and just use that! Thanks for the help debugging this, I'll know how to switch from upstream to latest in case I need to

@blindFS
Copy link
Contributor Author

blindFS commented Nov 18, 2024

@melMass Yes, we will keep minimum updates to queries files so this kind of trouble won't happen often.

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.

3 participants