Failed to expand with nvim-cmp

[ray-x]

Got an error:

LuaSnip/lua/luasnip/util/parser.lua:351: Invalid text after $ at49  

Language Go

Sample code :

package main

import (
    "fmt"
    "testing"

    "github.com/stretchr/testify/require"
)

func TestDog(t *testing.T) {
    require.NoError(t, nil)
    require.NoEr
}

If I press enter. Will see the error.

[L3MON4D3]

Could you add a print(body) to this function and post the output after completing? I don't have anything go-related installed and need to see the snippet that's causing the error.

[ray-x]

The body:

"NoError(${1:t require.TestingT}, ${2:err error}, ${3:msgAndArgs ...interface{\\}})"  

[L3MON4D3]

Oh, yeah that's technically malformed (check grammar) I think, the opening { in $3 would have to be escaped, too. :/ Does it expand correctly in eg. vscode?

[ray-x]

VSCode does not expand arguments. I am not sure if an additional plugin is needed.

[L3MON4D3]

Hmm, probably. This seems to be a mistake in gopls, maybe open an issue there? We could've fixed it temporarily by providing a table where known faulty snippets from language servers can be replaced with working ones, but that would probably require quite a bit of work (I think some LS, including gopls, change snippets based on context, so basic string-equality wouldn't cut it) and it'd be obviously nicer to get working snippets from the LS.

[ray-x]

From the spec:

 With \ (backslash), you can escape $, } and \. Within choice elements, the backslash also escapes comma and pipe characters.

I think the grammar is ok here:

"NoError(${1:t require.TestingT}, ${2:err error}, ${3:msgAndArgs ...interface{\\}})"  

If I understand correctly, only need to put \ before }

[L3MON4D3]

Oh no, yes you're right, I've always automatically assumed it extends to {. Well, that complicates things a bit on the parser-side, seems unnecessarily complicated that only one of {,} needs to be escaped, but so be it. I'll look into fixing it

[L3MON4D3]

Okay, I think only the } has to be escaped because if no unescaped $ is before the {, it counts as escaped. I pushed a fix, it should work now

[leiserfg]

After this change, this snippet (that was used previously to test the selected text env) is failing to parse.

    ls.parser.parse_snippet({trig='boo'},
      [[
   \\section{${1:section name}} % (fold)
   \\label{sec:${2:${1/(\\w+)(\\W+$)?|\\W+/${1:?${1:/asciify/downcase}:_}/g}}}
   "${0:$TM_SELECTED_TEXT}",
    % section $2 (end)
    ]])

[L3MON4D3]

Yeah, before it didn't really matter if a pair of {,} wasn't escaped, now it does, which makes the parser fail there. I was thinking about putting it down as a breaking change, but the previous behaviour was incorrect/too loose... I probably should've put it in breaking changes, at least to avoid confusion, I'll do that now.

[bew]

Hello, I don't understand this change: Why would we need to escape the closing brace in "\\begin{$1\}" ? What's the logic behind this? Oo

[leiserfg]

@bew it's not an extra logic, that's what the specification says. Mind that it's only when you wanna the } to show off not when you wanna close the input.

[bew]

The specification says exactly this:

With \ (backslash), you can escape $, } and \.

But nowhere it says that you must escape }. My understanding of the spec is that we CAN escape } if necessary. I see one case where it is necessary: when inside a default text, for example: ${1:bar\}baz} but the simple case like { $1 } or with a simple default { ${1:default} } should still work without special escaping of the text }.

Or did I miss something else in the spec?

[L3MON4D3]

I interpreted it as "\ can be used to escape... [if you want to put them in the text literally]", but it's open to interpretation, yes. As always, how does vscode handle these cases?

[leiserfg]

Imho it's like @L3MON4D3 said, the other way it can become a non-free of context grammar and I don't see vscode doing rare backtrackings in their implementation. Painfully testing it by hand in vscode is the only way to know for sure.

[bew]

As always, how does vscode handle these cases?

@L3MON4D3 I've tested in vscode using this snippet:

