garymjr / nvim-snippets

Snippet support using native neovim snippets
MIT License
215 stars 13 forks source link

Support built-in variables #11

Open abusch opened 3 months ago

abusch commented 3 months ago

Some snippets from friendly-snippets rely on some built-in variables to be provided to work. For instance, the date snippet expands to ${CURRENT_YEAR}-${CURRENT_MONTH}-${CURRENT_DATE}. These used to work with LuaSnip as it provides those variables, but they don't work with nvim-snippets.

For reference, the list of variables VSCode supports is here: https://code.visualstudio.com/docs/editor/userdefinedsnippets#_variables

abusch commented 3 months ago

Hmm, never mind. This should be handled directly by vim.snippet.expand() but from what I can tell, it only recognises the TM_XYZ variables, not the other ones such as CURRENT_DATE 😞

Edit: ah, that's because the LSP spec only defines a subset of what VSCode supports.

chrisgrieser commented 3 months ago

Two important ones defined by the VSCode spec are $CLIPBOARD and $LINECOMMENT, which are included in a lot of snippets.

I think they could simply be inserted before passing the snippet to vim.snippet.expand?

dpetka2001 commented 3 months ago

$UUID also maybe? I can't seem to expand the uuid snippet (it just expands to UUID), whereas Luasnip cmp source expands it correctly.

telemachus commented 3 months ago

I think they could simply be inserted before passing the snippet to vim.snippet.expand?

I wondered the same thing, but after looking at the code, I don't see any explicit call to vim.snippet.expand. Am I missing something obvious?

garymjr commented 3 months ago

I'm going to spend some time looking to see how luasnip is handling some of these variables. Currently I'm relying on vim.snippet.expand to handle them, but it appears not all of them are currently supported.

abusch commented 3 months ago

I wondered the same thing, but after looking at the code, I don't see any explicit call to vim.snippet.expand. Am I missing something obvious?

Yeah, I was confused too... I think the way it works is via the nvim-cmp source, which returns the snippets as LSP snippets, which neovim's LSP code now knows how to expand using vim.snippet.expand(). I could be wrong though 🤷🏻

From what I can tell, the built-in vars are defined here in luasnip: https://github.com/L3MON4D3/LuaSnip/blob/master/lua/luasnip/util/_builtin_vars.lua

folke commented 3 months ago

Variables are handled by vim.snippet.

Someone should create a PR in core for https://github.com/neovim/neovim/blob/master/runtime/lua/vim/snippet.lua#L24

chrisgrieser commented 3 months ago

Looks straightforward, I can make a PR later today

telemachus commented 3 months ago

Looks straightforward, I can make a PR later today

Right now, vim.snippet doesn't support TM_SELECTED_TEXT because (as the comment says), -- Snippets are expanded in insert mode only, so there's no selection.

I don't understand that. Can't a snippet begin by hitting <Tab> in visual mode? In any case, it may be worth trying to get support for TM_SELECTED_TEXT into vim.snippet.

More background: Currently, vim.snippet implements TM_SELECTED_TEXT as a virtual no-op. vim.snippet returns the default string if one is present or an empty string if there is no default. The "selection" implied by TM_SELECTED_TEXT is ignored.

The specific question of TM_SELECTED_TEXT was discussed at least twice when vim.snippet was in process.

  1. https://github.com/neovim/neovim/pull/25301#discussion_r1348178743
  2. https://github.com/neovim/neovim/pull/25301/files#r1360184463

There was also a pull request related to this that was eventually closed: https://github.com/neovim/neovim/pull/27375.

I saw two reasons why TM_SELECTED_TEXT isn't implemented.

  1. vim.snippet was intended to be relatively minimal and only to provide "expansion of LSP snippet completion results" (per this comment).
  2. The original author of vim.snippet said in several places that snippet expansion only happens in insert mode. (E.g., here and here.)

I think (2) makes sense from the point of view of LSP completion results. In other contexts, however, it's definitely possible to visually select, say, three statements, hit Tab, start a conditional snippet, and then have the three statements inserted into the condition as TM_SELECTED_TEXT. But now I've violated (1): I'm imagining cases for TM_SELECTED_TEXT for manual snippets rather than LSP suggestions.

