⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@theofabilous
Copy link

Completions for functions with snippet placeholders are broken when an argument's type is rendered with a right brace (}). The right brace symbol is rendered unescaped, which causes snippet parsers to interpret it as the end of the placeholder, leaving an extra trailing }.

Example:

fn foo(x: struct { x: i32 }) void {
    _ = x;
}
When expanding the completion item for a function call of `foo` with snippet placeholders,
we get the following:

     selected region
     ______________
    |              |
foo(|x: struct {...|})


The snippet is parsed as follows:

      __ placeholder start
     |
foo(${1:x: struct {...}})
                  ^   ^^
                 /    | \_____ not part of snippet placeholder
            ignored   |
                     placeholder end

This occurs (at least) in the following cases:

  1. type is an anonymous container literal (enum { ... })
  2. type's name is a raw identifier with a } (@"Evil}Name")
  3. type is a generic function with a type argument that is itself problematic (std.ArrayList(struct {}), ...)

Fixed by adding an escape_r_braces option to FormatOptions and escaping '}' symbols (as specified by LSP protocol - Snippet syntax) in Type.rawStringify() when this option is set to true. When rawStringifyParameter calls this function, it sets options.escape_r_braces to true if snippet placeholders are enabled.

In Type.rawStringify, I only specifically handle escape_r_braces in cases where I've encountered this bug. There are a few lines in that function that render strings as is (without potentially escaping r-braces) which I didn't touch because I haven't encountered errors related to those code paths (and in most cases, wasn't sure if those code paths are reachable with snippet placeholders and/or can ever yield } symbols).

I did this so that the changes map nicely onto the added test cases (removing any change causes at least one of the added tests to fail).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant