fix: escape receiver texts in completion #12646
Conversation
| // | ||
| // Note that we don't need to escape the other characters that can be escaped, | ||
| // because they wouldn't be treated as snippet-specific constructs without '$'. | ||
| text.replace('\\', "\\\\").replace('$', "\\$") |
There was a problem hiding this comment.
According to the spec https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#grammar we also need to escape } then do we not? (I have the bad feeling we might need to be smarter about escaping, but if that is the case that can be something for another PR. This fixes the bigger problematic ones.
There was a problem hiding this comment.
We only need to escape } inside the snippet-specific construct, i.e. ${}, to disambiguate it from the ending token of the syntax. Now that we escape every occurrence of $ with this patch, we wouldn't have ${} in the first place so I believe there's no need (I've built r-a and tested the behavior, and it worked as expected at least in my local environment).
I think this was my reasoning when writing this patch, but let me know if I'm missing anything!
|
Thanks! |
|
✌️ @lowr can now approve this pull request |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
This PR fixes #11897 by escaping '\' and '$' in the text of the receiver position expression. See here for the specification of the snippet syntax (especially this section discusses escaping).
Although not all occurrences of '\' and '$' have to be replaced, I chose to replace all as that's simpler and easier to understand. There are more clever ways to implement it, but I thought they were premature optimization for the time being (maybe I should put FIXME notes?).