telemachus commented 3 months ago

One other thing worth noting: after a little trial and error, vim.snippet does not seem to support things like this: ${1:$TM_FILENAME_BASE}. As a result, even for variables that vim.snippet supports, some of the snippets in friendly-snippets will be broken.

garymjr commented 3 months ago

Looking at https://github.com/L3MON4D3/LuaSnip/blob/master/lua/luasnip/util/_builtin_vars.lua I should be able to bring that logic over and do something similar. So I would handle expanding variables in the plugin prior to expanding the snippet with vim.snippet.

chrisgrieser commented 3 months ago

I have made a PR to neovim core, adding all the missing variables from the VSCode spec: https://github.com/neovim/neovim/pull/28949. (The PR only covers the missing variables, it does not deal with cases like ${1:$TM_FILENAME_BASE}.)

Haven't made a PR to core yet, so anyone with more experience is welcome to take a look whether I got everything right.

In the meantime, you can manually patch the resolve_variables function with the updated function from my PR in your local nvim installation, if you want to have support for the missing variables right now.

chrisgrieser commented 3 months ago

Well, turns out the VSCode snippet spec differs from the LSP snippet spec, which is why those variables will not be included in core 😔 https://github.com/neovim/neovim/pull/28949#issuecomment-2127997756

I guess porting over the Luasnip logic as suggested by @garymjr is the way to go then

garymjr commented 3 months ago

Initial variables support has been added in #21. There's few variables I still need to figure out how to support, but this covers a majority of them.

garymjr commented 3 months ago

Remaining variables should all be covered in #22. I'll leave this issue open for now as a place to discuss any possible regressions or missing variables.

folke commented 3 months ago

great!

Useful: https://github.com/neovim/neovim/pull/28949#issuecomment-2128729153

telemachus commented 3 months ago

@garymjr This is terrific—thanks!

I see one missing variable, but it is kind of an edge case. At the moment, you don't handle TM_SELECTED_TEXT.

Technically, vim.snippet supports this variable, but their implementation is effectively a no-op. I explain this in too much detail in an earlier comment, but basically vim.snippet always returns an empty string or a default variable if one is given. There's no attempt to use the currently selected text for the body of the snippet.

If you want to see other versions, LuaSnip seems to handle this variable in util/select.lua, and nvim-snippy in snippy/shared.lua.

chrisgrieser commented 3 months ago

@garymjr thank you!

One issue I noticed is that the variable expansion only works when using ${LINE_COMMENT}, but not when using $LINE_COMMENT. At least in the VSCode spec, both are valid

telemachus commented 3 months ago

At least in the VSCode spec, both are valid

In the overall LSP spec as well: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#variables

garymjr commented 3 months ago

With #26 I'm now using the same logic as vim to parse snippets so both "$" and "${}" should be getting matched now. Also #27 adds support for TM_SELECTED_TEXT variable. I tried to match the logic of luasnip as close as possible, but since we're not handing the actual expansion of the snippet I had to handle it slightly different. Let me know if you see any issues with it.

chrisgrieser commented 3 months ago

works like a charm!

I think a minor issue left is that $CLIPBOARD uses the " register and not the + register. I personally think the latter is preferable, as one use case of $CLIPBOARD is to enter things copypasted from outside vim (such as a URL copied from the browser).

dpetka2001 commented 3 months ago

Regarding $CLIPBOARD maybe vim.v.register could be used, which by default is set to " unless clipboard = unnamedplus is used from the user in which case the + will be used instead.

garymjr commented 3 months ago

I ended up going with @dpetka2001's idea and used vim.v.register to determine which register to use for $CLIPBOARD. I think this should get variable support in a good state. Unless anyone sees any remaining issues with variables I'll close this issue as completed.

telemachus commented 3 months ago

Snippets like the following don't seem to expand properly: ${1:${TM_SELECTED_TEXT:text}}.

The problem here, I think, is that if the built-in variable is the default text for a tab stop, and the built-in variable itself has default text, then neither the built-in variable or its default text is used. Instead, the tab stop is empty (as if there were no defaults).

