Don't use locale-aware sorting in completions (#1668)

Author: gabritto

internal/fourslash/tests/util/util.go

@@ -8,8 +8,7 @@ import (
 	"github.com/microsoft/typescript-go/internal/fourslash"
 	"github.com/microsoft/typescript-go/internal/ls"
 	"github.com/microsoft/typescript-go/internal/lsp/lsproto"
-	"golang.org/x/text/collate"
-	"golang.org/x/text/language"
+	"github.com/microsoft/typescript-go/internal/stringutil"
 )
 
 func PtrTo[T any](v T) *T {
@@ -13265,10 +13264,8 @@ var CompletionGlobals = sortCompletionItems(append(
 	CompletionUndefinedVarItem,
 ))
 
-var defaultLanguage = language.AmericanEnglish
-
 func sortCompletionItems(items []fourslash.CompletionsExpectedItem) []fourslash.CompletionsExpectedItem {
-	compareStringsUI := collate.New(defaultLanguage).CompareString
+	compareStrings := stringutil.CompareStringsCaseInsensitiveThenSensitive
 	items = slices.Clone(items)
 	slices.SortStableFunc(items, func(a fourslash.CompletionsExpectedItem, b fourslash.CompletionsExpectedItem) int {
 		defaultSortText := string(ls.SortTextLocationPriority)
@@ -13287,7 +13284,7 @@ func sortCompletionItems(items []fourslash.CompletionsExpectedItem) []fourslash.
 		}
 		aSortText = core.OrElse(aSortText, defaultSortText)
 		bSortText = core.OrElse(bSortText, defaultSortText)
-		bySortText := compareStringsUI(aSortText, bSortText)
+		bySortText := compareStrings(aSortText, bSortText)
 		if bySortText != 0 {
 			return bySortText
 		}
@@ -13308,7 +13305,7 @@ func sortCompletionItems(items []fourslash.CompletionsExpectedItem) []fourslash.
 		default:
 			panic(fmt.Sprintf("unexpected completion item type: %T", b))
 		}
-		return compareStringsUI(aLabel, bLabel)
+		return compareStrings(aLabel, bLabel)
 	})
 	return items
 }

internal/ls/completions.go

