Avoid mutating completion item in resolve #2009
Merged
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.
Motivation
Closes #1983
Like the author of the issue, I had also noticed that we stopped using short names for constants in completion. Which puzzled me a lot given that we explicitly handle that and have unit tests to verify the behaviour.
After reading the specification again, I finally realized the issue.
When we split completion into completion + resolve, there was a mistake in the original PR. The resolve request should never mutate any existing fields of the completion item other than label details and documentation. But instead, we were creating a new completion item from scratch without including all of the original fields.
That means that the
insertText
we were specifying in completion was getting erased during resolve, which forces the editor to use thelabel
instead, resulting in the behaviour of showing the fully qualified name.Implementation
We never want to mutate anything in resolve that isn't label details and documentation. We should take the original item as a hash, change only those fields and then return it.
This fixes the issue of using the wrong sorting text and the wrong insertion text, which we're currently seeing in
main
.Automated Tests
Improved the resolve test a bit.