From 8626b927be4c022853b608035341f1d59b51076f Mon Sep 17 00:00:00 2001 From: kpreisser Date: Mon, 20 Aug 2018 13:54:57 +0200 Subject: [PATCH 01/11] 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 a2f2b24550207..36398e06b35de 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/11] 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 36398e06b35de..aa5c13ebaefd8 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/11] 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 aa5c13ebaefd8..ad984a208b535 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/11] 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 ad984a208b535..61b9f52cffa10 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/11] 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 849906e24cf7b..7e75c0105a2d3 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 0000000000000..e169f1963cb9f --- /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/11] 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 | 144 ++++++++++++++++++++++++++++--------------- 1 file changed, 96 insertions(+), 48 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 61b9f52cffa10..606c54c033764 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 { - class MapIterator { - index = 0; + interface MapEntry { + key?: string; + value?: T; - private shimMap: ShimMap; - private originalIteratorKeys: string[]; - private selector: (data: MapLike, key: string) => U; + // Linked list references + nextEntry?: MapEntry; + previousEntry?: MapEntry; - constructor(shimMap: ShimMap, selector: (data: MapLike, key: string) => U) { - this.shimMap = shimMap; - this.selector = selector; + /** + * 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; + } - 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); - } + class MapIterator { + private currentEntry?: MapEntry; + private selector: (key: string, value: T) => U; - // Copy the key array to allow us later to check if the map has cleared - // or replaced the array. - this.originalIteratorKeys = shimMap.currentIteratorKeys; + constructor(currentEntry: MapEntry, selector: (key: string, value: T) => U) { + this.currentEntry = currentEntry; + this.selector = selector; } 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>(); + + // Linked list references for iterators. + // See https://github.com/Microsoft/TypeScript/pull/27292 + // for more information. + private readonly linkedListStart: MapEntry; + private linkedListEnd: MapEntry; - currentIteratorKeys?: string[]; + 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/11] 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 606c54c033764..0daf80a53c9f2 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/11] 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 e169f1963cb9f..a47a1d18205fd 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/11] 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 0daf80a53c9f2..3f5d3207cddb7 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/11] 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 3f5d3207cddb7..c0d5cae7bae70 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 5043bf72f5baefbac541a88ea3be5976cf631ec7 Mon Sep 17 00:00:00 2001 From: kpreisser Date: Thu, 7 Feb 2019 09:01:27 +0100 Subject: [PATCH 11/11] 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 c0d5cae7bae70..1e02da30c39fc 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