Skip to content

Commit

Permalink
Fixed issue that results in a false positive "type could not be deter…
Browse files Browse the repository at this point in the history 
…mined because it refers to itself" error caused by a false dependency due to narrowing logic. This may also improve type analysis performance in some code. This addresses #9139.
  • Loading branch information
@erictraut
erictraut committed Oct 24, 2024
1 parent a326745 commit b8b5881
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 89 deletions.
156 changes: 67 additions & 89 deletions packages/pyright-internal/src/analyzer/binder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ interface ClassVarInfo {
classVarTypeNode: ExpressionNode | undefined;
}

interface NarrowExprOptions {
filterForNeverNarrowing?: boolean;
isComplexExpression?: boolean;
allowDiscriminatedNarrowing?: boolean;
}

// For each flow node within an execution context, we'll add a small
// amount to the complexity factor. Without this, the complexity
// calculation fails to take into account large numbers of non-cyclical
Expand Down Expand Up @@ -2870,7 +2876,7 @@ export class Binder extends ParseTreeWalker {
// Limit only to expressions that contain a narrowable subexpression
// that is a name. This avoids complexities with composite expressions like
// member access or index expressions.
if (this._isNarrowingExpression(node, expressionList, /* neverNarrowingExpressions */ true)) {
if (this._isNarrowingExpression(node, expressionList, { filterForNeverNarrowing: true })) {
const filteredExprList = expressionList.filter((expr) => expr.nodeType === ParseNodeType.Name);
if (filteredExprList.length > 0) {
this._currentFlowNode = this._createFlowConditional(
Expand Down Expand Up @@ -2942,13 +2948,9 @@ export class Binder extends ParseTreeWalker {

const expressionList: CodeFlowReferenceExpressionNode[] = [];
if (
!this._isNarrowingExpression(
expression,
expressionList,
/* filterForNeverNarrowing */ (flags &
(FlowFlags.TrueNeverCondition | FlowFlags.FalseNeverCondition)) !==
0
)
!this._isNarrowingExpression(expression, expressionList, {
filterForNeverNarrowing: (flags & (FlowFlags.TrueNeverCondition | FlowFlags.FalseNeverCondition)) !== 0,
})
) {
return antecedent;
}
Expand Down Expand Up @@ -3000,14 +3002,13 @@ export class Binder extends ParseTreeWalker {
private _isNarrowingExpression(
expression: ExpressionNode,
expressionList: CodeFlowReferenceExpressionNode[],
filterForNeverNarrowing = false,
isComplexExpression = false
options: NarrowExprOptions = {}
): boolean {
switch (expression.nodeType) {
case ParseNodeType.Name:
case ParseNodeType.MemberAccess:
case ParseNodeType.Index: {
if (filterForNeverNarrowing) {
if (options.filterForNeverNarrowing) {
// Never narrowing doesn't support member access or index
// expressions.
if (expression.nodeType !== ParseNodeType.Name) {
Expand All @@ -3017,19 +3018,19 @@ export class Binder extends ParseTreeWalker {
// Never narrowing doesn't support simple names (falsy
// or truthy narrowing) because it's too expensive and
// provides relatively little utility.
if (!isComplexExpression) {
if (!options.isComplexExpression) {
return false;
}
}

if (isCodeFlowSupportedForReference(expression)) {
expressionList.push(expression);

if (!filterForNeverNarrowing) {
if (!options.filterForNeverNarrowing) {
// If the expression is a member access expression, add its
// leftExpression to the expression list because that expression
// can be narrowed based on the attribute type.
if (expression.nodeType === ParseNodeType.MemberAccess) {
if (expression.nodeType === ParseNodeType.MemberAccess && options.allowDiscriminatedNarrowing) {
if (isCodeFlowSupportedForReference(expression.d.leftExpr)) {
expressionList.push(expression.d.leftExpr);
}
Expand Down Expand Up @@ -3057,12 +3058,10 @@ export class Binder extends ParseTreeWalker {

case ParseNodeType.AssignmentExpression: {
expressionList.push(expression.d.name);
this._isNarrowingExpression(
expression.d.rightExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
this._isNarrowingExpression(expression.d.rightExpr, expressionList, {
...options,
isComplexExpression: true,
});
return true;
}

Expand All @@ -3079,12 +3078,11 @@ export class Binder extends ParseTreeWalker {
expression.d.rightExpr.nodeType === ParseNodeType.Constant &&
expression.d.rightExpr.d.constType === KeywordType.None
) {
return this._isNarrowingExpression(
expression.d.leftExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
return this._isNarrowingExpression(expression.d.leftExpr, expressionList, {
...options,
isComplexExpression: true,
allowDiscriminatedNarrowing: true,
});
}

// Look for "type(X) is Y" or "type(X) is not Y".
Expand All @@ -3099,17 +3097,15 @@ export class Binder extends ParseTreeWalker {
return this._isNarrowingExpression(
expression.d.leftExpr.d.args[0].d.valueExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
{ ...options, isComplexExpression: true }
);
}

const isLeftNarrowing = this._isNarrowingExpression(
expression.d.leftExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
const isLeftNarrowing = this._isNarrowingExpression(expression.d.leftExpr, expressionList, {
...options,
isComplexExpression: true,
allowDiscriminatedNarrowing: true,
});

// Look for "X is Y" or "X is not Y".
// Look for X == <literal> or X != <literal>
Expand All @@ -3125,12 +3121,10 @@ export class Binder extends ParseTreeWalker {
expression.d.operator === OperatorType.GreaterThan ||
expression.d.operator === OperatorType.GreaterThanOrEqual
) {
const isLeftNarrowing = this._isNarrowingExpression(
expression.d.leftExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
const isLeftNarrowing = this._isNarrowingExpression(expression.d.leftExpr, expressionList, {
...options,
isComplexExpression: true,
});

return isLeftNarrowing;
}
Expand All @@ -3140,32 +3134,26 @@ export class Binder extends ParseTreeWalker {
if (expression.d.operator === OperatorType.In || expression.d.operator === OperatorType.NotIn) {
if (
expression.d.leftExpr.nodeType === ParseNodeType.StringList &&
this._isNarrowingExpression(
expression.d.rightExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
)
this._isNarrowingExpression(expression.d.rightExpr, expressionList, {
...options,
isComplexExpression: true,
})
) {
return true;
}
}

// Look for "X in Y" or "X not in Y".
if (expression.d.operator === OperatorType.In || expression.d.operator === OperatorType.NotIn) {
const isLeftNarrowable = this._isNarrowingExpression(
expression.d.leftExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
const isLeftNarrowable = this._isNarrowingExpression(expression.d.leftExpr, expressionList, {
...options,
isComplexExpression: true,
});

const isRightNarrowable = this._isNarrowingExpression(
expression.d.rightExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
const isRightNarrowable = this._isNarrowingExpression(expression.d.rightExpr, expressionList, {
...options,
isComplexExpression: true,
});

return isLeftNarrowable || isRightNarrowable;
}
Expand All @@ -3176,22 +3164,18 @@ export class Binder extends ParseTreeWalker {
case ParseNodeType.UnaryOperation: {
return (
expression.d.operator === OperatorType.Not &&
this._isNarrowingExpression(
expression.d.expr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ false
)
this._isNarrowingExpression(expression.d.expr, expressionList, {
...options,
isComplexExpression: false,
})
);
}

case ParseNodeType.AugmentedAssignment: {
return this._isNarrowingExpression(
expression.d.rightExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
return this._isNarrowingExpression(expression.d.rightExpr, expressionList, {
...options,
isComplexExpression: true,
});
}

case ParseNodeType.Call: {
Expand All @@ -3201,41 +3185,35 @@ export class Binder extends ParseTreeWalker {
expression.d.leftExpr.d.value === 'issubclass') &&
expression.d.args.length === 2
) {
return this._isNarrowingExpression(
expression.d.args[0].d.valueExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
return this._isNarrowingExpression(expression.d.args[0].d.valueExpr, expressionList, {
...options,
isComplexExpression: true,
});
}

if (
expression.d.leftExpr.nodeType === ParseNodeType.Name &&
expression.d.leftExpr.d.value === 'callable' &&
expression.d.args.length === 1
) {
return this._isNarrowingExpression(
expression.d.args[0].d.valueExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
return this._isNarrowingExpression(expression.d.args[0].d.valueExpr, expressionList, {
...options,
isComplexExpression: true,
});
}

// Is this potentially a call to a user-defined type guard function?
if (expression.d.args.length >= 1) {
// Never narrowing doesn't support type guards because they do not
// offer negative narrowing.
if (filterForNeverNarrowing) {
if (options.filterForNeverNarrowing) {
return false;
}

return this._isNarrowingExpression(
expression.d.args[0].d.valueExpr,
expressionList,
filterForNeverNarrowing,
/* isComplexExpression */ true
);
return this._isNarrowingExpression(expression.d.args[0].d.valueExpr, expressionList, {
...options,
isComplexExpression: true,
});
}
}
}
Expand Down
21 changes: 21 additions & 0 deletions packages/pyright-internal/src/tests/samples/loop51.py
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# This sample tests a case where type evaluation for a type guard
# within a loop may trigger a false positive "type depends on itself"
# error message.

# For details, see https://github.com/microsoft/pyright/issues/9139.

from enum import StrEnum


class MyEnum(StrEnum):
A = "A"


for _ in range(2):
x: dict[MyEnum, int] = {}

if MyEnum.A in x:
...

for _ in x.values():
...
6 changes: 6 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator3.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,12 @@ test('Loop50', () => {
TestUtils.validateResults(analysisResults, 0);
});

test('Loop51', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['loop51.py']);

TestUtils.validateResults(analysisResults, 0);
});

test('ForLoop1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['forLoop1.py']);

Expand Down

0 comments on commit b8b5881

Please sign in to comment.