From 2b76fbd414b414494c65e7b869a594f34828203a Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 5 Jul 2024 22:24:33 +0000 Subject: [PATCH 1/4] Create a `SourceFile`-level indirection on WeakMaps, store `SyntaxList` children directly on nodes. --- src/compiler/factory/nodeChildren.ts | 45 +++++++++++++++++++++++----- src/compiler/factory/nodeFactory.ts | 3 +- src/compiler/parser.ts | 14 ++++----- src/compiler/types.ts | 1 + src/compiler/utilities.ts | 7 +++-- src/services/services.ts | 6 ++-- 6 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/compiler/factory/nodeChildren.ts b/src/compiler/factory/nodeChildren.ts index 79e8563f6f593..188727eab2a1d 100644 --- a/src/compiler/factory/nodeChildren.ts +++ b/src/compiler/factory/nodeChildren.ts @@ -1,25 +1,54 @@ import { + Debug, emptyArray, isNodeKind, Node, + SourceFileLike, + SyntaxKind, + SyntaxList, } from "../_namespaces/ts.js"; -const nodeChildren = new WeakMap(); +const sourceFileToNodeChildren = new WeakMap>(); /** @internal */ -export function getNodeChildren(node: Node): readonly Node[] | undefined { - if (!isNodeKind(node.kind)) return emptyArray; +export function getNodeChildren(node: Node, sourceFile: SourceFileLike): readonly Node[] | undefined { + const kind = node.kind; + if (!isNodeKind(kind)) { + return emptyArray; + } + if (kind === SyntaxKind.SyntaxList) { + return (node as SyntaxList)._children; + } - return nodeChildren.get(node); + return sourceFileToNodeChildren.get(sourceFile)?.get(node); } /** @internal */ -export function setNodeChildren(node: Node, children: readonly Node[]): readonly Node[] { - nodeChildren.set(node, children); +export function setNodeChildren(node: Node, sourceFile: SourceFileLike, children: readonly Node[]): readonly Node[] { + if (node.kind === SyntaxKind.SyntaxList) { + // Store the children directly on the syntax list node. + // This makes orphaned syntax lists easier to clean up, + // and avoids the need to track all of them in a WeakMap. + (node as SyntaxList)._children = children; + } + else { + let map = sourceFileToNodeChildren.get(sourceFile); + if (map === undefined) { + map = new WeakMap(); + sourceFileToNodeChildren.set(sourceFile, map); + } + map.set(node, children); + } return children; } /** @internal */ -export function unsetNodeChildren(node: Node) { - nodeChildren.delete(node); +export function unsetNodeChildren(node: Node, origSourceFile: SourceFileLike) { + if (node.kind === SyntaxKind.SyntaxList) { + // Syntax lists are synthesized and we store their children directly on them. + // They are a special case where we expect incremental parsing to toss them away entirely + // if a change intersects with their containing parents. + Debug.fail("Did not expect to unset the children of a SyntaxList."); + } + sourceFileToNodeChildren.get(origSourceFile)?.delete(node); } diff --git a/src/compiler/factory/nodeFactory.ts b/src/compiler/factory/nodeFactory.ts index 6c277479a509c..c90e4c71f19f5 100644 --- a/src/compiler/factory/nodeFactory.ts +++ b/src/compiler/factory/nodeFactory.ts @@ -388,7 +388,6 @@ import { setEmitFlags, setIdentifierAutoGenerate, setIdentifierTypeArguments, - setNodeChildren, setParent, setTextRange, ShorthandPropertyAssignment, @@ -6212,7 +6211,7 @@ export function createNodeFactory(flags: NodeFactoryFlags, baseFactory: BaseNode // @api function createSyntaxList(children: readonly Node[]) { const node = createBaseNode(SyntaxKind.SyntaxList); - setNodeChildren(node, children); + node._children = children; return node; } diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 23267f21866e7..955887c8168e2 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -10021,9 +10021,9 @@ namespace IncrementalParser { } } - function moveElementEntirelyPastChangeRange(element: Node, isArray: false, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void; - function moveElementEntirelyPastChangeRange(element: NodeArray, isArray: true, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void; - function moveElementEntirelyPastChangeRange(element: Node | NodeArray, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) { + function moveElementEntirelyPastChangeRange(element: Node, origSourceFile: SourceFile, isArray: false, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void; + function moveElementEntirelyPastChangeRange(element: NodeArray, origSourceFile: SourceFile, isArray: true, delta: number, oldText: string, newText: string, aggressiveChecks: boolean): void; + function moveElementEntirelyPastChangeRange(element: Node | NodeArray, origSourceFile: SourceFile, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) { if (isArray) { visitArray(element as NodeArray); } @@ -10040,7 +10040,7 @@ namespace IncrementalParser { // Ditch any existing LS children we may have created. This way we can avoid // moving them forward. - unsetNodeChildren(node); + unsetNodeChildren(node, origSourceFile); setTextRangePosEnd(node, node.pos + delta, node.end + delta); @@ -10187,7 +10187,7 @@ namespace IncrementalParser { if (child.pos > changeRangeOldEnd) { // Node is entirely past the change range. We need to move both its pos and // end, forward or backward appropriately. - moveElementEntirelyPastChangeRange(child, /*isArray*/ false, delta, oldText, newText, aggressiveChecks); + moveElementEntirelyPastChangeRange(child, sourceFile, /*isArray*/ false, delta, oldText, newText, aggressiveChecks); return; } @@ -10197,7 +10197,7 @@ namespace IncrementalParser { const fullEnd = child.end; if (fullEnd >= changeStart) { markAsIntersectingIncrementalChange(child); - unsetNodeChildren(child); + unsetNodeChildren(child, sourceFile); // Adjust the pos or end (or both) of the intersecting element accordingly. adjustIntersectingElement(child, changeStart, changeRangeOldEnd, changeRangeNewEnd, delta); @@ -10220,7 +10220,7 @@ namespace IncrementalParser { if (array.pos > changeRangeOldEnd) { // Array is entirely after the change range. We need to move it, and move any of // its children. - moveElementEntirelyPastChangeRange(array, /*isArray*/ true, delta, oldText, newText, aggressiveChecks); + moveElementEntirelyPastChangeRange(array, sourceFile, /*isArray*/ true, delta, oldText, newText, aggressiveChecks); return; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 508042f69bee8..c485e7c41b79c 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -9816,6 +9816,7 @@ export interface DiagnosticCollection { // SyntaxKind.SyntaxList export interface SyntaxList extends Node { kind: SyntaxKind.SyntaxList; + /** @internal */ _children: readonly Node[]; } // dprint-ignore diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 1b972e990f7fb..bfa2f81a22a9f 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1187,7 +1187,7 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu if (isJSDocNode(node) || node.kind === SyntaxKind.JsxText) { // JsxText cannot actually contain comments, even though the scanner will think it sees comments - return skipTrivia((sourceFile || getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); + return skipTrivia((sourceFile ??= getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); } if (includeJsDoc && hasJSDocNodes(node)) { @@ -1199,14 +1199,15 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu // trivia for the list, we may have skipped the JSDocComment as well. So we should process its // first child to determine the actual position of its first token. if (node.kind === SyntaxKind.SyntaxList) { - const first = firstOrUndefined(getNodeChildren(node)); + sourceFile ??= getSourceFileOfNode(node); + const first = firstOrUndefined(getNodeChildren(node, sourceFile)); if (first) { return getTokenPosOfNode(first, sourceFile, includeJsDoc); } } return skipTrivia( - (sourceFile || getSourceFileOfNode(node)).text, + (sourceFile ??= getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ false, diff --git a/src/services/services.ts b/src/services/services.ts index 0afdecd646e0f..adc4ec8013ff0 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -457,9 +457,9 @@ class NodeObject implements Node { return this.getChildren(sourceFile)[index]; } - public getChildren(sourceFile?: SourceFileLike): readonly Node[] { + public getChildren(sourceFile: SourceFileLike = getSourceFileOfNode(this)): readonly Node[] { this.assertHasRealPosition("Node without a real position cannot be scanned and thus has no token nodes - use forEachChild and collect the result if that's fine"); - return getNodeChildren(this) ?? setNodeChildren(this, createChildren(this, sourceFile)); + return getNodeChildren(this, sourceFile) ?? setNodeChildren(this, sourceFile, createChildren(this, sourceFile)); } public getFirstToken(sourceFile?: SourceFileLike): Node | undefined { @@ -558,7 +558,7 @@ function createSyntaxList(nodes: NodeArray, parent: Node): Node { pos = node.end; } addSyntheticNodes(children, pos, nodes.end, parent); - setNodeChildren(list, children); + list._children = children; return list; } From 8e96f4d0bde94b3f564afc84c077da407ab8bd17 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Mon, 8 Jul 2024 16:49:07 +0000 Subject: [PATCH 2/4] Don't assign. --- src/compiler/utilities.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index bfa2f81a22a9f..38146135ae715 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1187,7 +1187,7 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu if (isJSDocNode(node) || node.kind === SyntaxKind.JsxText) { // JsxText cannot actually contain comments, even though the scanner will think it sees comments - return skipTrivia((sourceFile ??= getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); + return skipTrivia((sourceFile ?? getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); } if (includeJsDoc && hasJSDocNodes(node)) { @@ -1207,7 +1207,7 @@ export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, inclu } return skipTrivia( - (sourceFile ??= getSourceFileOfNode(node)).text, + (sourceFile ?? getSourceFileOfNode(node)).text, node.pos, /*stopAfterLineBreak*/ false, /*stopAtComments*/ false, From 6299362a5dc7428e2d97e0a013360699dc0be866 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Mon, 8 Jul 2024 17:02:24 +0000 Subject: [PATCH 3/4] Turn the branch in `setNodeChildren` into an assertion failure. --- src/compiler/factory/nodeChildren.ts | 20 +++++++++----------- src/compiler/types.ts | 5 +++++ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/compiler/factory/nodeChildren.ts b/src/compiler/factory/nodeChildren.ts index 188727eab2a1d..89a5755739504 100644 --- a/src/compiler/factory/nodeChildren.ts +++ b/src/compiler/factory/nodeChildren.ts @@ -26,19 +26,17 @@ export function getNodeChildren(node: Node, sourceFile: SourceFileLike): readonl /** @internal */ export function setNodeChildren(node: Node, sourceFile: SourceFileLike, children: readonly Node[]): readonly Node[] { if (node.kind === SyntaxKind.SyntaxList) { - // Store the children directly on the syntax list node. - // This makes orphaned syntax lists easier to clean up, - // and avoids the need to track all of them in a WeakMap. - (node as SyntaxList)._children = children; + // SyntaxList children are always eagerly created in the process of + // creating their parent's `children` list. We shouldn't need to set them here. + Debug.fail("Should not need to re-set the children of a SyntaxList."); } - else { - let map = sourceFileToNodeChildren.get(sourceFile); - if (map === undefined) { - map = new WeakMap(); - sourceFileToNodeChildren.set(sourceFile, map); - } - map.set(node, children); + + let map = sourceFileToNodeChildren.get(sourceFile); + if (map === undefined) { + map = new WeakMap(); + sourceFileToNodeChildren.set(sourceFile, map); } + map.set(node, children); return children; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index c485e7c41b79c..f30bee224d507 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -9816,6 +9816,11 @@ export interface DiagnosticCollection { // SyntaxKind.SyntaxList export interface SyntaxList extends Node { kind: SyntaxKind.SyntaxList; + + // Unlike other nodes which may or may not have their child nodes calculated, + // the entire purpose of a SyntaxList is to hold child nodes. + // Instead of using the WeakMap machinery in `nodeChildren.ts`, + // we just store the children directly on the SyntaxList. /** @internal */ _children: readonly Node[]; } From 55d49e82e9e1ccce3bd328f966a5370b39a9a5cd Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Mon, 8 Jul 2024 19:00:50 +0000 Subject: [PATCH 4/4] Transfer children between old and new `SourceFile` objects. --- src/compiler/factory/nodeChildren.ts | 9 +++++++++ src/compiler/parser.ts | 2 ++ 2 files changed, 11 insertions(+) diff --git a/src/compiler/factory/nodeChildren.ts b/src/compiler/factory/nodeChildren.ts index 89a5755739504..3946ee89417bc 100644 --- a/src/compiler/factory/nodeChildren.ts +++ b/src/compiler/factory/nodeChildren.ts @@ -50,3 +50,12 @@ export function unsetNodeChildren(node: Node, origSourceFile: SourceFileLike) { } sourceFileToNodeChildren.get(origSourceFile)?.delete(node); } + +/** @internal */ +export function transferSourceFileChildren(sourceFile: SourceFileLike, targetSourceFile: SourceFileLike) { + const map = sourceFileToNodeChildren.get(sourceFile); + if (map !== undefined) { + sourceFileToNodeChildren.delete(sourceFile); + sourceFileToNodeChildren.set(targetSourceFile, map); + } +} diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 955887c8168e2..03bd93f05a9e4 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -369,6 +369,7 @@ import { tokenIsIdentifierOrKeywordOrGreaterThan, tokenToString, tracing, + transferSourceFileChildren, TransformFlags, TryStatement, TupleTypeNode, @@ -9969,6 +9970,7 @@ namespace IncrementalParser { aggressiveChecks, ); result.impliedNodeFormat = sourceFile.impliedNodeFormat; + transferSourceFileChildren(sourceFile, result); return result; }