Skip to content

Commit

Permalink
Added optimization that reduces the time it takes to perform code flo…
Browse files Browse the repository at this point in the history 
…w analysis in the presence of if/elf/else statements, try statements, with statements, and ternary expressions.
  • Loading branch information
@msfterictraut
msfterictraut committed Sep 15, 2021
1 parent f39520a commit a41cdc0
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 37 deletions.
62 changes: 43 additions & 19 deletions packages/pyright-internal/src/analyzer/binder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ import {
createKeyForReference,
FlowAssignment,
FlowAssignmentAlias,
FlowBranchLabel,
FlowCall,
FlowCondition,
FlowExhaustedMatch,
FlowFlags,
FlowLabel,
FlowLoopLabel,
FlowNarrowForPattern,
FlowNode,
FlowPostContextManagerLabel,
Expand Down Expand Up @@ -1078,10 +1078,12 @@ export class Binder extends ParseTreeWalker {
}

override visitIf(node: IfNode): boolean {
const preIfFlowNode = this._currentFlowNode!;
const thenLabel = this._createBranchLabel();
const elseLabel = this._createBranchLabel();
const postIfLabel = this._createBranchLabel();
const postIfLabel = this._createBranchLabel(preIfFlowNode);

postIfLabel.affectedExpressions = this._trackCodeFlowExpressions(() => {
// Determine if the test condition is always true or always false. If so,
// we can treat either the then or the else clause as unconditional.
const constExprValue = StaticExpressions.evaluateStaticBoolLikeExpression(
Expand Down Expand Up @@ -1111,6 +1113,7 @@ export class Binder extends ParseTreeWalker {
}
this._addAntecedent(postIfLabel, this._currentFlowNode);
this._currentFlowNode = this._finishFlowLabel(postIfLabel);
});

return false;
}
Expand Down Expand Up @@ -1266,21 +1269,24 @@ export class Binder extends ParseTreeWalker {
// (after finally)

// Create one flow label for every except clause.
const preTryFlowNode = this._currentFlowNode!;
const curExceptTargets = node.exceptClauses.map(() => this._createBranchLabel());
const preFinallyLabel = this._createBranchLabel();
const preFinallyLabel = this._createBranchLabel(preTryFlowNode);
let isAfterElseAndExceptsReachable = false;

// Create a label for all of the return or raise labels that are
// encountered within the try/except/else blocks. This conditionally
// connects the return/raise statement to the finally clause.
const preFinallyReturnOrRaiseLabel = this._createBranchLabel();
let isAfterElseAndExceptsReachable = false;
const preFinallyReturnOrRaiseLabel = this._createBranchLabel(preTryFlowNode);

const preFinallyGate: FlowPreFinallyGate = {
flags: FlowFlags.PreFinallyGate,
id: getUniqueFlowNodeId(),
antecedent: preFinallyReturnOrRaiseLabel,
isGateClosed: false,
};

preFinallyLabel.affectedExpressions = this._trackCodeFlowExpressions(() => {
if (node.finallySuite) {
this._addAntecedent(preFinallyLabel, preFinallyGate);
}
Expand Down Expand Up @@ -1339,6 +1345,8 @@ export class Binder extends ParseTreeWalker {

// Handle the finally block.
this._currentFlowNode = this._finishFlowLabel(preFinallyLabel);
});

if (node.finallySuite) {
this.walk(node.finallySuite);

Expand All @@ -1348,7 +1356,7 @@ export class Binder extends ParseTreeWalker {
flags: FlowFlags.PostFinally,
id: getUniqueFlowNodeId(),
finallyNode: node.finallySuite,
antecedent: this._currentFlowNode,
antecedent: this._currentFlowNode!,
preFinallyGate,
};
this._currentFlowNode = isAfterElseAndExceptsReachable ? postFinallyNode : Binder._unreachableFlowNode;
Expand Down Expand Up @@ -1727,9 +1735,11 @@ export class Binder extends ParseTreeWalker {
);
this._addAntecedent(contextManagerExceptionTarget, this._currentFlowNode!);

const postContextManagerLabel = this._createBranchLabel();
const preWithSuiteNode = this._currentFlowNode!;
const postContextManagerLabel = this._createBranchLabel(preWithSuiteNode);
this._addAntecedent(postContextManagerLabel, contextManagerExceptionTarget!);

postContextManagerLabel.affectedExpressions = this._trackCodeFlowExpressions(() => {
this._useExceptTargets([contextManagerExceptionTarget], () => {
this.walk(node.suite);
});
Expand All @@ -1743,15 +1753,18 @@ export class Binder extends ParseTreeWalker {
this._addError(Localizer.Diagnostic.asyncNotInAsyncFunction(), node.asyncToken);
}
}
});

return false;
}

override visitTernary(node: TernaryNode): boolean {
const preTernaryFlowNode = this._currentFlowNode!;
const trueLabel = this._createBranchLabel();
const falseLabel = this._createBranchLabel();
const postExpressionLabel = this._createBranchLabel();
const postExpressionLabel = this._createBranchLabel(preTernaryFlowNode);

postExpressionLabel.affectedExpressions = this._trackCodeFlowExpressions(() => {
// Handle the test expression.
this._bindConditional(node.testExpression, trueLabel, falseLabel);

Expand All @@ -1766,6 +1779,7 @@ export class Binder extends ParseTreeWalker {
this._addAntecedent(postExpressionLabel, this._currentFlowNode);

this._currentFlowNode = this._finishFlowLabel(postExpressionLabel);
});

return false;
}
Expand Down Expand Up @@ -2290,11 +2304,13 @@ export class Binder extends ParseTreeWalker {
return flowNode;
}

private _createBranchLabel() {
const flowNode: FlowLabel = {
private _createBranchLabel(preBranchAntecedent?: FlowNode) {
const flowNode: FlowBranchLabel = {
flags: FlowFlags.BranchLabel,
id: getUniqueFlowNodeId(),
antecedents: [],
preBranchAntecedent,
affectedExpressions: undefined,
};
return flowNode;
}
Expand All @@ -2320,17 +2336,18 @@ export class Binder extends ParseTreeWalker {
id: getUniqueFlowNodeId(),
antecedents: [],
expressions,
affectedExpressions: undefined,
isAsync,
};
return flowNode;
}

private _createLoopLabel() {
const flowNode: FlowLoopLabel = {
const flowNode: FlowLabel = {
flags: FlowFlags.LoopLabel,
id: getUniqueFlowNodeId(),
antecedents: [],
expressions: undefined,
affectedExpressions: undefined,
};
return flowNode;
}
Expand Down Expand Up @@ -2902,17 +2919,12 @@ export class Binder extends ParseTreeWalker {
}
}

private _bindLoopStatement(preLoopLabel: FlowLoopLabel, postLoopLabel: FlowLabel, callback: () => void) {
const savedContinueTarget = this._currentContinueTarget;
const savedBreakTarget = this._currentBreakTarget;
private _trackCodeFlowExpressions(callback: () => void): Set<string> {
const savedExpressions = this._currentScopeCodeFlowExpressions;
this._currentContinueTarget = preLoopLabel;
this._currentBreakTarget = postLoopLabel;
this._currentScopeCodeFlowExpressions = new Set<string>();

callback();

preLoopLabel.expressions = this._currentScopeCodeFlowExpressions;
const scopedExpressions = this._currentScopeCodeFlowExpressions;

if (savedExpressions) {
this._currentScopeCodeFlowExpressions.forEach((value) => {
Expand All @@ -2921,6 +2933,18 @@ export class Binder extends ParseTreeWalker {
}

this._currentScopeCodeFlowExpressions = savedExpressions;

return scopedExpressions;
}

private _bindLoopStatement(preLoopLabel: FlowLabel, postLoopLabel: FlowLabel, callback: () => void) {
const savedContinueTarget = this._currentContinueTarget;
const savedBreakTarget = this._currentBreakTarget;
this._currentContinueTarget = preLoopLabel;
this._currentBreakTarget = postLoopLabel;

preLoopLabel.affectedExpressions = this._trackCodeFlowExpressions(callback);

this._currentContinueTarget = savedContinueTarget;
this._currentBreakTarget = savedBreakTarget;
}
Expand Down
41 changes: 35 additions & 6 deletions packages/pyright-internal/src/analyzer/codeFlow.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,21 @@ export interface FlowNode {
// preceding control flows.
export interface FlowLabel extends FlowNode {
antecedents: FlowNode[];
}

export interface FlowLoopLabel extends FlowLabel {
// Set of all expressions that require code flow analysis
// through the loop to determine their types. If an expression
// is not within this map, loop analysis can be skipped and
// determined from the first antecedent only.
expressions: Set<string> | undefined;
// through the loop or in branch paths to determine their types.
// If an expression is not within this map, branch or loop analysis
// can be skipped and determined from the first antecedent only.
affectedExpressions: Set<string> | undefined;
}

export interface FlowBranchLabel extends FlowLabel {
// If specified, this label represents a flow node that precedes
// (i.e. is higher up in the control flow graph) than all of
// the antecedents of this branch label. If an expression is
// not affected by the branch label, the entire flow node can be
// skipped, and processing can proceed at this label.
preBranchAntecedent: FlowNode | undefined;
}

// FlowAssignment represents a node that assigns a value.
Expand Down Expand Up @@ -228,3 +235,25 @@ export function createKeyForReference(reference: CodeFlowReferenceExpressionNode

return key;
}

export function createKeysForReferenceSubexpressions(reference: CodeFlowReferenceExpressionNode): string[] {
if (reference.nodeType === ParseNodeType.Name) {
return [createKeyForReference(reference)];
}

if (reference.nodeType === ParseNodeType.MemberAccess) {
return [
...createKeysForReferenceSubexpressions(reference.leftExpression as CodeFlowReferenceExpressionNode),
createKeyForReference(reference),
];
}

if (reference.nodeType === ParseNodeType.Index) {
return [
...createKeysForReferenceSubexpressions(reference.baseExpression as CodeFlowReferenceExpressionNode),
createKeyForReference(reference),
];
}

fail('createKeyForReference received unexpected expression type');
}
41 changes: 29 additions & 12 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,15 @@ import * as AnalyzerNodeInfo from './analyzerNodeInfo';
import {
CodeFlowReferenceExpressionNode,
createKeyForReference,
createKeysForReferenceSubexpressions,
FlowAssignment,
FlowAssignmentAlias,
FlowBranchLabel,
FlowCall,
FlowCondition,
FlowExhaustedMatch,
FlowFlags,
FlowLabel,
FlowLoopLabel,
FlowNarrowForPattern,
FlowNode,
FlowPostContextManagerLabel,
Expand Down Expand Up @@ -17527,6 +17528,7 @@ export function createTypeEvaluator(
isInitialTypeIncomplete: boolean
): FlowNodeTypeResult {
const referenceKey = reference !== undefined ? createKeyForReference(reference) : undefined;
let subexpressionReferenceKeys: string[] | undefined;
const referenceKeyWithSymbolId =
referenceKey !== undefined && targetSymbolId !== undefined
? referenceKey + `.${targetSymbolId.toString()}`
Expand Down Expand Up @@ -17821,6 +17823,7 @@ export function createTypeEvaluator(
}

if (curFlowNode.flags & FlowFlags.BranchLabel) {
const branchFlowNode = curFlowNode as FlowBranchLabel;
if (curFlowNode.flags & FlowFlags.PostContextManager) {
// Determine whether any of the context managers support exception
// suppression. If not, none of its antecedents are reachable.
Expand All @@ -17839,6 +17842,22 @@ export function createTypeEvaluator(
}
}

// Is the current symbol modified in any way within the scope of the branch?
// If not, we can skip all processing within the branch scope.
if (reference && branchFlowNode.preBranchAntecedent && branchFlowNode.affectedExpressions) {
if (!subexpressionReferenceKeys) {
subexpressionReferenceKeys = createKeysForReferenceSubexpressions(reference);
}

if (
!subexpressionReferenceKeys.some((key) => branchFlowNode.affectedExpressions!.has(key)) &&
isFlowNodeReachable(curFlowNode, branchFlowNode.preBranchAntecedent)
) {
curFlowNode = branchFlowNode.preBranchAntecedent;
continue;
}
}

const labelNode = curFlowNode as FlowLabel;
const typesToCombine: Type[] = [];
let branchUsedOuterScopeAlias = usedOuterScopeAlias;
Expand Down Expand Up @@ -17873,14 +17892,17 @@ export function createTypeEvaluator(
}

if (curFlowNode.flags & FlowFlags.LoopLabel) {
const loopNode = curFlowNode as FlowLoopLabel;
const loopNode = curFlowNode as FlowLabel;

// Is the current symbol modified in any way within the
// loop? If not, we can skip all processing within the loop
// and assume that the type comes from the first antecedent,
// Is the current symbol modified in any way within the loop? If not, we can skip all
// processing within the loop and assume that the type comes from the first antecedent,
// which feeds the loop.
if (referenceKey) {
if (!loopNode.expressions || !loopNode.expressions.has(referenceKey)) {
if (reference) {
if (!subexpressionReferenceKeys) {
subexpressionReferenceKeys = createKeysForReferenceSubexpressions(reference);
}

if (!subexpressionReferenceKeys.some((key) => loopNode.affectedExpressions!.has(key))) {
curFlowNode = loopNode.antecedents[0];
continue;
}
Expand Down Expand Up @@ -18280,17 +18302,12 @@ export function createTypeEvaluator(
if (curFlowNode.flags & FlowFlags.Call) {
const callFlowNode = curFlowNode as FlowCall;

// If we're determining whether a specified source flow node is
// reachable, don't take into consideration possible "no return"
// calls.
if (sourceFlowNode === undefined) {
// If this function returns a "NoReturn" type, that means
// it always raises an exception or otherwise doesn't return,
// so we can assume that the code before this is unreachable.
if (isCallNoReturn(callFlowNode.node)) {
return false;
}
}

curFlowNode = callFlowNode.antecedent;
continue;
Expand Down

0 comments on commit a41cdc0

Please sign in to comment.