Skip to content

Clean up outdated string comparison logic #19452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Nov 7, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
71ab810
Clean up outdated string comparison logic
rbuckton Oct 24, 2017
763d17e
Manually inline a few cases to reduce repeated 'ignoreCase' condition…
rbuckton Oct 24, 2017
5f8b392
Revert use of Intl/localeCompare for CS comparisons
rbuckton Oct 24, 2017
dee77ce
Added 'equateStrings' for string equality comparisons (rather than so…
rbuckton Oct 24, 2017
7bc3b73
Clean up existing equality comparisons
rbuckton Oct 24, 2017
27d0b9f
Minor name change
rbuckton Oct 24, 2017
6d4e2a0
Revert change in navigateTo
rbuckton Oct 24, 2017
4d7923a
Consistent naming
rbuckton Oct 24, 2017
130c407
More control over which collator to use in each situation
rbuckton Oct 25, 2017
c0ed26e
Simplify comparers
rbuckton Oct 26, 2017
dfa1ffe
Cleanup and reordering
rbuckton Oct 26, 2017
3605e4e
Remove unnecessary comment
rbuckton Oct 26, 2017
e08f8d2
Remove unnecessary comment
rbuckton Oct 26, 2017
2443777
Further simplification
rbuckton Oct 26, 2017
bfba32b
Cleanup, merge #19475
rbuckton Oct 26, 2017
d9775cd
Ensure explicit default locale before each test
rbuckton Oct 26, 2017
3cb1537
Improve performance of deduplication of sorted arrays
rbuckton Oct 27, 2017
e6cdd63
PR Feedback
rbuckton Oct 29, 2017
fcb8296
Updated comments
rbuckton Oct 29, 2017
1f961cd
PR Feedback, cleanup
rbuckton Oct 29, 2017
c83eeaa
Added comment
rbuckton Oct 29, 2017
967a426
Unify deduplication, fix deferred global diagnostics
rbuckton Oct 31, 2017
1d0a9ee
PR feedback
rbuckton Oct 31, 2017
88e56f3
Assert arrays passed to relativeComplement are sorted
rbuckton Oct 31, 2017
542a060
Remove case-insensitive UI comparisons for now
rbuckton Oct 31, 2017
31c3d44
Merge branch 'master' into compareStrings
rbuckton Nov 4, 2017
ed914a8
Fix new lint error
rbuckton Nov 4, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7350,28 +7350,12 @@ namespace ts {
unionIndex?: number;
}

function binarySearchTypes(types: Type[], type: Type): number {
let low = 0;
let high = types.length - 1;
const typeId = type.id;
while (low <= high) {
const middle = low + ((high - low) >> 1);
const id = types[middle].id;
if (id === typeId) {
return middle;
}
else if (id > typeId) {
high = middle - 1;
}
else {
low = middle + 1;
}
}
return ~low;
function getTypeId(type: Type) {
return type.id;
}

function containsType(types: Type[], type: Type): boolean {
return binarySearchTypes(types, type) >= 0;
return binarySearch(types, type, getTypeId, compareValues) >= 0;
}

// Return true if the given intersection type contains (a) more than one unit type or (b) an object
Expand Down Expand Up @@ -7412,7 +7396,7 @@ namespace ts {
if (flags & TypeFlags.Number) typeSet.containsNumber = true;
if (flags & TypeFlags.StringOrNumberLiteral) typeSet.containsStringOrNumberLiteral = true;
const len = typeSet.length;
const index = len && type.id > typeSet[len - 1].id ? ~len : binarySearchTypes(typeSet, type);
const index = len && type.id > typeSet[len - 1].id ? ~len : binarySearch(typeSet, type, getTypeId, compareValues);
if (index < 0) {
if (!(flags & TypeFlags.Object && (<ObjectType>type).objectFlags & ObjectFlags.Anonymous &&
type.symbol && type.symbol.flags & (SymbolFlags.Function | SymbolFlags.Method) && containsIdenticalType(typeSet, type))) {
Expand Down
486 changes: 361 additions & 125 deletions src/compiler/core.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6142,7 +6142,7 @@ namespace ts {
const checkJsDirectiveMatchResult = checkJsDirectiveRegEx.exec(comment);
if (checkJsDirectiveMatchResult) {
checkJsDirective = {
enabled: compareStrings(checkJsDirectiveMatchResult[1], "@ts-check", /*ignoreCase*/ true) === Comparison.EqualTo,
enabled: equateStringsCaseInsensitive(checkJsDirectiveMatchResult[1], "@ts-check"),
end: range.end,
pos: range.pos
};
Expand Down
5 changes: 3 additions & 2 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1093,11 +1093,12 @@ namespace ts {

// If '--lib' is not specified, include default library file according to '--target'
// otherwise, using options specified in '--lib' instead of '--target' default library file
const equalityComparer = host.useCaseSensitiveFileNames() ? equateStringsCaseSensitive : equateStringsCaseInsensitive;
if (!options.lib) {
return compareStrings(file.fileName, getDefaultLibraryFileName(), /*ignoreCase*/ !host.useCaseSensitiveFileNames()) === Comparison.EqualTo;
return equalityComparer(file.fileName, getDefaultLibraryFileName());
}
else {
return forEach(options.lib, libFileName => compareStrings(file.fileName, combinePaths(defaultLibraryPath, libFileName), /*ignoreCase*/ !host.useCaseSensitiveFileNames()) === Comparison.EqualTo);
return forEach(options.lib, libFileName => equalityComparer(file.fileName, combinePaths(defaultLibraryPath, libFileName)));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ namespace ts {
* We assume the first line starts at position 0 and 'position' is non-negative.
*/
export function computeLineAndCharacterOfPosition(lineStarts: ReadonlyArray<number>, position: number): LineAndCharacter {
let lineNumber = binarySearch(lineStarts, position);
let lineNumber = binarySearch(lineStarts, position, identity, compareValues);
if (lineNumber < 0) {
// If the actual position was not found,
// the binary search returns the 2's-complement of the next line start
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/tsc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ namespace ts {

// Sort our options by their names, (e.g. "--noImplicitAny" comes before "--watch")
const optsList = showAllOptions ?
optionDeclarations.slice().sort((a, b) => compareValues<string>(a.name.toLowerCase(), b.name.toLowerCase())) :
sort(optionDeclarations, (a, b) => compareStringsCaseInsensitive(a.name, b.name)) :
filter(optionDeclarations.slice(), v => v.showInSimplifiedHelpView);

// We want our descriptions to align at the same column in our output,
Expand Down
13 changes: 13 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,19 @@ namespace ts {
push(...values: T[]): void;
}

/* @internal */
export type EqualityComparer<T> = (a: T, b: T) => boolean;

/* @internal */
export type Comparer<T> = (a: T, b: T) => Comparison;

/* @internal */
export const enum Comparison {
LessThan = -1,
EqualTo = 0,
GreaterThan = 1
}

// branded string type used to store absolute, normalized and canonicalized paths
// arbitrary file name can be converted to Path via toPath function
export type Path = string & { __pathBrand: any };
Expand Down
21 changes: 16 additions & 5 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,16 +321,16 @@ namespace ts {
return getSourceTextOfNodeFromSourceFile(getSourceFileOfNode(node), node, includeTrivia);
}

function getPos(range: Node) {
return range.pos;
}

/**
* Note: it is expected that the `nodeArray` and the `node` are within the same file.
* For example, searching for a `SourceFile` in a `SourceFile[]` wouldn't work.
*/
export function indexOfNode(nodeArray: ReadonlyArray<Node>, node: Node) {
return binarySearch(nodeArray, node, compareNodePos);
}

function compareNodePos({ pos: aPos }: Node, { pos: bPos}: Node) {
return aPos < bPos ? Comparison.LessThan : bPos < aPos ? Comparison.GreaterThan : Comparison.EqualTo;
return binarySearch(nodeArray, node, getPos, compareValues);
}

/**
Expand Down Expand Up @@ -2292,6 +2292,7 @@ namespace ts {
let nonFileDiagnostics: Diagnostic[] = [];
const fileDiagnostics = createMap<Diagnostic[]>();

let hasReadNonFileDiagnostics = false;
let diagnosticsModified = false;
let modificationCount = 0;

Expand Down Expand Up @@ -2321,6 +2322,12 @@ namespace ts {
}
}
else {
// If we've already read the non-file diagnostics, do not modify the existing array.
if (hasReadNonFileDiagnostics) {
hasReadNonFileDiagnostics = false;
nonFileDiagnostics = nonFileDiagnostics.slice();
}

diagnostics = nonFileDiagnostics;
}

Expand All @@ -2331,6 +2338,7 @@ namespace ts {

function getGlobalDiagnostics(): Diagnostic[] {
sortAndDeduplicate();
hasReadNonFileDiagnostics = true;
return nonFileDiagnostics;
}

Expand Down Expand Up @@ -3950,6 +3958,9 @@ namespace ts {
trySetLanguageAndTerritory(language, /*territory*/ undefined, errors);
}

// Set the UI locale for string collation
setUILocale(locale);

function trySetLanguageAndTerritory(language: string, territory: string, errors?: Push<Diagnostic>): boolean {
const compilerFilePath = normalizePath(sys.getExecutingFilePath());
const containingDirectoryPath = getDirectoryPath(compilerFilePath);
Expand Down
24 changes: 20 additions & 4 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -568,15 +568,31 @@ namespace FourSlash {
}

for (const { start, length, messageText, file } of errors) {
Harness.IO.log(" from: " + showPosition(file, start) +
", to: " + showPosition(file, start + length) +
Harness.IO.log(" " + this.formatRange(file, start, length) +
", message: " + ts.flattenDiagnosticMessageText(messageText, Harness.IO.newLine()) + "\n");
}
}

private formatRange(file: ts.SourceFile, start: number, length: number) {
if (file) {
return `from: ${this.formatLineAndCharacterOfPosition(file, start)}, to: ${this.formatLineAndCharacterOfPosition(file, start + length)}`;
}
return "global";
}

function showPosition(file: ts.SourceFile, pos: number) {
private formatLineAndCharacterOfPosition(file: ts.SourceFile, pos: number) {
if (file) {
const { line, character } = ts.getLineAndCharacterOfPosition(file, pos);
return `${line}:${character}`;
}
return "global";
}

private formatPosition(file: ts.SourceFile, pos: number) {
if (file) {
return file.fileName + "@" + pos;
}
return "global";
}

public verifyNoErrors() {
Expand All @@ -586,7 +602,7 @@ namespace FourSlash {
if (errors.length) {
this.printErrorLog(/*expectErrors*/ false, errors);
const error = errors[0];
this.raiseError(`Found an error: ${error.file.fileName}@${error.start}: ${error.messageText}`);
this.raiseError(`Found an error: ${this.formatPosition(error.file, error.start)}: ${error.messageText}`);
}
});
}
Expand Down
4 changes: 2 additions & 2 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,7 @@ namespace Harness {
export const diagnosticSummaryMarker = "__diagnosticSummary";
export const globalErrorsMarker = "__globalErrors";
export function *iterateErrorBaseline(inputFiles: ReadonlyArray<TestFile>, diagnostics: ReadonlyArray<ts.Diagnostic>, pretty?: boolean): IterableIterator<[string, string, number]> {
diagnostics = diagnostics.slice().sort(ts.compareDiagnostics);
diagnostics = ts.sort(diagnostics, ts.compareDiagnostics);
let outputLines = "";
// Count up all errors that were found in files other than lib.d.ts so we don't miss any
let totalErrorsReportedInNonLibraryFiles = 0;
Expand Down Expand Up @@ -1707,7 +1707,7 @@ namespace Harness {

export function *iterateOutputs(outputFiles: Harness.Compiler.GeneratedFile[]): IterableIterator<[string, string]> {
// Collect, test, and sort the fileNames
outputFiles.sort((a, b) => ts.compareStrings(cleanName(a.fileName), cleanName(b.fileName)));
outputFiles.sort((a, b) => ts.compareStringsCaseSensitive(cleanName(a.fileName), cleanName(b.fileName)));
const dupeCase = ts.createMap<number>();
// Yield them
for (const outputFile of outputFiles) {
Expand Down
8 changes: 8 additions & 0 deletions src/harness/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,14 @@ function beginTests() {
ts.Debug.enableDebugInfo();
}

// run tests in en-US by default.
let savedUILocale: string | undefined;
beforeEach(() => {
savedUILocale = ts.getUILocale();
ts.setUILocale("en-US");
});
afterEach(() => ts.setUILocale(savedUILocale));

runTests(runners);

if (!runUnitTests) {
Expand Down
4 changes: 2 additions & 2 deletions src/harness/unittests/compileOnSave.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ namespace ts.projectSystem {
describe("CompileOnSave affected list", () => {
function sendAffectedFileRequestAndCheckResult(session: server.Session, request: server.protocol.Request, expectedFileList: { projectFileName: string, files: FileOrFolder[] }[]) {
const response = session.executeCommand(request).response as server.protocol.CompileOnSaveAffectedFileListSingleProject[];
const actualResult = response.sort((list1, list2) => compareStrings(list1.projectFileName, list2.projectFileName));
expectedFileList = expectedFileList.sort((list1, list2) => compareStrings(list1.projectFileName, list2.projectFileName));
const actualResult = response.sort((list1, list2) => ts.compareStringsCaseSensitive(list1.projectFileName, list2.projectFileName));
expectedFileList = expectedFileList.sort((list1, list2) => ts.compareStringsCaseSensitive(list1.projectFileName, list2.projectFileName));

assert.equal(actualResult.length, expectedFileList.length, `Actual result project number is different from the expected project number`);

Expand Down
6 changes: 4 additions & 2 deletions src/server/editorServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,10 @@ namespace ts.server {
* This helper function processes a list of projects and return the concatenated, sortd and deduplicated output of processing each project.
*/
export function combineProjectOutput<T>(projects: ReadonlyArray<Project>, action: (project: Project) => ReadonlyArray<T>, comparer?: (a: T, b: T) => number, areEqual?: (a: T, b: T) => boolean) {
const result = flatMap(projects, action).sort(comparer);
return projects.length > 1 ? deduplicate(result, areEqual) : result;
const outputs = flatMap(projects, action);
return comparer
? sortAndDeduplicate(outputs, comparer, areEqual)
: deduplicate(outputs, areEqual);
}

export interface HostConfiguration {
Expand Down
3 changes: 2 additions & 1 deletion src/server/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,8 @@ namespace ts.server {
const scriptInfo = this.projectService.getOrCreateScriptInfoNotOpenedByClient(inserted, this.directoryStructureHost);
scriptInfo.attachToProject(this);
},
removed => this.detachScriptInfoFromProject(removed)
removed => this.detachScriptInfoFromProject(removed),
compareStringsCaseSensitive
);
const elapsed = timestamp() - start;
this.writeLog(`Finishing updateGraphWorker: Project: ${this.getProjectName()} structureChanged: ${hasChanges} Elapsed: ${elapsed}ms`);
Expand Down
4 changes: 2 additions & 2 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -974,7 +974,7 @@ namespace ts.server {
projects,
project => project.getLanguageService().findReferences(file, position),
/*comparer*/ undefined,
/*areEqual (TODO: fixme)*/ undefined
equateValues
);
}

Expand Down Expand Up @@ -1209,7 +1209,7 @@ namespace ts.server {
// Use `hasAction || undefined` to avoid serializing `false`.
return { name, kind, kindModifiers, sortText, replacementSpan: convertedSpan, hasAction: hasAction || undefined, source };
}
}).sort((a, b) => compareStrings(a.name, b.name));
}).sort((a, b) => compareStringsCaseSensitiveUI(a.name, b.name));
}
else {
return completions;
Expand Down
9 changes: 4 additions & 5 deletions src/server/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ namespace ts.server {
return;
}

const insertIndex = binarySearch(array, insert, compare);
const insertIndex = binarySearch(array, insert, identity, compare);
if (insertIndex < 0) {
array.splice(~insertIndex, 0, insert);
}
Expand All @@ -266,7 +266,7 @@ namespace ts.server {
return;
}

const removeIndex = binarySearch(array, remove, compare);
const removeIndex = binarySearch(array, remove, identity, compare);
if (removeIndex >= 0) {
array.splice(removeIndex, 1);
}
Expand All @@ -288,16 +288,15 @@ namespace ts.server {
return index === 0 || value !== array[index - 1];
}

export function enumerateInsertsAndDeletes<T>(newItems: SortedReadonlyArray<T>, oldItems: SortedReadonlyArray<T>, inserted: (newItem: T) => void, deleted: (oldItem: T) => void, compare?: Comparer<T>) {
compare = compare || compareValues;
export function enumerateInsertsAndDeletes<T>(newItems: SortedReadonlyArray<T>, oldItems: SortedReadonlyArray<T>, inserted: (newItem: T) => void, deleted: (oldItem: T) => void, comparer: Comparer<T>) {
let newIndex = 0;
let oldIndex = 0;
const newLen = newItems.length;
const oldLen = oldItems.length;
while (newIndex < newLen && oldIndex < oldLen) {
const newItem = newItems[newIndex];
const oldItem = oldItems[oldIndex];
const compareResult = compare(newItem, oldItem);
const compareResult = comparer(newItem, oldItem);
if (compareResult === Comparison.LessThan) {
inserted(newItem);
newIndex++;
Expand Down
5 changes: 4 additions & 1 deletion src/services/jsTyping.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ namespace ts.JsTyping {

// add typings for unresolved imports
if (unresolvedImports) {
const module = deduplicate(unresolvedImports.map(moduleId => nodeCoreModules.has(moduleId) ? "node" : moduleId));
const module = deduplicate(
unresolvedImports.map(moduleId => nodeCoreModules.has(moduleId) ? "node" : moduleId),
equateStringsCaseSensitive,
compareStringsCaseSensitive);
addInferredTypings(module, "Inferred typings from unresolved imports");
}
// Add the cached typing locations for inferred typings that are already installed
Expand Down
10 changes: 3 additions & 7 deletions src/services/navigateTo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,10 @@ namespace ts.NavigateTo {
return bestMatchKind;
}

function compareNavigateToItems(i1: RawNavigateToItem, i2: RawNavigateToItem): number {
function compareNavigateToItems(i1: RawNavigateToItem, i2: RawNavigateToItem) {
// TODO(cyrusn): get the gamut of comparisons that VS already uses here.
// Right now we just sort by kind first, and then by name of the item.
// We first sort case insensitively. So "Aaa" will come before "bar".
// Then we sort case sensitively, so "aaa" will come before "Aaa".
return i1.matchKind - i2.matchKind ||
ts.compareStringsCaseInsensitive(i1.name, i2.name) ||
ts.compareStrings(i1.name, i2.name);
return compareValues(i1.matchKind, i2.matchKind)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may run inside of a unit test, causing that test to fail for developers from other countries...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a beforeEach/afterEach that explicitly sets the UI locale to "en-US" before running each test.

|| compareStringsCaseSensitiveUI(i1.name, i2.name);
}

function createNavigateToItem(rawItem: RawNavigateToItem): NavigateToItem {
Expand Down
12 changes: 3 additions & 9 deletions src/services/navigationBar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,15 +366,9 @@ namespace ts.NavigationBar {
children.sort(compareChildren);
}

function compareChildren(child1: NavigationBarNode, child2: NavigationBarNode): number {
const name1 = tryGetName(child1.node), name2 = tryGetName(child2.node);
if (name1 && name2) {
const cmp = ts.compareStringsCaseInsensitive(name1, name2);
return cmp !== 0 ? cmp : navigationBarNodeKind(child1) - navigationBarNodeKind(child2);
}
else {
return name1 ? 1 : name2 ? -1 : navigationBarNodeKind(child1) - navigationBarNodeKind(child2);
}
function compareChildren(child1: NavigationBarNode, child2: NavigationBarNode) {
return compareStringsCaseSensitiveUI(tryGetName(child1.node), tryGetName(child2.node))
|| compareValues(navigationBarNodeKind(child1), navigationBarNodeKind(child2));
}

/**
Expand Down
Loading