{
    "Test brace parsing": {
        "prefix": "brace",
        "body": [
            "test ${1:with {\\} default} for {} <-- this",
        ],
        "description": "Test brace parsing in snippets"
    },
    "Test brace parsing, nested": {
        "prefix": "otherbrace",
        "body": [
            "{ ${1:with {\\} default} }",
        ],
        "description": "Test brace parsing in snippets, nested"
    }
}

So vscode does what I'm thinking as far as I can see. :eyes:

Try for yourself if you want! See https://stackoverflow.com/a/35403563/5655255 to know how to add a snippet

[leiserfg]

Yes, you are right, it's out of a placeholder so there is no ambiguity.

[stasjok]

I agree with bew here. Escaping of } is required only if otherwise it would be treated as closing bracket for placeholder. All these snippets expands as expected in vscode:

"test": {
    "prefix": "test",
    "body": "{ $1 }"
},
"test1": {
    "prefix": "test1",
    "body": "$1 }"
},
"test2": {
    "prefix": "test2",
    "body": "${1:a}b}"
},
"test3": {
    "prefix": "test3",
    "body": "${1:a\\}b}"
}

test2: ab} # only `a` is placeholder
test3: a}b # everything is a placeholder

[L3MON4D3]

Yikes, okay, but I think I know how to implement that. How about "${1: {} texttexttext"?

[leiserfg]

That should be |{| texttexttext isn't @bew

[L3MON4D3]

Sure hope it isn't😬😬

[bew]

Hmm with the space, so like | {| texttexttext:

[leiserfg]

Yes, I forgot the space. @L3MON4D3 we can use github.dev to test the snippets.

[L3MON4D3]

Riiight, that exists.

[L3MON4D3]

Okay, it wasn't hard at all, I just wasn't thinking. It should work correctly now.

[L3MON4D3]

Also thank you @bew for pointing out the error :)

[leiserfg]

Gosh, I did a fix for it last night but was not working fine, now I see the problem, I did the same as you but was using not expr instead of expr == 0 again :facepalm:.

[leiserfg]

I'll have to set up a custom linter for it :smile:

[L3MON4D3]

Just after getting used to arrays starting at 1, lua hits you with 0 not being false :D

[bew]

Also thank you @bew for pointing out the error :)

You're welcome! I'm actually not using LuaSnip yet 👀, I'm still on Ultisnips (with its quirks and crashes.....) and was procrastinating the migration of all my custom snippets to the node system of this plugin which seamed less readable at first glance... But since this change I see I can write the snippets in a readable manner, as lsp snippets are similar to Ultisnips's (and only use advanced nodes for specific usecases), so I'm happy 😃 I think I'll migrate soon (or on next Ultisnips unexpected crash... which can be very soon...)

👉 NOTE: The parse_snippet is a good selling point, and I think it should be better advertised in the main readme and the examples, the node system is incredible but can be less readable for simple/not-advanced snippets, don't you think?

[leiserfg]

I was thinking of making another loader for ultisnippets files, but those including python code will be really hard.

[leiserfg]

Yes, that's the idea, you write 80% of the snippets with the lsp (derived from textmate) syntax and then use the lua nodes for the rest.

[bew]

I was thinking of making another loader for ultisnippets files, but those including python code will be really hard.

\o/ What about starting by giving warnings for not-simple snippets and skip them?

(maybe move to another issue? we're going completely offtopic here)

[L3MON4D3]

Also thank you @bew for pointing out the error :)

You're welcome! I'm actually not using LuaSnip yet 👀, I'm still on Ultisnips (with its quirks and crashes.....) and was procrastinating the migration of all my custom snippets to the node system of this plugin which seamed less readable at first glance... But since this change I see I can write the snippets in a readable manner, as lsp snippets are similar to Ultisnips's (and only use advanced nodes for specific usecases), so I'm happy 😃 I think I'll migrate soon (or on next Ultisnips unexpected crash... which can be very soon...)

Come on over, it's nice here :P

👉 NOTE: The parse_snippet is a good selling point, and I think it should be better advertised in the main readme and the examples, the node system is incredible but can be less readable for simple/not-advanced snippets, don't you think?

You're right, it's currently only a subpoint in features. Maybe it'd be nice to put it into drawbacks as well, propose a solution next to the problem

[L3MON4D3]

(maybe move to another issue? we're going completely offtopic here)

Probably best, I'll actually close this one, it should be fixed now :D