-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Create a SourceFile
-level indirection on children maps, store SyntaxList
children directly on nodes.
#59154
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
Create a SourceFile
-level indirection on children maps, store SyntaxList
children directly on nodes.
#59154
Changes from all commits
2b76fbd
8e96f4d
6299362
55d49e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,61 @@ | ||
import { | ||
Debug, | ||
emptyArray, | ||
isNodeKind, | ||
Node, | ||
SourceFileLike, | ||
SyntaxKind, | ||
SyntaxList, | ||
} from "../_namespaces/ts.js"; | ||
|
||
const nodeChildren = new WeakMap<Node, readonly Node[] | undefined>(); | ||
const sourceFileToNodeChildren = new WeakMap<SourceFileLike, WeakMap<Node, readonly Node[] | undefined>>(); | ||
|
||
/** @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) { | ||
// 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."); | ||
} | ||
|
||
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); | ||
} | ||
|
||
/** @internal */ | ||
export function transferSourceFileChildren(sourceFile: SourceFileLike, targetSourceFile: SourceFileLike) { | ||
const map = sourceFileToNodeChildren.get(sourceFile); | ||
if (map !== undefined) { | ||
sourceFileToNodeChildren.delete(sourceFile); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it harmful to leave the old entry behind? Theoretically it will get GC'd once the old one goes away, but I guess maybe it's bad to have two people able to write to this? (Theoretically their keys would be distinct anyway?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not a bad reason, but I guess the main thing I was thinking about is avoiding a Technically you could argue we never need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since WeakMap is not iterable and therefore doesn't have a defined order of keys, I'm unclear on whether or not deleting entries from a WeakMap is a trivial operation. It's an interesting question which way performance goes on trimming a WeakMap vs. letting the values get GC'd, but there is something to be said for cleanliness. |
||
sourceFileToNodeChildren.set(targetSourceFile, map); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this wasted work when the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apparently not - this is a recursive function, and at almost every other terminal branch the source file is consulted for something. So I can remove the check, but in practice we'll pull on Happy to remove it if you all think it's cleaner to just get the |
||
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, | ||
|
Uh oh!
There was an error while loading. Please reload this page.