Skip to content

Conversation

@Soupstraw
Copy link

@Soupstraw Soupstraw commented Oct 23, 2025

This PR adds sanitization to completion snippets. It does that by introducing the Snippet type and a snippetToText function that converts a Snippet to a syntactically valid snippet string.

close #4363

@Soupstraw Soupstraw requested a review from wz1000 as a code owner October 23, 2025 09:47
@sgillespie
Copy link
Collaborator

It looks like there are some tests to update:

ghcide
  completion
    topLevel
      recordsConstructor: FAIL (1.06s)
        ghcide-test/exe/CompletionTests.hs:112:
        expected: [("XyRecord",Just CompletionItemKind_Constructor,Just "XyRecord",Nothing),("XyRecord",Just CompletionItemKind_Snippet,Just "XyRecord {x=${1:_x}, y=${2:_y}}",Nothing)]
         but got: [("XyRecord",Just CompletionItemKind_Constructor,Just "XyRecord",Nothing),("XyRecord",Just CompletionItemKind_Snippet,Just "XyRecord {x=\\${1:_x}, y=\\${2:_y}}",Nothing)]

@Soupstraw
Copy link
Author

Soupstraw commented Oct 23, 2025

Oh well, I think I'm gonna have to do this properly after all 😃

Completely forgot about record snippets..

@Soupstraw Soupstraw marked this pull request as draft October 23, 2025 16:49
@Soupstraw Soupstraw marked this pull request as ready for review October 23, 2025 17:25
@Soupstraw Soupstraw force-pushed the sanitize-snippets branch 3 times, most recently from 696eeef to 2e0e266 Compare October 23, 2025 18:44
@Soupstraw
Copy link
Author

Should be good now, I think the test failures are from flaky tests @sgillespie.

@fendor
Copy link
Collaborator

fendor commented Nov 24, 2025

@Soupstraw Do you want to upstream the StructuredCompletionItem to lsp or do you want to rather merge it into HLS as is and think about it later in follow-up work?

If possible, I want to merge this PR before the next HLS release, which will be soon ™️

@fendor fendor self-assigned this Nov 24, 2025
@fendor fendor added the status: needs review This PR is ready for review label Nov 24, 2025
@fendor fendor requested review from fendor and sgillespie November 24, 2025 17:00
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I am happy to merge once SnippetAny has some docs.

Comment on lines +95 to +101
data SnippetAny
= SText Text
| STabStop Int
| SPlaceholder Int SnippetAny
| SChoice Int (NonEmpty Text)
| SVariable Text (Maybe SnippetAny)
deriving (Eq, Show)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little more documentation for what the individual constructors mean so that it is easier to follow here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs review This PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid snippet syntax causes failures in neovim text editor

3 participants