Improved parsing of completions with InsertTextFormat.Snippet

[ChristophKaser]

Hello everyone,

we are making extensive use of completions with InsertTextFormat.Snippet and had some problems with completions that try to insert keys with $ in them, because \$ was inserted as is.

So I tried to improve the parsing of snippets to be closer to the spec (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#snippet_syntax) and/or closer to what VSCode does.

Since the snippet format is fairly complex, I extracted the parser into its own class, so I could keep the current parser offset in a field.

This PR fixes

• variables with default values (like ${TM_SELECTED_TEXT:defaultval}}
• escaping of special characters like dollar, brackets and comma with \ (like special\$key)

The following complicated parts of the snippet format are not yet implemented:

• Variable transforms (like ${TM_FILENAME/(.*)\..+$/$1/})
• Nested tabstops in placeholders (like ${1:Place${2:holder}})
• Nested tabstops in default values (like ${TM_SELECTED_TEXT:Place${1:holder}})

[angelozerr]

I did a similar work for IJ and I created a snippet parser at https://github.com/redhat-developer/intellij-quarkus/blob/main/src/main/java/com/redhat/devtools/intellij/lsp4ij/operations/completion/snippet/LspSnippetParser.java

This parser parse characters by characters the snippet syntax and don't use some string substring.

IMHO I think this parser should belong to LSP4J. See my comments at eclipse-lsp4j/lsp4j#149 (comment)

[mickaelistria]

@ChristophKaser Can you please check whether you can reproduce the test failure exposed by https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-844/1/display/redirect locally?

[ChristophKaser]

@ChristophKaser Can you please check whether you can reproduce the test failure exposed by https://ci.eclipse.org/lsp4e/job/lsp4e-github/job/PR-844/1/display/redirect locally?

Yes, I can reproduce it locally. I had missed that test class and only tested against CompleteCompletionTest to save time, sorry.
This seems to be one of the cases I did not implement (Nested expressions in placeholders), I will try to at least support variables there.

[ChristophKaser]

I don't see any test failures any more (the build failure is due to some issue with the version number that I don't really understand).

BTW, it seems the test testVariableReplacement is duplicated in CompleteCompletionTest and VariableReplacementTest.

[ChristophKaser]

I did a similar work for IJ and I created a snippet parser at https://github.com/redhat-developer/intellij-quarkus/blob/main/src/main/java/com/redhat/devtools/intellij/lsp4ij/operations/completion/snippet/LspSnippetParser.java

This parser parse characters by characters the snippet syntax and don't use some string substring.

IMHO I think this parser should belong to LSP4J. See my comments at eclipse-lsp4j/lsp4j#149 (comment)

I wish I had seen that before I implemented the parser from scratch ;)
However, there are some differences between my implementation and yours, especially if the snippet is not syntactically valid. I tried to reproduce the way VScode handles those snippets (by keeping invalid expressions as-is).

[mickaelistria]

the build failure is due to some issue with the version number that I don't really understand

This message shows up because this is the 1st change in the bundle since last release, so we can't use 0.15.6 any longer. Can you please bump the version to 0.15.7 in the MANIFEST.MF and pom.xml for the test bundle as part of this PR?

[mickaelistria]

The version bump do touch the wrong bundles. According to the former build error, only org.eclipse.lsp4e.tests needs to be upversioned.

[mickaelistria]

This one doesn't need to be changed as you didn't modify it. Only the bundles you change needs to be upversioned (if it wasn't already done since last release)

[mickaelistria]

This one doesn't have to be changed because it was already upversions since last release.

[ChristophKaser]

The version bump do touch the wrong bundles. According to the former build error, only org.eclipse.lsp4e.tests needs to be upversioned.

Sorry, I got mixed up in my changes and the rebase...
I have now rebased my branch on master, squashed the commits to avoid multiple commits for one logical change and bumped the version of only org.eclipse.lsp4e.tests.
Thank you for your patience!

[mickaelistria]

Thanks, it's all good now!