@@ -29,8 +29,6 @@ import (
 	"github.com/microsoft/typescript-go/internal/scanner"
 	"github.com/microsoft/typescript-go/internal/stringutil"
 	"github.com/microsoft/typescript-go/internal/tspath"
-	"golang.org/x/text/collate"
-	"golang.org/x/text/language"
 )
 
 func (l *LanguageService) ProvideCompletion(
@@ -1884,7 +1882,6 @@ func (l *LanguageService) completionInfoFromData(
 		clientOptions,
 	)
 
-	compareCompletionEntries := getCompareCompletionEntries(ctx)
 	if data.keywordFilters != KeywordCompletionFiltersNone {
 		keywordCompletions := getKeywordCompletions(data.keywordFilters, !data.insideJSDocTagTypeExpression && ast.IsSourceFileJS(file))
 		for _, keywordEntry := range keywordCompletions {
@@ -1958,7 +1955,6 @@ func (l *LanguageService) getCompletionEntriesFromSymbols(
 	// true otherwise. Based on the order we add things we will always see locals first, then globals, then module exports.
 	// So adding a completion for a local will prevent us from adding completions for external module exports sharing the same name.
 	uniques := make(uniqueNamesMap)
-	compareCompletionEntries := getCompareCompletionEntries(ctx)
 	for _, symbol := range data.symbols {
 		symbolId := ast.GetSymbolId(symbol)
 		origin := data.symbolToOriginInfoMap[symbolId]
@@ -3295,59 +3291,35 @@ func getCompletionsSymbolKind(kind ScriptElementKind) lsproto.CompletionItemKind
 	}
 }
 
-var collatorCache collections.SyncMap[language.Tag, *sync.Pool]
-
-func getCollator(tag language.Tag) *collate.Collator {
-	pool, ok := collatorCache.Load(tag)
-	if !ok {
-		pool, _ = collatorCache.LoadOrStore(tag, &sync.Pool{
-			New: func() any {
-				return collate.New(tag)
-			},
-		})
-	}
-	return pool.Get().(*collate.Collator)
-}
-
-func putCollator(tag language.Tag, collator *collate.Collator) {
-	pool, _ := collatorCache.Load(tag)
-	pool.Put(collator)
-}
-
 // Editors will use the `sortText` and then fall back to `name` for sorting, but leave ties in response order.
 // So, it's important that we sort those ties in the order we want them displayed if it matters. We don't
 // strictly need to sort by name or SortText here since clients are going to do it anyway, but we have to
 // do the work of comparing them so we can sort those ties appropriately; plus, it makes the order returned
 // by the language service consistent with what TS Server does and what editors typically do. This also makes
 // completions tests make more sense. We used to sort only alphabetically and only in the server layer, but
 // this made tests really weird, since most fourslash tests don't use the server.
-func getCompareCompletionEntries(ctx context.Context) func(entryInSlice *lsproto.CompletionItem, entryToInsert *lsproto.CompletionItem) int {
-	return func(entryInSlice *lsproto.CompletionItem, entryToInsert *lsproto.CompletionItem) int {
-		locale := core.GetLocale(ctx)
-		collator := getCollator(locale)
-		defer putCollator(locale, collator)
-		compareStrings := collator.CompareString
-		result := compareStrings(*entryInSlice.SortText, *entryToInsert.SortText)
-		if result == stringutil.ComparisonEqual {
-			result = compareStrings(entryInSlice.Label, entryToInsert.Label)
-		}
-		if result == stringutil.ComparisonEqual && entryInSlice.Data != nil && entryToInsert.Data != nil {
-			sliceEntryData, ok1 := (*entryInSlice.Data).(*completionEntryData)
-			insertEntryData, ok2 := (*entryToInsert.Data).(*completionEntryData)
-			if ok1 && ok2 && sliceEntryData.ModuleSpecifier != "" && insertEntryData.ModuleSpecifier != "" {
-				// Sort same-named auto-imports by module specifier
-				result = compareNumberOfDirectorySeparators(
-					sliceEntryData.ModuleSpecifier,
-					insertEntryData.ModuleSpecifier,
-				)
-			}
-		}
-		if result == stringutil.ComparisonEqual {
-			// Fall back to symbol order - if we return `EqualTo`, `insertSorted` will put later symbols first.
-			return stringutil.ComparisonLessThan
+func compareCompletionEntries(entryInSlice *lsproto.CompletionItem, entryToInsert *lsproto.CompletionItem) int {
+	compareStrings := stringutil.CompareStringsCaseInsensitiveThenSensitive
+	result := compareStrings(*entryInSlice.SortText, *entryToInsert.SortText)
+	if result == stringutil.ComparisonEqual {
+		result = compareStrings(entryInSlice.Label, entryToInsert.Label)
+	}
+	if result == stringutil.ComparisonEqual && entryInSlice.Data != nil && entryToInsert.Data != nil {
+		sliceEntryData, ok1 := (*entryInSlice.Data).(*completionEntryData)
+		insertEntryData, ok2 := (*entryToInsert.Data).(*completionEntryData)
+		if ok1 && ok2 && sliceEntryData.ModuleSpecifier != "" && insertEntryData.ModuleSpecifier != "" {
+			// Sort same-named auto-imports by module specifier
+			result = compareNumberOfDirectorySeparators(
+				sliceEntryData.ModuleSpecifier,
+				insertEntryData.ModuleSpecifier,
+			)
 		}
-		return result
 	}
+	if result == stringutil.ComparisonEqual {
+		// Fall back to symbol order - if we return `EqualTo`, `insertSorted` will put later symbols first.
+		return stringutil.ComparisonLessThan
+	}
+	return result
 }
 
 // True if the first character of `lowercaseCharacters` is the first character
@@ -3582,7 +3554,6 @@ func (l *LanguageService) getJSCompletionEntries(
 	uniqueNames *collections.Set[string],
 	sortedEntries []*lsproto.CompletionItem,
 ) []*lsproto.CompletionItem {
-	compareCompletionEntries := getCompareCompletionEntries(ctx)
 	nameTable := getNameTable(file)
 	for name, pos := range nameTable {
 		// Skip identifiers produced only from the current location
@@ -4929,11 +4900,6 @@ func hasCompletionItem(clientOptions *lsproto.CompletionClientCapabilities) bool
 	return clientOptions != nil && clientOptions.CompletionItem != nil
 }
 
-// strada TODO: this function is, at best, poorly named. Use sites are pretty suspicious.
-func compilerOptionsIndicateEsModules(options *core.CompilerOptions) bool {
-	return options.Module == core.ModuleKindNone || options.GetEmitScriptTarget() >= core.ScriptTargetES2015 || options.NoEmit.IsTrue()
-}
-
 func clientSupportsItemLabelDetails(clientOptions *lsproto.CompletionClientCapabilities) bool {
 	return hasCompletionItem(clientOptions) && ptrIsTrue(clientOptions.CompletionItem.LabelDetailsSupport)
 }

internal/stringutil/compare.go

@@ -90,3 +90,11 @@ func HasSuffix(s string, suffix string, caseSensitive bool) bool {
 	}
 	return strings.EqualFold(s[len(s)-len(suffix):], suffix)
 }
+
+func CompareStringsCaseInsensitiveThenSensitive(a, b string) Comparison {
+	cmp := CompareStringsCaseInsensitive(a, b)
+	if cmp != ComparisonEqual {
+		return cmp
+	}
+	return CompareStringsCaseSensitive(a, b)
+}