From 8626b927be4c022853b608035341f1d59b51076f Mon Sep 17 00:00:00 2001 From: kpreisser Date: Mon, 20 Aug 2018 13:54:57 +0200 Subject: [PATCH 01/13] Adjust the iteration behavior of the shimMap (used for IE 11) to visit values that are added while forEach is running. Fixes #26090 --- src/compiler/core.ts | 109 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 89 insertions(+), 20 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index a2f2b245502..36398e06b35 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -121,29 +121,62 @@ namespace ts { function shimMap(): new () => Map { class MapIterator { - private data: MapLike; - private keys: ReadonlyArray; - private index = 0; + index = 0; + + private shimMap: ShimMap; private selector: (data: MapLike, key: string) => U; - constructor(data: MapLike, selector: (data: MapLike, key: string) => U) { - this.data = data; + + constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { + this.shimMap = shimMap; this.selector = selector; - this.keys = Object.keys(data); + + if (!shimMap.iteratorState) { + // Create the initial iterator state. + shimMap.iteratorState = { + iterators: [], + keys: Object.keys(shimMap.data) + }; + } + + // Add ourselves to the list of iterators. + shimMap.iteratorState.iterators.push(this); } public next(): { value: U, done: false } | { value: never, done: true } { - const index = this.index; - if (index < this.keys.length) { - this.index++; - return { value: this.selector(this.data, this.keys[index]), done: false }; + const iteratorState = this.shimMap.iteratorState!; + if (this.index != -1 && this.index < iteratorState.keys.length) { + const index = this.index++; + return { value: this.selector(this.shimMap.data, iteratorState.keys[index]), done: false }; + } + else { + // Ensure subsequent invocations will always return done. + this.index = -1; + + // Remove ourselves from the list of iterators. + iteratorState.iterators.splice( + iteratorState.iterators.indexOf(this), 1); + + if (iteratorState.iterators.length == 0) { + // No other iterator is active, so clear the iterator state. + this.shimMap.iteratorState = undefined; + } + + return { value: undefined as never, done: true }; } - return { value: undefined as never, done: true }; } } - return class implements Map { - private data = createDictionaryObject(); - public size = 0; + class ShimMap implements Map { + size = 0; + + data = createDictionaryObject(); + + iteratorState: { + readonly keys: string[]; + readonly iterators: { + index: number; + }[]; + } | undefined; get(key: string): T | undefined { return this.data[key]; @@ -152,6 +185,11 @@ namespace ts { set(key: string, value: T): this { if (!this.has(key)) { this.size++; + + if (this.iteratorState) { + // Add the new entry. + this.iteratorState.keys.push(key); + } } this.data[key] = value; return this; @@ -166,6 +204,22 @@ namespace ts { if (this.has(key)) { this.size--; delete this.data[key]; + + if (this.iteratorState) { + // Remove the key and adjust the iterator indexes. + // Note that this operation isn't very performant as we need to + // iterate over the "keys" array; however, we expect that no one + // will delete entries while iterators are still active. + const keys = this.iteratorState.keys; + const keyIndex = keys.indexOf(key); + keys.splice(keyIndex, 1); + + const iterators = this.iteratorState.iterators; + for (let i = 0; i < iterators.length; i++) + if (iterators[i].index > keyIndex) + iterators[i].index--; + } + return true; } return false; @@ -174,26 +228,41 @@ namespace ts { clear(): void { this.data = createDictionaryObject(); this.size = 0; + + if (this.iteratorState) { + this.iteratorState.keys.splice(0, this.iteratorState.keys.length); + + const iterators = this.iteratorState.iterators; + for (let i = 0; i < iterators.length; i++) + iterators[i].index = 0; + } } keys(): Iterator { - return new MapIterator(this.data, (_data, key) => key); + return new MapIterator(this, (_data, key) => key); } values(): Iterator { - return new MapIterator(this.data, (data, key) => data[key]); + return new MapIterator(this, (data, key) => data[key]); } entries(): Iterator<[string, T]> { - return new MapIterator(this.data, (data, key) => [key, data[key]] as [string, T]); + return new MapIterator(this, (data, key) => [key, data[key]] as [string, T]); } forEach(action: (value: T, key: string) => void): void { - for (const key in this.data) { - action(this.data[key], key); + const iterator = this.entries(); + while (true) { + const { value: entry, done } = iterator.next(); + if (done) + break; + + action(entry[1], entry[0]); } } - }; + } + + return ShimMap; } export function length(array: ReadonlyArray | undefined): number { From bff5436ac964dc57f1fdcf8be3d7930f9c94646a Mon Sep 17 00:00:00 2001 From: kpreisser Date: Thu, 23 Aug 2018 13:22:55 +0200 Subject: [PATCH 02/13] Follow-Up: Avoid the memory leak problem by simply not allowing existing iterators to continue once an entry has been deleted from the map. Fixes #26090 --- src/compiler/core.ts | 78 +++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 48 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 36398e06b35..aa5c13ebaef 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -124,43 +124,40 @@ namespace ts { index = 0; private shimMap: ShimMap; + private originalIteratoKeys: string[]; private selector: (data: MapLike, key: string) => U; constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { this.shimMap = shimMap; this.selector = selector; - - if (!shimMap.iteratorState) { - // Create the initial iterator state. - shimMap.iteratorState = { - iterators: [], - keys: Object.keys(shimMap.data) - }; + + if (!shimMap.currentIteratoKeys) { + // Create the key array on the map over which we (and other new iterators) + // will iterate. + shimMap.currentIteratoKeys = Object.keys(shimMap.data); } - // Add ourselves to the list of iterators. - shimMap.iteratorState.iterators.push(this); + // Copy the key array to allow us later to check if the map has cleared + // or replaced the array. + this.originalIteratoKeys = shimMap.currentIteratoKeys; } public next(): { value: U, done: false } | { value: never, done: true } { - const iteratorState = this.shimMap.iteratorState!; - if (this.index != -1 && this.index < iteratorState.keys.length) { + // Check if we still have the same key array. Otherwise, this means + // an element has been deleted from the map in the meanwhile, so we + // cannot continue. + if (this.index != -1 && this.originalIteratoKeys != this.shimMap.currentIteratoKeys) + throw new Error("Cannot continue iteration because a map element has been deleted."); + + const iteratorKeys = this.originalIteratoKeys; + if (this.index != -1 && this.index < iteratorKeys.length) { const index = this.index++; - return { value: this.selector(this.shimMap.data, iteratorState.keys[index]), done: false }; + return { value: this.selector(this.shimMap.data, iteratorKeys[index]), done: false }; } else { // Ensure subsequent invocations will always return done. this.index = -1; - // Remove ourselves from the list of iterators. - iteratorState.iterators.splice( - iteratorState.iterators.indexOf(this), 1); - - if (iteratorState.iterators.length == 0) { - // No other iterator is active, so clear the iterator state. - this.shimMap.iteratorState = undefined; - } - return { value: undefined as never, done: true }; } } @@ -171,12 +168,7 @@ namespace ts { data = createDictionaryObject(); - iteratorState: { - readonly keys: string[]; - readonly iterators: { - index: number; - }[]; - } | undefined; + currentIteratoKeys?: string[]; get(key: string): T | undefined { return this.data[key]; @@ -186,11 +178,12 @@ namespace ts { if (!this.has(key)) { this.size++; - if (this.iteratorState) { + if (this.currentIteratoKeys) { // Add the new entry. - this.iteratorState.keys.push(key); + this.currentIteratoKeys.push(key); } } + this.data[key] = value; return this; } @@ -205,19 +198,10 @@ namespace ts { this.size--; delete this.data[key]; - if (this.iteratorState) { - // Remove the key and adjust the iterator indexes. - // Note that this operation isn't very performant as we need to - // iterate over the "keys" array; however, we expect that no one - // will delete entries while iterators are still active. - const keys = this.iteratorState.keys; - const keyIndex = keys.indexOf(key); - keys.splice(keyIndex, 1); - - const iterators = this.iteratorState.iterators; - for (let i = 0; i < iterators.length; i++) - if (iterators[i].index > keyIndex) - iterators[i].index--; + if (this.currentIteratoKeys) { + // Clear the iteratorKeys array. This means if iterators are still active + // they will throw on the next() call. + this.currentIteratoKeys = undefined; } return true; @@ -229,12 +213,10 @@ namespace ts { this.data = createDictionaryObject(); this.size = 0; - if (this.iteratorState) { - this.iteratorState.keys.splice(0, this.iteratorState.keys.length); - - const iterators = this.iteratorState.iterators; - for (let i = 0; i < iterators.length; i++) - iterators[i].index = 0; + if (this.currentIteratoKeys) { + // Clear the iteratorKeys array. This means if iterators are still active + // they will throw on their next() call. + this.currentIteratoKeys = undefined; } } From c0c71bf65e87ddacb6c1ce7af1671f0b1640e5fd Mon Sep 17 00:00:00 2001 From: kpreisser Date: Thu, 3 Jan 2019 16:09:56 +0100 Subject: [PATCH 03/13] Fix lint issues. --- src/compiler/core.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index aa5c13ebaef..ad984a208b5 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -130,7 +130,7 @@ namespace ts { constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { this.shimMap = shimMap; this.selector = selector; - + if (!shimMap.currentIteratoKeys) { // Create the key array on the map over which we (and other new iterators) // will iterate. @@ -146,17 +146,18 @@ namespace ts { // Check if we still have the same key array. Otherwise, this means // an element has been deleted from the map in the meanwhile, so we // cannot continue. - if (this.index != -1 && this.originalIteratoKeys != this.shimMap.currentIteratoKeys) + if (this.index !== -1 && this.originalIteratoKeys !== this.shimMap.currentIteratoKeys) { throw new Error("Cannot continue iteration because a map element has been deleted."); + } const iteratorKeys = this.originalIteratoKeys; - if (this.index != -1 && this.index < iteratorKeys.length) { + if (this.index !== -1 && this.index < iteratorKeys.length) { const index = this.index++; return { value: this.selector(this.shimMap.data, iteratorKeys[index]), done: false }; } else { // Ensure subsequent invocations will always return done. - this.index = -1; + this.index = -1; return { value: undefined as never, done: true }; } @@ -167,7 +168,7 @@ namespace ts { size = 0; data = createDictionaryObject(); - + currentIteratoKeys?: string[]; get(key: string): T | undefined { @@ -183,7 +184,7 @@ namespace ts { this.currentIteratoKeys.push(key); } } - + this.data[key] = value; return this; } @@ -236,8 +237,9 @@ namespace ts { const iterator = this.entries(); while (true) { const { value: entry, done } = iterator.next(); - if (done) + if (done) { break; + } action(entry[1], entry[0]); } From 41bef5f77c5727bdda840c55cf94ab643a40e27f Mon Sep 17 00:00:00 2001 From: kpreisser Date: Fri, 4 Jan 2019 14:00:24 +0100 Subject: [PATCH 04/13] PR review: Fix typo. --- src/compiler/core.ts | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index ad984a208b5..61b9f52cffa 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -124,33 +124,33 @@ namespace ts { index = 0; private shimMap: ShimMap; - private originalIteratoKeys: string[]; + private originalIteratorKeys: string[]; private selector: (data: MapLike, key: string) => U; constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { this.shimMap = shimMap; this.selector = selector; - if (!shimMap.currentIteratoKeys) { + if (!shimMap.currentIteratorKeys) { // Create the key array on the map over which we (and other new iterators) // will iterate. - shimMap.currentIteratoKeys = Object.keys(shimMap.data); + shimMap.currentIteratorKeys = Object.keys(shimMap.data); } // Copy the key array to allow us later to check if the map has cleared // or replaced the array. - this.originalIteratoKeys = shimMap.currentIteratoKeys; + this.originalIteratorKeys = shimMap.currentIteratorKeys; } public next(): { value: U, done: false } | { value: never, done: true } { // Check if we still have the same key array. Otherwise, this means // an element has been deleted from the map in the meanwhile, so we // cannot continue. - if (this.index !== -1 && this.originalIteratoKeys !== this.shimMap.currentIteratoKeys) { + if (this.index !== -1 && this.originalIteratorKeys !== this.shimMap.currentIteratorKeys) { throw new Error("Cannot continue iteration because a map element has been deleted."); } - const iteratorKeys = this.originalIteratoKeys; + const iteratorKeys = this.originalIteratorKeys; if (this.index !== -1 && this.index < iteratorKeys.length) { const index = this.index++; return { value: this.selector(this.shimMap.data, iteratorKeys[index]), done: false }; @@ -169,7 +169,7 @@ namespace ts { data = createDictionaryObject(); - currentIteratoKeys?: string[]; + currentIteratorKeys?: string[]; get(key: string): T | undefined { return this.data[key]; @@ -179,9 +179,9 @@ namespace ts { if (!this.has(key)) { this.size++; - if (this.currentIteratoKeys) { + if (this.currentIteratorKeys) { // Add the new entry. - this.currentIteratoKeys.push(key); + this.currentIteratorKeys.push(key); } } @@ -199,10 +199,10 @@ namespace ts { this.size--; delete this.data[key]; - if (this.currentIteratoKeys) { + if (this.currentIteratorKeys) { // Clear the iteratorKeys array. This means if iterators are still active // they will throw on the next() call. - this.currentIteratoKeys = undefined; + this.currentIteratorKeys = undefined; } return true; @@ -214,10 +214,10 @@ namespace ts { this.data = createDictionaryObject(); this.size = 0; - if (this.currentIteratoKeys) { + if (this.currentIteratorKeys) { // Clear the iteratorKeys array. This means if iterators are still active // they will throw on their next() call. - this.currentIteratoKeys = undefined; + this.currentIteratorKeys = undefined; } } From b502ae98e110303457fc87b79dec8f7b8920fa15 Mon Sep 17 00:00:00 2001 From: kpreisser Date: Fri, 4 Jan 2019 18:02:43 +0100 Subject: [PATCH 05/13] Add a unit test for the shimMap (currently failing). This will test that iteration is in insertion order, new entries added during iteration will be visited by the iterator, and values can be deleted while an iterator is running. --- src/testRunner/tsconfig.json | 1 + src/testRunner/unittests/shimMap.ts | 103 ++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 src/testRunner/unittests/shimMap.ts diff --git a/src/testRunner/tsconfig.json b/src/testRunner/tsconfig.json index 849906e24cf..7e75c0105a2 100644 --- a/src/testRunner/tsconfig.json +++ b/src/testRunner/tsconfig.json @@ -58,6 +58,7 @@ "unittests/publicApi.ts", "unittests/reuseProgramStructure.ts", "unittests/semver.ts", + "unittests/shimMap.ts", "unittests/transform.ts", "unittests/tsbuild.ts", "unittests/tsbuildWatchMode.ts", diff --git a/src/testRunner/unittests/shimMap.ts b/src/testRunner/unittests/shimMap.ts new file mode 100644 index 00000000000..e169f1963cb --- /dev/null +++ b/src/testRunner/unittests/shimMap.ts @@ -0,0 +1,103 @@ +namespace ts { + describe("unittests:: shimMap", () => { + + function testMapIterationAddedValues(map: Map, useForEach: boolean): string { + let resultString = ""; + + map.set("1", "1"); + map.set("3", "3"); + map.set("2", "2"); + map.set("4", "4"); + + let addedThree = false; + const doForEach = (value: string, key: string) => { + resultString += `${key}:${value};`; + + // Add a new key ("0") - the map should provide this + // one in the next iteration. + if (key === "1") { + map.set("1", "X1"); + map.set("0", "X0"); + map.set("4", "X4"); + } + else if (key === "3") { + if (!addedThree) { + addedThree = true; + + // Remove and re-add key "3"; the map should + // visit it after "0". + map.delete("3"); + map.set("3", "Y3"); + + // Change the value of "2"; the map should provide + // it when visiting the key. + map.set("2", "Y2"); + } + else { + // Check that an entry added when we visit the + // currently last entry will still be visited. + map.set("999", "999"); + } + } + else if (key === "999") { + // Ensure that clear() behaves correctly same as removing all keys. + map.set("A", "A"); + map.set("B", "B"); + map.set("C", "C"); + } + else if (key === "A") { + map.clear(); + map.set("Z", "Z"); + } + else if (key === "Z") { + // Check that the map behaves correctly when an item is + // added and removed immediately. + map.set("X", "X"); + map.delete("X"); + map.set("Y", "Y"); + } + }; + + if (useForEach) { + map.forEach(doForEach); + } + else { + // Use an iterator. + const iterator = map.entries(); + while (true) { + const { value: tuple, done } = iterator.next(); + if (done) { + break; + } + + doForEach(tuple[1], tuple[0]); + } + } + + return resultString; + } + + it("iterates values in insertion order and handles changes", () => { + const expectedResult = "1:1;3:3;2:Y2;4:X4;0:X0;3:Y3;999:999;A:A;Z:Z;Y:Y;"; + + // First, ensure the test actually has the same behavior as a native Map. + let nativeMap = createMap(); + const nativeMapForEachResult = testMapIterationAddedValues(nativeMap, /* useForEach */ true); + assert.equal(nativeMapForEachResult, expectedResult, "nativeMap-forEach"); + + nativeMap = createMap(); + const nativeMapIteratorResult = testMapIterationAddedValues(nativeMap, /* useForEach */ false); + assert.equal(nativeMapIteratorResult, expectedResult, "nativeMap-iterator"); + + // Then, test the shimMap. + let localShimMap = new (shimMap())(); + const shimMapForEachResult = testMapIterationAddedValues(localShimMap, /* useForEach */ true); + assert.equal(shimMapForEachResult, expectedResult, "shimMap-forEach"); + + localShimMap = new (shimMap())(); + const shimMapIteratorResult = testMapIterationAddedValues(localShimMap, /* useForEach */ false); + assert.equal(shimMapIteratorResult, expectedResult, "shimMap-iterator"); + + }); + }); +} From c8e329bcbc2b5940c314298e16e0de69482a92dc Mon Sep 17 00:00:00 2001 From: kpreisser Date: Fri, 4 Jan 2019 19:09:44 +0100 Subject: [PATCH 06/13] Rework the implementation of the shimMap to ensure iterators will behave the same as with native Maps. This includes: - Entries are visited in insertion order - New entries added during iteration will be visited - Entries that are removed during iteration (before being visited) will not be visited Fixes #26090 --- src/compiler/core.ts | 146 ++++++++++++++++++++++++++++--------------- 1 file changed, 97 insertions(+), 49 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 61b9f52cffa..606c54c0337 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -118,47 +118,48 @@ namespace ts { export const MapCtr = typeof Map !== "undefined" && "entries" in Map.prototype ? Map : shimMap(); // Keep the class inside a function so it doesn't get compiled if it's not used. - function shimMap(): new () => Map { + export function shimMap(): new () => Map { + + interface MapEntry { + key?: string; + value?: T; + + // Linked list references + nextEntry?: MapEntry; + previousEntry?: MapEntry; + + /** + * Specifies if the iterator should skip the next entry. + * This will be set when an entry is deleted. + * See https://github.com/Microsoft/TypeScript/pull/27292 for more information. + */ + skipNext?: boolean; + } class MapIterator { - index = 0; + private currentEntry?: MapEntry; + private selector: (key: string, value: T) => U; - private shimMap: ShimMap; - private originalIteratorKeys: string[]; - private selector: (data: MapLike, key: string) => U; - - constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { - this.shimMap = shimMap; + constructor(currentEntry: MapEntry, selector: (key: string, value: T) => U) { + this.currentEntry = currentEntry; this.selector = selector; - - if (!shimMap.currentIteratorKeys) { - // Create the key array on the map over which we (and other new iterators) - // will iterate. - shimMap.currentIteratorKeys = Object.keys(shimMap.data); - } - - // Copy the key array to allow us later to check if the map has cleared - // or replaced the array. - this.originalIteratorKeys = shimMap.currentIteratorKeys; } public next(): { value: U, done: false } | { value: never, done: true } { - // Check if we still have the same key array. Otherwise, this means - // an element has been deleted from the map in the meanwhile, so we - // cannot continue. - if (this.index !== -1 && this.originalIteratorKeys !== this.shimMap.currentIteratorKeys) { - throw new Error("Cannot continue iteration because a map element has been deleted."); + // Navigate to the next element. + while (this.currentEntry) { + const skipNext = !!this.currentEntry.skipNext; + this.currentEntry = this.currentEntry.nextEntry; + + if (!skipNext) { + break; + } } - const iteratorKeys = this.originalIteratorKeys; - if (this.index !== -1 && this.index < iteratorKeys.length) { - const index = this.index++; - return { value: this.selector(this.shimMap.data, iteratorKeys[index]), done: false }; + if (this.currentEntry) { + return { value: this.selector(this.currentEntry.key!, this.currentEntry.value!), done: false }; } else { - // Ensure subsequent invocations will always return done. - this.index = -1; - return { value: undefined as never, done: true }; } } @@ -167,25 +168,48 @@ namespace ts { class ShimMap implements Map { size = 0; - data = createDictionaryObject(); + private data = createDictionaryObject>(); - currentIteratorKeys?: string[]; + // Linked list references for iterators. + // See https://github.com/Microsoft/TypeScript/pull/27292 + // for more information. + private readonly linkedListStart: MapEntry; + private linkedListEnd: MapEntry; + + constructor() { + // Create a (stub) start element that will not + // contain a key and value. + this.linkedListStart = {}; + // When the map is empty, the end element is the same as the + // start element. + this.linkedListEnd = this.linkedListStart; + } get(key: string): T | undefined { - return this.data[key]; + const entry = this.data[key] as MapEntry | undefined; + return entry && entry.value!; } set(key: string, value: T): this { if (!this.has(key)) { this.size++; - if (this.currentIteratorKeys) { - // Add the new entry. - this.currentIteratorKeys.push(key); - } + // Append the new element at the end of the linked list. + const newEntry: MapEntry = { + key, + value + }; + this.data[key] = newEntry; + + const previousEndElement = this.linkedListEnd; + previousEndElement.nextEntry = newEntry; + newEntry.previousEntry = previousEndElement; + this.linkedListEnd = newEntry; + } + else { + this.data[key].value = value; } - this.data[key] = value; return this; } @@ -197,16 +221,28 @@ namespace ts { delete(key: string): boolean { if (this.has(key)) { this.size--; + const entry = this.data[key]; delete this.data[key]; - if (this.currentIteratorKeys) { - // Clear the iteratorKeys array. This means if iterators are still active - // they will throw on the next() call. - this.currentIteratorKeys = undefined; + // Adjust the linked list references. + const previousElement = entry.previousEntry!; + previousElement.nextEntry = entry.nextEntry; + + // Adjust the forward reference of the deleted element + // in case an iterator still references it. + entry.previousEntry = undefined; + entry.nextEntry = previousElement; + entry.skipNext = true; + + // When the deleted entry was the last one, we need to + // adust the endElement reference + if (this.linkedListEnd === entry) { + this.linkedListEnd = previousElement; } return true; } + return false; } @@ -214,23 +250,35 @@ namespace ts { this.data = createDictionaryObject(); this.size = 0; - if (this.currentIteratorKeys) { - // Clear the iteratorKeys array. This means if iterators are still active - // they will throw on their next() call. - this.currentIteratorKeys = undefined; + // Reset the linked list. Note that we must adjust the forward + // references of the deleted entries to ensure iterators stuck + // in the middle of the list don't continue with deleted entries, + // but can continue with new entries added after the clear() + // operation. + const startEntry = this.linkedListStart; + let currentEntry = startEntry.nextEntry; + while (currentEntry) { + const nextEntry = currentEntry.nextEntry; + currentEntry.previousEntry = undefined; + currentEntry.nextEntry = startEntry; + currentEntry.skipNext = true; + + currentEntry = nextEntry; } + this.linkedListStart.nextEntry = undefined; + this.linkedListEnd = this.linkedListStart; } keys(): Iterator { - return new MapIterator(this, (_data, key) => key); + return new MapIterator(this.linkedListStart, key => key); } values(): Iterator { - return new MapIterator(this, (data, key) => data[key]); + return new MapIterator(this.linkedListStart, (_key, value) => value); } entries(): Iterator<[string, T]> { - return new MapIterator(this, (data, key) => [key, data[key]] as [string, T]); + return new MapIterator(this.linkedListStart, (key, value) => [key, value] as [string, T]); } forEach(action: (value: T, key: string) => void): void { From 472157e5237ae73ce8c7631eaef63f7c76ce9c0f Mon Sep 17 00:00:00 2001 From: kpreisser Date: Sat, 5 Jan 2019 11:25:50 +0100 Subject: [PATCH 07/13] Revert a few unneeded (stylistic) changes. --- src/compiler/core.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 606c54c0337..0daf80a53c9 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -165,10 +165,9 @@ namespace ts { } } - class ShimMap implements Map { - size = 0; - + return class implements Map { private data = createDictionaryObject>(); + public size = 0; // Linked list references for iterators. // See https://github.com/Microsoft/TypeScript/pull/27292 @@ -242,7 +241,6 @@ namespace ts { return true; } - return false; } @@ -292,9 +290,7 @@ namespace ts { action(entry[1], entry[0]); } } - } - - return ShimMap; + }; } export function length(array: ReadonlyArray | undefined): number { From c4960d3c11ce1bdf85ded5acbc0c48dd10d28e6d Mon Sep 17 00:00:00 2001 From: kpreisser Date: Sat, 5 Jan 2019 11:26:37 +0100 Subject: [PATCH 08/13] Adjust the unit test to spot a missed bug in the delete() implementation. --- src/testRunner/unittests/shimMap.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/testRunner/unittests/shimMap.ts b/src/testRunner/unittests/shimMap.ts index e169f1963cb..a47a1d18205 100644 --- a/src/testRunner/unittests/shimMap.ts +++ b/src/testRunner/unittests/shimMap.ts @@ -50,10 +50,13 @@ namespace ts { map.set("Z", "Z"); } else if (key === "Z") { - // Check that the map behaves correctly when an item is + // Check that the map behaves correctly when two items are // added and removed immediately. map.set("X", "X"); - map.delete("X"); + map.set("X1", "X1"); + map.set("X2", "X2"); + map.delete("X1"); + map.delete("X2"); map.set("Y", "Y"); } }; @@ -78,7 +81,7 @@ namespace ts { } it("iterates values in insertion order and handles changes", () => { - const expectedResult = "1:1;3:3;2:Y2;4:X4;0:X0;3:Y3;999:999;A:A;Z:Z;Y:Y;"; + const expectedResult = "1:1;3:3;2:Y2;4:X4;0:X0;3:Y3;999:999;A:A;Z:Z;X:X;Y:Y;"; // First, ensure the test actually has the same behavior as a native Map. let nativeMap = createMap(); From 9140cbc29e29298f4ba21b0b0a098c512433f864 Mon Sep 17 00:00:00 2001 From: kpreisser Date: Sat, 5 Jan 2019 11:45:46 +0100 Subject: [PATCH 09/13] Correctly adjust the backward reference of the next entry when deleting an entry. --- src/compiler/core.ts | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 0daf80a53c9..3f5d3207cdd 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -223,22 +223,25 @@ namespace ts { const entry = this.data[key]; delete this.data[key]; - // Adjust the linked list references. - const previousElement = entry.previousEntry!; - previousElement.nextEntry = entry.nextEntry; + // Adjust the linked list references of the neighbor entries. + const previousEntry = entry.previousEntry!; + previousEntry.nextEntry = entry.nextEntry; + if (entry.nextEntry) { + entry.nextEntry.previousEntry = previousEntry; + } + + // When the deleted entry was the last one, we need to + // adust the endElement reference. + if (this.linkedListEnd === entry) { + this.linkedListEnd = previousEntry; + } // Adjust the forward reference of the deleted element // in case an iterator still references it. entry.previousEntry = undefined; - entry.nextEntry = previousElement; + entry.nextEntry = previousEntry; entry.skipNext = true; - // When the deleted entry was the last one, we need to - // adust the endElement reference - if (this.linkedListEnd === entry) { - this.linkedListEnd = previousElement; - } - return true; } return false; From 97d2ea8f321cbcc530098b5ec819ec2b539396b4 Mon Sep 17 00:00:00 2001 From: kpreisser Date: Sat, 5 Jan 2019 17:10:18 +0100 Subject: [PATCH 10/13] Minor improvements of code and comments. --- src/compiler/core.ts | 72 ++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 3f5d3207cdd..c0d5cae7bae 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -121,15 +121,15 @@ namespace ts { export function shimMap(): new () => Map { interface MapEntry { - key?: string; + readonly key?: string; value?: T; - // Linked list references + // Linked list references for iterators. nextEntry?: MapEntry; previousEntry?: MapEntry; /** - * Specifies if the iterator should skip the next entry. + * Specifies if iterators should skip the next entry. * This will be set when an entry is deleted. * See https://github.com/Microsoft/TypeScript/pull/27292 for more information. */ @@ -146,7 +146,7 @@ namespace ts { } public next(): { value: U, done: false } | { value: never, done: true } { - // Navigate to the next element. + // Navigate to the next entry. while (this.currentEntry) { const skipNext = !!this.currentEntry.skipNext; this.currentEntry = this.currentEntry.nextEntry; @@ -172,16 +172,22 @@ namespace ts { // Linked list references for iterators. // See https://github.com/Microsoft/TypeScript/pull/27292 // for more information. - private readonly linkedListStart: MapEntry; - private linkedListEnd: MapEntry; + + /** + * The first entry in the linked list. + * Note that this is only a stub that serves as starting point + * for iterators and doesn't contain a key and a value. + */ + private readonly firstEntry: MapEntry; + private lastEntry: MapEntry; constructor() { - // Create a (stub) start element that will not - // contain a key and value. - this.linkedListStart = {}; - // When the map is empty, the end element is the same as the - // start element. - this.linkedListEnd = this.linkedListStart; + // Create a first (stub) map entry that will not contain a key + // and value but serves as starting point for iterators. + this.firstEntry = {}; + // When the map is empty, the last entry is the same as the + // first one. + this.lastEntry = this.firstEntry; } get(key: string): T | undefined { @@ -193,17 +199,19 @@ namespace ts { if (!this.has(key)) { this.size++; - // Append the new element at the end of the linked list. + // Create a new entry that will be appended at the + // end of the linked list. const newEntry: MapEntry = { key, value }; this.data[key] = newEntry; - const previousEndElement = this.linkedListEnd; - previousEndElement.nextEntry = newEntry; - newEntry.previousEntry = previousEndElement; - this.linkedListEnd = newEntry; + // Adjust the references. + const previousLastEntry = this.lastEntry; + previousLastEntry.nextEntry = newEntry; + newEntry.previousEntry = previousLastEntry; + this.lastEntry = newEntry; } else { this.data[key].value = value; @@ -231,13 +239,17 @@ namespace ts { } // When the deleted entry was the last one, we need to - // adust the endElement reference. - if (this.linkedListEnd === entry) { - this.linkedListEnd = previousEntry; + // adust the lastEntry reference. + if (this.lastEntry === entry) { + this.lastEntry = previousEntry; } - // Adjust the forward reference of the deleted element - // in case an iterator still references it. + // Adjust the forward reference of the deleted entry + // in case an iterator still references it. This allows us + // to throw away the entry, but when an active iterator + // (which points to the current entry) continues, it will + // navigate to the entry that originally came before the + // current one and skip it. entry.previousEntry = undefined; entry.nextEntry = previousEntry; entry.skipNext = true; @@ -256,30 +268,30 @@ namespace ts { // in the middle of the list don't continue with deleted entries, // but can continue with new entries added after the clear() // operation. - const startEntry = this.linkedListStart; - let currentEntry = startEntry.nextEntry; + const firstEntry = this.firstEntry; + let currentEntry = firstEntry.nextEntry; while (currentEntry) { const nextEntry = currentEntry.nextEntry; currentEntry.previousEntry = undefined; - currentEntry.nextEntry = startEntry; + currentEntry.nextEntry = firstEntry; currentEntry.skipNext = true; currentEntry = nextEntry; } - this.linkedListStart.nextEntry = undefined; - this.linkedListEnd = this.linkedListStart; + firstEntry.nextEntry = undefined; + this.lastEntry = firstEntry; } keys(): Iterator { - return new MapIterator(this.linkedListStart, key => key); + return new MapIterator(this.firstEntry, key => key); } values(): Iterator { - return new MapIterator(this.linkedListStart, (_key, value) => value); + return new MapIterator(this.firstEntry, (_key, value) => value); } entries(): Iterator<[string, T]> { - return new MapIterator(this.linkedListStart, (key, value) => [key, value] as [string, T]); + return new MapIterator(this.firstEntry, (key, value) => [key, value] as [string, T]); } forEach(action: (value: T, key: string) => void): void { From 056028b4c656ecd64e1ab85b6e02d05511060116 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Wed, 6 Feb 2019 13:10:36 -0800 Subject: [PATCH 11/13] Modifies the jakefile to use the LKG by default unless USE_BUILT=true is set --- Jakefile.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Jakefile.js b/Jakefile.js index ea81881e7d9..c6b1cde9d4e 100644 --- a/Jakefile.js +++ b/Jakefile.js @@ -23,6 +23,10 @@ else if (process.env.PATH !== undefined) { const host = process.env.TYPESCRIPT_HOST || process.env.host || "node"; const defaultTestTimeout = 40000; +const useBuilt = + process.env.USE_BUILT === "true" ? true : + process.env.LKG === "true" ? false : + false; let useDebugMode = true; @@ -296,7 +300,7 @@ task(TaskNames.buildFoldEnd, [], function () { desc("Compiles tslint rules to js"); task(TaskNames.buildRules, [], function () { - tsbuild(ConfigFileFor.lint, false, () => complete()); + tsbuild(ConfigFileFor.lint, !useBuilt, () => complete()); }, { async: true }); desc("Cleans the compiler output, declare files, and tests"); @@ -368,7 +372,7 @@ file(ConfigFileFor.tsserverLibrary, [], function () { // tsserverlibrary.js // tsserverlibrary.d.ts file(Paths.tsserverLibraryFile, [TaskNames.coreBuild, ConfigFileFor.tsserverLibrary], function() { - tsbuild(ConfigFileFor.tsserverLibrary, false, () => { + tsbuild(ConfigFileFor.tsserverLibrary, !useBuilt, () => { if (needsUpdate([Paths.tsserverLibraryOutFile, Paths.tsserverLibraryDefinitionOutFile], [Paths.tsserverLibraryFile, Paths.tsserverLibraryDefinitionFile])) { const copyright = readFileSync(Paths.copyright); @@ -427,7 +431,7 @@ file(ConfigFileFor.typescriptServices, [], function () { // typescriptServices.js // typescriptServices.d.ts file(Paths.servicesFile, [TaskNames.coreBuild, ConfigFileFor.typescriptServices], function() { - tsbuild(ConfigFileFor.typescriptServices, false, () => { + tsbuild(ConfigFileFor.typescriptServices, !useBuilt, () => { if (needsUpdate([Paths.servicesOutFile, Paths.servicesDefinitionOutFile], [Paths.servicesFile, Paths.servicesDefinitionFile])) { const copyright = readFileSync(Paths.copyright); From fb1b7a218a1534b2e0e490da983efeeb6a88bc07 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Wed, 6 Feb 2019 16:35:09 -0800 Subject: [PATCH 12/13] Accept baseline diff --- tests/baselines/reference/api/tsserverlibrary.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index ee13a377f5e..8f2f0c8cbbd 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -8346,7 +8346,7 @@ declare namespace ts.server { excludedFiles: ReadonlyArray; private typeAcquisition; updateGraph(): boolean; - getExcludedFiles(): readonly NormalizedPath[]; + getExcludedFiles(): ReadonlyArray; getTypeAcquisition(): TypeAcquisition; setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void; } From 5043bf72f5baefbac541a88ea3be5976cf631ec7 Mon Sep 17 00:00:00 2001 From: kpreisser Date: Thu, 7 Feb 2019 09:01:27 +0100 Subject: [PATCH 13/13] PR feedback. --- src/compiler/core.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index c0d5cae7bae..1e02da30c39 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -260,7 +260,7 @@ namespace ts { } clear(): void { - this.data = createDictionaryObject(); + this.data = createDictionaryObject>(); this.size = 0; // Reset the linked list. Note that we must adjust the forward