Choice node with placeholder set to 0 causes error and should be disallowed

[smjonas]

Expanding this VSCode snippet:

  "set": {
    "prefix": "set",
    "description": "set",
    "body": "${0|a,b,c|}"
  }

causes the following error:

E5108: Error executing lua ...ck/packer/start/LuaSnip/lua/luasnip/nodes/choiceNode.lua:95: attempt to get length of field 'absolute_insert_position' (a nil value)                                                                      
stack traceback:                                                                                                                                                                                                                        
        ...ck/packer/start/LuaSnip/lua/luasnip/nodes/choiceNode.lua:95: in function 'make_args_absolute'                                                                                                                                
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:813: in function 'make_args_absolute'                                                                                                                               
        .../pack/packer/start/LuaSnip/lua/luasnip/nodes/snippet.lua:417: in function 'trigger_expand'                                                                                                                                   
        ...nvim/site/pack/packer/start/LuaSnip/lua/luasnip/init.lua:169: in function 'snip_expand'                                                                                                                                      
        ...ite/pack/packer/opt/cmp_luasnip/lua/cmp_luasnip/init.lua:142: in function 'execute'                                                                                                                                          
        .../nvim/site/pack/packer/start/nvim-cmp/lua/cmp/source.lua:360: in function 'execute'                                                                                                                                          
        ...e/nvim/site/pack/packer/start/nvim-cmp/lua/cmp/entry.lua:446: in function 'execute'                                                                                                                                          
        ...re/nvim/site/pack/packer/start/nvim-cmp/lua/cmp/core.lua:470: in function <...re/nvim/site/pack/packer/start/nvim-cmp/lua/cmp/core.lua:469>                                                                                  
        ...te/pack/packer/start/nvim-cmp/lua/cmp/utils/feedkeys.lua:51: in function <...te/pack/packer/start/nvim-cmp/lua/cmp/utils/feedkeys.lua:49>        

Expected behavior: no error

Suggestions to solve the issue:

1. discard the whole snippet once parsed
2. show an error message to the user

Regarding 2., this is what UltiSnips shows to the user:

UltiSnips Error:

Choices selection is not supported on $0

[L3MON4D3]

Ah yep, that's a problem.

1 and 2 would work, what do you think of (semi-)silently fixing the snippet? We could maintain a log of the changes, that way one can still see which snippets were modified by luasnip. This would be particularly useful for snippets that cannot easily be changed (from lsp, for example (see #376)).

[leiserfg]

@L3MON4D3 I think automatically handling the snippets with rare $0 usages will be better (mainly for lsp/vscode compatibility). Replacing them with $(N+1)$0 may work.

[smjonas]

That seems like a good idea! To expand on @leiserfg 's suggestion, ${0|a,b,c|} ${1|d,e,f|} would be corrected to ${2|a,b,c|} ${1|d,e,f|}, right? Because 2 is the highest available placeholder that is not 0.

I also think that logging could also be added later, separately from the silent handling of the error.

[L3MON4D3]

Jup to both of these :+1: Basically if there is some conflict in a snippet with n placeholders: first ${0:...} becomes ${n+1:...}$0, subsequent ${0:...} are just replaced with ${n+1:...}. I think here the switch to the neovim-parser would be really advantageous, we could just pre-process the AST and then go on from there, whereas further modifying the current one could be cumbersome.