Skip to content
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.

Sign up for GitHub

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

Port pyright optimization that skips loop flow nodes that dont contain relevant references #54460

Closed
wants to merge 1 commit into from
Closed

Port pyright optimization that skips loop flow nodes that dont contain relevant references #54460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1295,8 +1295,8 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
return initFlowNode({ flags: FlowFlags.BranchLabel, antecedents: undefined });
}

function createLoopLabel(): FlowLabel {
return initFlowNode({ flags: FlowFlags.LoopLabel, antecedents: undefined });
function createLoopLabel(node: NonNullable<FlowLabel["node"]>): FlowLabel {
return initFlowNode({ flags: FlowFlags.LoopLabel, antecedents: undefined, node });
}

function createReduceLabel(target: FlowLabel, antecedents: FlowNode[], antecedent: FlowNode): FlowReduceLabel {
Expand Down Expand Up @@ -1445,7 +1445,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
}

function bindWhileStatement(node: WhileStatement): void {
const preWhileLabel = setContinueTarget(node, createLoopLabel());
const preWhileLabel = setContinueTarget(node, createLoopLabel(node));
const preBodyLabel = createBranchLabel();
const postWhileLabel = createBranchLabel();
addAntecedent(preWhileLabel, currentFlow);
Expand All @@ -1458,7 +1458,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
}

function bindDoStatement(node: DoStatement): void {
const preDoLabel = createLoopLabel();
const preDoLabel = createLoopLabel(node);
const preConditionLabel = setContinueTarget(node, createBranchLabel());
const postDoLabel = createBranchLabel();
addAntecedent(preDoLabel, currentFlow);
Expand All @@ -1471,7 +1471,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
}

function bindForStatement(node: ForStatement): void {
const preLoopLabel = setContinueTarget(node, createLoopLabel());
const preLoopLabel = setContinueTarget(node, createLoopLabel(node));
const preBodyLabel = createBranchLabel();
const postLoopLabel = createBranchLabel();
bind(node.initializer);
Expand All @@ -1486,7 +1486,7 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void {
}

function bindForInOrForOfStatement(node: ForInOrOfStatement): void {
const preLoopLabel = setContinueTarget(node, createLoopLabel());
const preLoopLabel = setContinueTarget(node, createLoopLabel(node));
const postLoopLabel = createBranchLabel();
bind(node.expression);
addAntecedent(preLoopLabel, currentFlow);
Expand Down
127 changes: 96 additions & 31 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ import {
EmitResolver,
EmitTextWriter,
emptyArray,
emptySet,
endsWith,
EntityName,
EntityNameExpression,
Expand Down Expand Up @@ -25375,47 +25376,101 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function isMatchingReference(source: Node, target: Node): boolean {
switch (target.kind) {
case SyntaxKind.ParenthesizedExpression:
case SyntaxKind.NonNullExpression:
return isMatchingReference(source, (target as NonNullExpression | ParenthesizedExpression).expression);
case SyntaxKind.BinaryExpression:
return (isAssignmentExpression(target) && isMatchingReference(source, target.left)) ||
(isBinaryExpression(target) && target.operatorToken.kind === SyntaxKind.CommaToken && isMatchingReference(source, target.right));
}
switch (source.kind) {
const keySource = getReferenceCacheKey(source);
const keyTarget = getReferenceCacheKey(target);
return keySource !== undefined && keyTarget !== undefined && keySource === keyTarget;
}

/**
* @returns A cache key that looks like `1234."field"."\"prop\".\"name\""` or `undefined` if the node can't be a reference
Copy link
Member Author

Choose a reason for hiding this comment

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

While this is somewhat resilient against antagonistic field names, the escaping is potentially costly, and I'd like to try to avoid it if this shows promise. A less readable key format, like 1234.23.57 might be better (where 23 and 57 are indexes into a known-field-name-strings holding array). Still, I've cached the results, so it's not like we repeat the work, so savings by making this less readable aren't likely to be huge.

* `1234` can be any of `new`, `import`, `super`, or `this` if one of those special expressions is the root of the reference, rather than an identifier with a known symbol
*/
function getReferenceCacheKeyWorker(reference: Node): string | undefined {
switch (reference.kind) {
case SyntaxKind.MetaProperty:
return target.kind === SyntaxKind.MetaProperty
&& (source as MetaProperty).keywordToken === (target as MetaProperty).keywordToken
&& (source as MetaProperty).name.escapedText === (target as MetaProperty).name.escapedText;
return `${(reference as MetaProperty).keywordToken === SyntaxKind.NewKeyword ? "new" : "import"}."${escapeString(idText((reference as MetaProperty).name))}"`;
case SyntaxKind.Identifier:
case SyntaxKind.PrivateIdentifier:
return isThisInTypeQuery(source) ?
target.kind === SyntaxKind.ThisKeyword :
target.kind === SyntaxKind.Identifier && getResolvedSymbol(source as Identifier) === getResolvedSymbol(target as Identifier) ||
(isVariableDeclaration(target) || isBindingElement(target)) &&
getExportSymbolOfValueSymbolIfExported(getResolvedSymbol(source as Identifier)) === getSymbolOfDeclaration(target);
case SyntaxKind.PrivateIdentifier: {
if (isThisInTypeQuery(reference)) {
return "this";
}
// Sometimes this identifier is the RHS of a property name expression - in those cases, the symbol is only known if expression checking on the property access expression is already
// complete (or complete enough to have assigned the resolved symbol)
// (it will also trigger undesirable behavior to call `getResolvedSymbol` on such an identifier, since it'll try to lookup a nonsense name!)
const sym = getNodeLinks(reference as Identifier).resolvedSymbol || isIdentifier(reference) && !(isPropertyAccessExpression(reference.parent) && reference.parent.name === reference) && getResolvedSymbol(reference);
const exportSymbol = sym && getExportSymbolOfValueSymbolIfExported(sym);
return exportSymbol ? `${getSymbolId(exportSymbol)}` : undefined;
}
case SyntaxKind.VariableDeclaration:
case SyntaxKind.BindingElement: {
const declSymbol = getSymbolOfDeclaration(reference as VariableDeclaration | BindingElement);
const exportSymbol = declSymbol && getExportSymbolOfValueSymbolIfExported(declSymbol);
return `${getSymbolId(exportSymbol)}`;
}
case SyntaxKind.ThisKeyword:
return target.kind === SyntaxKind.ThisKeyword;
return "this";
case SyntaxKind.SuperKeyword:
return target.kind === SyntaxKind.SuperKeyword;
return "super";
case SyntaxKind.NonNullExpression:
case SyntaxKind.ParenthesizedExpression:
return isMatchingReference((source as NonNullExpression | ParenthesizedExpression).expression, target);
return getReferenceCacheKey((reference as NonNullExpression | ParenthesizedExpression).expression);
case SyntaxKind.PropertyAccessExpression:
case SyntaxKind.ElementAccessExpression:
const sourcePropertyName = getAccessedPropertyName(source as AccessExpression);
const targetPropertyName = isAccessExpression(target) ? getAccessedPropertyName(target) : undefined;
return sourcePropertyName !== undefined && targetPropertyName !== undefined && targetPropertyName === sourcePropertyName &&
isMatchingReference((source as AccessExpression).expression, (target as AccessExpression).expression);
const propertyName = getAccessedPropertyName(reference as AccessExpression);
const lhsName = getReferenceCacheKey((reference as AccessExpression).expression);
return propertyName !== undefined && lhsName !== undefined ? `${lhsName}."${escapeString(unescapeLeadingUnderscores(propertyName))}"` : undefined;
case SyntaxKind.QualifiedName:
return isAccessExpression(target) &&
(source as QualifiedName).right.escapedText === getAccessedPropertyName(target) &&
isMatchingReference((source as QualifiedName).left, target.expression);
case SyntaxKind.BinaryExpression:
return (isBinaryExpression(source) && source.operatorToken.kind === SyntaxKind.CommaToken && isMatchingReference(source.right, target));
return `${getReferenceCacheKey((reference as QualifiedName).left)!}."${escapeString(idText((reference as QualifiedName).right))}"`;
case SyntaxKind.BinaryExpression: {
Debug.assert(isBinaryExpression(reference));
return reference.operatorToken.kind === SyntaxKind.CommaToken ? getReferenceCacheKey(reference.right) :
isAssignmentExpression(reference) ? getReferenceCacheKey(reference.left) :
undefined;
}
}
return undefined;
}

function getReferenceCacheKey(node: Node) {
const links = getNodeLinks(node);
if (!links.referenceCacheKey) {
const result = getReferenceCacheKeyWorker(node);
links.referenceCacheKey = result !== undefined ? result : "<none>";
}
return links.referenceCacheKey === "<none>" ? undefined : links.referenceCacheKey;
}

function getContainedReferences(node: Node): ReadonlySet<string> {
const links = getNodeLinks(node);
if (!links.containedReferences) {
links.containedReferences = getContainedReferencesWorker(node);
}
return links.containedReferences;
}

function getContainedReferencesWorker(node: Node): ReadonlySet<string> {
let references: ReadonlySet<never> | Set<string> = emptySet;
if (isStatement(node) || isExpressionNode(node)) {
// don't descend into variable declarations or binding elements, since those contain identifiers we don't want to collect
forEachChild(node, visitor);
Copy link
Member Author

Choose a reason for hiding this comment

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

The self-check failure points out that I need to swap this to use forEachChildRecursively (since that trampolines internally), since we allow binary expressions to nest greater than the max stack depth possible to traverse in the normal cached call/worker function used in the checker (because the parser is very stack depth efficient). That probably won't affect performance much.

I knew I'd need to do this, I didn't think we had hugely chained binary expressions in our own codebase, though - very much wondering which expressions are long enough to be an issue.

const key = getReferenceCacheKey(node);
if (key !== undefined) {
addReference(key);
}
}
return references;

function visitor(n: Node) {
const refs = getContainedReferences(n);
refs.forEach(r => addReference(r));
}

function addReference(ref: string) {
if (references === emptySet) {
references = new Set<string>();
}
(references as Set<String>).add(ref);
}
return false;
}

function getAccessedPropertyName(access: AccessExpression | BindingElement | ParameterDeclaration): __String | undefined {
Expand Down Expand Up @@ -26866,6 +26921,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const antecedentTypes: Type[] = [];
let subtypeReduction = false;
let firstAntecedentType: FlowType | undefined;
let possiblyNarrowsRef: boolean | undefined;
for (const antecedent of flow.antecedents!) {
let flowType;
if (!firstAntecedentType) {
Expand All @@ -26874,6 +26930,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
flowType = firstAntecedentType = getTypeAtFlowNode(antecedent);
}
else {
if (possiblyNarrowsRef === undefined) {
const key = getReferenceCacheKey(reference);
if (key !== undefined) {
possiblyNarrowsRef = getContainedReferences(flow.node!).has(key);
}
}
if (!possiblyNarrowsRef) {
break;
}
// All but the first antecedent are the looping control flow paths that lead
// back to the loop junction. We track these on the flow loop stack.
flowLoopNodes[flowLoopCount] = flow;
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4152,6 +4152,7 @@ export interface FlowStart extends FlowNodeBase {
// FlowLabel represents a junction with multiple possible preceding control flows.
export interface FlowLabel extends FlowNodeBase {
antecedents: FlowNode[] | undefined;
node?: ForInOrOfStatement | ForStatement | WhileStatement | DoStatement;
}

// FlowAssignment represents a node that assigns a value to a narrowable reference,
Expand Down Expand Up @@ -6061,6 +6062,8 @@ export interface NodeLinks {
parameterInitializerContainsUndefined?: boolean; // True if this is a parameter declaration whose type annotation contains "undefined".
fakeScopeForSignatureDeclaration?: boolean; // True if this is a fake scope injected into an enclosing declaration chain.
assertionExpressionType?: Type; // Cached type of the expression of a type assertion
referenceCacheKey?: string; // Cached reference equivalence cache key
containedReferences?: ReadonlySet<string>; // Set of references contained within the node, including itself
}

/** @internal */
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6088,6 +6088,7 @@ declare namespace ts {
}
interface FlowLabel extends FlowNodeBase {
antecedents: FlowNode[] | undefined;
node?: ForInOrOfStatement | ForStatement | WhileStatement | DoStatement;
}
interface FlowAssignment extends FlowNodeBase {
node: Expression | VariableDeclaration | BindingElement;
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2035,6 +2035,7 @@ declare namespace ts {
}
interface FlowLabel extends FlowNodeBase {
antecedents: FlowNode[] | undefined;
node?: ForInOrOfStatement | ForStatement | WhileStatement | DoStatement;
}
interface FlowAssignment extends FlowNodeBase {
node: Expression | VariableDeclaration | BindingElement;
Expand Down