Fix LSP completions when all items have default commit characters #55262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dibarbet
commented
Jul 30, 2021
| @@ -49,8 +49,7 @@ static CompletionItemRules MakeRule((bool importDirective, bool preselect, bool | |||
| { | |||
| // '<' should not filter the completion list, even though it's in generic items like IList<> | |||
| var generalBaseline = CompletionItemRules.Default. | |||
| WithFilterCharacterRule(CharacterSetModificationRule.Create(CharacterSetModificationKind.Remove, '<')). | |||
| WithCommitCharacterRule(CharacterSetModificationRule.Create(CharacterSetModificationKind.Add, '<')); | |||
There was a problem hiding this comment.
< is part of the default commit characters set and doesn't need to be specified here as best I can tell, cc @genlu
dibarbet
commented
Jul 30, 2021
| @@ -365,7 +366,9 @@ static void PromoteCommonCommitCharactersOntoList(LSP.VSCompletionList completio | |||
| var commitCharacters = completionItem.CommitCharacters; | |||
| if (commitCharacters == null) | |||
| { | |||
| continue; | |||
| // The commit characters on the item are null, this means the commit characters are actually | |||
There was a problem hiding this comment.
This fixes two issues
- We would hit a null ref on line 384 if all of the completion items had default commit characters (aka the commit characters field on every completion item was null).
- The algorithm is trying to promote the most used commit character set onto the list itself, then null out commit chars on any completion item using that set to save serialization. However this skipped originally null commit chars (indicating that the item should use commit chars from
Initialize). This resulted in the second most used commit char set being promoted to theCompletionList.CommitChars, and both items with default chars and second most used commit chars having their commit chars nulled out. This means for items that originally had null commit chars (aka use default fromInitialize), the client would use the wrong commit chars from the completion list.
There was a problem hiding this comment.
resulted in the second most used commit char set being promoted
lol 🤦♂️
Good find!
davidwengier
approved these changes
Jul 30, 2021
ghost
added this to the
This was referenced Aug 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes an exception stack trace seen in https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1362284
Also fixes behavior of promotion of commit characters to the list to include default commit characters