I think that this is a valid snippet, but I'm not sure whether this is something you want to handle or that the built-in vim.snippet should handle. I'm also not sure whether vim.snippet supports nested defaults like this yet (or whether it will ever?). In any case, I discovered the problem in one of my own snippets, but I took the example from the LaTeX snippets in friendly-snippets. There don't seem to be a ton of examples like that in friendly-snippets, but there are definitely some in latex.json and asciidoc.json.

garymjr commented 3 months ago

@telemachus this should be fixed in #34

telemachus commented 3 months ago

@garymjr Thanks, but I think it's not quite there. If you have a snippet body like this: ${1:${TM_SELECTED_TEXT:hello}} ${2:cruel} ${3:world} !$0, the returned item for the first tabstop seems to still have an extra : at the front and } at the end. In other words, the result of your utils.expand_vars looks like this: ${1::hello}} ${2:cruel} ${3:world} !$0.

Sorry that this is all such a pain. (It's annoying that we have to semi-parse snippets before giving them to vim.snippet.expand to parse again.) I can try to look at this in the next few days or over the weekend.

telemachus commented 3 months ago

@garymjr I looked more closely at #34. The problem is the gsub call here:

local function resolve_variable(input, name, replacement)
    return input:gsub("%$[{]?(" .. name .. ")[}]?", replacement)
end

Sometimes replacement is an empty string. (For example, TM_SELECTED_TEXT and TM_FILENAME{_BASE} can be empty.) If the replacement is empty, then an original like ${1:${TM_SELECTED_TEXT:hello}} becomes ${1::hello}}.

The problem is somewhat complicated. You have to do different things depending on whether the replacement string is empty or not. Also, as I mentioned above, vim.snippet doesn't currently seem to support things like ${1:${TM_FILENAME:placeholder}}. You may want to revert #34 and wait to see how vim.snippet develops.

If you want to try something more complex, let me know.

garymjr commented 3 months ago

Agreed. Thanks for digging into this some more as well. I'm currently working on a better solution, but it's requiring me to walk through the parsed parts to determine the action I should perform. This means I need to walk through each placeholder and rebuilding the snippet at the end to retain all the expected tabstops.

telemachus commented 3 months ago

Agreed. Thanks for digging into this some more as well. I'm currently working on a better solution, but it's requiring me to walk through the parsed parts to determine the action I should perform. This means I need to walk through each placeholder and rebuilding the snippet at the end to retain all the expected tabstops.

Right: I looked at this over the weekend, and...it's a pain. Here's about as far as I got.

  1. Things like $TM_SELECTED_TEXT or ${1:$TM_SELECTED_TEXT} are easy. You just replace $TM_SELECTED_TEXT with its replacement string. (If the replacement string is empty, so be it.)
  2. Things like ${1:${TM_SELECTED_TEXT:default}} are not easy. First, you have to figure out what would replace TM_SELECTED_TEXT. If the replacement is an empty string, then you want to promote the inner default to the default for the whole tabstop. (I.e., ${1:${TM_SELECTED_TEXT:default}} should become ${1:default}.) If the replacement is not an empty string, then you want to make that the simple default for the tabstop. (I.e., ${1:${TM_SELECTED_TEXT:default}} should become ${1:whatever}—assuming that TM_SELECTED_TEXT is whatever.)

But it's around at this point that you realize you almost need to write an entire snippets parser only to hand the resolved text to vim.snippets.expand—which will immediately start parsing the snippets again. I wish that Neovim core would consider something like this pull request. Snippet plugins that build on top of vim.snippet need access to some sort of callback to get at values as vim.snippet parses them. (EDIT: I see now that on the roadmap there is a planned API to expose more information to plugin writers. So I take this complaint back.)

telemachus commented 3 months ago

@garymjr I think that according to the LSP spec and neovim's roadmap, vim.snippet.expand should already support things like ${1:${TM_FILENAME:placeholder}}. So I opened an issue for that. You may want to wait to see how that goes.

https://github.com/neovim/neovim/issues/29169