Don't use locale-aware sorting in completions #1668
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes locale-aware sorting from completions to improve performance. The change replaces the collator-based string comparison with a simple case-insensitive then case-sensitive comparison approach.
- Removes locale-aware collator usage from completion sorting
- Adds a new string comparison function that performs case-insensitive comparison first, then case-sensitive as a tiebreaker
- Updates both the language service and test utilities to use the new comparison approach
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/stringutil/compare.go | Adds new CompareStringsCaseInsensitiveThenSensitive function for non-locale-aware sorting |
| internal/ls/completions.go | Removes collator-based sorting and uses new string comparison function |
| internal/fourslash/tests/util/util.go | Updates test utility to use new comparison function instead of collator |
| compareStrings := stringutil.CompareStringsCaseInsensitiveThenSensitive | ||
| result := compareStrings(*entryInSlice.SortText, *entryToInsert.SortText) | ||
| if result == stringutil.ComparisonEqual { | ||
| result = compareStrings(entryInSlice.Label, entryToInsert.Label) |
There was a problem hiding this comment.
The compareStrings assignment is unnecessary since the function is only used twice. Consider calling stringutil.CompareStringsCaseInsensitiveThenSensitive directly in both places to reduce code complexity.
| compareStrings := stringutil.CompareStringsCaseInsensitiveThenSensitive | |
| result := compareStrings(*entryInSlice.SortText, *entryToInsert.SortText) | |
| if result == stringutil.ComparisonEqual { | |
| result = compareStrings(entryInSlice.Label, entryToInsert.Label) | |
| result := stringutil.CompareStringsCaseInsensitiveThenSensitive(*entryInSlice.SortText, *entryToInsert.SortText) | |
| if result == stringutil.ComparisonEqual { | |
| result = stringutil.CompareStringsCaseInsensitiveThenSensitive(entryInSlice.Label, entryToInsert.Label) |
Copilot uses AI. Check for mistakes.
| return strings.EqualFold(s[len(s)-len(suffix):], suffix) | ||
| } | ||
|
|
||
| func CompareStringsCaseInsensitiveThenSensitive(a, b string) Comparison { |
There was a problem hiding this comment.
Why do this instead of just insensitive entirely? Is insensitive not a total order or something?
There was a problem hiding this comment.
Yes. "Ab" and "ab" are going to be equal under case insensitive comparison, but we still want one of them to be consistently sorted before the other for the tests.
There was a problem hiding this comment.
Hm, it's concerning that our case insensitive sort isn't correct, then
There was a problem hiding this comment.
I think it really depends on what you're using it for. If you're using it to, say, dedupe paths in a case insensitive FS, then "Ab" and "ab" should be equal.
Haven't removed the collator package because presumably we'll need it for organize imports.
Collator sorting is slow, and using non-locale aware sorting doesn't seem like it would noticeably impact the user experience. For instance, vscode sorts its completion items by using regular string comparison on labels: https://github.com/microsoft/vscode/blob/8ee3a001179ba5153d152a3256c10158e18e42f9/src/vs/editor/contrib/suggest/browser/suggest.ts#L338